fix/231-setter-rollback
1250
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 logan
|
bcf3856b6f |
fix(config): put the old value back when a setter is rejected
Config.Save() validates the whole config, so a setter that assigned before validating did not merely fail its own call: the rejected value stayed in memory and failed every later save, of every unrelated setting, silently and for the rest of the session. Nothing reached disk, so a restart cleared it — which is what made the fault invisible and unreportable. The defect is precisely "assignment precedes a validation that can reject that argument", and that predicate enumerates seven setters rather than the whole file. Each snapshots the field and restores it on the error path. The remaining setters were read rather than assumed and are unchanged: shortcuts.Config.Validate returns nil unconditionally, the bools and SetFavoritesPlaylistID pass through no validation that inspects them, Config.Validate does not validate Downloads at all, and SetViewVisible refuses an unknown, non-hideable or launch-page view before assigning. SetLibraryDirectory was already correct and is the precedent the new comment points at: it validates a candidate before assigning, so there is nothing to undo. The rationale sits above the setter section rather than on Save(), which is bound — a doc comment there renders into frontend/bindings for an audience with no use for it. Closes #231 |
||
|
logan
|
5e25e14994 |
Merge pull request 'Split the README into a user-facing landing page and CONTRIBUTING.md' (#221) from docs/50-readme-landing-page into main
CI / check (push) Skipped
CI / e2e (push) Skipped
Build & publish the Android APK / apk (push) Successful in 1m50s
Build & publish Arch package / arch-package (push) Successful in 2m44s
Attach the desktop build to the release / linux (push) Successful in 1m17s
Sync Homebrew formula / sync-formula (push) Successful in 10s
|
||
|
logan
|
94ccea185c | Merge pull request 'fix(ui): the phone's nav sheet says when it scrolls' (#222) from fix/210-nav-sheet-scroll-affordance into main | ||
|
logan
|
d21b842d86 | Merge pull request 'fix(queue): name the queue header's two older actions' (#223) from fix/170-queue-header-action-names into main | ||
|
logan
|
f79249dfba | Merge pull request 'test(e2e): name the fixture tracks that really have no album' (#226) from test/217-fixture-names-in-queue-selection into main | ||
|
logan
|
f8c8d374d1 | Merge pull request 'fix(riff): grow a chunk buffer with what arrives' (#224) from fix/216-riff-parse-allocation into main | ||
|
logan
|
ec4961ae50 |
test(e2e): name the fixture tracks that really have no album
`queueSixAndOpen` filters the queue down to tracks that have an album, because `explore-link` renders a name it cannot route as plain text and one test clicks that name. The filter is right and unchanged; the comment explaining it named the wrong two files. Since #104 read a WAV's `id3 ` chunk, the two tracks under `Field Recordings/Test Tones` are tagged, scanned and ordinary. Asked of a seeded app rather than of the comment, exactly two tracks in the fixture library have no album: `unsorted/no-tags-at-all.mp3` and `unsorted/title-only.mp3`. The clause saying which change made the old names wrong is there so the next reader does not restore them. Closes #217 |
||
|
logan
|
a113b7bd62 |
fix(riff): grow a chunk buffer with what arrives
Parse sized its buffer from the chunk header, which is four bytes read off the file, so a truncated or malformed WAV declaring a 4 GB data chunk in a 2 kB file got 4 GB from the allocator before the read discovered there was nothing to put in it. The error was always right; the allocation happened first. io.CopyN into a bytes.Buffer is what ID3Chunk beside it has done since #104, and needs nothing new: the reader stays an io.Reader and the buffer grows with what actually arrives. The regression test measures rather than asserts the error, because the error is identical on a build that allocates the gigabyte. Measured on the pre-fix build: 1,073,750,920 bytes of TotalAlloc for a 42-byte container whose data chunk claimed 1 GiB. Closes #216 |
||
|
logan
|
b5bdba2f38 |
fix(queue): name the queue header's two older actions
Clear queue and Add queue to playlist were named by a `title` attribute and nothing else, while the close button beside them has carried an `aria-label` since #24. They get one too. `title` is a name, so this is the weak-name case rather than the missing one: it is the *last* fallback in the accname order, so any content put inside the button later silently outranks it, and a phone has no hover to show it. The `title`s stay — on a desktop they are also the tooltip for an icon-only control, which is a different job. The assertion is the part worth reading. The obvious spec — `getByRole` by name, which is what `queue-overlay.spec.ts` already does for the close button — is **green on the broken build**: measured against the running pre-fix app, both buttons matched. So the second test states the property as what it is, that the name is not the tooltip: it removes the `title` attributes and asks again, which was 0 and 0 on main and is 1 and 1 now. Closes #170 |
||
|
logan
|
20c337651f |
fix(ui): the phone's nav sheet says when it scrolls
Since #71 the phone's "More" is a bottom sheet, and at the reference viewport it does not fit: measured at 424x439 with the seed's eight destinations, the body is scrollHeight 412 against clientHeight 373, so 39px is below a fold nothing announces. Where the cut lands on a row boundary the sheet ends in a clean edge that reads as the end of the list, which is what #207 fixed one sheet over. The rule is that sheet's, not a second answer to the same question: #207's two background layers move into styles/sheet-scroll.css.ts and both sheets adopt them, with the colour left to each host as --yj-sheet-surface. The nav sheet paints the sidebar's --yj-bg-surface and the context sheet the menus' --yj-bg-elevated, so a shared rule that hard-coded either would draw that seam across the other one. The half that makes it visible is that nothing inside the sheet may repaint the surface. These are layers on the scroller, and app-sidebar's host carries the same grey -- in the shell its own background, in the sheet a second opaque copy of the sheet's, over the fade. With the fragment adopted and that rule missing, the running app measured a flat 52,58,64 to the bottom edge with 39px still below: the defect unchanged, with every assertion about background-attachment passing. menu-surface already meets it from the other side, where the sheet's panel is background-color: transparent. Closes #210 |
||
|
logan
|
1c08d8db90 |
docs: split the README into a landing page and CONTRIBUTING
The README was two documents in one, and neither reader was served by the other's half. It opened on a feature list, then spent its second half on Go versions, WebKitGTK packages and `make` targets — while its install table named a `darwin-universal.app.zip` and a `windows-amd64.exe` that nothing has ever produced, and its header claimed Windows and never mentioned Android, which is the one platform with a published, self-updating channel. So this is a correctness pass as much as a friendliness one. The README now answers a user's questions only: what the app is, three screenshots from the seeded fixture library so anyone can retake them, the four formats, one install section per channel that names what is actually published, first run, where the data lives, and pointers out. The version-restart note is linked to the two documents that own it rather than copied, because a copy is a second thing to keep true. CONTRIBUTING.md takes the technical half: prerequisites, the system libraries, the build and codegen commands, which verification tier a change demands, the tracker workflow, the commit grammar and the style rules. CLAUDE.md is unchanged apart from one paragraph naming the split — it was already the deep reference both of the others point at, and stays the only one of the three that explains why a shape is what it is. Closes #50 |
||
|
logan
|
245647f12b | Merge pull request 'feat(ui): warm album art ahead of the scroll' (#215) from feat/65-art-prefetch-ahead into main | ||
|
logan
|
3479ae8d39 |
feat(ui): warm album art ahead of the scroll
Scrolling the albums grid pops art in: the cards already draw the smallest adequate tier and are already lazy, so what was left is *when* the request happens. The grids are virtualized, so the `<img>` — and therefore the fetch — does not exist until the virtualizer renders its card, which is about 1000px past the viewport, or two screens on the reference device. The issue asks for a larger overscan and that is not available: `_overhang` is a hard-coded `protected` field on `BaseLayout` with no configuration surface. So the request is issued ahead of the element instead. `utils/image-prefetch.ts` warms a bounded window either side of the rendered range, from `rangeChanged` rather than `visibilityChanged` — the two report different ranges, and a window measured from what is *visible* is spent on cards that already exist. Cover and artist URLs are served under `Cache-Control: immutable` (content-hashed filenames), so a prefetched image is a cache hit by the time its card is drawn. The bytes are the browser's; what this holds is the set of URLs asked for, capped and reported to `__yjCacheStats()`. Measured on the bulk seed (4 988 albums), ten 2 400px jumps, covers in the viewport with `naturalWidth === 0`: 254 of 258 blank one frame after the jump and 214 two frames after, against 117 and 77 with the prefetch. Closes #65 |
||
|
logan
|
e23e6f9a54 | Merge pull request 'feat(android): the phone's "More" is a bottom sheet' (#211) from feat/71-more-as-a-bottom-sheet into main | ||
|
logan
|
52d095e3c6 |
feat(android): the phone's "More" is a bottom sheet
The tab bar's fifth item opened `<app-sidebar>` in a `wa-drawer` sliding in from the side, which is a desktop shape put on a phone: a 200px column of a 424px screen, opening away from the thumb that asked for it, with the rest of its 400px band empty. It also had three nested scrollers in it -- the dialog, its body, and the sidebar's own `overflow-y: auto` host -- so which box a drag moved depended on where the finger landed, which is the "only part of the screen scrolls under my finger" in the report. It is the same element with `placement="bottom"` and `without-header`, so the surface is the sheet #60 already built rather than a second pattern: a `wa-drawer` is a native `<dialog>` opened with `showModal()`, which is exactly the top layer that finding rests on, so the focus trap, Escape, tap-outside and `wa-after-hide` come along unchanged and nothing new has to be proved about paint containment. The sidebar is still mounted rather than re-listed as data, because the shell's own copy is `display: none` below 600px rather than removed -- a second list drawing `nav-*` handles is the duplicate-testid failure this component already renders conditionally to avoid. What `expanded` means had to grow to say the host owns the *box*: `app-sidebar` writes an inline width and caps itself at 400px, which beats any rule the host could write, so the width, the scrolling and the mouse-only resize handle now follow that attribute. The rows are 48px below 600px, stated in the sidebar's own stylesheet since that is the only place it renders there. Measured in the running app at 424x439: the sheet is 424 wide, 373 tall (85vh, so there is an outside to tap), rows 48px, one scroller with `overscroll-behavior: contain`, and Settings' row reachable at the end of it. Desktop and Compact are untouched. Closes #71 |
||
|
logan
|
939915b1fa | Merge pull request 'feat(android): the tap highlight goes, a press state replaces it' (#214) from feat/54-native-touch-feel into main | ||
|
logan
|
3aa2a434b4 |
feat(android): the tap highlight goes, a press state replaces it
The phone drew a grey box over the bounding rect of whatever was tapped, which is the web view saying what it is. It is gone in one declaration: `-webkit-tap-highlight-color` is inherited and an inherited property crosses a shadow boundary, so `html` in index.css reaches every shadow root in the app. Measured three roots deep, rgba(0, 0, 0, 0.18) before and rgba(0, 0, 0, 0) after. Removing it removes the only touch feedback several surfaces had, so the press state is part of the same change rather than a later polish item — with the highlight gone a held row measured the *hover* tint, which on a phone is synthesised by the hold itself and outlives it. The four lists' rows, the tab bar, the sidebar's destinations and the shared context-menu item take --yj-press-overlay on :active; the cards already had scale(0.97). The press selector carries a state class because a row is .track-row.selected.active, so a bare :active shows nothing on the row a phone is most likely to press. And those surfaces' hover tints move behind (hover: hover) and (pointer: fine), which is #68's gate applied to a tint rather than a revealed control. user-select, the other half of the Findings, was already done: the first rule in index.css covers the shadow roots for the same reason. touch-action: manipulation is declined — the 300ms delay it is offered for is already absent on a width=device-width viewport, and what it would really change is the gesture stack tuned by measurement on a device this session cannot measure. Closes #54 |
||
|
logan
|
944995dc3c | Merge pull request 'feat(android): a name is not a link on a phone, the menu carries it' (#208) from feat/67-entity-links-into-menus into main | ||
|
logan
|
8de412cf36 |
feat(android): a name is not a link on a phone, the menu carries it
Every track, album and artist name in the app navigates through `utils/explore-link.ts`, and every sentence of how it does that is a desktop compromise: the navigation is held for one double-click interval so double-clicking the row can still play it, and the target is a few characters of text inside a row. On touch that is a delay on an ambiguous target, and since #63 the row's own tap claims the click anyway -- so the link was unreachable as well as fiddly. So below the phone breakpoint a name renders as plain text and the row's context menu carries the destination instead: `go-to-menu.ts` draws "Go to Artist" / "Go to Album" under exactly the condition the link is not, using `explore-link`'s own exported routing so an untagged entity reaches the library page by the same lookup. Three things this leans on. Suppressing a link with no menu behind it is not a smaller affordance but a destination the phone cannot reach, so `keepOnPhone` is the exception for the three surfaces with no row menu. The items are drawn for a single selection only, which is the Play item's rule one step on. And there is no "Go to Genre", because no row renders a genre link to lose -- that would be new navigation rather than a replacement. Closes #67 |
||
|
logan
|
aeb173c684 | Merge pull request 'fix(ui): the phone's context sheet says when it scrolls' (#209) from fix/207-sheet-scroll-affordance into main | ||
|
logan
|
025ed59480 | Merge pull request 'test(ui): refresh two stale baselines and settle whether they gate' (#205) from test/196-visual-tier-gates into main | ||
|
logan
|
a82d29abd7 |
docs(agent): drop #204's workaround from the baseline rule
#204 landed first, so the ui-tier rule can name `make ui-visual-update UI_ARGS=<path>` rather than the raw vitest invocation it needed while the recipe swallowed its filter. |
||
|
logan
|
d365850321 |
test(ui): refresh two stale baselines and settle whether they gate
`make ui-visual` had been red on main since #27, and nothing runs it, so four references had drifted across three unrelated merges. Two were refreshed with #186; these are the other two. Each recorded two changes, not one. `app-sidebar` lost Jobs (#27, shipped) and moved its highlight from Home to Tracks; `now-playing` gained the source line (shipped) and was playing from "a dynamic mix". Both are singleton stores read by a case that sets nothing, so the shot photographs whatever the case above it left behind — blessing that would have pinned the file's own ordering into a PNG. Both cases state their world now, and only then are the references re-recorded. The second half of the issue asks whether this tier should gate, and the answer is measured rather than preferred: replayed in a bare ubuntu:24.04 container — CI's `check` image — three of the ten baselines fail on rendering alone (`track-info` and one `page-header` shot at ratio 0.03 against a 0.02 allowance, `seek-bar` one pixel shorter), and the two stale ones disagree about their new height between the machines. So CI cannot run this suite without a second, container-recorded baseline set that every local run would then fail against, and a pre-push hook is the same fault with the machines swapped. It stays local and opt-in; what replaces the gate is the rule that a change moving a component's geometry refreshes that component's baseline in the same commit, having read the image, and never one it did not cause. Written where a person meets it: the skill's tier doc has the table, SKILL.md has the obligation, CLAUDE.md has the constraint. Deleting the baselines was the third option and is declined: this tier has caught one thing no other could, the `<span>` that lost the UA stylesheet's `box-sizing` and grew a badge 36→38px. Closes #196 |
||
|
logan
|
3a2d3e8ef8 | Merge pull request 'fix(metadata): read a WAV's tags out of its RIFF id3 chunk' (#218) from fix/104-wav-tags-read into main | ||
|
logan
|
871a3b7aac | Merge pull request 'test(ui): make ui-visual-update honour its file filter' (#206) from fix/204-ui-visual-update-filter into main | ||
|
logan
|
f3207e8bf9 | Merge pull request 'test(ui): clear localStorage between component tests' (#219) from fix/138-ui-test-storage-leak into main | ||
|
logan
|
772c71c49f |
test(ui): clear localStorage between component tests
A test file does not get its own origin. `@vitest/browser-playwright` opens one BrowserContext per session and runs several files in it, one after another, so everything a component persists survives from file to file. `track-list` restores its sort in `connectedCallback` and `aria-tail.test.ts` activates the Title column header, so any file that mounts a track list later in that tab opens sorted by title — where `track-11` precedes `track-3`, which is #138's failure exactly. Nothing about it is specific to that pair: a probe that throws when a test starts with a non-empty `localStorage` failed 24 test-starts in one full run (11 with the two sort keys, 8 with `cover-grid-size`, 5 with `track-list-column-widths`) and cascaded into 248 failures. Which files share a tab, and in what order, changes run to run, which is the whole of why this reads as a 1-in-3 flake and passes in isolation. The clear belongs in `setup.ts` rather than in the specs that write, because the spec that reads is never the one that knows — and it is safe for the same reason the leak exists: files within a session are sequential, so it cannot wipe storage a concurrent file is using. The spec now also states the order it asserts rather than inheriting a default, and checks the row it is about to double-click carries the path it expects, so a stray sort fails by naming itself instead of as an off-by-eight file path. Closes #138 |
||
|
logan
|
c56eae2959 |
fix(metadata): read a WAV's tags out of its RIFF id3 chunk
tagwriter has always written a WAV's tags into a RIFF "id3 " chunk correctly, and dhowden/tag -- which metadata.ExtractTags is built on -- has no RIFF reader at all. So the app could not see tags it had just written: editing tags on a WAV, autotagging a WAV folder or importing a WAV download all appeared to succeed and changed nothing the library could show, while the file on disk really was tagged and other players read it. backend/riff is a new package rather than a move into either half, because tagwriter already imports metadata: reaching back for parseRIFF is an import cycle, not merely the wrong direction. backend/tagtotals is the precedent. Its two readers are deliberately different. Parse holds every chunk in memory, which is what rewriting a file needs -- and a WAV's audio *is* a chunk, so doing that on the scan path would read every WAV in the library in full. ID3Chunk seeks over what it is not looking for. The container is asked before tag.ReadFrom rather than after it fails, because that library's last resort is an ID3v1 trailer and a WAV carrying both would otherwise be read by the wrong one. An untagged WAV -- no chunk, an RF64 container, a tag with every frame cleared -- reads as empty metadata with no TagReadWarning: the scanner's filename fallback is the right answer there, and a warning would report a fault on a healthy file. The gap was pinned by TestWAVTagsAreNotReadableYet, which failed the moment the reader learned and said in its own comment what to update. So it goes, TestFixturesMatchManifest no longer skips wav, and totals_test.go's WAV case reads through metadata.ExtractTags like the other three formats -- a round trip asserted through the writer's own parser was a test of the writer, which is why nothing caught this. Closes #104 |
||
|
logan
|
8c85db8968 |
docs(ui): quote the shipped fade's own measurement
The comment on `wa-dialog::part(body)` carried bottom-edge pixels from an intermediate probe (29,33,36) while `.planning/NOTES.md` recorded the final sample against the shipped rule (22,24,27) — the same gradient, read a few pixels higher up the box. A measurement written in two places has to agree, or neither can be trusted. |
||
|
logan
|
02e2251bb2 |
fix(ui): the phone's context sheet says when it scrolls
The bottom sheet's body has scrolled since #60 and said nothing about it. Measured at 424x439, the track list's menu ended at y=470 with the fold at 439 — reachable, since the body is `overflow-y: auto`, but with no affordance saying so, and worst where the cut lands on a row boundary and the sheet ends in a clean edge that reads as the end of the list. The cap stays: `menu-surface`'s own comment says a surface covering the whole screen is a page, not a sheet. What changes is that the body draws a fade, from two background layers whose *attachments* are the feature — a shadow pinned to the box (`scroll`) under a cover of the sheet's own colour painted at the end of the content (`local`), which scrolls up over the shadow exactly when there is nothing more to see. So the fade is absent on a menu that fits, present the moment one does not, and gone again at the end of the list, with no scroll listener and nothing reaching into `wa-dialog`'s shadow root for the scroller. `background-attachment` is Chrome 4; the reference device is Chrome 113. The other two options in the report — a shortened last row, or a max height that makes the cut obvious — both need `height mod 48`, which CSS cannot express, and the observed case is exactly the one where the cut already lands on a row boundary. The curve is steep rather than linear because the rows under it stay live: a scrim over a menu item is that item's text surface, so the 4.5:1 rule reaches it, and the light ramp is what makes that real. A 48px linear scrim at 0.8 greyed the last label to 5.0:1; 32px already down to a quarter strength at 14px measures 9.9:1 there and spends its weight on the strip below it. The test asserts the pair of attachments rather than the pixels, on this file's existing grounds that no tier here renders like the device — it fails on the pre-fix stylesheet with `expected 'scroll' to be 'local, scroll'`. The rendered result was measured in the harness and is recorded in `.planning/NOTES.md`. Closes #207 |
||
|
logan
|
e5d0f2714b |
test(ui): make ui-visual-update honour its file filter
vitest parses a bare `--update` as taking the next positional as its value, so `make ui-visual-update UI_ARGS=<path>` handed the path to the flag and ran with no filter at all: 99 files, every baseline in the repo re-recorded, any stale one blessed in silence. Two paths were worse still — the first was eaten and only the second ran. That is #196's own hazard living in the tool meant to resolve it: the rule is "refresh the reference your change moved and never one you did not cause", and the documented way to refresh one refreshed the set. `--update=true` is the whole fix, with the reason beside it because `=true` reads like something to tidy away. `make ui-visual` and `make ui-test` are unaffected — their `$(UI_ARGS)` follows `run`, with no flag to swallow it — and no other target interpolates a variable after a boolean flag. Closes #204 |
||
|
logan
|
ee1d8b3179 |
Merge pull request 'Android touch model, phases 2-4: swipe to queue, and the other three lists' (#201) from 63-touch-model-phase-2 into main
CI / e2e (push) Successful in 9m28s
CI / check (push) Successful in 2m31s
Build & publish the Android APK / apk (push) Successful in 1m27s
Build & publish Arch package / arch-package (push) Successful in 2m39s
Attach the desktop build to the release / linux (push) Successful in 58s
Sync Homebrew formula / sync-formula (push) Successful in 6s
|
||
|
logan
|
29feb4b94b |
feat(android): the touch model reaches the other three lists
Plan 019 phases 3 and 4, which finish #63. The queue panel and both playlist detail views get tap-to-play and hold-to-select; the playlist views get swipe-to-queue as well. Phase 3 was not the pure wiring the plan expected, in two places. A tap on a queue row plays that position. Copying track-list's tap -- which sets the queue to the list the row is in -- would rebuild the queue from the queue, discarding its source, its shuffle order and anything inserted by hand. It reads as a no-op and is not one. And the queue panel has no swipe, deliberately. A right swipe means add to the queue everywhere else it exists, and a queue row is already in the queue; the only thing it could mean there is remove, which is the same gesture with the opposite effect one screen away. Removing a queue row is on the row, on its sheet since #60, and now on its selection bar. The assertion is that its rows do not opt in. The reveal became utils/swipe-to-queue.ts rather than being copied into three lists, keyed on a data-swipe attribute so one stylesheet carries the touch-action half of the device fix to rows that are called two different things. Phase 4 was already true and is now asserted: a claimed tap has its click swallowed, so an explore-link inside a row never sees one and tap-to-play wins with no rule of its own. Its test was vacuous when written -- the tap helper sent no click, so there was nothing to swallow -- which also weakened phase 1's. It sends one now. Escape leaves selection mode, from selection-bar rather than from each of the four hosts, since that element exists only while the mode does. The platform's back gesture deliberately does not reach it: the shell owns the history stack and four lists reaching for history is four stacks. That is #200. Verified on the reference phone: a queue row taps to its own index and refuses a swipe, a playlist row queues on a swipe and plays its playlist on a tap, and a hold raises the bar without the menu. Closes #63 |
||
|
logan
|
4e667759c4 |
feat(android): swipe a track row right to queue it
Plan 019 phase 2. A finger on a track row now drags a reveal out from under it and queues the track on release, with the affordance saying what it will do before it does it. Two things the device said that the plan did not predict, and both change the implementation rather than decorate it. The gesture runs on touch events, not pointer events. Chrome 113's WebView cancels the pointer stream ~16px into any drag whatever touch-action says -- measured at auto, pan-y and none alike -- while touchmove keeps firing. So touch-action: pan-y is half the fix and a non-passive touchmove calling preventDefault is the other half, and neither works alone: with the preventDefault in place and touch-action back at auto the gesture died after one move. Both are correct in Chromium either way, which is why the module's header carries the measurement and the component tier asserts the stylesheet. And a phase 1 defect the device found on the way past: the native contextmenu arrives in either order and only one was handled. Our 500ms timer firing first, a component claiming it, and Chrome delivering its own menu 50-70ms later was suppressed by nothing -- so the context menu opened over the selection bar, two holds in four, on the one surface this issue exists to have changed. Six holds clean after. draggable="true" is not a competitor: no dragstart fires from a touch drag on this WebView at all. |
||
|
logan
|
ff3875b55d | Merge pull request 'Android touch model, phase 1: tap to play, hold to select' (#199) from 63-android-touch-model into main | ||
|
logan
|
76e1c444cc |
feat(android): tap to play, hold to select
Phase 1 of #63, and the design the issue asks for as one piece is .planning/plans/active/019-android-touch-model.md. **A finger has no second button and no modifier keys**, so the primary action has to be the primary gesture: tap plays the row, and the hold that opened a context menu now enters selection mode with that row selected. Three decisions in it, and two diverge from the report. **The predicate is the pointer, not the platform or the viewport.** `pointerType === 'touch'`, per event, which is already how long-press.ts decided and is the only such test in the frontend. This is #64's rule -- named after the capability -- and it carries #64's warning: keyed on a width, an Android *tablet* at 600px gets click-selects/double-click-plays on a touchscreen, which is the inversion this issue exists to fix, on the platform it exists for. A touchscreen laptop cannot be described by a width at all. Per event, a mouse keeps desktop semantics on the very same row, and there is no second declaration of what a phone does. **There is no double-tap, and the number is why.** The report asks for single tap to play *and* double tap for the menu. Those cannot both be honoured: the first tap of a double tap is indistinguishable from a single tap until the interval expires, so "tap plays" becomes "tap waits". Measured on the device, the play command to TrackChanged is 155/123/85/56/91 ms -- median ~100 -- and the app's own DOUBLE_CLICK_GRACE_MS is 250. That is 3.5x the primary interaction, 250ms of it spent deliberately doing nothing, on every track anyone plays, to reach a menu the hold already reaches. So the menu and the selection action bar are the same surface, which is also the platform's convention and removes a concept rather than adding one. **Tap-to-play and selection mode ship together**, because splitting them is a regression dressed as an increment: a touch user selects by tapping today and acts through the long-press menu, so moving tap to play on its own would leave a window with no way to select forty tracks at all. **What lets this reassign the hold without touching one of the fourteen context menus**: the layer announces `yj-tap` / `yj-long-press` (composed, cancelable) and acts on nothing. A component claims one with preventDefault. An **unclaimed long press still becomes a `contextmenu`**, so the card grids, Explore, the playlist rows and every other menu behave exactly as they did, and only lists that opt in get selection mode. An unclaimed *tap* does nothing at all and the click follows normally, which is what leaves every button in the app alone -- only a claimed tap has its click swallowed, or playing a track would also select it. **And the device found the one thing no browser tier can see.** Chrome 113's Android WebView fires its own `contextmenu` on a long press. long-press.ts stood down when a trusted one arrived, which was right while both paths ended in a context menu; they no longer do, so standing down means the gesture silently does the *old* thing. Measured, before the fix, holding a track row: {"log":["contextmenu isTrusted=true"], "state":{"bar":null,"menuActive":true,"selected":1}} `yj-long-press` was never announced, the menu opened, and all 26 tests passed -- dispatched pointer events do not make a browser synthesise one. So the native event is a **trigger, not a competitor**: the gesture is announced from it and only a claim suppresses it. Unclaimed it propagates untouched, which is the same "browser wins" outcome reached by asking instead of assuming. The tier could not find that and can hold it, because this module has always told its own events apart by identity rather than isTrusted, so an untrusted one from a test takes exactly the browser's path. Verified on the device by *performing* the gestures rather than describing the page -- `adb shell input tap` and `input swipe x y x y 700` reach the WebView as real pointer events, which is new here and is written down in the plan with the pixel mapping. Tap plays; a hold raises the bar with one selected and no menu; a tap toggles to two, back to one, and the mode ends with the last row; an album card still opens its context menu. 29 new tests. The e2e spec is rewritten to assert **both** halves -- the row selects, and a card elsewhere still opens the real menu -- because a spec that only checked the row would pass on a build that had silently broken the other thirteen. Phases 2-4 (swipe to queue, the other three surfaces, and what #67 inherits) are in the plan and not in this commit. |
||
|
logan
|
4f32d4e13c |
Merge pull request 'Android: raise every remaining control to the 44px touch floor' (#198) from 186-touch-targets-settings into main
Closes #186 |
||
|
logan
|
4f628b1f52 |
fix(ui): raise the last controls below the touch floor
The rest of #186's second table, and one thing it could not have said. .section-toggle 187x15 autotag .folders-menu-trigger 32x18 autotag .back-button 32x32 artist-details Requests / Downloads tabs 85x34, 96x34 .search-mode-tab 89x26, 79x26 explore explore search input 325x18 in a 36px box **back-button was six controls, not one.** The issue names it in artist-details because that is the view the sweep opened; the same declaration is byte-identical in artist-details, genre-details, playlist-details, smart-playlist-details, explore-artist-details and explore-album-details, 32px in all six. So it is styles/back-button. css.ts now, adopted by each, and a source sweep fails on a seventh copy -- because the failure this invites is not a size changing, it is somebody adding a detail view and writing `.back-button` out again, which no device sweep would catch for the same reason this one did not. That is icon-language.test.ts's shape, and the argument for it here is the inverse of the column arrows': one declaration covering 36 controls is cheap to fix, and six declarations of one control are six chances to miss five. It is a real 44px box rather than padding with the width handed back: a detail header runs no fit pass, and this button has a visible background, so a hit area larger than the circle would be a control bigger than it looks. The size is #55's, reached there for the same reason -- "the way out is 44px on a phone". **The explore search box was two faults.** The row was 36px *and* the input inside it was 18, so eight pixels at each edge were not a target at all: a tap near the top of the box landed on the container and did nothing. The container is 44 and the input stretches to it. **The Downloads tabs take padding rather than a min-size**, because the mark for the selected tab is its bottom border -- a min-size centres the label and leaves the underline 10px beneath it. page-action-check-now (113x29) is in that table and is not here: it is a PageAction, so #195 raised it with the rest of the header's actions and touch-targets.test.ts already covers it. **The Downloads tabs needed a min-size as well as the padding, and CI is what said so.** Padding alone made them 44px on this machine and **43px in the container**: the total is 13 + 13 + 2 + whatever line box the font gives 13px text, and ubuntu:24.04's is a pixel shorter than Arch's. A height computed from a font's line box is not a height you control -- which is #195's "stated as a property on the strength of one engine" one layer down, in the same PR that recorded it. The padding stays, because it is what keeps the underline against the label; the min-size is the floor. Caught by the new test rather than by a person, which is the half of this that worked. Verified on the device, sweeping each view the way the issue was filed: explore, downloads, autotag and artist-details now report **one** control under the floor apiece, and it is the skip link, which #186 already ruled out as keyboard-only. .search-mode-tab 89x44 and 79x44, the search input 325x44, the Downloads tabs 85x44 and 96x44, .section-toggle 174x44, .folders-menu-trigger 44x44, .back-button 44x44. All 12 new tests fail on main, the source sweep naming all six copies. make ui-test 1041 pass; make e2e 236 pass on chromium, which is half an answer -- CI had the other half, and used it. Closes #186 |
||
|
logan
|
2100f0022f |
fix(settings): raise every Settings control to the touch floor
#56 named 44px and #195 took the page header there. Settings is the other half of #186 and much the larger one: swept on the reference device (TLP301, 424x439) with all eleven config-sections expanded, **120 controls** were under the floor -- not the 93 the issue's table implies, and config-field is eight of them. The bulk is behind the disclosures, which is why nobody had counted it: 36 .column-arrow-btn 16x14 <- smallest in the app 29 .column-toggle 16x16 26 shortcut-capture button 80x25 8 download format checkbox 16x16 7 config-field select 335x30 6 wa-input / wa-button 204x20, 185x21 **The density argument, measured rather than guessed, and it is smaller than it looks.** The rows were already near the floor -- .column-item is 335x36 and .shortcut-row 335x37; it is the controls *inside* them that were 14-25px. So a control grows into the row it already occupies and the row goes 36 to 44. Measured after: the two column lists went 373->447 and 690->850, +234px over the whole page. Half a screen of extra scroll on a page that already scrolls, against 36 targets of 16x14. **Settings is cheaper than the header was, and for a stated reason.** There is no overflow fit on this page, so the header's "only width is contested" rule does not bind at all and nothing here needs padding with a negative margin. Height is a min-size, and the two square controls can simply be square. Three shapes, because one rule does not fit three kinds of control: **A native checkbox is targeted through its label.** It cannot grow its hit area without growing its paint, and a 44px checkbox is not what anyone wants -- so .column-label is a real <label for> now and the column's *name* is the target, 70x44 rather than 16x16. That is the argument config-field already makes one file over ("a real label association also makes the label text a click target, which is behaviour, not annotation"), and here it is the whole fix. The download formats already had the label; they only needed the height. **The arrows take padding, which is invisible.** They carry background: none and a transparent border, so 16x14 -> 44x44 changes nothing anyone can see until hover -- #186's Direction exactly. **Web Awesome's controls come from the library's own API.** Their height is decided inside somebody else's shadow root, and --wa-form-control-height is the variable that decides it. A custom property inherits through a shadow boundary, so a :host declaration reaches them; styles/wa-touch-floor.css.ts is that, once, adopted rather than written at :root in index.css -- a :root rule would be invisible to the component tier, which renders a component and no page stylesheet. **Two controls no sweep can see are fixed by name**, and they are the trap this issue keeps setting. config-field's toggle has an <input> that is opacity: 0; width: 0; height: 0, so a walk of every input skips it as a zero-sized node -- what a finger hits is the <label>, which measured **34x19**, smaller than anything in either of #186's tables and absent from both. It is 44x44 with the pill still painted at 2.5em x 1.4em and negative inline margins keeping it flush with the inputs above. And shortcut-capture's reset button renders only for a shortcut somebody has rebound, so a sweep of a fresh install never meets it. Verified on the device, same method as the sweep that filed it: 120 controls under the floor before, 42 after. All 42 are accounted for -- 37 are checkboxes whose labels measure 70x44 and 57x44, four are wa-input's inner input at 204x**42**, which is the control measured *inside* its own 1px border (part=base is 238x44), and one is the skip link, which #186 already ruled out as keyboard-only. The e2e suite passes, top-bar-fit and header-action-overflow included -- but that is **chromium**, which is half an answer, and saying so is the whole of what #195's second commit was about. What can be argued rather than run: library-filter is the only thing here in a container that measures itself, and its width did not change. The fit measures inline size. Two page-header screenshots are refreshed because they are this issue's own debris -- #195's taller sort control, merged last session, with its references never re-recorded. app-sidebar's and now-playing's are deliberately left: they are unrelated drift, and blessing an unrelated screenshot is how the sidebar reference came to still list a destination #27 retired. That is #196. |
||
|
logan
|
52038dc5ae | Merge pull request 'Android: raise the page header and the phone search button to the touch floor' (#195) from 186-touch-targets-page-header into main | ||
|
logan
|
0d331666d6 |
fix(shell): make the header's touch targets cost no width
The first pass grew the two square controls to 44px as boxes, which
added 22px to the header. That fit at every width Chromium was checked
at and **clipped the overflow trigger at 320x600 in WebKit** -- the
engine closest to what ships, and the one no machine here can run:
every action is reachable at 320x600 (400% zoom)
- Array []
+ Array [ "more" ]
Two things were wrong, and only one of them was the code.
**The claim was checked on one engine and stated as a property.** The
previous commit said #69's fit "does not move ... the check rather than
the assumption", on the strength of running that spec against chromium
alone. CI runs both browsers precisely because they are not the same
answer.
**And the box was the wrong thing to grow**, which the issue already
said: "reached by growing the *hit* area rather than the visual weight
where the two can differ -- padding on the control, not size on the
icon". #69's pass measures inline size, so a taller control is free and
a wider one is not.
So height stays a box -- the header has the room and nothing measures
it -- and width is padding with a negative margin handing the space
back, which is the seek bar's shape from #187. Measured in the
component tier at 320px: the arrow's rect is 45x44 and it occupies 29,
the overflow trigger 44x44 occupying 38, the search button 44x44
occupying 40. Those three occupancies are what they were before any of
this, so the fit pass sees a header identical to main's and the
320px case cannot regress.
The arrow's target is lopsided for #187's reason: the select is 6px to
its left and there is open space to its right, so it takes the side
with nothing to steal from. The overflow trigger's can be symmetric,
the actions row having an 8px gap.
`search-trigger` is border-box, so its 44px min-width is the whole
target and the margin alone gives the four pixels back.
The new assertion is the one that would have caught this: every grown
control must carry negative inline margins, because that is what keeps
the box out of the fit. The rect assertions stay -- getBoundingClientRect
includes padding, so the target is still measured directly rather than
inferred.
|
||
|
logan
|
6a5a3c33dc |
fix(shell): raise the page header's controls to the touch floor
#56 sized the playback transport for a thumb and named 44px; the queue header keeps it. Nothing else was resized, so the controls a user meets on *every* screen sat between a third and two thirds of the app's own floor. Measured on the reference device at 424x439: page-sort 99x23, page-sort-direction **28x21**, page-actions-more 38x27, and search-trigger 40x40. **Both questions the issue left open are answered by one measurement.** The header is 63px tall and its controls are 20-23px, so the vertical room was already there; the select and its direction arrow are 6px apart, so the horizontal room was not. That makes this min-size rather than padding with a negative margin, which is what the seek bar needed (#187), and the difference decides everything else. There the painted track had to stay thin, so the target was grown past its own box and had to be checked against its neighbours. Here the control *is* the target: the boxes are flex items, so the gap keeps them apart and **no two targets can overlap by construction**. From which: **There is no phone branch.** A 44px control on a desktop is merely large, and a second declaration of what a phone shows is a second thing to keep in step -- which is why this component has never had one. It also avoids a media query no tier here renders, which is exactly how the seek bar's phone rule came to be dead for months. **#69's overflow fit does not move.** That pass measures inline size, so the height costs it nothing, and only the two square controls grow the header's content -- by 22px in total. header-action-overflow.spec.ts passes unchanged at all four of its widths, which was the check rather than the assumption. Verified on the device that the count is still shown at 424px, so nothing has started yielding. search-trigger is the sharpest case and is fixed in the same pass: #57 created it as the phone's replacement for the header search box, so it exists *only* where there is a thumb, and it shipped at 40x40 under a comment calling that "the smallest a touch target should be". That was the floor restated four pixels short rather than a second opinion about it, and the comment now says so. Unlike #187 this can be measured rather than inferred: the controls are plain elements and the rule is a min-size, so it holds at every width and a real Chromium rendering a real page-header gives the actual answer. The tests fail with the device's own numbers -- 29x21, 38, 40. Verified on the device: every control in the header is now at least 44x44, and so is the phone's search button. **This is the Direction's first step, not all of it.** config-field's 93 Settings controls and explore-view's search row are the second pass; Settings is a form with one shape for every row and wants its own argument. #186 stays open for them. |
||
|
logan
|
1668b9e0d2 | Merge pull request 'Android: a seek bar you can actually hit, and the phone rule that never applied' (#193) from 187-seek-bar-hit-area into main | ||
|
logan
|
ec64dbded0 |
fix(player): give the seek bar a thumb-sized hit area
On now-playing-view -- the screen that exists so a phone has somewhere to seek from -- the slider measured 261x6 on the reference device. Six pixels is the whole of the drag target on the app's primary seeking affordance, against the 44px floor the app set for itself in #56 and holds to in the queue panel. **The phone rule had never applied**, which is why the issue read as "the thickening stops short" rather than "there is no thickening". seek-bar's stylesheet asked for a 12px track below 599px and then set 6px in a plain `wa-slider` rule *written after it*. A media query adds no specificity, so the plain rule won at every width: the source said 12 and the device said 6. That is index.css's documented rule -- "the phone section is last on purpose" -- met inside a component's own stylesheet, where nothing in any tier renders differently to say so. The block is last now, and the 12px track it always asked for is real. **And 12px is still under the floor**, so the target is built around the painted track rather than by thickening it. The two are allowed to differ and a slider is the clearest case where they should: a 44px progress bar would be wrong-looking and would cost the album art the vertical space #51 spent an issue recovering. Two things about how it is built, both settled by measurement on the device rather than by choosing a number. **The padding goes on ::part(slider), not on the host.** That is the issue's untested claim, and the answer is the pessimistic one: the inner div is what carries the gesture -- it holds the listener and the touch-action: none -- and it is exactly the host's size, so padding the host would grow a box that does not take the press. **The padding is asymmetric and the margins cancel it**, so the row does not grow by the difference. The seek row is 19px -- its clocks, not the track, decide that -- and the play button's top edge is 8px below it, while `.art` above is a non-interactive div. A symmetric 44px target reaches into the play button, and growing the row instead cost the art 25px of 143 when it was tried. So the target takes the space above. Verified on the device at 424x439: hit area 261x44 where it was 261x6, painted track 12px, seek row still 19px, album art still 143px, 7px of clearance left under the play button, a press 26px above the track seeks, and a hit test on the play button's top edge still reaches the play button. The desktop bottom bar is untouched: the rule is inside the phone query and that instance is display:none below 600px anyway. The test asserts the parsed stylesheet, on hover-affordance.test.ts's precedent and with the same limitation stated -- no tier here lays out a real wa-slider at a phone width, and a number measured on a phone is not a number CI can assert. What it holds is the shape: that the phone block is last, that padding plus track clears 44, that the margins cancel the padding, and that the growth is upward. All four are invisible on a desktop, and the first is exactly what a tidy-up undoes. Closes #187 |
||
|
logan
|
dad852a8a0 | Merge pull request 'Explore: two things that have not worked since plan 013, and the temp directory Android never had' (#192) from 189-190-explore-correctness into main | ||
|
logan
|
30c6b665f1 |
fix(system): give the process a temp directory that exists
Android has no /tmp and hands an app no TMPDIR. Go's os.TempDir() falls
back to "/tmp" when the variable is unset, so every library in this
process that wants scratch space was being handed a path that has never
existed.
SQLite is the one that noticed, and it said so precisely:
W/yellowjacket: msg="champion index rebuild failed"
explore.search-index.error="populate champion fts: disk I/O error (6410)"
6410 is not a generic I/O error. `6410 & 0xff` is 10, SQLITE_IOERR, and
`6410 >> 8` is 25 -- SQLITE_IOERR_GETTEMPPATH. SQLite could not work out
where to put a temporary file. Two measurements on the device say why:
`ls -d /tmp` does not exist, and the app process's environment carries
no TMPDIR. A shell's does (/data/local/tmp), which is why this is easy
to miss from `adb shell`.
The cost was a silent performance cliff on the slowest device this app
runs on: `championReady` stayed false, so every Explore search took the
generic path over the whole 1,079,667-row index instead of the champion
subset, and the rebuild was re-attempted on every launch.
**The class is fixed rather than the statement.** The trigger is the
*size* of the work, not that query -- anything that spills fails the
same way there, so large sorts, large joins and VACUUM were all waiting
their turn. The repair belongs at the process's one answer to "where do
temporary files go".
`PRAGMA temp_store = MEMORY` was the alternative: cheaper, more local,
and a promise that every future spill fits in RAM on a phone. The
catalog is the largest thing in this app and that is not a promise
worth making silently.
UseTempDir sits beside UseHomeOverride and carries its two rules for
the same reasons. **An empty base is a no-op**, because that is what
application.Mobile.StoragePath() returns on desktop -- so this needs no
build tag and changes nothing off mobile, where /tmp is real. And **an
explicit TMPDIR wins**, so anyone who set one deliberately gets it;
nothing sets it on the platform this exists for. It needs no new Wails
API and no Java change: StoragePath() is already what YJ_HOME is
pointed at, and the directory goes under it.
Two things beyond the rename of a variable.
**Writability is probed, not assumed.** MkdirAll on an existing
unwritable directory succeeds, so without the probe this could set
TMPDIR to a directory nothing can use -- which is the same bug one
directory over, and just as quiet.
**It returns its error, and main logs it.** A temp directory that could
not be created is the same silent failure one step earlier. A failure
is not fatal: it leaves the platform's answer in place, which is what
every release before this one ran with. That log line is readable on
the platform only because of #160.
Verified on the reference device, where the same launch that used to
print the failure now prints:
I/yellowjacket: msg="champion index rebuilt"
explore.search-index.elapsed=6.496s
Closes #190
|
||
|
logan
|
d034d6e571 |
fix(explore): resolve pending release MBIDs against the real table
The release-group MBID backfill queried `release_groups`, which plan 013
renamed to `albums`. It failed on its first statement on every launch
since
|
||
|
logan
|
25ea1f3511 | Merge pull request 'Android: make the app say what it is doing, then count what the audio path misses' (#191) from 135-android-underrun-instrumentation into main | ||
|
logan
|
842fe47e9e |
feat(player): count what the ring buffer misses
An underrun is audible and nothing counted it. When the ring is empty BufferedStreamer.Stream zeroes the caller's buffer and returns ok, so a run of zeros is spliced into the waveform and the step discontinuity at each edge is a click; a series of short ones is static. That is the one candidate in #135 whose audible signature matches the report. `starved` and `starvedSince` already existed, from #122's stall fix, and could not answer this: they are a *stall* detector, reset by every arriving sample, because their job is to end a track whose source has died and a merely slow source must not be cut short. That reset is exactly what made the audible case invisible -- a hundred 20ms underruns a minute never approach the give-up threshold, so they were invisible to the log, to the UI and to every tier. UnderrunStats counts runs, calls and samples for the life of the streamer. Runs is the number that means something audible: one episode is one pop however many callbacks it spans, and the ratio of runs to calls is what separates clicking from dropping out. Three things about the reporting are deliberate. **Counting is in Stream and reporting is not.** Stream runs on the speaker callback's real-time deadline, so a log line there would allocate, format and write on the exact path whose missed deadline is the defect -- measuring by making it worse. The count is two increments under a lock that was already held; the report is on the 1 Hz position ticker, which only runs while playing. **An unchanged count is not logged**, which is emitStatus' rule one package over. A healthy player is silent, so anything in the log is news and the line appears exactly while it is popping. **It is Info rather than Debug.** The default level is Info and a phone has no convenient way to set YJ_LOG_LEVEL, so a debug line here would be a counter nobody on the affected platform could read -- which is the shape of the bug that made #160 necessary. underrunDelta clamps at zero because the counter belongs to the streamer and the streamer is replaced on every track: a baseline carried across that boundary is the previous track's total subtracted from a fresh zero. The baseline is reset at the load as well; a negative count in a log line reads as a broken instrument and would discredit the measurement this exists to make. This is the instrument, not a fix. What it measures is on the issue. |
||
|
logan
|
168e588387 |
feat(android): route slog to logcat
Every slog line the app wrote on Android went to /dev/null, including the one naming the error it was about to os.Exit on. #52 is what that cost: a process that vanished with no tombstone, no AndroidRuntime stack and nothing in `logcat -b crash`, at Priority/Critical for months, whose entire diagnosis was one sLogger.Error main.go was already writing. backend/androidlog is a slog.Handler over __android_log_write, chosen in main() by build tag rather than by a runtime check so that a desktop binary links no cgo for a platform it cannot run on. **Everything except the write itself is untagged.** That is androidpayload.go's discipline pushed as far as it goes: the only toolchain that compiles the android tag is a cross-compiler and the only thing that runs it is a phone, so the priority mapping, the formatting, the chunking and the handler's own attr and group bookkeeping are ordinary Go that `go test` exercises everywhere, and android.go is fifteen lines that hand a string to liblog. Four things in it are load-bearing. **The tag is a fixed string, not the application id.** The debug build carries `applicationIdSuffix ".dev"` so it can be installed beside the release app, and it is the only build whose WebView can be inspected -- so a tag derived from the id is a different tag on the one build anybody debugging this app is running, and the filter meant to show these lines would hide them exactly where they were being looked for. **The priorities are android/log.h's own values, asserted twice.** android.go carries constant expressions that do not compile as uint if the header renumbers; the untagged test writes the six numbers out longhand, because comparing a constant to itself passes on any renumbering. A wrong priority is the failure that hides rather than breaks -- logcat prints whatever number it is handed, so an Error filed as Info is present, correct, and invisible to every filter. **Formatting is delegated to slog's TextHandler.** WithAttrs and WithGroup are the half of slog.Handler that is easy to get subtly wrong, and a logger whose groups are wrong is a logger nobody reads. The derived handlers share the parent's buffer *and its mutex*: a second mutex would guard nothing, and two loggers derived from one would splice their bytes into a single line under load. **A line is chunked, because liblog drops what does not fit.** The kernel logger's entry is 4068 bytes for tag and message together and the remainder goes without comment, so a long record would be truncated in the middle of the thing worth reading. Time and level are dropped from the formatted line, since logcat stamps every entry with both -- and dropping them by *key* also ate a caller's own "level" attribute, which the on-device probe caught and TestACallersOwnLevelAttrSurvives now holds. ReplaceAttr sees an empty group path for the built-ins and for every top-level attribute alike, so the kinds are what separate them. Verified on the reference device (TLP301, Android 14): a debug build logs I/W/E under the `yellowjacket` tag at the right priorities, and the first thing it surfaced was a real warning nobody could previously see -- `champion index rebuild failed ... disk I/O error (6410)`. Closes #160 |