diff --git a/server.py b/server.py index 9dfc572..e71c8cc 100644 --- a/server.py +++ b/server.py @@ -455,8 +455,12 @@ def _apply_pending_db_restore(config_dir: Path) -> None: # TOTAL — which also fixes a latent OFFSET skip/dupe across equal-key rows. # (column, collate-clause, primary-direction) — tiebreak is always `filename` ASC. _KEYSET_SORTS = { - "artist": ("artist", "COLLATE NOCASE", "ASC"), - "artist-desc": ("artist", "COLLATE NOCASE", "DESC"), + # artist/artist-desc left OUT deliberately: their ORDER BY carries a + # title secondary (so cards within an artist read alphabetically, like + # the tree view) which a two-term (value, filename) cursor can't seek + # correctly — they page by OFFSET, which is measured-trivial at real + # library sizes. Restore them with a composite sort-key column if + # 50k-song libraries ever make OFFSET hurt. "title": ("title", "COLLATE NOCASE", "ASC"), "title-desc": ("title", "COLLATE NOCASE", "DESC"), "recent": ("mtime", "", "DESC"), @@ -3517,7 +3521,13 @@ class MetadataDB: where += self._GROUP_REP_PREDICATE sort_map = { - "artist": "artist COLLATE NOCASE", "artist-desc": "artist COLLATE NOCASE DESC", + # Artist sorts order WITHIN an artist by title (the tree view's + # artist -> album -> title feel) instead of raw filename — the + # "list is organised, cards look random" report. Direction is + # baked per entry (the legacy `dir=desc` append would otherwise + # land on the title term); title stays ascending under Z->A. + "artist": "artist COLLATE NOCASE ASC, title COLLATE NOCASE ASC", + "artist-desc": "artist COLLATE NOCASE DESC, title COLLATE NOCASE ASC", "title": "title COLLATE NOCASE", "title-desc": "title COLLATE NOCASE DESC", "recent": "mtime DESC", # Tuning sort uses musical distance from E Standard @@ -3589,14 +3599,23 @@ class MetadataDB: "FROM work_display w1 WHERE w1.filename = songs.filename))") sort_map["mastery"] = f"({_gm} IS NULL) ASC, {_gm} ASC" sort_map["mastery-desc"] = f"({_gm} IS NULL) ASC, {_gm} DESC" - order = sort_map.get(sort, "artist COLLATE NOCASE") + # Fold the legacy `dir=desc` toggle into the canonical sort key BEFORE + # the lookup, so the ORDER BY is built from the effective sort — mirrors + # what `_effective_keyset_sort` does on the cursor side. Needed because + # the artist clause now bakes in `ASC` (for the title secondary), so the + # ` DESC` append below is suppressed and would otherwise silently ignore + # `sort=artist&dir=desc` (return A→Z). Only artist/title fold (they have + # `-desc` twins); tuning/year/mastery keep their own dir handling. + eff = _effective_keyset_sort(sort, direction) + order = sort_map.get(eff, "artist COLLATE NOCASE") # Legacy `dir=desc` toggle: only safe to append on simple sort # clauses that don't already encode a direction. Compound / - # multi-term entries above (tuning, year, year-desc) bake their + # multi-term entries above (artist, tuning, year, year-desc) bake their # ASC/DESC into the clause, so a global ` DESC` append would # produce invalid SQL like `CAST(year AS INTEGER) ASC DESC`. # Skip the append in that case — clients flipping direction on - # those sorts use the explicit `-desc` sort key instead. + # those sorts use the explicit `-desc` sort key instead. (For + # artist/title the fold above already picked the `-desc` clause.) if direction == "desc" and " ASC" not in order and " DESC" not in order: order += " DESC" # Unique, deterministic tiebreak → a TOTAL order. Without it, rows with diff --git a/static/v3/songs.js b/static/v3/songs.js index ffbe7aa..6d6a341 100644 --- a/static/v3/songs.js +++ b/static/v3/songs.js @@ -2247,6 +2247,11 @@ async function loadTree() { const host = document.getElementById('v3-songs-tree'); if (!host) return; + // The list view always groups artist -> album (query_artists has no + // free sort) — when the picked sort is something else, say so instead + // of silently ignoring it ("why does the sort do nothing here?"). + const _treeSortNote = railSortColumn() === 'artist' ? '' + : '

List view groups by artist — the selected sort applies to the card grid.

'; // Capture expanded groups BEFORE the "Loading…" wipe below, so a reload // (e.g. toggling select mode) restores them instead of collapsing all. const openArtists = new Set( @@ -2266,7 +2271,7 @@ } if (!artists.length) { host.innerHTML = '

Nothing here.

