From 521db2f81da7a7a37818cf6755ea6ac78336b8f7 Mon Sep 17 00:00:00 2001 From: trav Date: Tue, 25 Aug 2026 14:50:32 -0400 Subject: [PATCH] v0.12.0: the merge report stops reporting things you didn't do MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "unplayed", "missed and never skipped" and "top 100 in past year" showed up in the merge window constantly, and nobody had touched their rules. A live smart playlist's membership is derived, but it was being persisted — and recompute_smart_playlist rewrites it (and bumped date_modified) every time a play count moves. Both machines did that against the same file after every song, so Syncthing conflicted on a list the next load throws away and rebuilds anyway. Membership now stays in memory: save_playlist writes track_ids: [] for a live smart playlist, and the recompute passes touch=False so it marks nothing dirty and moves no timestamp. live_update=False and unsupported criteria are unchanged — their track_ids are a snapshot, which is real content. Since _mark_playlist also dirties the metadata, library_metadata.json stops being rewritten every song too. The rest was presentation. Every summary read at the same weight, so someone resizing a column on the other machine popped and raised the same window as a 21-track reconciliation, described as "Library columns/settings taken from the most recently edited copy." Summaries now carry a level — WARNING for a merge that couldn't resolve cleanly or discarded a side, CHANGE for real content, INFO for cosmetic or derived — and the dialog has a Show: selector that filters to one level and above. It opens at the highest level in the batch, so nothing routine steals focus and a blank window can't happen; a later warning always pulls the view back up to it. The wording says what happened instead of how the merge works: a metadata merge names the keys that actually differed, and adopting the other machine's music folder grades CHANGE rather than INFO, because that one is a setting somebody chose. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Y9ZEFi4qNJ39FMiBtiAxy2 --- CLAUDE.md | 18 +- lintunes/__init__.py | 2 +- lintunes/gui/conflict_dialog.py | 98 ++++++- lintunes/gui/main_window.py | 23 +- lintunes/library_manager.py | 20 +- lintunes/models/playlist.py | 15 ++ lintunes/storage/conflict_resolver.py | 113 +++++++- lintunes/storage/json_storage.py | 10 +- tests/test_round42.py | 367 ++++++++++++++++++++++++++ 9 files changed, 631 insertions(+), 35 deletions(-) create mode 100644 tests/test_round42.py diff --git a/CLAUDE.md b/CLAUDE.md index b00fe40..263f76b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -64,7 +64,13 @@ persistence) → GUI (Qt widgets that read the manager and connect to its signal `Playlist.date_modified` — bumped *only* in `LibraryManager._set_track_ids` — not by the file's mtime, which moves for cosmetic reasons. `library_manager._reconcile_playlist` uses the same helper - for the live-reload path. + for the live-reload path. Since Round 42 every `ConflictSummary` carries a + **`level`** (`WARNING` / `CHANGE` / `INFO`): a merge that only reconciled + column widths, or a smart playlist whose rules are byte-identical on both + sides, is `INFO` and must never read as an edit the user made. The dialog + (`gui/conflict_dialog.py`) shows one level *and above* and opens at the + highest level in the batch, so a routine merge never steals focus but a + blank window is impossible either. - **`lintunes/storage/play_journal.py`** — why play counts can't conflict. Since Round 38 `library.json` holds only a **base** count and each machine owns @@ -233,6 +239,16 @@ persistence) → GUI (Qt widgets that read the manager and connect to its signal change; `track_table._on_section_resized` ignores that section, or resizing the window would rewrite the open playlist's JSON (and hand Syncthing a conflict) purely for a width `apply_settings` overrides on load anyway. + Round 42 is the same rule one level down: a **live smart playlist's membership + is never persisted** (`Playlist.has_derived_membership` gates it in + `json_storage.save_playlist`, which writes `track_ids: []` + + `derived_membership: true`) and `recompute_smart_playlist` passes + `touch=False` to `_set_track_ids` so it marks nothing dirty and moves no + timestamp. Membership is rebuilt from the criteria on every load + (`recompute_all_smart`), so storing it only bought a conflict per song — one + per finished track, on the same file, from both machines. The exceptions are + `live_update=False` and `unsupported` criteria: their `track_ids` *are* the + content (a snapshot), so they still persist and still count as edits. - **Qt/Wayland gotchas (GNOME/Mutter):** `QDrag.setPixmap` / `setDragCursor` / `QCursor.pos()` are unreliable during a drag — `gui/drag_ghost.py` paints its diff --git a/lintunes/__init__.py b/lintunes/__init__.py index a7e0a18..cdecade 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.11.1" +__version__ = "0.12.0" diff --git a/lintunes/gui/conflict_dialog.py b/lintunes/gui/conflict_dialog.py index 836c5c0..7bf884f 100644 --- a/lintunes/gui/conflict_dialog.py +++ b/lintunes/gui/conflict_dialog.py @@ -4,6 +4,12 @@ option to open or restore the pre-merge backup. One dialog per session, not one per merge: Syncthing can deliver conflicts every few minutes, so ``add_event`` folds each new merge into the open window as a timestamped entry instead of stacking another window on the desktop. + +Entries are graded (see conflict_resolver's WARNING/CHANGE/INFO) and the window +shows one level and above. It opens at whatever the *highest* level in the batch +is, so there is always something to read: a batch of nothing but column-width +merges opens on the details view rather than blank, and a real divergence is +never hidden behind a filter the user left set to something quieter. """ from datetime import datetime from pathlib import Path @@ -11,8 +17,11 @@ from pathlib import Path from PyQt6.QtCore import QUrl from PyQt6.QtGui import QDesktopServices from PyQt6.QtWidgets import ( - QDialog, QVBoxLayout, QHBoxLayout, QLabel, QTextEdit, QPushButton, - QMessageBox) + QDialog, QVBoxLayout, QHBoxLayout, QComboBox, QLabel, QTextEdit, + QPushButton, QMessageBox) + +from lintunes.storage.conflict_resolver import ( + WARNING, CHANGE, INFO, LEVEL_ORDER, at_least, max_level) INTRO = ("LinTunes found changes made on more than one machine and merged them " @@ -20,12 +29,23 @@ INTRO = ("LinTunes found changes made on more than one machine and merged them " "versions were backed up first — restore them if a merge isn't what " "you wanted.") +# Coarsest first, so the combo reads top-down like a volume knob. +LEVEL_CHOICES = [ + (WARNING, "Warnings only"), + (CHANGE, "Changes and warnings"), + (INFO, "Everything"), +] + +MARKERS = {WARNING: "!", CHANGE: "●", INFO: "·"} + class ConflictSummaryDialog(QDialog): def __init__(self, summaries, manager, parent=None): super().__init__(parent) self._manager = manager self._events = [] # newest last: (datetime, list[ConflictSummary]) + self._level = INFO + self._user_picked = False self.setWindowTitle("Synced changes merged") self.resize(560, 440) @@ -35,6 +55,16 @@ class ConflictSummaryDialog(QDialog): self._intro.setWordWrap(True) layout.addWidget(self._intro) + filter_row = QHBoxLayout() + filter_row.addWidget(QLabel("Show:")) + self._level_box = QComboBox() + for level, label in LEVEL_CHOICES: + self._level_box.addItem(label, level) + self._level_box.activated.connect(self._on_level_picked) + filter_row.addWidget(self._level_box) + filter_row.addStretch(1) + layout.addLayout(filter_row) + self._body = QTextEdit() self._body.setReadOnly(True) layout.addWidget(self._body, 1) @@ -57,9 +87,49 @@ class ConflictSummaryDialog(QDialog): # ---- events ---- - def add_event(self, summaries, when=None): - """Fold another merge into this window as its own timestamped entry.""" - self._events.append((when or datetime.now(), list(summaries or []))) + def add_event(self, summaries, when=None) -> bool: + """Fold another merge into this window as its own timestamped entry. + + Returns True if the new batch has anything to show at the level now on + screen — the caller uses that to decide whether to raise the window, so + a column resize on the other machine never steals focus.""" + summaries = list(summaries or []) + self._events.append((when or datetime.now(), summaries)) + self._set_level(self._level_for(summaries)) + self._refresh() + return any(at_least(s.level, self._level) for s in summaries) + + @property + def level(self) -> str: + return self._level + + def _level_for(self, incoming) -> str: + """Which level to display now that ``incoming`` has arrived. + + Untouched by the user, the view follows the most serious thing seen so + far. Once the user has picked a level, their choice stands — except that + a batch more serious than what they are looking at pulls the view back + up to it.""" + incoming_max = max_level(incoming) + if not self._user_picked: + return max( + (max_level(s) for _when, s in self._events), + key=lambda lvl: LEVEL_ORDER.get(lvl, 0), default=INFO) + if LEVEL_ORDER.get(incoming_max, 0) > LEVEL_ORDER.get(self._level, 0): + return incoming_max + return self._level + + def _set_level(self, level: str): + self._level = level + index = self._level_box.findData(level) + if index >= 0: + self._level_box.blockSignals(True) + self._level_box.setCurrentIndex(index) + self._level_box.blockSignals(False) + + def _on_level_picked(self, index: int): + self._user_picked = True + self._level = self._level_box.itemData(index) self._refresh() @property @@ -85,16 +155,30 @@ class ConflictSummaryDialog(QDialog): def _format(self) -> str: blocks = [] + hidden = 0 + shown = 0 multi = len(self._events) > 1 for when, summaries in reversed(self._events): # newest first + visible = [s for s in summaries if at_least(s.level, self._level)] + hidden += len(summaries) - len(visible) + shown += len(visible) + if not visible: + continue # no orphan timestamp headers for a filtered-out merge lines = [] if multi: lines.append(f"── {when.strftime('%-I:%M %p')} " + "─" * 30) - for s in summaries: - lines.append(f"● {s.file}") + for s in visible: + lines.append(f"{MARKERS.get(s.level, '●')} {s.file}") lines.extend(f" {line}" for line in s.lines) lines.append("") blocks.append("\n".join(lines).rstrip()) + + if not shown: + blocks.append("Nothing at this level.") + if hidden: + entries = "entry" if hidden == 1 else "entries" + blocks.append(f"({hidden} routine {entries} hidden — switch to " + f"“Everything” to see them.)") return "\n\n".join(blocks) # ---- actions ---- diff --git a/lintunes/gui/main_window.py b/lintunes/gui/main_window.py index b1f19cd..cfe7f06 100644 --- a/lintunes/gui/main_window.py +++ b/lintunes/gui/main_window.py @@ -751,22 +751,27 @@ class MainWindow(QMainWindow): def _on_conflict_resolved(self, summaries): self.statusBar().showMessage( f"Merged changes from your other machine " - f"({len(summaries)} item(s)) — see details", 8000) + f"({len(summaries)} item(s))", 8000) self.show_conflict_summary(summaries) def show_conflict_summary(self, summaries): """Show (or extend) the one merge window. Syncthing can deliver conflicts every few minutes; each merge becomes another entry in the open window - rather than another window.""" + rather than another window. + + Only raise it when the new batch has something to say at the level on + screen — otherwise the entry is folded in quietly and is there when the + user dials the filter down.""" from lintunes.gui.conflict_dialog import ConflictSummaryDialog if self._conflict_dialog is not None: - self._conflict_dialog.add_event(summaries) - else: - # Modeless so a merge never blocks playback or what you're doing. - self._conflict_dialog = ConflictSummaryDialog( - summaries, self._manager, self) - self._conflict_dialog.finished.connect(self._on_conflict_dialog_closed) - self._conflict_dialog.show() + if self._conflict_dialog.add_event(summaries): + self._conflict_dialog.raise_() + return + # Modeless so a merge never blocks playback or what you're doing. + self._conflict_dialog = ConflictSummaryDialog( + summaries, self._manager, self) + self._conflict_dialog.finished.connect(self._on_conflict_dialog_closed) + self._conflict_dialog.show() self._conflict_dialog.raise_() def _on_conflict_dialog_closed(self, _result): diff --git a/lintunes/library_manager.py b/lintunes/library_manager.py index cedc93e..c5067bd 100644 --- a/lintunes/library_manager.py +++ b/lintunes/library_manager.py @@ -317,17 +317,23 @@ class LibraryManager(QObject): self._set_track_ids(pid, after) self._push_track_ids("Reorder Playlist", pid, before, after) - def _set_track_ids(self, pid: str, ids: list[int]): + def _set_track_ids(self, pid: str, ids: list[int], touch: bool = True): + """Set a playlist's membership. ``touch=False`` means "this wasn't an + edit": no timestamp, no dirty mark, just the signal — used by the smart + recompute, whose result isn't persisted.""" playlist = self.library.playlists.get(pid) if playlist is None: return playlist.track_ids = list(ids) # The single funnel for every content change (add / remove / reorder / # undo), and deliberately the *only* place date_modified moves — a - # column resize must not look like an edit to the merge. See - # conflict_resolver._playlist_newer. - playlist.date_modified = _utc_now_iso() - self._mark_playlist(pid) + # column resize must not look like an edit to the merge. Nor may a smart + # recompute: it fires every time a play count moves, and bumping the + # timestamp there made "most recently edited" meaningless for smart + # playlists. See conflict_resolver._playlist_newer. + if touch: + playlist.date_modified = _utc_now_iso() + self._mark_playlist(pid) self.playlist_content_changed.emit(pid) def _push_track_ids(self, label, pid, before, after): @@ -813,7 +819,9 @@ class LibraryManager(QObject): ids = smart.evaluate(criteria, self.library.tracks.values(), seed=seed) if ids == playlist.track_ids: return False - self._set_track_ids(pid, ids) # marks dirty + emits; never undoable + # Never undoable. For a derived playlist nothing is written or timestamped + # either — the membership lives in memory and is rebuilt on the next load. + self._set_track_ids(pid, ids, touch=not playlist.has_derived_membership) return True def recompute_all_smart(self, force: bool = False) -> list[str]: diff --git a/lintunes/models/playlist.py b/lintunes/models/playlist.py index 6cc0972..73c200a 100644 --- a/lintunes/models/playlist.py +++ b/lintunes/models/playlist.py @@ -71,6 +71,21 @@ class Playlist: def is_smart(self) -> bool: return self.playlist_type == PlaylistType.SMART + @property + def has_derived_membership(self) -> bool: + """True when track_ids are rebuilt from the criteria on every load, so + they are in-memory state rather than something to persist or merge. + + A live smart playlist's membership moves every time a play count does. + Persisting it meant both machines rewrote the same file continuously and + Syncthing conflicted on it once a song — for a list the next load throws + away and recomputes anyway. Non-live and `unsupported` criteria are the + exception: their track_ids *are* the content (a snapshot), so they keep + being stored.""" + c = self.smart_criteria + return bool(self.is_smart and c is not None and not c.unsupported + and c.live_update) + def to_dict(self) -> dict: d = { "name": self.name, diff --git a/lintunes/storage/conflict_resolver.py b/lintunes/storage/conflict_resolver.py index cb82ca6..e6345eb 100644 --- a/lintunes/storage/conflict_resolver.py +++ b/lintunes/storage/conflict_resolver.py @@ -16,6 +16,25 @@ CONFLICT_PATTERN = re.compile(r"^(.+)\.sync-conflict-(\d{8}-\d{6})-\w+(\.\w+)$") BACKUPS_TO_KEEP = 10 +# How much a merge is worth interrupting for. Most conflicts reconcile something +# nobody typed — a column width, a derived list — and burying a real divergence +# under twenty of those is how the merge window became wallpaper. +WARNING = "warning" # couldn't merge cleanly, or one side was discarded +CHANGE = "change" # real content reconciled: playlist order, track fields +INFO = "info" # cosmetic or derived; nothing you did, nothing you lost +LEVEL_ORDER = {INFO: 0, CHANGE: 1, WARNING: 2} + + +def max_level(summaries) -> str: + """The most serious level in a batch (INFO when it's empty).""" + return max((s.level for s in summaries), + key=lambda lvl: LEVEL_ORDER.get(lvl, 0), default=INFO) + + +def at_least(level: str, minimum: str) -> bool: + return LEVEL_ORDER.get(level, 0) >= LEVEL_ORDER.get(minimum, 0) + + @dataclass class ConflictSummary: """A human-readable record of one merged Syncthing conflict, surfaced to the @@ -24,6 +43,7 @@ class ConflictSummary: kind: str # "library" | "playlist" | "metadata" | "other" lines: list[str] = field(default_factory=list) # what was reconciled backup_dir: str = "" # /.resolved/ + level: str = CHANGE # WARNING | CHANGE | INFO def resolve_conflicts(data_dir: Path) -> list[ConflictSummary]: @@ -59,7 +79,8 @@ def resolve_conflicts(data_dir: Path) -> list[ConflictSummary]: summary = ConflictSummary( str(rel), "other", ["Deleted here but edited elsewhere — kept the deletion; " - "the other copy is in the backup."]) + "the other copy is in the backup."], + level=WARNING) elif original_path.name == "library.json": summary = _merge_library(original_path, conflict_path) elif original_path.name == "library_metadata.json": @@ -74,7 +95,8 @@ def resolve_conflicts(data_dir: Path) -> list[ConflictSummary]: summary = ConflictSummary( str(rel), "other", [f"Could not merge automatically ({e}); both versions are in the " - "backup, current copy left as-is."]) + "backup, current copy left as-is."], + level=WARNING) summary.backup_dir = str(backup_dir) summaries.append(summary) @@ -159,7 +181,8 @@ def _merge_library(original_path: Path, conflict_path: Path) -> ConflictSummary: if not lines: lines.append("No differences needed reconciling.") lines.extend(examples) - return ConflictSummary("library.json", "library", lines) + return ConflictSummary("library.json", "library", lines, + level=CHANGE if (changed or added) else INFO) # Every field _merge_track_fields reads or writes. If two copies of a track @@ -292,9 +315,22 @@ def _merge_playlist(original_path: Path, conflict_path: Path) -> ConflictSummary if original.get("playlist_type") == "smart" or conflict.get("playlist_type") == "smart": if conflict_newer: write_json(original_path, conflict) + # Almost always the rules are identical on both sides and only the + # derived membership drifted apart — which is not something the user did + # and not something the merge decides, since the next recompute rebuilds + # it either way. Saying "rules taken from the most recently edited copy" + # for that read as an edit nobody made. + if original.get("smart_criteria") == conflict.get("smart_criteria"): + return ConflictSummary( + name, "playlist", + ["Rules are identical on both machines — only the auto-updated " + "membership differed. Rebuilt from the rules."], + level=INFO) + src = "the other machine" if conflict_newer else "this machine" return ConflictSummary( name, "playlist", - ["Smart-playlist rules taken from the most recently edited copy."]) + [f"Rules taken from {src} (edited more recently)."], + level=CHANGE) orig_ids = list(original.get("track_ids", [])) conf_ids = list(conflict.get("track_ids", [])) @@ -318,11 +354,21 @@ def _merge_playlist(original_path: Path, conflict_path: Path) -> ConflictSummary write_json(original_path, original) added = len(set(original["track_ids"]) - orig_set) + if orig_ids == conf_ids: + # The two copies held the same tracks in the same order, so they differed + # only in column widths or sort order — a window resize, not an edit. + # Note this is a test on the *inputs*: a merge whose result happens to + # equal our list still discarded the other machine's ordering, and that + # is a decision worth reporting. + return ConflictSummary( + name, "playlist", + ["Only the column layout differed; track order unchanged."], + level=INFO) lines = [f"Order kept from {order_src} (most recently edited)."] if added: lines.append(f"{added} track(s) that were only in the other copy were kept, " "back in position (nothing is removed on a merge).") - return ConflictSummary(name, "playlist", lines) + return ConflictSummary(name, "playlist", lines, level=CHANGE) def _merge_play_journal(original_path: Path, conflict_path: Path) -> ConflictSummary: @@ -350,16 +396,39 @@ def _merge_play_journal(original_path: Path, conflict_path: Path) -> ConflictSum changed += 1 merged[tid] = combined write_json(original_path, merged) + if not changed: + return ConflictSummary(f"plays/{original_path.name}", "plays", + ["No differences needed reconciling."], level=INFO) + # A journal has exactly one writer, so reaching this line at all means + # something unexpected happened to the file — worth saying out loud. return ConflictSummary( f"plays/{original_path.name}", "plays", - [f"{changed} track(s) of play history reconciled (highest total kept)."] - if changed else ["No differences needed reconciling."]) + [f"{changed} track(s) of play history reconciled (highest total kept).", + "A play journal is only ever written by the machine that owns it, so a " + "conflict here is unusual."], + level=WARNING) + + +# What library_metadata.json actually holds, in words a person recognizes. +_METADATA_LABELS = { + "library_settings": "column layout", + "music_folder": "music folder", + "music_folder_rel": "music folder", + "music_folder_set_at": "music folder", + "import_date": "import date", + "track_count": "track count", + "playlist_count": "playlist count", +} _MUSIC_KEYS = ("music_folder", "music_folder_rel", "music_folder_set_at") def _merge_metadata(original_path: Path, conflict_path: Path) -> ConflictSummary: + """Mostly cosmetic, so it grades INFO: this file is the library's column + widths and sort order plus a couple of counts, and someone resizing a column + on the other machine is not news. The music folder is the exception — a + deliberate setting, so adopting the other copy's is a real CHANGE.""" # Read both before _keep_newer overwrites one of them. try: before = read_json(original_path) @@ -368,8 +437,9 @@ def _merge_metadata(original_path: Path, conflict_path: Path) -> ConflictSummary before = other = None summary = _keep_newer(original_path, conflict_path) - summary.file, summary.kind = "library_metadata.json", "metadata" - summary.lines = ["Library columns/settings taken from the most recently edited copy."] + summary.file, summary.kind, summary.level = ( + "library_metadata.json", "metadata", INFO) + summary.lines = [_describe_metadata_diff(before, other)] # The whole-file pick above goes by mtime, which moves for cosmetic reasons # (a column resize rewrites this file). The music folder is a deliberate @@ -387,9 +457,28 @@ def _merge_metadata(original_path: Path, conflict_path: Path) -> ConflictSummary write_json(original_path, kept) summary.lines.append( "Music folder taken from the copy that set it most recently.") + summary.level = CHANGE return summary +def _describe_metadata_diff(original, conflict) -> str: + """Name what actually differed. "Library columns/settings taken from the most + recently edited copy" described the mechanism, not the event, and read like + an edit the user had made.""" + if original is None or conflict is None: + return "Kept the most recently edited copy." + differing = [] + for key in sorted(set(original) | set(conflict)): + if original.get(key) == conflict.get(key): + continue + label = _METADATA_LABELS.get(key, key) + if label not in differing: # the three music_folder keys are one thing + differing.append(label) + if not differing: + return "No differences needed reconciling." + return f"Library {', '.join(differing)} differed; kept the newer copy." + + def _newer_stamp(candidate, current) -> bool: if not candidate: return False @@ -397,12 +486,16 @@ def _newer_stamp(candidate, current) -> bool: def _keep_newer(original_path: Path, conflict_path: Path) -> ConflictSummary: + """Last resort for a file with no merge rule: one side is discarded whole, + so it is always worth flagging.""" took = "this machine" if conflict_path.stat().st_mtime > original_path.stat().st_mtime: shutil.copy2(conflict_path, original_path) took = "the other machine" return ConflictSummary(original_path.name, "other", - [f"Kept the copy from {took} (most recently edited)."]) + [f"Kept the copy from {took} (most recently edited); " + "the other version is in the backup."], + level=WARNING) def _stars(rating) -> str: diff --git a/lintunes/storage/json_storage.py b/lintunes/storage/json_storage.py index 7dfd5fb..86115ce 100644 --- a/lintunes/storage/json_storage.py +++ b/lintunes/storage/json_storage.py @@ -73,7 +73,15 @@ def save_metadata(library: Library, data_dir: Path): def save_playlist(playlist: Playlist, data_dir: Path): playlists_dir = data_dir / "playlists" playlists_dir.mkdir(parents=True, exist_ok=True) - write_json(playlists_dir / f"{playlist.persistent_id}.json", playlist.to_dict()) + data = playlist.to_dict() + if playlist.has_derived_membership: + # Membership is a pure function of the criteria and the library, and + # every machine recomputes it on load — storing it only bought a + # Syncthing conflict per song. Same shape as the base-play-count rule in + # save_tracks: to_dict() stays honest, the gate lives at the boundary. + data["track_ids"] = [] + data["derived_membership"] = True + write_json(playlists_dir / f"{playlist.persistent_id}.json", data) def delete_playlist_file(persistent_id: str, data_dir: Path): diff --git a/tests/test_round42.py b/tests/test_round42.py new file mode 100644 index 0000000..5bdaaa7 --- /dev/null +++ b/tests/test_round42.py @@ -0,0 +1,367 @@ +"""Round 42: the merge report stops shouting, and smart playlists stop +conflicting. + +Two halves of one complaint — the merge window was full of entries for things +nobody did: + +* A live smart playlist's membership is derived from its rules, but it was being + *persisted*, and `recompute_smart_playlist` rewrites it (and bumped + `date_modified`) every time a play count moves. Both machines did that + continuously against the same file, so `unplayed` handed Syncthing a conflict + once a song — for a list the next load throws away and recomputes anyway. +* Every summary was presented at the same weight, so a column resize on the + other machine popped and raised the same window as a 21-track reconciliation. + Summaries are now graded WARNING / CHANGE / INFO and the dialog shows one + level and above, opening at the highest level in the batch. +""" +import json + +import pytest + +from lintunes.library_manager import LibraryManager +from lintunes.models import Library, Playlist, PlaylistType, Track +from lintunes.smart import SmartCriteria, SmartGroup, SmartRule +from lintunes.storage import json_storage, conflict_resolver +from lintunes.storage.conflict_resolver import ( + WARNING, CHANGE, INFO, ConflictSummary, at_least, max_level) + + +def _rock_criteria(**kwargs) -> SmartCriteria: + return SmartCriteria( + root=SmartGroup("all", [SmartRule("genre", "is", "Rock")]), **kwargs) + + +def _library(*genres) -> Library: + lib = Library() + for i, genre in enumerate(genres, start=1): + lib.tracks[i] = Track(track_id=i, name=f"t{i}", genre=genre) + return lib + + +def _conflict_name(original: str) -> str: + stem, _, ext = original.rpartition(".") + return f"{stem}.sync-conflict-20240101-120000-ABCDEFG.{ext}" + + +def _playlist_file(tmp_path, pid="AAAA1111"): + return tmp_path / "playlists" / f"{pid}.json" + + +# --------------------------------------------------------------------------- # +# Derived membership is never persisted +# --------------------------------------------------------------------------- # + +class TestDerivedMembership: + def _saved(self, tmp_path, criteria, track_ids=(1, 2)): + pl = Playlist(name="unplayed", persistent_id="AAAA1111", + playlist_type=PlaylistType.SMART, + track_ids=list(track_ids), smart_criteria=criteria) + lib = Library() + lib.playlists[pl.persistent_id] = pl + json_storage.save_library(lib, tmp_path) + return json.loads(_playlist_file(tmp_path).read_text()) + + def test_live_smart_playlist_stores_no_track_ids(self, tmp_path): + data = self._saved(tmp_path, _rock_criteria()) + assert data["track_ids"] == [] + assert data["derived_membership"] is True + + def test_non_live_smart_playlist_keeps_its_snapshot(self, tmp_path): + data = self._saved(tmp_path, _rock_criteria(live_update=False)) + assert data["track_ids"] == [1, 2] + assert "derived_membership" not in data + + def test_unsupported_criteria_keep_the_imported_snapshot(self, tmp_path): + data = self._saved(tmp_path, _rock_criteria(unsupported=True)) + assert data["track_ids"] == [1, 2] + + def test_regular_playlist_is_untouched(self, tmp_path): + pl = Playlist(name="mix", persistent_id="BBBB2222", + playlist_type=PlaylistType.REGULAR, track_ids=[3, 1]) + lib = Library() + lib.playlists[pl.persistent_id] = pl + json_storage.save_library(lib, tmp_path) + data = json.loads(_playlist_file(tmp_path, "BBBB2222").read_text()) + assert data["track_ids"] == [3, 1] + + def test_membership_survives_a_save_load_roundtrip(self, qapp, tmp_path): + library = _library("Rock", "Jazz", "Rock") + json_storage.save_library(library, tmp_path) # library.json for the reload + mgr = LibraryManager(library, tmp_path) + pl = mgr.create_smart_playlist("rockers", _rock_criteria()) + assert pl.track_ids == [1, 3] + mgr.flush() + + # Nothing on disk, but a fresh start rebuilds it from the rules. + assert json.loads( + _playlist_file(tmp_path, pl.persistent_id).read_text())["track_ids"] == [] + restarted = LibraryManager(json_storage.load_library(tmp_path), tmp_path) + assert restarted.library.playlists[pl.persistent_id].track_ids == [] + restarted.recompute_all_smart() + assert restarted.library.playlists[pl.persistent_id].track_ids == [1, 3] + + +class TestRecomputeIsNotAnEdit: + """A recompute must leave no trace: no dirty file, no timestamp. Bumping + date_modified there made Round 39's "who edited last" tiebreaker meaningless + for every smart playlist.""" + + def _manager(self, tmp_path, criteria): + mgr = LibraryManager(_library("Rock", "Jazz"), tmp_path) + pl = mgr.create_smart_playlist("rockers", criteria) + mgr.flush() + mgr._dirty_playlists.clear() + return mgr, pl + + def test_derived_recompute_leaves_no_trace(self, qapp, tmp_path): + mgr, pl = self._manager(tmp_path, _rock_criteria()) + stamp = pl.date_modified + + mgr.library.tracks[2].genre = "Rock" # track 2 now matches + assert mgr.recompute_smart_playlist(pl.persistent_id) is True + + assert pl.track_ids == [1, 2] # in memory, recomputed + assert pl.date_modified == stamp # not an edit + assert pl.persistent_id not in mgr._dirty_playlists + + def test_derived_recompute_still_announces_itself(self, qapp, tmp_path): + """No file write, but the views must still redraw.""" + mgr, pl = self._manager(tmp_path, _rock_criteria()) + seen = [] + mgr.playlist_content_changed.connect(seen.append) + + mgr.library.tracks[2].genre = "Rock" + mgr.recompute_smart_playlist(pl.persistent_id) + + assert seen == [pl.persistent_id] + + def test_a_snapshot_playlist_still_persists_its_recompute(self, qapp, tmp_path): + mgr, pl = self._manager(tmp_path, _rock_criteria(live_update=False)) + stamp = pl.date_modified + + mgr.library.tracks[2].genre = "Rock" + assert mgr.recompute_smart_playlist(pl.persistent_id, force=True) is True + + assert pl.date_modified != stamp + assert pl.persistent_id in mgr._dirty_playlists + + def test_a_real_edit_still_counts_as_one(self, qapp, tmp_path): + """The touch=False path must not leak into user edits.""" + mgr = LibraryManager(_library("Rock", "Jazz"), tmp_path) + pl = mgr.create_playlist("mix") + mgr.flush() + mgr._dirty_playlists.clear() + stamp = pl.date_modified + + mgr.add_tracks_to_playlist(pl.persistent_id, [1]) + + assert pl.date_modified != stamp + assert pl.persistent_id in mgr._dirty_playlists + + +# --------------------------------------------------------------------------- # +# Severity +# --------------------------------------------------------------------------- # + +class TestLevels: + def test_ordering(self): + assert at_least(WARNING, INFO) and at_least(INFO, INFO) + assert not at_least(INFO, CHANGE) + assert max_level([ConflictSummary("a", "other", level=INFO), + ConflictSummary("b", "other", level=WARNING)]) == WARNING + assert max_level([]) == INFO + + def _write_conflict(self, tmp_path, name, payload): + path = tmp_path / name + path.parent.mkdir(parents=True, exist_ok=True) + cfile = path.parent / _conflict_name(path.name) + cfile.write_text(json.dumps(payload)) + return cfile + + def _smart_pair(self, tmp_path, their_criteria, their_stamp): + pl = Playlist(name="unplayed", persistent_id="AAAA1111", + playlist_type=PlaylistType.SMART, + smart_criteria=_rock_criteria(), + date_modified="2026-08-01T00:00:00") + lib = Library() + lib.playlists[pl.persistent_id] = pl + json_storage.save_library(lib, tmp_path) + theirs = json.loads(_playlist_file(tmp_path).read_text()) + theirs["smart_criteria"] = their_criteria.to_dict() + theirs["date_modified"] = their_stamp + theirs["track_ids"] = [7, 8, 9] # their derived membership drifted + self._write_conflict(tmp_path, "playlists/AAAA1111.json", theirs) + return conflict_resolver.resolve_conflicts(tmp_path) + + def test_identical_smart_rules_are_info(self, tmp_path): + summaries = self._smart_pair(tmp_path, _rock_criteria(), + "2026-08-09T00:00:00") + assert summaries[0].level == INFO + assert "identical" in summaries[0].lines[0] + + def test_genuinely_edited_smart_rules_are_a_change(self, tmp_path): + other = SmartCriteria( + root=SmartGroup("all", [SmartRule("genre", "is", "Jazz")])) + summaries = self._smart_pair(tmp_path, other, "2026-08-09T00:00:00") + assert summaries[0].level == CHANGE + assert "the other machine" in summaries[0].lines[0] + + def test_metadata_is_always_info_and_says_what_differed(self, tmp_path): + lib = Library() + lib.music_folder = "/media/muzak" + json_storage.save_library(lib, tmp_path) + theirs = json.loads((tmp_path / "library_metadata.json").read_text()) + theirs["library_settings"]["column_widths"] = {"name": 400} + self._write_conflict(tmp_path, "library_metadata.json", theirs) + + summaries = conflict_resolver.resolve_conflicts(tmp_path) + + assert summaries[0].level == INFO + assert "column layout" in summaries[0].lines[0] + + def test_but_adopting_a_music_folder_is_a_change(self, tmp_path): + """The one thing in this file nobody sets by resizing a window. Round 40 + made the music folder win on its own stamp rather than the file's mtime; + when that fires, the merge changed a deliberate setting.""" + lib = Library() + lib.music_folder = "/media/muzak" + lib.music_folder_set_at = "2026-08-01T00:00:00" + json_storage.save_library(lib, tmp_path) + theirs = json.loads((tmp_path / "library_metadata.json").read_text()) + theirs["music_folder"] = "/media/elsewhere" + theirs["music_folder_rel"] = "../elsewhere" + theirs["music_folder_set_at"] = "2026-08-09T00:00:00" + self._write_conflict(tmp_path, "library_metadata.json", theirs) + + summaries = conflict_resolver.resolve_conflicts(tmp_path) + + assert summaries[0].level == CHANGE + assert "Music folder" in summaries[0].lines[-1] + assert json.loads((tmp_path / "library_metadata.json").read_text())[ + "music_folder"] == "/media/elsewhere" + + def test_a_cosmetic_only_playlist_merge_is_info(self, tmp_path): + pl = Playlist(name="mix", persistent_id="BBBB2222", + playlist_type=PlaylistType.REGULAR, track_ids=[1, 2, 3], + date_modified="2026-08-01T00:00:00") + lib = Library() + lib.playlists[pl.persistent_id] = pl + json_storage.save_library(lib, tmp_path) + theirs = json.loads(_playlist_file(tmp_path, "BBBB2222").read_text()) + theirs["settings"]["column_widths"] = {"name": 500} + theirs["date_modified"] = "2026-08-09T00:00:00" + self._write_conflict(tmp_path, "playlists/BBBB2222.json", theirs) + + summaries = conflict_resolver.resolve_conflicts(tmp_path) + + assert summaries[0].level == INFO + assert "column layout" in summaries[0].lines[0] + + def test_a_real_order_merge_is_a_change(self, tmp_path): + pl = Playlist(name="mix", persistent_id="BBBB2222", + playlist_type=PlaylistType.REGULAR, track_ids=[1, 2, 3], + date_modified="2026-08-01T00:00:00") + lib = Library() + lib.playlists[pl.persistent_id] = pl + json_storage.save_library(lib, tmp_path) + theirs = json.loads(_playlist_file(tmp_path, "BBBB2222").read_text()) + theirs["track_ids"] = [1, 99, 2, 3] + theirs["date_modified"] = "2026-08-09T00:00:00" + self._write_conflict(tmp_path, "playlists/BBBB2222.json", theirs) + + summaries = conflict_resolver.resolve_conflicts(tmp_path) + + assert summaries[0].level == CHANGE + assert json.loads( + _playlist_file(tmp_path, "BBBB2222").read_text())["track_ids"] == [1, 99, 2, 3] + + def test_a_deleted_file_edited_elsewhere_is_a_warning(self, tmp_path): + json_storage.save_library(Library(), tmp_path) + self._write_conflict(tmp_path, "playlists/CCCC3333.json", + {"name": "gone", "persistent_id": "CCCC3333"}) + + summaries = conflict_resolver.resolve_conflicts(tmp_path) + + assert summaries[0].level == WARNING + + def test_an_unmergeable_file_is_a_warning(self, tmp_path): + json_storage.save_library(Library(), tmp_path) + (tmp_path / "something.json").write_text("{}") + self._write_conflict(tmp_path, "something.json", {"a": 1}) + + summaries = conflict_resolver.resolve_conflicts(tmp_path) + + assert summaries[0].level == WARNING + + def test_a_play_journal_conflict_is_a_warning(self, tmp_path): + json_storage.save_library(Library(), tmp_path) + (tmp_path / "plays").mkdir(exist_ok=True) + (tmp_path / "plays" / "boxA.json").write_text( + json.dumps({"1": {"plays": 2}})) + self._write_conflict(tmp_path, "plays/boxA.json", {"1": {"plays": 5}}) + + summaries = conflict_resolver.resolve_conflicts(tmp_path) + + assert summaries[0].level == WARNING + assert json.loads( + (tmp_path / "plays" / "boxA.json").read_text())["1"]["plays"] == 5 + + +# --------------------------------------------------------------------------- # +# The dialog +# --------------------------------------------------------------------------- # + +@pytest.fixture +def dialog_cls(): + from lintunes.gui.conflict_dialog import ConflictSummaryDialog + return ConflictSummaryDialog + + +def _summary(name, level, line="something happened"): + return ConflictSummary(name, "playlist", [line], backup_dir="/tmp/b", + level=level) + + +class TestConflictDialog: + def test_an_all_info_batch_opens_on_the_info_view(self, qapp, dialog_cls): + dlg = dialog_cls([_summary("library_metadata.json", INFO)], None) + assert dlg.level == INFO + assert "library_metadata.json" in dlg._body.toPlainText() + assert "Nothing at this level" not in dlg._body.toPlainText() + + def test_a_warning_batch_hides_the_routine_entries(self, qapp, dialog_cls): + dlg = dialog_cls([_summary("library_metadata.json", INFO), + _summary("plays/boxA.json", WARNING)], None) + body = dlg._body.toPlainText() + assert dlg.level == WARNING + assert "plays/boxA.json" in body + assert "library_metadata.json" not in body + assert "1 routine entry hidden" in body + + def test_a_later_warning_pulls_the_view_back_up(self, qapp, dialog_cls): + dlg = dialog_cls([_summary("library_metadata.json", INFO)], None) + assert dlg.add_event([_summary("plays/boxA.json", WARNING)]) is True + assert dlg.level == WARNING + assert "library_metadata.json" not in dlg._body.toPlainText() + + def test_routine_events_do_not_ask_to_be_raised(self, qapp, dialog_cls): + dlg = dialog_cls([_summary("mix", CHANGE)], None) + assert dlg.level == CHANGE + assert dlg.add_event([_summary("library_metadata.json", INFO)]) is False + # Still recorded — it's there once the user dials the filter down. + dlg._on_level_picked(dlg._level_box.findData(INFO)) + assert "library_metadata.json" in dlg._body.toPlainText() + + def test_a_user_choice_survives_a_quieter_batch(self, qapp, dialog_cls): + dlg = dialog_cls([_summary("mix", CHANGE)], None) + dlg._on_level_picked(dlg._level_box.findData(WARNING)) + dlg.add_event([_summary("other mix", CHANGE)]) + assert dlg.level == WARNING + assert "Nothing at this level" in dlg._body.toPlainText() + + def test_filtered_out_events_leave_no_orphan_headers(self, qapp, dialog_cls): + dlg = dialog_cls([_summary("mix", CHANGE)], None) + dlg.add_event([_summary("library_metadata.json", INFO)]) + headers = [ln for ln in dlg._body.toPlainText().splitlines() + if ln.startswith("── ")] + assert len(headers) == 1 # only the event that still has content