v0.9.1: playlist merges stop losing your track order
What scrambled `a nissa one` took three defects at once. Reorder it on machine A; open it on machine B and resize the window; B's file is now newer, so the merge takes B's order — the old one — wholesale. _merge_playlist was a 2-way union with no common ancestor: one side's order wholesale, the other side's extras appended at the tail, so a track inserted in the middle on one machine arrived at the bottom on the other. merge_track_order re-inserts each side-only track after the nearest track both copies share instead. _reconcile_playlist (the live-reload path) uses the same helper. The union invariant is unchanged and property-tested over 300 random pairs — a merge never drops a track. Whose order wins is now Playlist.date_modified, bumped only in _set_track_ids (add / remove / reorder / undo) and deliberately not by mark_playlist_settings_dirty, with a file-mtime fallback for playlists written before the field existed. The file's mtime was a lie: the last column is stretch-sized, so Qt re-fires sectionResized whenever the viewport width changes, and a window resize or splitter drag rewrote the open playlist's JSON — moving its mtime and handing Syncthing another conflict — for a width apply_settings overrides on load anyway. _on_section_resized now skips that section; genuine drags on every other column still persist. Replayed on a copy of the real playlists: `a nissa one` keeps its reorder against a newer-by-mtime opponent, and `a nissa ideas` takes the other machine's two mid-list inserts at 5 and 12 rather than 26 and 27. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M8Mzze7shr5pZoEgKQU5NW
This commit is contained in:
@@ -0,0 +1,247 @@
|
||||
"""Round 39: playlist merges stop losing your track order.
|
||||
|
||||
Three defects, and it took all three to scramble `a nissa one`:
|
||||
|
||||
* `_merge_playlist` was a 2-way union with no common ancestor — it took one
|
||||
side's order wholesale and *appended* the other side's extras, so a track
|
||||
inserted in the middle on one machine arrived at the bottom on the other.
|
||||
* Whose order won was decided by the file's mtime, and the last column is
|
||||
stretch-sized — so resizing the window rewrote the open playlist's JSON and
|
||||
"most recently edited" meant "most recently resized".
|
||||
* Those cosmetic rewrites were themselves handing Syncthing conflicts.
|
||||
"""
|
||||
import json
|
||||
import random
|
||||
|
||||
import pytest
|
||||
|
||||
from lintunes.models import Library, Playlist, PlaylistType
|
||||
from lintunes.storage import json_storage, conflict_resolver
|
||||
from lintunes.storage.conflict_resolver import merge_track_order
|
||||
|
||||
|
||||
def _save(tmp_path, playlists=()):
|
||||
lib = Library()
|
||||
for p in playlists:
|
||||
lib.playlists[p.persistent_id] = p
|
||||
json_storage.save_library(lib, tmp_path)
|
||||
return lib
|
||||
|
||||
|
||||
def _conflict_name(original: str) -> str:
|
||||
stem, _, ext = original.rpartition(".")
|
||||
return f"{stem}.sync-conflict-20240101-120000-ABCDEFG.{ext}"
|
||||
|
||||
|
||||
class TestAnchoredOrder:
|
||||
"""merge_track_order: a track inserted in the middle stays in the middle."""
|
||||
|
||||
def test_middle_insert_is_not_appended(self):
|
||||
# The other copy slipped 99 between 2 and 3. It belongs there, not last.
|
||||
assert merge_track_order([1, 2, 3, 4], [1, 2, 99, 3, 4]) == [1, 2, 99, 3, 4]
|
||||
|
||||
def test_both_sides_inserted_at_different_points(self):
|
||||
assert merge_track_order([1, 7, 2, 3], [1, 2, 8, 3]) == [1, 7, 2, 8, 3]
|
||||
|
||||
def test_consecutive_inserts_keep_their_relative_order(self):
|
||||
assert merge_track_order([1, 2], [1, 8, 9, 2]) == [1, 8, 9, 2]
|
||||
|
||||
def test_a_leading_insert_goes_to_the_head(self):
|
||||
assert merge_track_order([1, 2, 3], [7, 1, 2, 3]) == [7, 1, 2, 3]
|
||||
|
||||
def test_primary_decides_the_order_of_shared_tracks(self):
|
||||
# Both copies hold 1-2-3; primary reversed them. Primary wins.
|
||||
assert merge_track_order([3, 2, 1], [1, 2, 3]) == [3, 2, 1]
|
||||
|
||||
def test_trailing_insert_still_lands_after_its_anchor(self):
|
||||
assert merge_track_order([1, 2, 3], [1, 2, 3, 4]) == [1, 2, 3, 4]
|
||||
|
||||
@pytest.mark.parametrize("primary,secondary", [
|
||||
([], []),
|
||||
([1, 2, 3], []),
|
||||
([], [1, 2, 3]),
|
||||
([1, 2, 3], [4, 5, 6]), # disjoint
|
||||
([1, 2, 2, 3], [1, 2, 3, 3]), # the same track twice
|
||||
])
|
||||
def test_edge_cases_never_drop_a_track(self, primary, secondary):
|
||||
merged = merge_track_order(primary, secondary)
|
||||
assert set(merged) == set(primary) | set(secondary)
|
||||
|
||||
def test_nothing_is_ever_dropped(self):
|
||||
"""The invariant that matters most: a merge is a union, always."""
|
||||
rng = random.Random(39)
|
||||
for _ in range(300):
|
||||
base = rng.sample(range(20), rng.randint(0, 10))
|
||||
a = [t for t in base if rng.random() > 0.3] + rng.sample(range(20, 30), 2)
|
||||
b = [t for t in base if rng.random() > 0.3] + rng.sample(range(30, 40), 2)
|
||||
rng.shuffle(a)
|
||||
merged = merge_track_order(a, b)
|
||||
assert set(merged) == set(a) | set(b)
|
||||
# Primary's entries keep primary's exact sequence — only tracks it
|
||||
# never had get woven in between them.
|
||||
assert [t for t in merged if t in set(a)] == a
|
||||
|
||||
|
||||
class TestHonestDateModified:
|
||||
"""The merge decides on the playlist's own date_modified, not file mtime."""
|
||||
|
||||
def _setup(self, tmp_path, ours, theirs, our_stamp, their_stamp):
|
||||
pl = Playlist(name="a nissa one", persistent_id="AAAA1111",
|
||||
playlist_type=PlaylistType.REGULAR, track_ids=ours,
|
||||
date_modified=our_stamp)
|
||||
_save(tmp_path, [pl])
|
||||
pfile = tmp_path / "playlists" / "AAAA1111.json"
|
||||
other = json.loads(pfile.read_text())
|
||||
other["track_ids"] = theirs
|
||||
if their_stamp is None:
|
||||
other.pop("date_modified", None)
|
||||
else:
|
||||
other["date_modified"] = their_stamp
|
||||
cfile = tmp_path / "playlists" / _conflict_name("AAAA1111.json")
|
||||
cfile.write_text(json.dumps(other))
|
||||
return pfile, cfile
|
||||
|
||||
def test_a_resize_no_longer_beats_a_reorder(self, tmp_path):
|
||||
"""The `a nissa one` regression. We reordered; the other machine only
|
||||
opened it and resized the window, so its *file* is newer — but its
|
||||
date_modified is older, and our order must survive."""
|
||||
pfile, cfile = self._setup(
|
||||
tmp_path,
|
||||
ours=[3, 1, 2], # we carefully reordered
|
||||
theirs=[1, 2, 3], # they never touched the contents
|
||||
our_stamp="2026-08-20T18:00:00",
|
||||
their_stamp="2026-08-19T09:00:00")
|
||||
import os, time
|
||||
os.utime(cfile, (time.time() + 100, time.time() + 100)) # their file is newer
|
||||
|
||||
summaries = conflict_resolver.resolve_conflicts(tmp_path)
|
||||
|
||||
assert json.loads(pfile.read_text())["track_ids"] == [3, 1, 2]
|
||||
assert "this machine" in summaries[0].lines[0]
|
||||
|
||||
def test_the_genuinely_newer_edit_still_wins(self, tmp_path):
|
||||
pfile, _ = self._setup(
|
||||
tmp_path, ours=[1, 2, 3], theirs=[3, 2, 1],
|
||||
our_stamp="2026-08-19T09:00:00",
|
||||
their_stamp="2026-08-20T18:00:00")
|
||||
|
||||
summaries = conflict_resolver.resolve_conflicts(tmp_path)
|
||||
|
||||
assert json.loads(pfile.read_text())["track_ids"] == [3, 2, 1]
|
||||
assert "the other machine" in summaries[0].lines[0]
|
||||
|
||||
def test_their_insert_lands_in_position_not_at_the_tail(self, tmp_path):
|
||||
pfile, _ = self._setup(
|
||||
tmp_path, ours=[1, 2, 3], theirs=[1, 2, 99, 3],
|
||||
our_stamp="2026-08-20T18:00:00",
|
||||
their_stamp="2026-08-19T09:00:00")
|
||||
|
||||
summaries = conflict_resolver.resolve_conflicts(tmp_path)
|
||||
|
||||
assert json.loads(pfile.read_text())["track_ids"] == [1, 2, 99, 3]
|
||||
assert "back in position" in summaries[0].lines[1]
|
||||
|
||||
def test_falls_back_to_mtime_before_the_field_existed(self, tmp_path):
|
||||
"""Playlists written by an older version carry no stamp."""
|
||||
pfile, cfile = self._setup(
|
||||
tmp_path, ours=[1, 2, 3], theirs=[3, 2, 1],
|
||||
our_stamp=None, their_stamp=None)
|
||||
import os, time
|
||||
os.utime(cfile, (time.time() + 100, time.time() + 100))
|
||||
|
||||
conflict_resolver.resolve_conflicts(tmp_path)
|
||||
assert json.loads(pfile.read_text())["track_ids"] == [3, 2, 1]
|
||||
|
||||
def test_smart_playlist_rules_also_use_date_modified(self, tmp_path):
|
||||
from lintunes.smart import SmartCriteria
|
||||
pl = Playlist(name="Recent", persistent_id="BBBB2222",
|
||||
playlist_type=PlaylistType.SMART, track_ids=[1],
|
||||
smart_criteria=SmartCriteria(), date_modified="2026-08-20T18:00:00")
|
||||
_save(tmp_path, [pl])
|
||||
pfile = tmp_path / "playlists" / "BBBB2222.json"
|
||||
ours = json.loads(pfile.read_text())
|
||||
other = dict(ours)
|
||||
other["name"] = "Stale"
|
||||
other["date_modified"] = "2026-08-19T09:00:00"
|
||||
cfile = tmp_path / "playlists" / _conflict_name("BBBB2222.json")
|
||||
cfile.write_text(json.dumps(other))
|
||||
import os, time
|
||||
os.utime(cfile, (time.time() + 100, time.time() + 100))
|
||||
|
||||
conflict_resolver.resolve_conflicts(tmp_path)
|
||||
assert json.loads(pfile.read_text())["name"] == "Recent"
|
||||
|
||||
|
||||
class TestDateModifiedMovesOnlyForContent:
|
||||
@pytest.fixture
|
||||
def manager(self, tmp_path, qapp, monkeypatch):
|
||||
monkeypatch.setenv("XDG_CONFIG_HOME", str(tmp_path / "config"))
|
||||
from lintunes.library_manager import LibraryManager
|
||||
from lintunes.models import Track
|
||||
lib = Library()
|
||||
for tid in (1, 2, 3):
|
||||
lib.tracks[tid] = Track(track_id=tid, name=f"T{tid}")
|
||||
lib.playlists["AAAA1111"] = Playlist(
|
||||
name="a nissa one", persistent_id="AAAA1111",
|
||||
playlist_type=PlaylistType.REGULAR, track_ids=[1, 2, 3])
|
||||
return LibraryManager(lib, tmp_path)
|
||||
|
||||
def _stamp(self, manager):
|
||||
return manager.library.playlists["AAAA1111"].date_modified
|
||||
|
||||
def test_reorder_bumps_it(self, manager):
|
||||
assert self._stamp(manager) is None
|
||||
manager.move_tracks_in_playlist("AAAA1111", [2], 0)
|
||||
assert self._stamp(manager) is not None
|
||||
|
||||
def test_add_and_remove_bump_it(self, manager):
|
||||
manager.remove_tracks_from_playlist("AAAA1111", [0])
|
||||
first = self._stamp(manager)
|
||||
assert first is not None
|
||||
manager.add_tracks_to_playlist("AAAA1111", [1])
|
||||
assert self._stamp(manager) >= first
|
||||
|
||||
def test_undo_bumps_it_too(self, manager):
|
||||
manager.move_tracks_in_playlist("AAAA1111", [2], 0)
|
||||
before = self._stamp(manager)
|
||||
manager.undo_stack.undo()
|
||||
assert manager.library.playlists["AAAA1111"].track_ids == [1, 2, 3]
|
||||
assert self._stamp(manager) >= before
|
||||
|
||||
def test_a_column_resize_does_not(self, manager):
|
||||
"""The whole point: cosmetic settings must not look like an edit."""
|
||||
manager.mark_playlist_settings_dirty("AAAA1111")
|
||||
assert self._stamp(manager) is None
|
||||
|
||||
|
||||
class TestStretchedColumnIsNotPersisted:
|
||||
@pytest.fixture
|
||||
def table(self, qapp):
|
||||
from lintunes.gui.track_table import TrackTableView
|
||||
from lintunes.models import Track
|
||||
view = TrackTableView()
|
||||
view.set_tracks([Track(track_id=1, name="A", artist="B")])
|
||||
return view
|
||||
|
||||
def test_a_middle_column_still_persists(self, table):
|
||||
seen = []
|
||||
table.column_width_changed.connect(lambda f, w: seen.append((f, w)))
|
||||
table.horizontalHeader().resizeSection(1, 123)
|
||||
assert seen and seen[0][1] == 123
|
||||
|
||||
def test_the_stretched_last_column_does_not(self, table):
|
||||
"""Qt re-fires sectionResized for the stretched column on every viewport
|
||||
width change — a window resize used to rewrite the playlist JSON."""
|
||||
header = table.horizontalHeader()
|
||||
last = header.logicalIndex(header.count() - 1)
|
||||
seen = []
|
||||
table.column_width_changed.connect(lambda f, w: seen.append((f, w)))
|
||||
header.resizeSection(last, 321)
|
||||
assert seen == []
|
||||
|
||||
def test_resizing_the_widget_is_silent(self, table):
|
||||
seen = []
|
||||
table.column_width_changed.connect(lambda f, w: seen.append((f, w)))
|
||||
table.resize(400, 300)
|
||||
table.resize(900, 300)
|
||||
assert seen == []
|
||||
Reference in New Issue
Block a user