'; return; } artists.forEach((a) => (a.albums || []).forEach((al) => (al.songs || []).forEach((s) => { state.songsById[cardKey(s)] = s; }))); - host.innerHTML = artists.map((a) => + host.innerHTML = _treeSortNote + artists.map((a) => '
' + '' + esc(a.name) + '' + esc(a.song_count) + '' + '
' + (a.albums || []).map((al) => diff --git a/tests/test_artist_sort_title_order.py b/tests/test_artist_sort_title_order.py new file mode 100644 index 0000000..7f16fd2 --- /dev/null +++ b/tests/test_artist_sort_title_order.py @@ -0,0 +1,81 @@ +"""The grid's artist sort orders WITHIN an artist by title (tree-view feel), +not raw filename — the tester's "list is organised by artist, but the cards +look alphabetical/random" report. Artist sorts page by OFFSET now (the title +secondary can't ride the two-term keyset cursor), so pagination across an +artist boundary is pinned too.""" + +import importlib +import sys + +import pytest +from fastapi.testclient import TestClient + + +@pytest.fixture() +def server(tmp_path, monkeypatch, isolate_logging): + monkeypatch.setenv("CONFIG_DIR", str(tmp_path)) + monkeypatch.setenv("FEEDBACK_SKIP_STARTUP_TASKS", "1") + sys.modules.pop("server", None) + srv = importlib.import_module("server") + try: + yield srv + finally: + conn = getattr(getattr(srv, "meta_db", None), "conn", None) + if conn is not None: + conn.close() + sys.modules.pop("server", None) + + +@pytest.fixture() +def client(server): + return TestClient(server.app) + + +def _put(server, fn, artist, title): + server.meta_db.put(fn, 0, 0, { + "title": title, "artist": artist, "album": "", "year": "", + "duration": 100, "arrangements": [{"name": "Lead", "index": 0}], + }) + + +def test_artist_sort_orders_titles_within_artist(server, client): + # Filenames deliberately REVERSE the title order, so filename ordering + # (the old behaviour) and title ordering disagree. + _put(server, "z1.sloppak", "Alpha Band", "Aardvark") + _put(server, "a9.sloppak", "Alpha Band", "Zebra") + _put(server, "m5.sloppak", "Alpha Band", "Mango") + _put(server, "q1.sloppak", "Beta Band", "Only Song") + body = client.get("/api/library", params={"sort": "artist", "size": 50}).json() + assert [s["title"] for s in body["songs"]] == ["Aardvark", "Mango", "Zebra", "Only Song"] + # Z→A flips the ARTIST order; titles stay A→Z within each artist. + body = client.get("/api/library", params={"sort": "artist-desc", "size": 50}).json() + assert [s["title"] for s in body["songs"]] == ["Only Song", "Aardvark", "Mango", "Zebra"] + + +def test_legacy_dir_desc_flips_artist_like_artist_desc(server, client): + # The legacy `sort=artist&dir=desc` shape must match the explicit + # `artist-desc` key: the artist clause now bakes in ASC (for the title + # secondary), so `dir=desc` is folded into the effective sort BEFORE the + # ORDER BY lookup — otherwise the append is suppressed and dir=desc would + # silently return A→Z. + _put(server, "z1.sloppak", "Alpha Band", "Aardvark") + _put(server, "a9.sloppak", "Alpha Band", "Zebra") + _put(server, "m5.sloppak", "Alpha Band", "Mango") + _put(server, "q1.sloppak", "Beta Band", "Only Song") + legacy = client.get("/api/library", params={"sort": "artist", "dir": "desc", "size": 50}).json() + explicit = client.get("/api/library", params={"sort": "artist-desc", "size": 50}).json() + assert [s["title"] for s in legacy["songs"]] == ["Only Song", "Aardvark", "Mango", "Zebra"] + assert [s["title"] for s in legacy["songs"]] == [s["title"] for s in explicit["songs"]] + + +def test_artist_sort_offset_pagination_no_skip_or_dupe(server, client): + for i in range(7): + _put(server, f"f{6 - i}.sloppak", "One Artist", f"Title {chr(65 + i)}") + seen = [] + for page in range(4): + body = client.get("/api/library", params={"sort": "artist", "size": 2, "page": page}).json() + seen += [s["title"] for s in body["songs"]] + assert seen == [f"Title {chr(65 + i)}" for i in range(7)] + # And no keyset cursor is offered for artist sorts (OFFSET path). + body = client.get("/api/library", params={"sort": "artist", "size": 2}).json() + assert body["next_cursor"] is None diff --git a/tests/test_group_filter_law.py b/tests/test_group_filter_law.py index 072c660..22cf98b 100644 --- a/tests/test_group_filter_law.py +++ b/tests/test_group_filter_law.py @@ -184,7 +184,9 @@ def test_grouped_keyset_pagination_with_intrinsic_filter(client, server): arrangements=1, tuning="Drop D") seen, cursor = [], None for _ in range(10): - params = {"group": 1, "tunings": "Drop D", "size": 2, "sort": "artist"} + # Title sort — the keyset proof needs a sort that still keysets + # (artist sorts page by OFFSET since the title-secondary change). + params = {"group": 1, "tunings": "Drop D", "size": 2, "sort": "title"} if cursor: params["after"] = cursor body = client.get("/api/library", params=params).json() diff --git a/tests/test_library_keyset.py b/tests/test_library_keyset.py index 47fdf4b..3037e77 100644 --- a/tests/test_library_keyset.py +++ b/tests/test_library_keyset.py @@ -73,7 +73,10 @@ def _walk_offset(client, sort, size, total): return seen -@pytest.mark.parametrize("sort", ["artist", "artist-desc", "title", "title-desc", "recent"]) +# artist/artist-desc left out: their ORDER BY carries a title secondary +# (grid cards read alphabetically within an artist, like the tree), which +# the two-term cursor cannot seek — they page by OFFSET (covered below). +@pytest.mark.parametrize("sort", ["title", "title-desc", "recent"]) def test_keyset_matches_offset_exactly(client, server_mod, sort): _seed(server_mod, 25) offset_order = _walk_offset(client, sort, 7, 25) @@ -87,15 +90,17 @@ def test_stable_tiebreak_on_equal_keys(client, server_mod): # 25 songs, all the SAME artist → the artist sort is decided entirely by the # filename tiebreak. Both pagers must still cover all 25 with no dupe. _seed(server_mod, 25, shared_artist=True) - keyset_order = _walk_keyset(client, "artist", 6, 25) + keyset_order = _walk_offset(client, "artist", 6, 25) assert len(keyset_order) == 25 and len(set(keyset_order)) == 25 assert keyset_order == sorted(keyset_order) # tiebreak is filename ASC def test_first_page_has_cursor_and_no_after_is_offset(client, server_mod): _seed(server_mod, 5) + body = client.get("/api/library", params={"sort": "title", "size": 2}).json() + assert body["next_cursor"] # keyset sort: cursor offered body = client.get("/api/library", params={"sort": "artist", "size": 2}).json() - assert body["next_cursor"] # cursor offered + assert body["next_cursor"] is None # artist sorts page by OFFSET assert [s["filename"] for s in body["songs"]] == ["song00.archive", "song01.archive"] @@ -111,7 +116,7 @@ def test_legacy_dir_desc_keysets_correctly(client, server_mod): _seed(server_mod, 20) offset_order, page = [], 0 while True: - body = client.get("/api/library", params={"sort": "artist", "dir": "desc", "size": 6, "page": page}).json() + body = client.get("/api/library", params={"sort": "title", "dir": "desc", "size": 6, "page": page}).json() if not body["songs"]: break offset_order.extend(s["filename"] for s in body["songs"]) @@ -119,7 +124,7 @@ def test_legacy_dir_desc_keysets_correctly(client, server_mod): keyset, cursor, guard = [], "", 0 while len(keyset) < 20 and guard < 25: guard += 1 - params = {"sort": "artist", "dir": "desc", "size": 6} + params = {"sort": "title", "dir": "desc", "size": 6} if cursor: params["after"] = cursor body = client.get("/api/library", params=params).json() @@ -131,7 +136,7 @@ def test_legacy_dir_desc_keysets_correctly(client, server_mod): assert len(set(keyset)) == 20 -@pytest.mark.parametrize("sort", ["artist", "artist-desc", "recent"]) +@pytest.mark.parametrize("sort", ["recent"]) def test_keyset_handles_null_sort_keys(client, server_mod, sort): # NULL artist/mtime (corrupt/legacy rows past put()'s '' defaults) sort # first in ASC / last in DESC; keyset must cover them exactly like OFFSET. diff --git a/tests/test_work_grouping.py b/tests/test_work_grouping.py index db3ca32..0403536 100644 --- a/tests/test_work_grouping.py +++ b/tests/test_work_grouping.py @@ -158,11 +158,13 @@ def test_grouped_keyset_pagination(client, server): for i in range(5): _put(server, f"w{i}_a.archive", f"Song {i}", "Artist") _put(server, f"w{i}_b.archive", f"Song {i}", "Artist") # 2 charts per work - body = client.get("/api/library", params={"group": 1, "size": 3}).json() + # Title sort: artist sorts page by OFFSET now (title-secondary ordering), + # and this test PROVES the grouped keyset, so it pins a keyset sort. + body = client.get("/api/library", params={"group": 1, "size": 3, "sort": "title"}).json() assert body["total"] == 5 and len(body["songs"]) == 3 cur = body["next_cursor"] assert cur - body2 = client.get("/api/library", params={"group": 1, "size": 3, "after": cur}).json() + body2 = client.get("/api/library", params={"group": 1, "size": 3, "sort": "title", "after": cur}).json() assert len(body2["songs"]) == 2 p1 = {s["filename"] for s in body["songs"]} p2 = {s["filename"] for s in body2["songs"]}