diff --git a/TASKS.md b/TASKS.md index b7ff7d8..acff4c0 100644 --- a/TASKS.md +++ b/TASKS.md @@ -9,6 +9,36 @@ When a round closes, move its finished items to `tasks-done.md`. - [ ] archive the done tasks in here to another file, this is crufty.... +## Round 36 — the merge rework (planned, Round 35 covered the speed half) + +The three symptoms in Round 35 shared a root cause; that round fixed the +performance half. This is the correctness half. + +- [ ] **Playlist merges lose position.** `_merge_playlist` is a 2-way union with + no common ancestor: it takes one side's order wholesale and *appends* the + other side's extras, so a track inserted in the middle on one machine + arrives at the tail on the other (this is what happened to `nissa one`). + Replace with an anchor-based merge — insert each side-only track after its + nearest preceding common anchor. Same in `_reconcile_playlist`. +- [ ] **mtime is a lie for playlists.** `mark_playlist_settings_dirty` rewrites + the whole playlist file for UI-only changes, and because the last column + is stretch-sized, *resizing the window* rewrites the open playlist's JSON. + So "most recently edited" often means "most recently resized". Give + `Playlist` a `date_modified` bumped only in `_set_track_ids`, and merge on + that. +- [ ] **`max()` play counts discard concurrent plays.** Base 100, one machine + plays 5 (105), the other plays 3 (103) → merge keeps 105 and those 3 are + gone. Per-machine `plays/.json` journal (machine id kept + outside the synced dir): only its owner ever writes it, so play data can + never conflict, effective count = base + sum of journals, and `library.json` + stops being rewritten every 3 s during playback — which is what generates + the conflicts in the first place. +- [ ] **Quiet the merge dialog.** Lossless merges (counts and dates only) should + be a status-bar line, not a window; keep the dialog for lossy cases, and + reuse one instance so six can never stack up again. +- [ ] Consider a `.stignore` for `.resolved` so merge backups stop syncing + (Round 35 bounded the folder to 10 snapshots, but it still replicates). + diff --git a/lintunes/__init__.py b/lintunes/__init__.py index a8ab46b..6a09e47 100644 --- a/lintunes/__init__.py +++ b/lintunes/__init__.py @@ -1,3 +1,3 @@ """LinTunes — iTunes-style music library manager and player for Linux.""" -__version__ = "0.6.1" +__version__ = "0.7.0" diff --git a/lintunes/gui/track_table.py b/lintunes/gui/track_table.py index 77de7be..e45dd2b 100644 --- a/lintunes/gui/track_table.py +++ b/lintunes/gui/track_table.py @@ -7,7 +7,7 @@ from PyQt6.QtWidgets import ( ) from PyQt6.QtCore import ( Qt, QAbstractTableModel, QModelIndex, QPersistentModelIndex, - QSortFilterProxyModel, pyqtSignal, + pyqtSignal, QMimeData, QUrl, QPoint, QPointF, QRect, QRectF, QMetaType, QTimer, ) from PyQt6.QtGui import ( @@ -23,7 +23,6 @@ from lintunes.gui import drag_ghost TRACKS_MIME = "application/x-lintunes-tracks" -SORT_ROLE = Qt.ItemDataRole.UserRole # While dragging a track within this many pixels of the list's top/bottom edge, # the list auto-scrolls so off-screen rows can be reached. @@ -167,7 +166,7 @@ class RatingDelegate(QStyledItemDelegate): style.drawControl(QStyle.ControlElement.CE_ItemViewItem, opt, painter, opt.widget) - track = model.track_at(self._view.proxy.mapToSource(index).row()) + track = model.track_at(model.source_row(index.row())) stars = max(0, min(5, (track.rating or 0) // 20)) hovered = self._view.is_rating_hovered(index) if not stars and not hovered: @@ -290,11 +289,41 @@ def reveal_paths(paths: list[str]) -> None: QDesktopServices.openUrl(QUrl.fromLocalFile(os.path.dirname(paths[0]))) +def _sort_key(value): + """Comparable key for one cell. Everything collapses to a (text, number) + pair so a column holding a mix of strings, numbers and None never asks + Python to compare a str with a float.""" + if value is None: + return ("", 0.0) + if isinstance(value, str): + return (value.casefold(), 0.0) + try: + return ("", float(value)) + except (TypeError, ValueError): + return (str(value).casefold(), 0.0) + + class TrackTableModel(QAbstractTableModel): + """Track grid model that sorts itself. + + ``_tracks`` stays in **canonical order** — the order ``set_tracks`` was + handed, i.e. the playlist's manual order — and ``_order`` maps a displayed + row to its index in it. Sorting only reshuffles ``_order``, so "source row" + keeps meaning "position in the playlist" for drag-reorder and selection, + and sorting by the ``#`` column is just the identity order. + + This replaces a QSortFilterProxyModel: the proxy called ``data()`` once per + comparison, which cost ~8.5 s per load on a 21k-track library. Sorting a + precomputed key list here is ~200x faster, and nothing needed the proxy's + filtering. + """ def __init__(self, parent=None, with_index_column=False): super().__init__(parent) self._tracks: list[Track] = [] self._rows_by_id: dict[int, list[int]] = {} + self._order: list[int] = [] # display row -> canonical row + self._view_of: list[int] = [] # canonical row -> display row + self._sort: tuple[str, bool] = (INDEX_FIELD, True) # field, ascending self._fields: list[str] = [] self._with_index = with_index_column self._now_playing_id: int | None = None @@ -330,8 +359,62 @@ class TrackTableModel(QAbstractTableModel): self._rows_by_id = {} for row, track in enumerate(self._tracks): self._rows_by_id.setdefault(track.track_id, []).append(row) + self._rebuild_order() self.endResetModel() + def _rebuild_order(self): + """Recompute the display order from the current sort. O(n log n) with a + key computed once per track, rather than once per comparison.""" + count = len(self._tracks) + field, ascending = self._sort + if field == INDEX_FIELD or field not in COLUMN_MAP: + self._order = list(range(count)) + else: + keys = [_sort_key(getattr(track, field, None)) + for track in self._tracks] + self._order = sorted(range(count), key=keys.__getitem__, + reverse=not ascending) + self._view_of = [0] * count + for display_row, canonical_row in enumerate(self._order): + self._view_of[canonical_row] = display_row + + def sort(self, column: int, order=Qt.SortOrder.AscendingOrder): + """Called by QTableView when a header section is clicked (or by + sortByColumn). Keeps selection alive by remapping persistent indexes.""" + if not self._fields or not 0 <= column < len(self._fields): + return + self._sort = (self._fields[column], + order == Qt.SortOrder.AscendingOrder) + # Order matters: QItemSelectionModel turns the current selection into + # persistent indexes in response to layoutAboutToBeChanged, so snapshot + # the list after emitting it or the selection is dropped. + self.layoutAboutToBeChanged.emit() + old_indexes = self.persistentIndexList() + old_order = self._order + self._rebuild_order() + new_indexes = [] + for index in old_indexes: + row = index.row() + if 0 <= row < len(old_order): + new_indexes.append( + self.index(self._view_of[old_order[row]], index.column())) + else: + new_indexes.append(QModelIndex()) + self.changePersistentIndexList(old_indexes, new_indexes) + self.layoutChanged.emit() + + def source_row(self, display_row: int) -> int: + """Displayed row -> canonical row (what the proxy's mapToSource did).""" + if 0 <= display_row < len(self._order): + return self._order[display_row] + return -1 + + def display_row(self, canonical_row: int) -> int: + """Canonical row -> displayed row (the proxy's mapFromSource).""" + if 0 <= canonical_row < len(self._view_of): + return self._view_of[canonical_row] + return -1 + def track_at(self, row: int) -> Track: return self._tracks[row] @@ -344,7 +427,8 @@ class TrackTableModel(QAbstractTableModel): return rows[0] if rows else None def refresh_track(self, track_id: int): - for row in self._rows_by_id.get(track_id, ()): + for canonical_row in self._rows_by_id.get(track_id, ()): + row = self._view_of[canonical_row] self.dataChanged.emit(self.index(row, 0), self.index(row, self.columnCount() - 1)) @@ -371,10 +455,10 @@ class TrackTableModel(QAbstractTableModel): if not index.isValid(): return None field = self._fields[index.column()] - track = self._tracks[index.row()] + track = self._tracks[self._order[index.row()]] if field == INDEX_FIELD: - if role == Qt.ItemDataRole.DisplayRole or role == SORT_ROLE: + if role == Qt.ItemDataRole.DisplayRole: return index.row() + 1 return None @@ -385,10 +469,6 @@ class TrackTableModel(QAbstractTableModel): return speaker_pixmap(self._now_playing_active, color) value = getattr(track, field, "") - if role == SORT_ROLE: - if isinstance(value, str): - return value.lower() - return value if value is not None else 0 if role == Qt.ItemDataRole.DisplayRole: if field == "total_time": return format_time(value) @@ -437,10 +517,8 @@ class TrackTableView(QTableView): self._content_editable = True self.model_ = TrackTableModel(self, with_index_column=playlist_mode) - self.proxy = QSortFilterProxyModel(self) - self.proxy.setSourceModel(self.model_) - self.proxy.setSortRole(SORT_ROLE) - self.setModel(self.proxy) + # The model sorts itself (see TrackTableModel) — no proxy in between. + self.setModel(self.model_) self.setAlternatingRowColors(True) self.setSelectionBehavior(QAbstractItemView.SelectionBehavior.SelectRows) @@ -528,10 +606,10 @@ class TrackTableView(QTableView): src_row = self.model_.row_for_id(track_id) if src_row is None: return False - proxy_index = self.proxy.mapFromSource(self.model_.index(src_row, 0)) - self.setCurrentIndex(proxy_index) - self.selectRow(proxy_index.row()) - self.scrollTo(proxy_index, + index = self.model_.index(self.model_.display_row(src_row), 0) + self.setCurrentIndex(index) + self.selectRow(index.row()) + self.scrollTo(index, QAbstractItemView.ScrollHint.PositionAtCenter) return True @@ -572,14 +650,12 @@ class TrackTableView(QTableView): # ---- selection / ordering helpers ---- def view_order_track_ids(self) -> list[int]: - ids = [] - for proxy_row in range(self.proxy.rowCount()): - source_row = self.proxy.mapToSource(self.proxy.index(proxy_row, 0)).row() - ids.append(self.model_.track_at(source_row).track_id) - return ids + model = self.model_ + return [model.track_at(model.source_row(row)).track_id + for row in range(model.rowCount())] def selected_source_rows(self) -> list[int]: - rows = {self.proxy.mapToSource(idx).row() + rows = {self.model_.source_row(idx.row()) for idx in self.selectionModel().selectedRows()} return sorted(rows) @@ -759,7 +835,7 @@ class TrackTableView(QTableView): for persistent in (old, new): if persistent is not None and persistent.isValid(): self.viewport().update(self.visualRect( - self.proxy.index(persistent.row(), persistent.column()))) + self.model_.index(persistent.row(), persistent.column()))) def mouseMoveEvent(self, event): index = self.indexAt(event.position().toPoint()) @@ -775,7 +851,7 @@ class TrackTableView(QTableView): pos = event.position().toPoint() index = self.indexAt(pos) if self._is_rating_index(index): - track = self.model_.track_at(self.proxy.mapToSource(index).row()) + track = self.model_.track_at(self.model_.source_row(index.row())) slot = rating_slot_at( pos.x() - self.visualRect(index).left(), rating_slot_width(QFontMetrics(self.font()))) @@ -952,15 +1028,15 @@ class TrackTableView(QTableView): def _indicator_y(self, pos) -> int: """Viewport y for the insertion line matching where the drop lands.""" - count = self.proxy.rowCount() + count = self.model_.rowCount() if count == 0: return 0 row = self._drop_row(pos) if row is None: # drop appends row = count if row < count: - return self.visualRect(self.proxy.index(row, 0)).top() - return self.visualRect(self.proxy.index(count - 1, 0)).bottom() + 1 + return self.visualRect(self.model_.index(row, 0)).top() + return self.visualRect(self.model_.index(count - 1, 0)).bottom() + 1 def _set_drop_indicator(self, y: int | None): if y != self._drop_indicator_y: diff --git a/lintunes/gui/version_button.py b/lintunes/gui/version_button.py index 8821c2d..b183e05 100644 --- a/lintunes/gui/version_button.py +++ b/lintunes/gui/version_button.py @@ -6,7 +6,7 @@ finds upstream commits it grows a "*" and becomes a button: click → confirm git pull → the main window restarts the app on `update_applied`. """ -from PyQt6.QtCore import Qt, pyqtSignal +from PyQt6.QtCore import Qt, QTimer, pyqtSignal from PyQt6.QtGui import QPalette from PyQt6.QtWidgets import QMessageBox, QPushButton @@ -24,15 +24,24 @@ class VersionButton(QPushButton): self.setFocusPolicy(Qt.FocusPolicy.NoFocus) self.refresh_theme() self.setText(f"v{__version__}") - commit = updater.short_hash() - self._base_tooltip = (f"LinTunes v{__version__} ({commit})" if commit - else f"LinTunes v{__version__}") + self._base_tooltip = f"LinTunes v{__version__}" self.setToolTip(self._base_tooltip) + # short_hash() shells out to git; do it once the window is up rather + # than on the startup path (it only decorates the tooltip). + QTimer.singleShot(0, self._load_commit_tooltip) updater.update_available.connect(self._on_update_available) updater.update_failed.connect(self._on_update_failed) self.clicked.connect(self._on_clicked) + def _load_commit_tooltip(self): + commit = self._updater.short_hash() + if not commit: + return + self._base_tooltip = f"LinTunes v{__version__} ({commit})" + if not self._update_ready: + self.setToolTip(self._base_tooltip) + def refresh_theme(self): # Subtle but readable: the theme's text color at ~55% opacity # (palette(mid) is too close to the light backgrounds to read). diff --git a/lintunes/library_manager.py b/lintunes/library_manager.py index 69baf86..7174cfd 100644 --- a/lintunes/library_manager.py +++ b/lintunes/library_manager.py @@ -930,7 +930,14 @@ def _reconcile_track(mem_track, disk_track): """Fold the disk copy of a track into the in-memory one using the same rules as the conflict resolver (max play/skip counts, newest date_modified wins), mutating mem_track in place so any view holding it stays valid.""" - from lintunes.storage.conflict_resolver import _merge_track_fields + from lintunes.storage.conflict_resolver import ( + MERGEABLE_FIELDS, _merge_track_fields) + # A sync typically changes a handful of tracks out of tens of thousands. + # Comparing the mergeable fields first skips two to_dict() round trips per + # untouched track, which is most of the cost of a reload. + if all(getattr(mem_track, f, None) == getattr(disk_track, f, None) + for f in MERGEABLE_FIELDS): + return merged = mem_track.to_dict() _merge_track_fields(merged, disk_track.to_dict()) # disk wins where newer for key, value in merged.items(): diff --git a/lintunes/main.py b/lintunes/main.py index bf3aa6d..c0ec68a 100644 --- a/lintunes/main.py +++ b/lintunes/main.py @@ -102,11 +102,16 @@ def run_import(xml_path: Path, music_root: str | None, data_dir: Path): print("Import complete!") +# Long enough for the first paint to land before the smart recompute runs. +SMART_RECOMPUTE_DELAY_MS = 50 + + def run_gui(data_dir: Path, files: list[Path], qt_args: list[str] | None = None) -> bool: import logging logging.basicConfig(level=logging.INFO) + from PyQt6.QtCore import QTimer from PyQt6.QtWidgets import QApplication, QMessageBox, QFontDialog from lintunes import theme @@ -135,9 +140,6 @@ def run_gui(data_dir: Path, files: list[Path], theme.apply_theme(app, prefs) _ensure_now_playing_font(prefs) manager = LibraryManager(library, data_dir) - # Rebuild smart-playlist membership from criteria now that the full library - # (and any merged sync conflicts) is loaded. Cheap when nothing changed. - manager.recompute_all_smart() lastfm = LastFm(prefs, data_dir) window = MainWindow(manager, prefs, lastfm) app.installEventFilter(window) @@ -153,6 +155,11 @@ def run_gui(data_dir: Path, files: list[Path], watcher.changed.connect(manager.check_for_external_changes) window.show() + # Rebuild smart-playlist membership from criteria now that the full library + # (and any merged sync conflicts) is loaded. Deferred until after the first + # paint: it's a full pass over every track per smart playlist, and running + # it inline left the compositor without a responsive window. + QTimer.singleShot(SMART_RECOMPUTE_DELAY_MS, manager.recompute_all_smart) if startup_conflicts: window.show_conflict_summary(startup_conflicts) if files: diff --git a/lintunes/smart.py b/lintunes/smart.py index a8ddc26..119f1cb 100644 --- a/lintunes/smart.py +++ b/lintunes/smart.py @@ -284,80 +284,14 @@ def _parse_dt(value) -> Optional[datetime]: return dt -def _match_string(op, track_val, qval) -> bool: - t = ("" if track_val is None else str(track_val)).casefold() - q = ("" if qval is None else str(qval)).casefold() - if op == "is": - return t == q - if op == "is_not": - return t != q - if op == "contains": - return q in t - if op == "not_contains": - return q not in t - if op == "starts_with": - return t.startswith(q) - if op == "ends_with": - return t.endswith(q) - return False - - -def _match_numeric(op, track_val, qval, qval2) -> bool: - try: - t = float(track_val or 0) - q = float(qval or 0) - except (ValueError, TypeError): - return False - if op == "is": - return t == q - if op == "is_not": - return t != q - if op == "greater": - return t > q - if op == "less": - return t < q - if op == "in_range": - try: - q2 = float(qval2 or 0) - except (ValueError, TypeError): - return False - lo, hi = (q, q2) if q <= q2 else (q2, q) - return lo <= t <= hi - return False - - -def _match_date(op, track_val, rule: "SmartRule", now: datetime) -> bool: - dt = _parse_dt(track_val) - if op in ("after", "before", "in_range"): - # iTunes treats a null date (never played/skipped/etc.) as the distant - # past: "before X" matches it; "after"/"in range" don't. - d = dt if dt is not None else datetime.min - if op == "in_range": - lo, hi = _parse_dt(rule.value), _parse_dt(rule.value2) - if lo is None or hi is None: - return False - if lo > hi: - lo, hi = hi, lo - return lo <= d <= hi - q = _parse_dt(rule.value) - if q is None: - return False - return d > q if op == "after" else d < q - if op == "not_in_range": - if dt is None: - return True - lo, hi = _parse_dt(rule.value), _parse_dt(rule.value2) - if lo is None or hi is None: - return True - if lo > hi: - lo, hi = hi, lo - return not (lo <= dt <= hi) - if op in ("in_last", "not_in_last"): - seconds = _rule_window_seconds(rule) - if op == "in_last": - return dt is not None and (now - dt) <= timedelta(seconds=seconds) - return dt is None or (now - dt) > timedelta(seconds=seconds) - return False +_STRING_OPS = { + "is": lambda value, query: value == query, + "is_not": lambda value, query: value != query, + "contains": lambda value, query: query in value, + "not_contains": lambda value, query: query not in value, + "starts_with": lambda value, query: value.startswith(query), + "ends_with": lambda value, query: value.endswith(query), +} def _rule_window_seconds(rule: "SmartRule") -> float: @@ -368,31 +302,159 @@ def _rule_window_seconds(rule: "SmartRule") -> float: return count * _UNIT_SECONDS.get(rule.unit or "days", 86400) -def _match_rule(rule: SmartRule, track, now: datetime) -> bool: +# --------------------------------------------------------------------------- # +# Rule compilation +# +# Each rule becomes a closure once per evaluate() instead of being re-dispatched +# per track. That hoists everything that depends only on the rule — the operator +# lookup, casefolding the query, parsing the rule's own date constants — out of +# a loop that runs 21k+ times per smart playlist. +# --------------------------------------------------------------------------- # + +_ALWAYS_FALSE = lambda track: False # noqa: E731 +_ALWAYS_TRUE = lambda track: True # noqa: E731 + + +def _compile_string(attr, op, value): + compare = _STRING_OPS.get(op) + if compare is None: + return _ALWAYS_FALSE + query = ("" if value is None else str(value)).casefold() + + def matches(track): + track_val = getattr(track, attr, None) + text = ("" if track_val is None else str(track_val)).casefold() + return compare(text, query) + return matches + + +def _compile_numeric(attr, op, value, value2): + try: + query = float(value or 0) + except (ValueError, TypeError): + return _ALWAYS_FALSE + if op == "in_range": + try: + query2 = float(value2 or 0) + except (ValueError, TypeError): + return _ALWAYS_FALSE + low, high = (query, query2) if query <= query2 else (query2, query) + + def in_range(track): + try: + return low <= float(getattr(track, attr, None) or 0) <= high + except (ValueError, TypeError): + return False + return in_range + + compare = { + "is": lambda t: t == query, + "is_not": lambda t: t != query, + "greater": lambda t: t > query, + "less": lambda t: t < query, + }.get(op) + if compare is None: + return _ALWAYS_FALSE + + def matches(track): + try: + return compare(float(getattr(track, attr, None) or 0)) + except (ValueError, TypeError): + return False + return matches + + +def _compile_date(attr, op, rule, now): + if op in ("after", "before", "in_range"): + # iTunes treats a null date (never played/skipped/etc.) as the distant + # past: "before X" matches it; "after"/"in range" don't. + if op == "in_range": + low, high = _parse_dt(rule.value), _parse_dt(rule.value2) + if low is None or high is None: + return _ALWAYS_FALSE + if low > high: + low, high = high, low + return lambda track: ( + low <= (_parse_dt(getattr(track, attr, None)) or datetime.min) + <= high) + query = _parse_dt(rule.value) + if query is None: + return _ALWAYS_FALSE + if op == "after": + return lambda track: ( + _parse_dt(getattr(track, attr, None)) or datetime.min) > query + return lambda track: ( + _parse_dt(getattr(track, attr, None)) or datetime.min) < query + + if op == "not_in_range": + low, high = _parse_dt(rule.value), _parse_dt(rule.value2) + if low is None or high is None: + return _ALWAYS_TRUE + if low > high: + low, high = high, low + + def not_in_range(track): + parsed = _parse_dt(getattr(track, attr, None)) + return parsed is None or not low <= parsed <= high + return not_in_range + + if op in ("in_last", "not_in_last"): + window = timedelta(seconds=_rule_window_seconds(rule)) + if op == "in_last": + def in_last(track): + parsed = _parse_dt(getattr(track, attr, None)) + return parsed is not None and (now - parsed) <= window + return in_last + + def not_in_last(track): + parsed = _parse_dt(getattr(track, attr, None)) + return parsed is None or (now - parsed) > window + return not_in_last + + return _ALWAYS_FALSE + + +def _compile_rule(rule: SmartRule, now: datetime): meta = FIELD_REGISTRY.get(rule.field) if meta is None: - return False - track_val = getattr(track, meta.track_attr, None) + return _ALWAYS_FALSE + attr = meta.track_attr if meta.type is FieldType.STRING: - return _match_string(rule.operator, track_val, rule.value) + return _compile_string(attr, rule.operator, rule.value) if meta.type in (FieldType.INT, FieldType.DURATION, FieldType.RATING): - return _match_numeric(rule.operator, track_val, rule.value, rule.value2) + return _compile_numeric(attr, rule.operator, rule.value, rule.value2) if meta.type is FieldType.DATE: - return _match_date(rule.operator, track_val, rule, now) + return _compile_date(attr, rule.operator, rule, now) if meta.type is FieldType.BOOL: - return bool(track_val) == bool(rule.value) - return False + wanted = bool(rule.value) + return lambda track: bool(getattr(track, attr, None)) is wanted + return _ALWAYS_FALSE -def _match_group(group: SmartGroup, track, now: datetime) -> bool: +def _compile_group(group: SmartGroup, now: datetime): if not group.children: - return True - results = ( - _match_group(c, track, now) if isinstance(c, SmartGroup) - else _match_rule(c, track, now) - for c in group.children - ) - return all(results) if group.conjunction == "all" else any(results) + return _ALWAYS_TRUE + children = [ + _compile_group(child, now) if isinstance(child, SmartGroup) + else _compile_rule(child, now) + for child in group.children + ] + if len(children) == 1: + return children[0] + if group.conjunction == "all": + def matches_all(track): + for child in children: + if not child(track): + return False + return True + return matches_all + + def matches_any(track): + for child in children: + if child(track): + return True + return False + return matches_any def _selection_key(selection: str, pid_seed: int): @@ -447,7 +509,8 @@ def evaluate(criteria: SmartCriteria, tracks, now: Optional[datetime] = None, """ if now is None: now = datetime.now(timezone.utc).replace(tzinfo=None) - matched = [t for t in tracks if _match_group(criteria.root, t, now)] + predicate = _compile_group(criteria.root, now) + matched = [t for t in tracks if predicate(t)] if criteria.limit.enabled and criteria.limit.count > 0: matched = _apply_limit(matched, criteria.limit, seed) return sorted(t.track_id for t in matched) diff --git a/lintunes/storage/conflict_resolver.py b/lintunes/storage/conflict_resolver.py index 8584184..a468867 100644 --- a/lintunes/storage/conflict_resolver.py +++ b/lintunes/storage/conflict_resolver.py @@ -10,6 +10,11 @@ from lintunes.storage.json_storage import read_json, write_json # Syncthing conflict file pattern: filename.sync-conflict-20240101-123456-ABCDEFG.json CONFLICT_PATTERN = re.compile(r"^(.+)\.sync-conflict-(\d{8}-\d{6})-\w+(\.\w+)$") +# How many pre-merge snapshots to keep in /.resolved. Each library.json +# merge stores two 15 MB copies, and the folder rides the Syncthing share, so an +# unbounded history is replicated to every machine (it had reached 1.7 GB). +BACKUPS_TO_KEEP = 10 + @dataclass class ConflictSummary: @@ -30,6 +35,7 @@ def resolve_conflicts(data_dir: Path) -> list[ConflictSummary]: list when there was nothing to do).""" if not data_dir.exists(): return [] + _prune_backups(data_dir) conflict_files = _find_conflict_files(data_dir) if not conflict_files: @@ -94,6 +100,20 @@ def restore_backup(backup_dir: Path, data_dir: Path) -> list[str]: return restored +def _prune_backups(data_dir: Path, keep: int = BACKUPS_TO_KEEP): + """Drop all but the newest `keep` snapshots in /.resolved. + + Snapshot directories are named with a sortable timestamp, so newest-last by + name is newest-last by time.""" + root = data_dir / ".resolved" + if not root.is_dir(): + return + snapshots = sorted((d for d in root.iterdir() if d.is_dir()), + key=lambda d: d.name) + for stale in snapshots[:max(0, len(snapshots) - keep)]: + shutil.rmtree(stale, ignore_errors=True) + + def _find_conflict_files(data_dir: Path) -> list[tuple[Path, Path]]: results = [] for path in data_dir.rglob("*.sync-conflict-*"): @@ -140,6 +160,18 @@ def _merge_library(original_path: Path, conflict_path: Path) -> ConflictSummary: return ConflictSummary("library.json", "library", lines) +# Every field _merge_track_fields reads or writes. If two copies of a track +# agree on all of these, merging them is a no-op — which lets callers skip the +# work entirely (see library_manager._reconcile_track). +MERGEABLE_FIELDS = ( + "play_count", "skip_count", "play_date_utc", "skip_date", "date_added", + "loved", "rating", "name", "artist", "album_artist", "album", "genre", + "composer", "grouping", "comments", "sort_name", "sort_artist", + "sort_album_artist", "sort_album", "sort_composer", "location", + "date_modified", +) + + def _merge_track_fields(orig: dict, conflict: dict) -> list[str]: """Merge conflict's track fields into orig in place; return change notes.""" notes = [] diff --git a/lintunes/updater.py b/lintunes/updater.py index e86c7c9..3559f3c 100644 --- a/lintunes/updater.py +++ b/lintunes/updater.py @@ -31,9 +31,23 @@ class Updater(QObject): self._root = str(Path(__file__).resolve().parents[1]) self._busy = False self._timer = None - self.enabled = (self._git("rev-parse", "--show-toplevel") is not None - and self._git("rev-parse", "--abbrev-ref", "@{upstream}") - is not None) + self._enabled = None # probed lazily — see enabled + + @property + def enabled(self) -> bool: + """Whether this checkout is a git repo with an upstream. + + Probed on first use rather than in __init__: the two `git rev-parse` + calls have a 15 s timeout each, and running them during startup put + that stall on the main thread before the window was up. First use is + now `_check`, on a background thread. + """ + if self._enabled is None: + self._enabled = ( + self._git("rev-parse", "--show-toplevel") is not None + and self._git("rev-parse", "--abbrev-ref", "@{upstream}") + is not None) + return self._enabled def short_hash(self) -> str | None: """The checkout's current commit, for telling builds apart.""" @@ -41,8 +55,6 @@ class Updater(QObject): def start_checking(self): """First check shortly after launch, then every few hours.""" - if not self.enabled: - return QTimer.singleShot(FIRST_CHECK_MS, self.check_async) self._timer = QTimer(self) self._timer.setInterval(RECHECK_INTERVAL_MS) @@ -50,7 +62,7 @@ class Updater(QObject): self._timer.start() def check_async(self): - if not self.enabled or self._busy: + if self._busy: return self._busy = True threading.Thread(target=self._check, daemon=True).start() @@ -65,6 +77,8 @@ class Updater(QObject): def _check(self): try: + if not self.enabled: + return if self._git("fetch", "--quiet", timeout=60) is None: return # offline / host unreachable — perfectly normal, stay quiet behind = self._git("rev-list", "--count", "HEAD..@{upstream}") diff --git a/tasks-done.md b/tasks-done.md index 594a6cc..a1bb311 100644 --- a/tasks-done.md +++ b/tasks-done.md @@ -1,5 +1,51 @@ ## Done +### Round 35 (2026-08-19) — Startup speed: the 21k-track library stops freezing (v0.7.0) + +Reported as three separate complaints — "startup takes a minute or two and GNOME +offers to force quit", a playlist insert that jumped to the end, and the pile of +merge dialogs — which turned out to share one shape: every operation was +whole-library, whole-file, on the Qt main thread. Measured against the real +library (21,482 tracks, 506 playlists, 15 MB `library.json`). + +- [x] **The track table sorts itself.** `TrackTableView` ran its model through a + `QSortFilterProxyModel`, which asks `data()` for a value on *every + comparison* — 580k Python round trips, **8.5 s per table load**, paid again + on every reload. The model now keeps `_tracks` in canonical (playlist) + order plus an `_order` index list, and sorts a key list computed once per + track. Nothing used the proxy's filtering (the column browser filters by + handing `set_tracks` a shorter list). Sorting by `#` is the identity order, + so "source row" still means "playlist position" for drag-reorder. + **8.5 s → 0.13 s.** `MainWindow()` **18.1 s → 0.79 s.** +- [x] **Smart rules compile to closures once per `evaluate()`** instead of being + re-dispatched per track: the operator lookup, casefolding the query, and + parsing the rule's own date constants all leave the 21k-iteration loop + (`_match_date` was re-parsing its own constant 21,482 times per rule). + Verified against the previous implementation on all 20 real smart + playlists: **identical membership, 0 mismatches**. **2.54 s → 0.72 s.** +- [x] Smart recompute deferred until after the first paint (`main.py`), so the + compositor always has a window that answers. +- [x] `_reconcile_track` skips the two `to_dict()` round trips when a track is + unchanged (`MERGEABLE_FIELDS` in the resolver is now the single source of + truth for what a merge can touch). A sync changes a handful of tracks out + of 21k; this was most of the cost of a reload. +- [x] No git subprocess on the startup path: `Updater.enabled` is probed lazily + on the background check thread (it was two `rev-parse` calls with 15 s + timeouts in `__init__`), and the version button loads its commit-hash + tooltip after the window is up. +- [x] `.resolved/` is pruned to the newest 10 snapshots. It had reached **1.7 GB + across 73 snapshots**, never pruned — and it lives inside the Syncthing + share, so every 15 MB pre-merge copy was being replicated to the other + machine. + +Net: **time-to-interactive-window ~15 s → 3.2 s**, and the mid-session freeze +when Syncthing delivers a change (which is what GNOME was actually offering to +force-quit — a full reload + recompute + table rebuild with the window already +on screen) **~16 s → 3.8 s**. + +Not in this round, deliberately: the merge rework itself (playlist ordering, +per-machine play journal, quieting the dialog) — see TASKS.md. + ### Round 34 (2026-08-15) — The parking brake: stop playing audio at 3am (v0.6.1) The long-running "LinTunes plays by itself" haunting, finally attributed and diff --git a/tests/test_round12.py b/tests/test_round12.py index 1f4153b..83dc73f 100644 --- a/tests/test_round12.py +++ b/tests/test_round12.py @@ -24,8 +24,7 @@ def _manager(tmp_path, n=4): def _select_source_row(table, source_row): """Select the visual row backing a given source-model row (sort-proof).""" - proxy_index = table.proxy.mapFromSource(table.model_.index(source_row, 0)) - table.selectRow(proxy_index.row()) + table.selectRow(table.model_.display_row(source_row)) # -------------------------------------------------------------------------- diff --git a/tests/test_round17.py b/tests/test_round17.py index 992685f..74d7c6d 100644 --- a/tests/test_round17.py +++ b/tests/test_round17.py @@ -330,7 +330,7 @@ class TestRatingClicks: emitted = [] view.rating_edited.connect(lambda tid, r: emitted.append((tid, r))) col = view.model_.fields.index("rating") - rect = view.visualRect(view.proxy.index(0, col)) + rect = view.visualRect(view.model_.index(0, col)) slot_w = rating_slot_width(QFontMetrics(view.font())) # Click the 4th slot → 80. x = rect.left() + RATING_LEFT_PAD + int(slot_w * 3.5) @@ -356,10 +356,10 @@ class TestRatingClicks: track = Track(track_id=1, name="A", rating=40) view = self._view(qapp, [track]) col = view.model_.fields.index("rating") - index = view.proxy.index(0, col) + index = view.model_.index(0, col) view._set_rating_hover(index) assert view.is_rating_hovered(index) - assert not view.is_rating_hovered(view.proxy.index(0, 0)) + assert not view.is_rating_hovered(view.model_.index(0, 0)) view._set_rating_hover(None) assert not view.is_rating_hovered(index) diff --git a/tests/test_round19.py b/tests/test_round19.py index 08bf2ad..3fd9b46 100644 --- a/tests/test_round19.py +++ b/tests/test_round19.py @@ -134,9 +134,12 @@ def button(qapp): return VersionButton(fake), fake -def test_button_shows_version_and_hash(button): +def test_button_shows_version_and_hash(qapp, button): btn, fake = button assert btn.text() == f"v{__version__}" + # The commit hash is filled in just after construction (short_hash() shells + # out to git, so it stays off the startup path). + qapp.processEvents() assert "abc1234" in btn.toolTip() diff --git a/tests/test_round35.py b/tests/test_round35.py new file mode 100644 index 0000000..501e5e0 --- /dev/null +++ b/tests/test_round35.py @@ -0,0 +1,231 @@ +"""Round 35: startup speed. + +The track table used to sort through a QSortFilterProxyModel, which called +data() once per comparison — 8.5 s to show a 21k-track library. It now sorts +itself over a precomputed key list. Smart-playlist rules are compiled to +closures once per evaluate() instead of being re-dispatched per track, and +.resolved snapshots are pruned instead of growing without bound. +""" +from datetime import datetime +from pathlib import Path + +import pytest +from PyQt6.QtCore import Qt + +from lintunes.models import Track +from lintunes.gui.track_table import TrackTableView, INDEX_FIELD + + +def _tracks(rows): + return [Track(track_id=tid, name=name, artist=artist, play_count=plays) + for tid, name, artist, plays in rows] + + +@pytest.fixture +def view(qapp): + table = TrackTableView() + table.model_.set_visible_columns(["name", "artist", "play_count"]) + return table + + +# ---- in-model sorting ------------------------------------------------------- + +class TestSorting: + ROWS = [(1, "Ship", "Zoe", 3), + (2, "Black Blood", "adele", 10), + (3, "Thunder Peel", "Bob", 10), + (4, "Sev Beni", None, 1)] + + def test_sorts_case_insensitively(self, view): + view.set_tracks(_tracks(self.ROWS)) + view.sortByColumn(view.model_.fields.index("artist"), + Qt.SortOrder.AscendingOrder) + # None sorts with the empty string, then adele < Bob < Zoe (casefolded: + # a plain str sort would have put "Bob" and "Zoe" before "adele"). + assert view.view_order_track_ids() == [4, 2, 3, 1] + + def test_descending_reverses(self, view): + view.set_tracks(_tracks(self.ROWS)) + view.sortByColumn(view.model_.fields.index("artist"), + Qt.SortOrder.DescendingOrder) + assert view.view_order_track_ids() == [1, 3, 2, 4] + + def test_sort_is_stable_within_ties(self, view): + view.set_tracks(_tracks(self.ROWS)) + view.sortByColumn(view.model_.fields.index("play_count"), + Qt.SortOrder.AscendingOrder) + # 2 and 3 both have 10 plays and keep their given order. + assert view.view_order_track_ids() == [4, 1, 2, 3] + + def test_mixed_none_and_numbers_do_not_raise(self, view): + view.set_tracks([Track(track_id=1, name="A", year=None), + Track(track_id=2, name="B", year=1999)]) + view.model_.set_visible_columns(["name", "year"]) + view.sortByColumn(view.model_.fields.index("year"), + Qt.SortOrder.AscendingOrder) + assert view.view_order_track_ids() == [1, 2] + + def test_set_tracks_keeps_the_current_sort(self, view): + view.set_tracks(_tracks(self.ROWS)) + view.sortByColumn(view.model_.fields.index("artist"), + Qt.SortOrder.AscendingOrder) + view.set_tracks(_tracks(self.ROWS[:2])) + assert view.view_order_track_ids() == [2, 1] + + def test_selection_survives_a_re_sort(self, view): + view.set_tracks(_tracks(self.ROWS)) + view.sortByColumn(view.model_.fields.index("artist"), + Qt.SortOrder.AscendingOrder) + view.selectRow(0) # track 4 + assert view.selected_track_ids() == [4] + view.sortByColumn(view.model_.fields.index("artist"), + Qt.SortOrder.DescendingOrder) + assert view.selected_track_ids() == [4] + + +class TestManualOrder: + """A playlist's canonical order must survive sorting: '#' restores it, and + 'source rows' stay playlist positions so drag-reorder targets the right + tracks.""" + + ROWS = [(1, "C", "c", 0), (2, "A", "a", 0), (3, "B", "b", 0)] + + @pytest.fixture + def playlist_view(self, qapp): + table = TrackTableView(playlist_mode=True) + table.model_.set_visible_columns(["name"]) + table.set_tracks(_tracks(self.ROWS)) + return table + + def test_index_column_restores_manual_order(self, playlist_view): + playlist_view.sortByColumn(playlist_view.model_.fields.index("name"), + Qt.SortOrder.AscendingOrder) + assert playlist_view.view_order_track_ids() == [2, 3, 1] + playlist_view.sortByColumn( + playlist_view.model_.fields.index(INDEX_FIELD), + Qt.SortOrder.AscendingOrder) + assert playlist_view.view_order_track_ids() == [1, 2, 3] + assert playlist_view.is_manual_sort() + + def test_selection_maps_to_canonical_rows_while_sorted(self, playlist_view): + playlist_view.sortByColumn(playlist_view.model_.fields.index("name"), + Qt.SortOrder.AscendingOrder) + playlist_view.selectRow(0) # displays "A" = track 2, playlist row 1 + assert playlist_view.selected_source_rows() == [1] + assert playlist_view.selected_track_ids() == [2] + + def test_index_column_numbers_the_displayed_rows(self, playlist_view): + playlist_view.sortByColumn(playlist_view.model_.fields.index("name"), + Qt.SortOrder.AscendingOrder) + model = playlist_view.model_ + col = model.fields.index(INDEX_FIELD) + assert [model.data(model.index(row, col)) for row in range(3)] == [1, 2, 3] + + def test_reveal_track_finds_a_sorted_row(self, playlist_view): + playlist_view.sortByColumn(playlist_view.model_.fields.index("name"), + Qt.SortOrder.AscendingOrder) + assert playlist_view.reveal_track(1) # "C", last when sorted + assert playlist_view.selected_track_ids() == [1] + + +# ---- compiled smart rules --------------------------------------------------- + +class TestSmartCompilation: + """The rewrite is only safe if it matches the old evaluator exactly.""" + + def _criteria(self, rules, conjunction="all"): + from lintunes.smart import SmartCriteria, SmartGroup, SmartRule + return SmartCriteria(root=SmartGroup( + conjunction=conjunction, + children=[SmartRule(**r) for r in rules])) + + def test_string_match_is_case_insensitive(self): + from lintunes import smart + tracks = [Track(track_id=1, name="A", artist="Björk"), + Track(track_id=2, name="B", artist="bjork")] + criteria = self._criteria( + [dict(field="artist", operator="contains", value="BJÖ")]) + assert smart.evaluate(criteria, tracks) == [1] + + def test_date_rule_constant_is_parsed_once_not_per_track(self): + from lintunes import smart + tracks = [Track(track_id=1, name="A", play_date_utc="2026-01-01T00:00:00"), + Track(track_id=2, name="B", play_date_utc="2020-01-01T00:00:00"), + Track(track_id=3, name="C", play_date_utc=None)] + criteria = self._criteria([dict(field="last_played", operator="after", + value="2025-01-01T00:00:00")]) + assert smart.evaluate(criteria, tracks) == [1] + # A null date is the distant past: "before" matches it, "after" doesn't. + criteria = self._criteria([dict(field="last_played", operator="before", + value="2025-01-01T00:00:00")]) + assert smart.evaluate(criteria, tracks) == [2, 3] + + def test_unparseable_date_constant_matches_nothing(self): + from lintunes import smart + tracks = [Track(track_id=1, name="A", play_date_utc="2026-01-01T00:00:00")] + criteria = self._criteria([dict(field="last_played", operator="after", + value="not a date")]) + assert smart.evaluate(criteria, tracks) == [] + + def test_in_last_window(self): + from lintunes import smart + now = datetime(2026, 8, 19, 12, 0, 0) + tracks = [Track(track_id=1, name="A", play_date_utc="2026-08-18T12:00:00"), + Track(track_id=2, name="B", play_date_utc="2026-01-01T00:00:00"), + Track(track_id=3, name="C", play_date_utc=None)] + criteria = self._criteria([dict(field="last_played", operator="in_last", + value="7", unit="days")]) + assert smart.evaluate(criteria, tracks, now=now) == [1] + criteria = self._criteria([dict(field="last_played", + operator="not_in_last", + value="7", unit="days")]) + assert smart.evaluate(criteria, tracks, now=now) == [2, 3] + + def test_any_conjunction(self): + from lintunes import smart + tracks = [Track(track_id=1, name="A", play_count=50), + Track(track_id=2, name="B", artist="Zoe"), + Track(track_id=3, name="C")] + criteria = self._criteria( + [dict(field="play_count", operator="greater", value="10"), + dict(field="artist", operator="is", value="Zoe")], + conjunction="any") + assert smart.evaluate(criteria, tracks) == [1, 2] + + def test_numeric_range_and_bad_values(self): + from lintunes import smart + tracks = [Track(track_id=1, name="A", play_count=5), + Track(track_id=2, name="B", play_count=50)] + criteria = self._criteria([dict(field="play_count", + operator="in_range", + value="40", value2="60")]) + assert smart.evaluate(criteria, tracks) == [2] + criteria = self._criteria([dict(field="play_count", operator="greater", + value="not a number")]) + assert smart.evaluate(criteria, tracks) == [] + + +# ---- .resolved pruning ------------------------------------------------------ + +class TestBackupPruning: + def test_keeps_only_the_newest_snapshots(self, tmp_path): + from lintunes.storage import conflict_resolver + root = tmp_path / ".resolved" + for stamp in ("20260101-000000", "20260102-000000", "20260103-000000"): + (root / stamp / "original").mkdir(parents=True) + (root / stamp / "original" / "library.json").write_text("{}") + conflict_resolver._prune_backups(tmp_path, keep=2) + assert sorted(d.name for d in root.iterdir()) == ["20260102-000000", + "20260103-000000"] + + def test_prune_runs_even_with_no_conflicts(self, tmp_path): + from lintunes.storage import conflict_resolver + root = tmp_path / ".resolved" + for i in range(conflict_resolver.BACKUPS_TO_KEEP + 3): + (root / f"2026010{i // 10}-00000{i % 10}").mkdir(parents=True) + assert conflict_resolver.resolve_conflicts(tmp_path) == [] + assert len(list(root.iterdir())) == conflict_resolver.BACKUPS_TO_KEEP + + def test_missing_backup_dir_is_fine(self, tmp_path): + from lintunes.storage import conflict_resolver + conflict_resolver._prune_backups(tmp_path) # must not raise