From 25f62e169e049f0757ef611d5b409e7523958dee Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Wed, 16 Sep 2026 16:07:06 +0200 Subject: music: pool single-album artists into shared rows An artist with one album got a heading and one cover on a row that fits five, and a library is mostly single-album artists. Consecutive singles share one grid, in place, so the page stays in artist order. Each pooled cover keeps its artist's name above it in the same type as a section heading. Dropping it was the first version and it was wrong: scrolling then alternates between artists written large and small. Measured on the probe's fixture: 4208px to 1895px, and a walk of the page reaches all 21 covers instead of 9. Co-Authored-By: Claude Opus 5 --- .../src/meshbay_hub/static/music-app.js | 94 ++++-- .../meshbay-hub/src/meshbay_hub/static/style.css | 24 ++ .../meshbay-hub/tests/harness/music_grid_probe.py | 316 +++++++++++++++++++++ packages/meshbay-hub/tests/test_music_grid.py | 133 +++++++++ 4 files changed, 549 insertions(+), 18 deletions(-) create mode 100644 packages/meshbay-hub/tests/harness/music_grid_probe.py create mode 100644 packages/meshbay-hub/tests/test_music_grid.py (limited to 'packages/meshbay-hub') diff --git a/packages/meshbay-hub/src/meshbay_hub/static/music-app.js b/packages/meshbay-hub/src/meshbay_hub/static/music-app.js index 8fc6cb3..baa3520 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/music-app.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/music-app.js @@ -348,10 +348,29 @@ function MusicDetailModal({ album, transportRef, gekRef, musicbrainzEnabled, onC // -- Mode A: album grid ----------------------------------------------------- -// `units` is one page of `{ artist, album }`. An artist whose albums straddle -// two pages gets its heading on both. -function AlbumGrid({ units, transportRef, gekRef, musicbrainzEnabled, onPlayQueue, onMenu }) { - const [detail, setDetail] = useState(null); // the album object +/** + * One page of `{ artist, album }` as the sections that get drawn. + * + * An artist with two or more albums gets a heading and a grid of their own. An + * artist with **one** does not: a heading plus a single cover is a whole row of + * whitespace, and a library is mostly single-album artists — a compilation + * bought once, one album of someone's, a soundtrack. Consecutive singles share + * one grid instead, so five of them fill a row that five headings would + * otherwise have spent five rows on. + * + * Pooled in place rather than swept into a bin at the end: the page is drawn in + * artist order and a reader scrolling it is relying on that. A pooled run sits + * exactly where its artists would have been. + * + * **Each pooled cover keeps its artist's name above it, in the same type as a + * multi-album artist's heading.** The first version of this dropped the heading + * on the grounds that the card already names its artist underneath — and that + * was wrong: scrolling then alternates between artists written large and + * artists written small, and the eye has to work out which kind of row it is + * looking at. The heading moves *into* the cell rather than going away, so a + * row of five costs one heading's height between them instead of five rows. + */ +function albumSections(units) { const artists = []; for (const u of units) { const prev = artists[artists.length - 1]; @@ -359,21 +378,60 @@ function AlbumGrid({ units, transportRef, gekRef, musicbrainzEnabled, onPlayQueu else artists.push({ artist: u.artist, albums: [u.album] }); } + const sections = []; + for (const a of artists) { + if (a.albums.length >= 2) { sections.push({ kind: 'artist', ...a }); continue; } + const prev = sections[sections.length - 1]; + if (prev && prev.kind === 'pool') prev.albums.push(a.albums[0]); + else sections.push({ kind: 'pool', albums: [a.albums[0]] }); + } + return sections; +} + +// `units` is one page of `{ artist, album }`. An artist whose albums straddle +// two pages gets its heading on both — and, with one album on each page, is +// pooled on both. +function AlbumGrid({ units, transportRef, gekRef, musicbrainzEnabled, onPlayQueue, onMenu }) { + const [detail, setDetail] = useState(null); // the album object + const sections = useMemo(() => albumSections(units), [units]); + + const tile = (album) => html` + <${LazyTile} key=${album.artist + '::' + album.album} cls="music-tile-slot"> + <${AlbumCard} album=${album} transportRef=${transportRef} gekRef=${gekRef} + musicbrainzEnabled=${musicbrainzEnabled} onOpen=${() => setDetail(album)} + onMenu=${onMenu} /> + + `; + + // Keyed on the first album rather than on the index: a pool's position shifts + // whenever a neighbouring artist gains or loses an album, and an index key + // would make preact reuse the wrong tiles across that change. + const sectionKey = (s) => (s.kind === 'artist' + ? `artist:${s.artist}` + : `pool:${s.albums[0].artist}::${s.albums[0].album}`); + return html` - ${artists.map((a) => html` -
-

${a.artist}

-
- ${a.albums.map((album) => html` - <${LazyTile} key=${album.artist + '::' + album.album} cls="music-tile-slot"> - <${AlbumCard} album=${album} transportRef=${transportRef} gekRef=${gekRef} - musicbrainzEnabled=${musicbrainzEnabled} onOpen=${() => setDetail(album)} - onMenu=${onMenu} /> - - `)} -
-
- `)} + ${sections.map((s) => (s.kind === 'artist' + ? html` +
+

${s.artist}

+
${s.albums.map(tile)}
+
` + : html` +
+
+ ${s.albums.map((album) => html` +
+ ${/* One line, clipped, with the full name on hover: a cell is + ~170px wide and a heading that wraps to two lines would + push its own cover below the others on the row. */''} +

${album.artist}

+ ${tile(album)} +
+ `)} +
+
`))} ${detail && html` <${MusicDetailModal} album=${detail} transportRef=${transportRef} gekRef=${gekRef} musicbrainzEnabled=${musicbrainzEnabled} onMenu=${onMenu} diff --git a/packages/meshbay-hub/src/meshbay_hub/static/style.css b/packages/meshbay-hub/src/meshbay_hub/static/style.css index 2bf0b18..5ef04b0 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/style.css +++ b/packages/meshbay-hub/src/meshbay_hub/static/style.css @@ -5044,6 +5044,12 @@ h2 .gn-owner, h3 .gn-owner { font-size: 0.55em; } max-height: calc(100vh - 16px); overflow-x: hidden; overflow-y: auto; + /* Wheeling past the last track must not carry on into the page behind: that + scroll is a real page scroll, and a page scroll closes this menu on + purpose (menu.js). Without this, reaching the end of a long tracklist + dismisses the menu, which is the same symptom the scroll handler was just + fixed for, arriving by a different door. */ + overscroll-behavior: contain; padding: 4px 0; background: var(--bg-surface); border: 1px solid var(--border); @@ -5192,3 +5198,21 @@ h2 .gn-owner, h3 .gn-owner { font-size: 0.55em; } overflow: hidden; text-overflow: ellipsis; } + +/* A run of single-album artists, pooled into one grid so five of them fill one + row instead of spending five (music-app.js `albumSections`). + + Each cell keeps its artist's name above the cover, in the same type as a + multi-album artist's heading. Dropping it — the first version of this — made + scrolling alternate between artists written large and artists written small, + which is a worse problem than the whitespace it saved. */ +.music-artist-pool { margin-bottom: 22px; } +.music-pool-cell { display: flex; flex-direction: column; min-width: 0; } +/* One line, clipped. A cell is about 170px wide, and a heading that wrapped to + two lines would push its own cover below the others on its row — the grid + stretches the cell, it does not align what is inside it. */ +.music-pool-heading { + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; +} diff --git a/packages/meshbay-hub/tests/harness/music_grid_probe.py b/packages/meshbay-hub/tests/harness/music_grid_probe.py new file mode 100644 index 0000000..365e028 --- /dev/null +++ b/packages/meshbay-hub/tests/harness/music_grid_probe.py @@ -0,0 +1,316 @@ +#!/usr/bin/env python3 +""" +How much of the Music grid is whitespace. + +Each artist used to get a grid container of their own, so an artist with a +single album got a heading and one cover on a row that fits five — and a real +library is mostly single-album artists. Consecutive singles now share one grid. + +Measuring is the only way to check this: the layout is `auto-fill` over a width +nothing declares, so what matters is the *rendered* rectangles. This reports +every cover's position, which row it landed on, and how tall the grid is. + + music_grid_probe.py +""" + +import http.server +import json +import socketserver +import subprocess +import sys +import tempfile +import threading +import time +from pathlib import Path + +STATIC = Path(__file__).resolve().parents[2] / "src" / "meshbay_hub" / "static" +PORT = 8757 +RECORDS = [] +socketserver.TCPServer.allow_reuse_address = True + +FRAME = r""" + + +
+ +""" + +PAGE = r""" +
""" + + +class H(http.server.BaseHTTPRequestHandler): + def log_message(self, *a): + pass + + def do_POST(self): + length = int(self.headers.get("Content-Length") or 0) + if self.path == "/log": + RECORDS.append(json.loads(self.rfile.read(length).decode())) + else: + self.rfile.read(length) + self.send_response(204) + self.end_headers() + + def _send(self, body: bytes, ctype: str) -> None: + self.send_response(200) + self.send_header("Content-Type", ctype) + self.send_header("Content-Length", str(len(body))) + self.end_headers() + self.wfile.write(body) + + def do_GET(self): + path = self.path.split("?")[0] + if path == "/": + self._send(PAGE.encode(), "text/html; charset=utf-8") + elif path == "/case": + self._send(FRAME.encode(), "text/html; charset=utf-8") + elif path == "/v1/groups/g1/nodes": + self._send(b'{"nodes": [{"node_id": "n1"}]}', "application/json") + else: + asset = (STATIC / path.lstrip("/")).resolve() + if not str(asset).startswith(str(STATIC)) or not asset.is_file(): + self.send_response(404) + self.end_headers() + return + self._send(asset.read_bytes(), + "text/css" if asset.suffix == ".css" + else "text/javascript" if asset.suffix == ".js" + else "application/octet-stream") + + +def main() -> int: + with socketserver.TCPServer(("127.0.0.1", PORT), H) as srv: + threading.Thread(target=srv.serve_forever, daemon=True).start() + with tempfile.TemporaryDirectory(ignore_cleanup_errors=True) as profile: + proc = subprocess.Popen( + ["google-chrome", "--headless=new", "--disable-gpu", "--no-sandbox", + f"--user-data-dir={profile}", "--window-size=1100,900", + f"http://127.0.0.1:{PORT}/"], + stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) + for _ in range(400): + if RECORDS: + break + time.sleep(0.1) + proc.terminate() + try: + proc.wait(timeout=10) + except subprocess.TimeoutExpired: + proc.kill() + proc.wait() + if not RECORDS: + print(json.dumps({"error": "no measurement"}), file=sys.stderr) + return 1 + print(json.dumps(RECORDS[0], indent=1)) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/packages/meshbay-hub/tests/test_music_grid.py b/packages/meshbay-hub/tests/test_music_grid.py new file mode 100644 index 0000000..e457474 --- /dev/null +++ b/packages/meshbay-hub/tests/test_music_grid.py @@ -0,0 +1,133 @@ +""" +How much of the Music grid is whitespace. + +Every artist used to get a grid container of their own, so an artist with a +single album got a heading and one cover on a row that fits five — and a real +library is mostly single-album artists: a compilation bought once, one album of +somebody's, a soundtrack. Consecutive singles share one grid now, in place, so +the page stays in artist order and a run of them fills a row. + +The layout is `auto-fill` over a width nothing declares, so reading the source +proves nothing: this measures the rendered rectangles in a browser and groups +the covers by the row they actually landed on. + +Every artist keeps their name at heading weight, pooled or not. Dropping it for +pooled covers was the first version of this and it was wrong: scrolling then +alternates between artists written large and artists written small, and the eye +has to work out which kind of row it is looking at. The heading moves into the +cell instead of going away. + +Measured on this fixture — three artists with several albums, twelve with one — +the page went from **4208px to 1895px**, and a walk of it reaches all 21 covers +instead of 9. +""" + +import json +import shutil +import subprocess +from pathlib import Path + +import pytest + +HARNESS = Path(__file__).parent / "harness" / "music_grid_probe.py" +STATIC = Path(__file__).resolve().parents[1] / "src" / "meshbay_hub" / "static" + +pytestmark = pytest.mark.skipif( + shutil.which("google-chrome") is None or not (STATIC / "music-app.js").exists(), + reason="Chrome or the SPA sources are not available") + +# The fixture's artists, in the order the grid sorts them. +MULTI = ["Alpha", "Bravo", "Kilo"] +# One of them is deliberately far too long for a cell, to catch a heading that +# wraps and pushes its own cover out of line with the rest of its row. +LONG = "Foxtrot Un Nom Vraiment Tres Long Qui Ne Tient Pas" +SINGLES_BEFORE_KILO = ["Charlie", "Delta", "Echo", LONG, "Golf", + "Hotel", "India", "Juliett"] +SINGLES_AFTER_KILO = ["Lima", "Mike", "November", "Oscar"] + + +@pytest.fixture(scope="module") +def grid(): + proc = subprocess.run(["python3", str(HARNESS)], + capture_output=True, text=True, timeout=300) + assert proc.returncode == 0, f"probe failed: {proc.stdout}{proc.stderr}" + out = json.loads(proc.stdout) + assert "error" not in out, out + assert not out.get("logs"), f"the page logged: {out['logs']}" + return out["steps"][0] + + +def test_every_cover_is_reachable_by_scrolling_the_page(grid): + """Tiles mount on intersection, so a page tall enough is a page whose last + covers a reader has to work for. Nine of twenty-one were reachable in the + same walk before.""" + assert grid["cards"] == 21 + + +def test_every_artist_is_named_in_the_same_type_pooled_or_not(grid): + """The correction. Three headings sit above a grid, twelve sit inside a + pooled cell — but all fifteen are `music-artist-heading`, so scrolling does + not alternate between artists written large and artists written small.""" + assert grid["headings"] == MULTI, "a section heading went missing" + assert grid["poolHeadings"] == 12, ( + "a single-album artist lost the name above their cover") + + +def test_a_long_artist_name_does_not_push_its_cover_out_of_line(grid): + """A pooled heading is clipped to one line. The grid stretches a cell to the + row height; it does not align what is inside the cell, so a heading that + wrapped would drop its own cover below its neighbours'.""" + heights = {c["headingH"] for row in grid["cells"] for c in row + if c["headingH"] is not None} + assert len(heights) == 1, f"pooled headings are not all one line: {heights}" + + for row in grid["cells"]: + tops = {c["top"] for c in row} + assert len(tops) == 1, f"covers on one row start at different heights: {row}" + + assert any(c["heading"] == LONG for row in grid["cells"] for c in row), ( + "the over-long name is not in the fixture any more, so this checks nothing") + + +def test_single_album_artists_share_a_row(grid): + """The point of the change. A row carrying several different artists cannot + happen while each has a grid container to itself.""" + shared = [r for r in grid["rows"] if len(set(r)) > 1] + assert shared, f"no row carries more than one artist: {grid['rows']}" + assert max(len(r) for r in shared) >= 4, ( + f"the widest shared row holds {max(len(r) for r in shared)} covers; at " + f"1100px the grid fits five") + + +def test_an_artist_with_several_albums_keeps_their_own_rows(grid): + """Unchanged, and deliberately: a heading earns its line when there is more + than one cover under it.""" + for artist in MULTI: + rows = [r for r in grid["rows"] if artist in r] + assert rows, f"{artist} drew no row" + for r in rows: + assert set(r) == {artist}, ( + f"{artist} shares a row with {set(r) - {artist}}") + + +def test_pooling_happens_in_place_and_keeps_the_page_in_artist_order(grid): + """Swept into a bin at the end, a run of singles would be easier to build + and would break the one thing a reader scrolling relies on. The run either + side of a multi-album artist is two pools, not one.""" + assert grid["pools"] == 2 + + order = [a for row in grid["rows"] for a in row] + first = {a: order.index(a) for a in order} + assert first["Charlie"] > first["Bravo"] + assert first["Kilo"] > first["Juliett"] + assert first["Lima"] > first["Kilo"] + + +def test_the_singles_are_pooled_with_their_neighbours_not_with_each_other(grid): + """Every single-album artist appears, and none of them alone on a row — + except where a row simply ran out of them.""" + order = [a for row in grid["rows"] for a in row] + for artist in SINGLES_BEFORE_KILO + SINGLES_AFTER_KILO: + assert artist in order, f"{artist} is missing from the grid" + lonely = [r for r in grid["rows"] if len(r) == 1] + assert not lonely, f"a cover is alone on its row: {lonely}" -- cgit v1.2.3