Merge pull request 'Genre index: sort A–Z, and repair casing damage at scan time' (#124) from dev into main

internal/library/genre.go

@@ -89,7 +89,7 @@ func normaliseGenreValue(v string) []string {
 	for strings.HasPrefix(v, "(") {
 		// "((" is the spec's escape for a literal "(" — the rest is plain text.
 		if strings.HasPrefix(v, "((") {
-			return append(out, strings.TrimSpace(v[1:]))
+			return append(out, trueUpCasing(strings.TrimSpace(v[1:])))
 		}
 		end := strings.IndexByte(v, ')')
 		if end < 0 {
@@ -106,7 +106,7 @@ func normaliseGenreValue(v string) []string {
 			if err != nil {
 				// Parenthesised but not a reference, e.g. "(Live)". Keep the
 				// whole remainder as written.
-				return append(out, v)
+				return append(out, trueUpCasing(v))
 			}
 			if name, ok := id3v1GenreName(n); ok {
 				out = append(out, name)
@@ -120,11 +120,104 @@ func normaliseGenreValue(v string) []string {
 	}
 	if n, err := strconv.Atoi(v); err == nil {
 		if name, ok := id3v1GenreName(n); ok {
+			// Canonical table name, deliberately NOT re-cased — see id3v1Genres.
 			return append(out, name)
 		}
 		return out
 	}
-	return append(out, v)
+	return append(out, trueUpCasing(v))
+}
+
+// genreAcronyms are tokens that belong in caps. Tag editors that title-case the
+// whole genre field turn "EDM" into "Edm" and "UK Garage" into "Uk Garage", and
+// the operator's library carries all of these (#2468).
+//
+// Matched case-INSENSITIVELY, so "edm", "Edm" and "EDM" all land on "EDM". That
+// is a deliberate, narrow exception to the project's rule that genre case is
+// exposed as the file says it: "Rock" and "rock" still stay separate rows,
+// because folding those would be a judgement about labels. Fixing a token that
+// is unambiguously an initialism is not — there is no genre named "Edm".
+//
+// Kept short and evidence-led. Add a token here only when a real library shows
+// it damaged; a speculative list risks mangling a word that legitimately looks
+// like an acronym.
+var genreAcronyms = map[string]string{
+	"edm": "EDM",
+	"idm": "IDM",
+	"aor": "AOR",
+	"uk":  "UK",
+	"us":  "US",
+	"ebm": "EBM",
+}
+
+// contractionSuffixes are the word-endings that follow an apostrophe in normal
+// English. Title-casing capitalises the letter after ANY non-letter, which is
+// how "Children's Music" became "Children'S Music".
+//
+// Deliberately a fixed list rather than "lowercase whatever follows an
+// apostrophe": that broader rule would break "O'Brien" and "D'Angelo", which are
+// correctly capitalised after the apostrophe.
+var contractionSuffixes = map[string]bool{
+	"s": true, "t": true, "re": true, "ll": true, "ve": true, "d": true, "m": true,
+}
+
+// trueUpCasing repairs casing damage done by tag editors that title-case the
+// genre field. It only ever changes case, never the letters — so it cannot
+// silently turn one genre into a different one, which is what separates it from
+// the label-remapping idea #2468 rejected.
+func trueUpCasing(s string) string {
+	if s == "" {
+		return s
+	}
+	words := strings.Split(s, " ")
+	for i, w := range words {
+		if w == "" {
+			continue
+		}
+		// Match the word's letter core, not the raw word, so surrounding
+		// punctuation doesn't hide the acronym: "(Edm)" and "Edm," both need
+		// fixing, and re-attaching the trimmed edges keeps them intact.
+		lead, core, trail := splitWordCore(w)
+		if fixed, ok := genreAcronyms[strings.ToLower(core)]; ok {
+			words[i] = lead + fixed + trail
+			continue
+		}
+		words[i] = fixApostrophe(w)
+	}
+	return strings.Join(words, " ")
+}
+
+// splitWordCore peels leading and trailing non-alphanumerics off a word.
+// Interior punctuation stays in the core, so "Lo-Fi" and "R&B" are compared
+// whole rather than being split into fragments that might match by accident.
+func splitWordCore(w string) (lead, core, trail string) {
+	isCore := func(r rune) bool {
+		return (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9')
+	}
+	start := 0
+	for start < len(w) && !isCore(rune(w[start])) {
+		start++
+	}
+	end := len(w)
+	for end > start && !isCore(rune(w[end-1])) {
+		end--
+	}
+	return w[:start], w[start:end], w[end:]
+}
+
+// fixApostrophe lowercases a capitalised contraction or possessive suffix:
+// "Children'S" -> "Children's". Leaves "O'Brien" alone, since "Brien" is not a
+// contraction suffix.
+func fixApostrophe(w string) string {
+	idx := strings.LastIndexByte(w, '\'')
+	if idx <= 0 || idx == len(w)-1 {
+		return w
+	}
+	suffix := w[idx+1:]
+	if !contractionSuffixes[strings.ToLower(suffix)] {
+		return w
+	}
+	return w[:idx+1] + strings.ToLower(suffix)
 }
 
 func id3v1GenreName(n int) (string, bool) {

internal/library/genre_test.go

@@ -419,3 +419,102 @@ func equalStrings(a, b []string) bool {
 	}
 	return true
 }
+
+func TestTrueUpCasing(t *testing.T) {
+	tests := []struct {
+		name string
+		in   string
+		want string
+	}{
+		// The damage actually present in the operator's library (#2468).
+		{"edm acronym", "Edm", "EDM"},
+		{"idm acronym", "Idm", "IDM"},
+		{"aor acronym", "Aor", "AOR"},
+		{"uk prefix", "Uk Garage", "UK Garage"},
+		{"uk hardcore", "Uk Hardcore", "UK Hardcore"},
+		{"acronym mid-phrase", "Trap Edm", "Trap EDM"},
+		{"acronym at the end", "Glitch Hop Edm", "Glitch Hop EDM"},
+		{"possessive", "Children'S Music", "Children's Music"},
+
+		// Already correct input must be left exactly alone.
+		{"correct acronym", "EDM", "EDM"},
+		{"correct possessive", "Children's Music", "Children's Music"},
+
+		// Case-insensitive, so a lower-cased tag also lands on the canonical
+		// form rather than becoming a third variant.
+		{"lowercase acronym", "edm", "EDM"},
+
+		// Names with an apostrophe followed by a real word are NOT contractions
+		// and must keep their capital — this is why the suffix list is fixed
+		// rather than "lowercase anything after an apostrophe".
+		{"irish surname", "O'Brien Core", "O'Brien Core"},
+		{"french elision", "D'Angelo Soul", "D'Angelo Soul"},
+
+		// Ordinary genres pass through untouched. Case is otherwise exposed as
+		// the file says it — "Rock" vs "rock" stays a real distinction.
+		{"plain", "Alternative Rock", "Alternative Rock"},
+		{"lowercase plain", "rock", "rock"},
+		{"hyphenated", "Lo-Fi Hip Hop", "Lo-Fi Hip Hop"},
+		{"ampersand", "R&B", "R&B"},
+		{"empty", "", ""},
+
+		// Punctuation around a word must not hide the acronym inside it.
+		{"parenthesised acronym", "Hip Hop (Edm)", "Hip Hop (EDM)"},
+		{"acronym with comma", "Edm, Trap", "EDM, Trap"},
+
+		// Interior punctuation stays in the core, so these are compared whole
+		// and cannot match a fragment by accident.
+		{"hyphenated stays whole", "Lo-Fi", "Lo-Fi"},
+		{"ampersand stays whole", "Drum & Bass", "Drum & Bass"},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			if got := trueUpCasing(tc.in); got != tc.want {
+				t.Errorf("trueUpCasing(%q) = %q, want %q", tc.in, got, tc.want)
+			}
+		})
+	}
+}
+
+// The casing repair must never touch a name resolved from the ID3v1 table. The
+// operator chose to keep that table canonical, so entry 40's 1990s spelling
+// "AlternRock" stays as-is even though it reads like damage.
+func TestNormaliseGenreValue_CanonicalTableNamesNotRecased(t *testing.T) {
+	if got := normaliseGenreValue("40"); !equalStrings(got, []string{"AlternRock"}) {
+		t.Errorf("bare 40 = %q, want [AlternRock]", got)
+	}
+	if got := normaliseGenreValue("(40)"); !equalStrings(got, []string{"AlternRock"}) {
+		t.Errorf("(40) = %q, want [AlternRock]", got)
+	}
+}
+
+// Casing runs on values that came from the file, including the parenthesised
+// and refinement paths.
+func TestNormaliseGenreValue_CasingAppliedToFileText(t *testing.T) {
+	tests := []struct {
+		in   string
+		want []string
+	}{
+		{"Edm", []string{"EDM"}},
+		{"(17)Uk Garage", []string{"Rock", "UK Garage"}},
+		// Punctuation around the acronym must not hide it — the letter core is
+		// what gets matched, and the trimmed edges are re-attached.
+		{"(Live Edm)", []string{"(Live EDM)"}},
+		{"((Children'S Music", []string{"(Children's Music"}},
+	}
+	for _, tc := range tests {
+		t.Run(tc.in, func(t *testing.T) {
+			if got := normaliseGenreValue(tc.in); !equalStrings(got, tc.want) {
+				t.Errorf("normaliseGenreValue(%q) = %q, want %q", tc.in, got, tc.want)
+			}
+		})
+	}
+}
+
+// Two spellings of one acronym in the same file collapse to a single tag rather
+// than surviving as near-duplicates.
+func TestNormaliseGenres_AcronymVariantsDedupe(t *testing.T) {
+	if got := normaliseGenres([]string{"Edm", "EDM", "edm"}); !equalStrings(got, []string{"EDM"}) {
+		t.Errorf("genres = %q, want [EDM]", got)
+	}
+}

internal/library/scanner.go

@@ -52,7 +52,11 @@ var audioExtensions = map[string]bool{
 //	   ("Alternative Rock" + "Rock" -> "Alternative RockRock"), which corrupted
 //	   the genre browse axis and polluted the taste profile's tag vocabulary,
 //	   and left bare ID3v1 numeric references unresolved (#2499).
-const tagReadVersion int16 = 1
+//	2: acronym and apostrophe casing repaired on genre values (#2468) — "Edm" ->
+//	   "EDM", "Children'S Music" -> "Children's Music". Bumped rather than left
+//	   to new files only because taste_profile.sql reads tracks.genre directly,
+//	   so a half-repaired library would carry both spellings as separate tags.
+const tagReadVersion int16 = 2
 
 type Stats struct {
 	Scanned int `json:"scanned"`

web/src/routes/library/genres/+page.svelte

@@ -30,6 +30,31 @@
     return genres.filter((g: GenreCount) => g.genre.toLowerCase().includes(q));
   });
 
+  // Count-first by default because that's what the server returns and it's the
+  // right default: the head of this list is genuinely where you're going. But a
+  // real library runs to several hundred genres with a long tail of one-offs
+  // (391 / ~3.7 per track on the operator's), and at that size "I know roughly
+  // what it's called" needs A-Z as much as filtering does.
+  //
+  // View state only, not a query parameter — same as `filter` above. Neither is
+  // worth making shareable, and putting one in the URL and not the other would
+  // be the inconsistent choice.
+  let sortMode = $state<'count' | 'name'>('count');
+
+  const visibleGenres = $derived.by(() => {
+    // Copy before sorting. With no filter applied `filteredGenres` IS the array
+    // held by the query cache, and Array.sort mutates in place — sorting it
+    // directly would reorder cached data for every other consumer.
+    const list = [...filteredGenres];
+    if (sortMode === 'name') {
+      // sensitivity 'base' so case and accents don't split neighbours apart.
+      return list.sort((a: GenreCount, b: GenreCount) =>
+        a.genre.localeCompare(b.genre, undefined, { sensitivity: 'base' })
+      );
+    }
+    return list; // server order: count DESC, then name
+  });
+
   let albums = $state<AlbumRef[]>([]);
   let total = $state(0);
   let loading = $state(false);
@@ -153,12 +178,30 @@
         <h1 class="font-display text-2xl font-medium text-text-primary">Genres</h1>
         {#if !index.isPending && !index.isError}
           <p class="text-sm text-text-secondary">
+            {#if filter.trim()}
+              {visibleGenres.length} of {genres.length} genres
+            {:else}
               {genres.length} {genres.length === 1 ? 'genre' : 'genres'}, straight from your file tags
+            {/if}
           </p>
         {/if}
       </div>
       {#if genres.length > 0}
+        <div class="flex flex-wrap items-center gap-2">
           <QuickFilter bind:value={filter} placeholder="Filter genres" />
+          <label class="flex items-center gap-1.5 text-xs text-text-secondary">
+            Sort
+            <select
+              bind:value={sortMode}
+              aria-label="Sort genres"
+              class="rounded border border-border bg-surface px-2 py-1 text-sm text-text-primary
+                     focus-visible:outline focus-visible:outline-2 focus-visible:outline-accent"
+            >
+              <option value="count">Most tracks</option>
+              <option value="name">A–Z</option>
+            </select>
+          </label>
+        </div>
       {/if}
     </header>
 
@@ -180,16 +223,17 @@
           </a>
         {/snippet}
       </EmptyState>
-    {:else if filter.trim() && filteredGenres.length === 0}
+    {:else if filter.trim() && visibleGenres.length === 0}
       <p class="text-text-secondary">
         No genres match <span class="font-medium">'{filter.trim()}'</span>.
       </p>
     {:else}
-      <!-- Ordered by track count, not alphabetically: raw tags carry a long
+      <!-- Defaults to track count rather than alphabetical: raw tags carry a
-           tail of one-offs, so alphabetical would bury the handful of genres
+           long tail of one-offs, so A-Z would bury the handful of genres you
-           you actually have a library's worth of. -->
+           actually have a library's worth of. The Sort control lets you ask for
+           A-Z when you already know roughly what you're looking for. -->
       <ul class="grid grid-cols-1 gap-2 sm:grid-cols-2 lg:grid-cols-3">
-        {#each filteredGenres as g (g.genre)}
+        {#each visibleGenres as g (g.genre)}
           <li>
             <a
               href={genreHref(g.genre)}

web/src/routes/library/genres/genres.test.ts

@@ -98,6 +98,57 @@ describe('/library/genres index', () => {
     expect(screen.getByRole('link', { name: /^rock 2$/ })).toBeInTheDocument();
   });
 
+  // #2468: the operator's real library is 391 genres at ~3.7 per track, so the
+  // count-first default buries anything you can already name. A–Z is the ask.
+  test('A–Z sort reorders the list without mutating the source array', async () => {
+    const data = [
+      { genre: 'Rock', track_count: 8974 },
+      { genre: 'Ambient', track_count: 646 },
+      { genre: 'jazz', track_count: 1406 }
+    ];
+    asMock(createGenresQuery).mockReturnValue(mockQuery({ data }));
+    render(GenresPage);
+
+    const order = () =>
+      screen.getAllByRole('link').map((a) => a.textContent?.trim().split(/\s+/)[0]);
+
+    // Count mode PRESERVES the server's order rather than re-sorting client
+    // side — the server already returns count DESC, name ASC, and duplicating
+    // that here would be two orderings to keep in step. The fixture is
+    // deliberately not in count order so this asserts pass-through, not luck.
+    expect(order()).toEqual(['Rock', 'Ambient', 'jazz']);
+
+    await fireEvent.change(screen.getByLabelText('Sort genres'), { target: { value: 'name' } });
+
+    // Case-insensitive, so 'jazz' sorts between Ambient and Rock rather than
+    // after both.
+    expect(order()).toEqual(['Ambient', 'jazz', 'Rock']);
+
+    // The query cache's own array must not have been reordered in place —
+    // Array.sort mutates, and with no filter applied the derived list IS that
+    // array.
+    expect(data.map((d) => d.genre)).toEqual(['Rock', 'Ambient', 'jazz']);
+  });
+
+  test('filtering reports the visible subset against the total', async () => {
+    asMock(createGenresQuery).mockReturnValue(
+      mockQuery({
+        data: [
+          { genre: 'Rock', track_count: 10 },
+          { genre: 'Ska Punk', track_count: 5 },
+          { genre: 'Jazz', track_count: 3 }
+        ]
+      })
+    );
+    render(GenresPage);
+    expect(screen.getByText(/3 genres, straight from your file tags/)).toBeInTheDocument();
+
+    await fireEvent.input(screen.getByLabelText('Filter genres'), { target: { value: 'punk' } });
+
+    // QuickFilter debounces by 120ms.
+    await waitFor(() => expect(screen.getByText('1 of 3 genres')).toBeInTheDocument());
+  });
+
   test('empty library explains where genres come from', () => {
     asMock(createGenresQuery).mockReturnValue(mockQuery({ data: [] }));
     render(GenresPage);