diff --git a/.planning/NOTES.md b/.planning/NOTES.md index dd1a3b0..f7bfee7 100644 --- a/.planning/NOTES.md +++ b/.planning/NOTES.md @@ -4573,3 +4573,137 @@ fault in it, and it is filed as #172 with the per-element budget. #64 umbrella; folding shuffle and repeat back onto the primary row was considered and rejected — it buys 52px, leaves the art at 91px, and costs a third arrangement of the same five buttons. + +## The volume is not ours on Android, and the predicate could not be a width (measured 2026-08-21) + +#64 asked for the in-app volume control to be absent on Android. Its +first Finding said `volume-control` "already stands down at narrow +widths", which was true of one of its two copies and is why the issue +had been read as nearly done. The bar's copy goes by width; the +full-screen view's copy was deliberately kept, with a comment saying a +slider does belong there. + +**The crux was platform versus width, and three options were on the +issue.** What settled it is that the *backend* half of the same issue — +pin the level at 1.0 — makes a width rule wrong on the platform the +issue is about: an Android tablet at >=600px gets the bottom bar, and +the bar's slider would then move a level that is pinned. That is a +control that cannot act, which `library-status-indicator` already +settled is worse than none. The same rule is wrong the other way below +600px, where a narrow desktop window has no hardware keys. + +So the frontend asks the player — `SystemOwnsVolume` — and the answer +is right at every width in both mount points. **The predicate is named +after the capability rather than the platform**, which is what makes it +testable: only `platformOwnsVolume` is behind a build tag, in two files +that declare nothing else, and everything else is decided against a +field a Go test sets either way. `frontend/test/components/ +volume-ownership.test.ts` stubs the binding and so exercises the +*Android* rendering on an ordinary Linux runner; both of its tests were +confirmed to fail on the build before the change. + +**Measured at 424x439, by flipping `platformOwnsVolume` to true in the +`!android` file and rebuilding** — the real binding, the real store, the +real component, everything except the tag: + +| element | before | after | +|---|---|---| +| header | 48 | 48 | +| **album art** | **39** | **68** | +| title / artist / album | 63 | 63 | +| transport (seek + controls + volume) | **172** | **143** | +| — seek bar | 19 | 19 | +| — player-controls | 116 | 116 | +| — volume-control | 21 | **0** | + +29px, which is the 21px control plus the 8px flex gap it stops drawing: +a gap is only painted between boxes, so `:host([hidden])` costs the +transport nothing rather than leaving a hole. That is #172's "~30px of +pure gain" confirmed, and the art is 74% larger. It is still the +second-smallest thing on the screen, which is #51's evidence. + +Three smaller things worth keeping. + +**`:host([hidden])` has to be written down.** The UA's `[hidden]` +rule is `display: none`, but `volume-control`'s own `:host` sets +`display: inline-flex` and outranks it — so setting `hidden` alone +hides nothing. Same family as the nested-`#queue-button` specificity +trap from the session before. + +**Rendering `nothing` and hiding the host are two different +assertions**, and the component test makes both: an empty shadow root +is what stops a by-role or positional query finding a button that +cannot act, and `hidden` is what stops the host occupying space. Either +alone passes on a build that gets the other wrong. + +**The bar's centring survives the control going away.** #23's outer +columns are the same `min()` expression rather than content-sized, so +at 900px with the volume gone the bar's centre, `audio-player`'s centre +and `player-controls`' centre are all 450 — checked, because "the +transport is centred with a slider bolted to one side" is the fault +that rule exists for and removing the slider is the obvious way to +re-break it. + +**What no tier here can check**: the constant itself, and ducking +against a real audio-focus change. The first is a source sweep +(`TestPlatformVolumeOwnershipIsDeclaredOncePerPlatform`), the second is +`TestSystemVolumeStillDucks` against the arithmetic. Neither is a +device, and no device was attached. + +### The device answered three of the four (measured 2026-08-21, TLP301 / Android 14 / SDK 34 / arm64, Chrome 113 at 424x439) + +A Light Phone III was attached after the PR was opened, so what that PR +listed as unverifiable was re-checked rather than left as a caveat. + +**The whole chain resolves on the device.** `__yj.call("player.Player. +SystemOwnsVolume", [])` answers `true` — build tag, `platformOwnsVolume`, +`Player.systemVolume` and the generated binding, end to end. That is the +one thing the source sweep only approximates, and it took a real arm64 +device because nothing else here compiles the `android` file at all. +(`GOOS=android GOARCH=arm64 CGO_ENABLED=1 go build ./backend/...` with +the NDK's clang compiles it in ~40 s and is worth running first; it +catches a type error but not a wrong constant.) + +**The control is absent in both mount points**, on the real engine: +`.bottom-bar volume-control` is `hidden` with an empty shadow root, and +so is `now-playing-view`'s. Measured on the device, transport **143px**, +which is the figure the desktop-headless "after" predicted exactly. The +art is 75px there rather than 68 because the fixture's `.names` block is +one line shorter, not because anything differs. + +**Nothing persists a level nobody chose, and this is the measurement +that took some care.** The default (50) surviving proves nothing, since +50 is also what a fresh row holds. So: force-stop, pull `yj.db`, set +`player_state.volume = 37`, push it back through +`run-as … dd` (a `cp` from `/sdcard` is refused — the app sandbox +cannot read it), relaunch, and drive a queue change to make the row be +rewritten. Reading it back **the WAL has to be pulled with it** — the +main file still showed the old `last_track_path` and reads as a write +that never happened. With `yj.db-wal` beside it: `last_track_path` is +the new track, so `saveState` ran, and `volume` is still **37**. + +**The duck cannot be verified on this device, and now for a stated +reason rather than for want of hardware.** `WailsForegroundService` +builds its `AudioFocusRequest` without `setWillPauseWhenDucked` on +API >= 26, so the framework attenuates the stream itself and never +delivers `AUDIOFOCUS_LOSS_TRANSIENT_CAN_DUCK`. The device's own log +says so: `MediaFocusControl: requestAudioFocus() … AA=USAGE_MEDIA/ +CONTENT_TYPE_MUSIC … req=1 flags=0x0` — no +`AUDIOFOCUS_FLAG_PAUSES_ON_DUCKABLE_LOSS`. **`minSdk` is 21**, so the +Go-side duck is not dead code; it is reachable on Android 5.0 to 7.1 +and on nothing newer. Any future "verify ducking on a device" needs one +of those, and asking for a modern phone will not do it. + +Two smaller things from the same session. + +**A fresh install downloads the real catalog, and it is 209px of the +screen while it does.** `YJ_CORE_INDEX_URL` is stubbed in +`dev-headless.sh` and in CI but is real on a device, so the first +measurement taken was of a screen with `job-band` on it and the art at +**0px**. That is not a defect and not #172 — it is the environment. +`explore.Service.StopIndexBuild` and a relaunch is the clean state. + +**The first-run wizard does not dismiss when a library appears by a +route other than its own** (#175) — it was still up, full-screen and +intercepting pointer events, after `AddLibrary` succeeded through the +binding, and was gone after a relaunch. Filed. diff --git a/CLAUDE.md b/CLAUDE.md index af2da7f..4af3651 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -646,7 +646,11 @@ rather than renaming them. level rather than writing through to the volume, so it cannot accumulate and nothing persists or emits a level the user did not choose — and it only ever fires below API 26, where the framework - does not already duck the app itself. + does not already duck the app itself. On that platform "the user's + level" is a constant, since #64 pins it at maximum and refuses every + way to move it; the duck is the one thing that still may, and it + works unchanged because it was always an offset applied *to* that + level rather than a write of it. - `system` — OS-specific paths (XDG on Linux, `%LOCALAPPDATA%` on Windows). - `explore` — Catalog search and browse over `explore_index`. See below. Its **shelves** (`shelves.go`) are the page Explore shows before @@ -1725,12 +1729,67 @@ where the zero value has to be the intended answer, so an existing `config.toml` with no key gets the new default without a migration. Inline, the icon becomes the mute toggle and is named after that action rather than after the state, because with the slider beside it there is -nothing left to disclose. It stands down below 600px whatever the -setting says — that is about the platform rather than preference, and -is why `mediacontrols`' Android handler implements no volume callback. -(Only the *bar's* copy: `now-playing-view` renders one and it is -visible on a phone. #64 asks for it to be gone on Android outright, -which is a platform question the frontend cannot currently ask.) +nothing left to disclose. The bar's copy stands down below 600px, which +is about *room*: five controls and a slider do not fit a 360px bar, and +`now-playing-view` is where seeking and volume go on a phone. + +**Whether there is a volume to control at all is a different question, +and it is asked of the player** (#64). On Android the hardware keys are +the volume control and the framework mixes our stream against the +device level, so `player`'s own level is pinned at maximum, `SetVolume` +/ `ChangeVolume` / `MuteToggle` are refused, and `volume-control` +renders `nothing` — in both of its mount points, at every width. +`mediacontrols`' Android handler implementing no volume callback is the +same fact one layer down. + +Five things about it are load-bearing. + +**It could not be a width, and that is not a preference.** Every other +stand-down rule in this app is keyed on a viewport, because a width is +what a browser can answer and what every tier can test. This one is a +property of the build: keyed on width, an Android *tablet* at 600px or +more draws the bottom bar's slider over a pinned level — a control that +cannot act, on exactly the platform the rule exists for, which +`library-status-indicator` already settled is worse than none. The +same rule is wrong in the other direction below 600px, where a narrow +desktop window has no hardware keys to fall back on. + +**The predicate is named after the capability, not the platform.** +`SystemOwnsVolume` is what the frontend asks; `platformOwnsVolume` is +the one build-tagged constant behind it, in two files that declare +nothing else. That is `mediacontrols`' split with +`androidpayload.go`'s reasoning: a tagged file is compiled by nothing +`make lint` or `make test` runs, so everything decidable off a phone is +decided against `Player.systemVolume`, a field a test sets either way. +The frontend's absent branch is therefore testable in the component +tier with a stubbed binding, and the constant itself is covered by a +source sweep plus, once, a real arm64 device answering `true` — which +is the only tier that compiles the `android` file at all. + +**Mute goes with it, because it is a level of zero by another name** — +and because with no control rendered it is the one state on such a +platform the user could not get out of. + +**Nothing persists a level nobody chose.** The maximum the player runs +at is synthetic, so `restoreStateLocked` *remembers* the stored volume +instead of applying it and `saveState` writes that same value back. +The alternative — a second query that omits the column — buys nothing +and is a second write path to keep in step. + +**And ducking is untouched, which is what makes the pin safe.** +`SetDuck` applies its attenuation by re-applying the *user's* level +through `setVolumeLocked`, so pinning that level to maximum leaves the +offset arithmetic exactly as it was. It is the only thing that may move +the output on such a platform, and it is the one volume-shaped path +that is not refused. + +One thing to know before anyone offers to test it on a phone: **the +duck is unreachable above API 25.** `WailsForegroundService` builds its +`AudioFocusRequest` without `setWillPauseWhenDucked` from Oreo, so the +framework attenuates us itself and never sends +`AUDIOFOCUS_LOSS_TRANSIENT_CAN_DUCK` — a device confirms it by logging +`requestAudioFocus() … flags=0x0`. `minSdk` is 21, so this is live code +rather than dead, on Android 5.0 to 7.1 and nowhere else. **And below 600px that bar carries three controls, not five** (#59). Shuffle, repeat and the queue button leave it; what is left is art, diff --git a/backend/player/player.go b/backend/player/player.go index 7ff3b5c..1779438 100644 --- a/backend/player/player.go +++ b/backend/player/player.go @@ -71,6 +71,19 @@ type Player struct { // not something the user chose. duckAmount float64 + // systemVolume is what SystemOwnsVolume answers: the platform's own + // control is the only one, so ours neither acts nor persists. It is + // a field rather than the build constant read directly so that a + // test can exercise both sides on any machine. See systemvolume.go. + systemVolume bool + + // storedVolume and storedMuted hold the persisted level as it was + // found at restore, for a platform whose volume we do not own: the + // maximum we then run at is not a level the user chose, so saveState + // writes back what it read rather than overwriting it. + storedVolume UserVolume + storedMuted bool + // trackLengthMs holds the authoritative track duration in // milliseconds, sourced from the database (which uses the // custom header parser). The go-mp3 decoder's Len() can be @@ -156,6 +169,8 @@ func NewPlayer(logger *slog.Logger, db *database.DB) *Player { logger: logger, db: db, state: Stopped, + systemVolume: platformOwnsVolume, + storedVolume: DefaultUserVol, baseStreamer: generators.Silence(-1), format: beep.Format{ SampleRate: speakerSampleRate, @@ -875,6 +890,10 @@ func (p *Player) SetVolume(desiredVolume UserVolume) { p.mu.Lock() defer p.mu.Unlock() + if p.systemVolume { + return + } + p.setVolumeLocked(desiredVolume) p.emitVolumeChanged() p.saveState() @@ -923,6 +942,10 @@ func (p *Player) ChangeVolume(deltaVolume int) error { p.mu.Lock() defer p.mu.Unlock() + if p.systemVolume { + return nil + } + p.setVolumeLocked(p.getUserVolume() + UserVolume(deltaVolume)) p.emitVolumeChanged() p.saveState() @@ -953,6 +976,14 @@ func (p *Player) MuteToggle() error { return errNoAudioFileLoaded } + // Mute is a level of zero by another name, so it goes with the rest + // of the volume where the system owns it -- and it would be the one + // state on such a platform the user could not get out of, since with + // no control rendered there is nothing left to un-mute with. + if p.systemVolume { + return nil + } + speaker.Lock() p.volume.Silent = !p.volume.Silent speaker.Unlock() @@ -1403,7 +1434,15 @@ func (p *Player) saveState() { volume := int64(DefaultUserVol) muted := false - if p.volume != nil { + switch { + case p.systemVolume: + // The maximum this platform runs at is not a level anybody + // chose, so it is not one to remember. Writing back what + // restore found keeps the row a description of the user's + // setting without needing a second query that omits the column. + volume = int64(p.storedVolume) + muted = p.storedMuted + case p.volume != nil: volume = int64(p.getUserVolume()) muted = p.volume.Silent } @@ -1487,11 +1526,20 @@ func (p *Player) restoreStateLocked() { } } - vol := clampVolume(UserVolume(state.Volume)) - p.setVolumeLocked(vol) + if p.systemVolume { + // Remembered, not applied: the device's keys are the volume + // control here, so the player runs wide open and hands the + // stored level back untouched at the next save. + p.storedVolume = clampVolume(UserVolume(state.Volume)) + p.storedMuted = state.Muted + p.setVolumeLocked(MaxUserVol) + } else { + vol := clampVolume(UserVolume(state.Volume)) + p.setVolumeLocked(vol) - if state.Muted { - p.volume.Silent = true + if state.Muted { + p.volume.Silent = true + } } // Restore last track if the file still exists. @@ -1531,8 +1579,9 @@ func (p *Player) restoreStateLocked() { } p.logger.Info("Player state restored", - "volume", vol, - "muted", state.Muted, + "volume", p.getUserVolume(), + "muted", p.volume.Silent, + "systemVolume", p.systemVolume, "trackPath", state.LastTrackPath, "positionSeconds", state.LastPositionSeconds, ) diff --git a/backend/player/systemvolume.go b/backend/player/systemvolume.go new file mode 100644 index 0000000..b52b6b7 --- /dev/null +++ b/backend/player/systemvolume.go @@ -0,0 +1,42 @@ +package player + +// Who owns the volume, and what follows when it is not us. +// +// On Android the hardware keys *are* the volume control and the +// framework mixes our stream against the device level, so a second +// control inside the app is a slider that moves something the user +// already moved (#64). Where that is true the player's own level sits +// at maximum, nothing changes it, and nothing persists it. +// +// **The predicate is named after the capability, not the platform.** +// The frontend asks "is there a volume for me to control", which is a +// question about this build; asking "is this a phone" instead would +// key the answer to a viewport, and an Android tablet at 600px or more +// would then draw the bottom bar's slider over a level pinned at +// maximum -- a control that cannot act, which is the thing +// `library-status-indicator` already settled is worse than none. +// +// **Only `platformOwnsVolume` is behind a build tag**, in two files +// that declare nothing else. A tagged file is compiled by nothing +// `make lint` or `make test` runs and is untestable off a phone, which +// is the reasoning `mediacontrols/androidpayload.go` states for +// keeping its contract out of one -- so everything decidable here is +// decided against `Player.systemVolume`, a field a test sets either +// way, and the tag decides only what that field starts as. +// +// The one thing this must not disturb is ducking. `SetDuck` applies +// its attenuation by re-applying the *user's* level through +// `setVolumeLocked`, so pinning that level to maximum leaves the +// offset arithmetic exactly as it was: an OS asking us to get out of +// the way of a navigation prompt is not the user setting a volume, and +// it is the only thing that may move the output on such a platform. + +// SystemOwnsVolume reports whether the platform's own control is the +// only volume control there is, so this app neither offers one nor +// remembers a level. +// +// It is bound: the frontend renders no `` when it is +// true, at any width. +func (p *Player) SystemOwnsVolume() bool { + return p.systemVolume +} diff --git a/backend/player/systemvolume_android.go b/backend/player/systemvolume_android.go new file mode 100644 index 0000000..52036c3 --- /dev/null +++ b/backend/player/systemvolume_android.go @@ -0,0 +1,11 @@ +//go:build android + +package player + +// platformOwnsVolume is true on Android: volume is the device's, set +// with the hardware keys, and `mediacontrols`' Android handler +// implements no volume callback for the same reason. +// +// See systemvolume.go for why this constant is the whole of what a +// build tag decides here. +const platformOwnsVolume = true diff --git a/backend/player/systemvolume_other.go b/backend/player/systemvolume_other.go new file mode 100644 index 0000000..f23124d --- /dev/null +++ b/backend/player/systemvolume_other.go @@ -0,0 +1,10 @@ +//go:build !android + +package player + +// platformOwnsVolume is false everywhere but Android: a desktop mixer +// is per-application, so our level is the one the user reaches for. +// +// See systemvolume.go for why this constant is the whole of what a +// build tag decides here. +const platformOwnsVolume = false diff --git a/backend/player/systemvolume_test.go b/backend/player/systemvolume_test.go new file mode 100644 index 0000000..1435517 --- /dev/null +++ b/backend/player/systemvolume_test.go @@ -0,0 +1,215 @@ +package player + +import ( + "log/slog" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/gopxl/beep/v2/effects" + + "yellowjacket/backend/database" +) + +// pinnedPlayer is a player on a platform whose volume belongs to the +// device. The field is set rather than the build constant read, +// because the constant is true on exactly one platform and no tier +// here runs on it -- see systemvolume.go. +func pinnedPlayer(t *testing.T, db *database.DB) *Player { + t.Helper() + + p := NewPlayer(slog.Default(), db) + p.systemVolume = true + p.volume = &effects.Volume{Base: 2} + p.setVolumeLocked(MaxUserVol) + + return p +} + +// TestSystemVolumeRefusesEveryWayToChangeTheLevel is the first half of +// #64: where the device owns the volume, ours sits at maximum and none +// of the three routes to a level moves it. Mute is in that list +// because it is a level of zero by another name, and because with no +// control rendered it is the one state on such a platform there would +// be nothing to get out of. +func TestSystemVolumeRefusesEveryWayToChangeTheLevel(t *testing.T) { + t.Parallel() + + p := pinnedPlayer(t, nil) + + if !p.SystemOwnsVolume() { + t.Fatal("SystemOwnsVolume() = false on a pinned player") + } + + if got := p.getUserVolume(); got != MaxUserVol { + t.Errorf("starting volume = %d, want %d", got, MaxUserVol) + } + + p.SetVolume(20) + + if got := p.getUserVolume(); got != MaxUserVol { + t.Errorf("volume after SetVolume(20) = %d, want %d", got, MaxUserVol) + } + + if err := p.ChangeVolume(-30); err != nil { + t.Fatalf("ChangeVolume: %v", err) + } + + if got := p.getUserVolume(); got != MaxUserVol { + t.Errorf("volume after ChangeVolume(-30) = %d, want %d", got, MaxUserVol) + } + + if err := p.MuteToggle(); err != nil { + t.Fatalf("MuteToggle: %v", err) + } + + if p.volume.Silent { + t.Error("MuteToggle silenced a player whose volume the system owns") + } +} + +// TestAnUnpinnedPlayerStillChangesItsVolume is the other side of the +// same switch. Without it the test above passes on a player that +// refuses everything, which is what a mis-wired field would produce. +func TestAnUnpinnedPlayerStillChangesItsVolume(t *testing.T) { + t.Parallel() + + p := NewPlayer(slog.Default(), nil) + p.volume = &effects.Volume{Base: 2} + p.setVolumeLocked(MaxUserVol) + + if p.SystemOwnsVolume() { + t.Fatal("SystemOwnsVolume() = true off Android") + } + + p.SetVolume(20) + + if got := p.getUserVolume(); got != 20 { + t.Errorf("volume after SetVolume(20) = %d, want 20", got) + } + + if err := p.MuteToggle(); err != nil { + t.Fatalf("MuteToggle: %v", err) + } + + if !p.volume.Silent { + t.Error("MuteToggle did not silence an ordinary player") + } +} + +// TestSystemVolumeStillDucks is the issue's second Finding, made a +// test: pinning the user's level must leave the OS's attenuation +// working, because a duck is not a volume the user chose and is the +// only thing that may move the output on such a platform. +func TestSystemVolumeStillDucks(t *testing.T) { + t.Parallel() + + p := pinnedPlayer(t, nil) + open := p.volume.Volume + + p.SetDuck(true) + + if p.volume.Volume >= open { + t.Errorf( + "ducked output = %v, want less than %v", p.volume.Volume, open, + ) + } + + if got := p.getUserVolume(); got != MaxUserVol { + t.Errorf("user volume while ducked = %d, want %d", got, MaxUserVol) + } + + // A refused SetVolume must not disturb the offset either: it + // returns before setVolumeLocked, which is what re-applies it. + ducked := p.volume.Volume + + p.SetVolume(10) + + if p.volume.Volume != ducked { + t.Errorf( + "output after a refused SetVolume = %v, want %v", + p.volume.Volume, ducked, + ) + } + + p.SetDuck(false) + + if p.volume.Volume != open { + t.Errorf("output after unduck = %v, want %v", p.volume.Volume, open) + } +} + +// TestSystemVolumeWritesBackTheLevelItFound is the rest of the +// Direction: "make sure nothing writes a persisted volume from that +// platform". The maximum the player runs at is synthetic, so saving +// must not record it over whatever the row already said. +func TestSystemVolumeWritesBackTheLevelItFound(t *testing.T) { + t.Parallel() + + db := database.NewTestDB(t) + + // A level set by some earlier, unpinned session. + writer := NewPlayer(slog.Default(), db) + writer.volume = &effects.Volume{Base: 2} + writer.setVolumeLocked(30) + writer.SaveState() + + p := pinnedPlayer(t, db) + p.RestoreState() + + if got := p.getUserVolume(); got != MaxUserVol { + t.Errorf("restored volume = %d, want %d (the level is pinned)", got, MaxUserVol) + } + + if p.volume.Silent { + t.Error("restore muted a player whose volume the system owns") + } + + p.SaveState() + + state, err := db.Queries.GetPlayerState(db.Ctx) + if err != nil { + t.Fatalf("GetPlayerState: %v", err) + } + + if state.Volume != 30 { + t.Errorf("persisted volume = %d, want 30 (untouched)", state.Volume) + } +} + +// TestPlatformVolumeOwnershipIsDeclaredOncePerPlatform sweeps the +// source, because the pair of tagged files is the one thing here no +// tier compiles both halves of: `make lint` and `make test` build the +// `!android` side only, so a deleted or edited android file fails +// nothing until somebody has a phone in their hand. +func TestPlatformVolumeOwnershipIsDeclaredOncePerPlatform(t *testing.T) { + t.Parallel() + + want := map[string]string{ + "systemvolume_other.go": "const platformOwnsVolume = false", + "systemvolume_android.go": "const platformOwnsVolume = true", + } + + tags := map[string]string{ + "systemvolume_other.go": "//go:build !android", + "systemvolume_android.go": "//go:build android", + } + + for name, decl := range want { + src, err := os.ReadFile(filepath.Join(".", name)) + if err != nil { + t.Errorf("%s: %v", name, err) + + continue + } + + if !strings.Contains(string(src), decl) { + t.Errorf("%s does not declare %q", name, decl) + } + + if !strings.Contains(string(src), tags[name]) { + t.Errorf("%s does not carry %q", name, tags[name]) + } + } +} diff --git a/e2e/specs/phone-shell.spec.ts b/e2e/specs/phone-shell.spec.ts index 897f998..8969511 100644 --- a/e2e/specs/phone-shell.spec.ts +++ b/e2e/specs/phone-shell.spec.ts @@ -130,6 +130,25 @@ test.describe('the shell on a phone', () => { // are here, and they are the *same* components -- this view // composes the transport rather than reimplementing it. await expect(app.locator('now-playing-view seek-bar')).toBeVisible(); + + // Volume is here **because the player says there is one** (#64), + // not because this is a phone. This tier is the platform that owns + // its own volume, so what it can assert is that the control's + // presence follows that answer -- an inverted polarity in + // `volume-style-store` fails here and in `bottom-bar.spec.ts`, and + // the *absent* branch is checked in the component tier, where the + // binding can be stubbed. Nothing here can reach the Android side. + const systemOwns = await app.evaluate( + async () => + (await window.__yjEvents.call( + 'player.Player.SystemOwnsVolume', + [], + 5_000, + )) as boolean, + ); + + expect(systemOwns, 'this platform should own its own volume').toBe(false); + await expect(app.locator('now-playing-view volume-control')).toBeVisible(); // Back goes where the user came from, through the nav stack. diff --git a/frontend/bindings/yellowjacket/backend/player/player.ts b/frontend/bindings/yellowjacket/backend/player/player.ts index aa95f8e..dd8c9a7 100644 --- a/frontend/bindings/yellowjacket/backend/player/player.ts +++ b/frontend/bindings/yellowjacket/backend/player/player.ts @@ -142,6 +142,18 @@ export function SetVolume(desiredVolume: $models.UserVolume): $CancellablePromis return $Call.ByID(1375836663, desiredVolume); } +/** + * SystemOwnsVolume reports whether the platform's own control is the + * only volume control there is, so this app neither offers one nor + * remembers a level. + * + * It is bound: the frontend renders no `` when it is + * true, at any width. + */ +export function SystemOwnsVolume(): $CancellablePromise { + return $Call.ByID(1027623185); +} + /** * TrackLengthInSeconds returns the duration of the current track. */ diff --git a/frontend/index.css b/frontend/index.css index a42aa45..30172ae 100644 --- a/frontend/index.css +++ b/frontend/index.css @@ -522,16 +522,20 @@ body div.sidebar { } /* Volume stands down here whatever the setting says, because this - is about room and about the platform rather than about - preference: the hardware keys own volume on a phone, which is - also why mediacontrols' Android handler implements no volume - callback. It moved from `audio-player`'s own media query when - #42 moved the control into the bar — same rule, and now stated - where the element actually is. + is about room: five controls and a slider do not fit a 360px + bar, and the full-screen now-playing view is where seeking and + volume go on a phone. It moved from `audio-player`'s own media + query when #42 moved the control into the bar — same rule, and + now stated where the element actually is. - `.bottom-bar volume-control`, not the one in - `now-playing-view`: that view is the phone's transport and is - where a slider does belong. */ + **This rule used to carry the platform argument too, and no + longer does** (#64). "The hardware keys own the volume" is not a + width: it is false of a narrow desktop window and true of an + Android tablet, which this selector gets backwards both ways. + The player answers it now — `SystemOwnsVolume` — and + `volume-control` renders nothing when it is true, at every + width and in both of its mount points. What is left here is the + question a stylesheet can actually answer. */ .bottom-bar volume-control { display: none; } diff --git a/frontend/src/components/audio-player/volume-control/volume-control.ts b/frontend/src/components/audio-player/volume-control/volume-control.ts index 1ad0ada..7dd070d 100644 --- a/frontend/src/components/audio-player/volume-control/volume-control.ts +++ b/frontend/src/components/audio-player/volume-control/volume-control.ts @@ -1,4 +1,4 @@ -import { LitElement, html, css } from 'lit'; +import { LitElement, html, css, nothing } from 'lit'; import { customElement, state } from 'lit/decorators.js'; import '@awesome.me/webawesome/dist/components/icon/icon.js'; import '@awesome.me/webawesome/dist/components/slider/slider.js'; @@ -27,6 +27,26 @@ export class VolumeControl extends LitElement { @state() private popup = volumeStyleStore.popup; + /** + * Whether there is a volume of ours to control at all (#64). + * + * The decision is made here rather than at either mount point, + * because there are two -- the bottom bar's copy lives in + * `index.html`, which has no module scope to make it conditional -- + * and one of them is a control the shell cannot un-render. So the + * control answers for itself, and the bar and the phone's + * full-screen transport get the same answer without either knowing + * the question exists. + * + * It renders `nothing` *and* hides the host: an empty shadow root is + * what stops a positional or role query finding a button that cannot + * act, and `:host([hidden])` is what stops the element occupying a + * flex item's worth of the transport -- the `:host` display above + * outranks the UA's `[hidden]` rule, so it has to be said. + */ + @state() + private available = volumeStyleStore.available; + private unsubscribeStyle?: () => void; // Locally-tracked volume while the user is actively dragging or scrolling. @@ -43,6 +63,13 @@ export class VolumeControl extends LitElement { align-items: center; } + /* See the available field. A gap is only drawn between boxes, + so a hidden host costs its parent nothing -- which is where the + 29px this gives back to Now Playing comes from (#172). */ + :host([hidden]) { + display: none; + } + button { background: none; border: none; @@ -154,12 +181,15 @@ export class VolumeControl extends LitElement { this.unsubscribeStyle = volumeStyleStore.subscribe(() => { this.popup = volumeStyleStore.popup; + this.setAvailable(volumeStyleStore.available); // Switching to the slider while the popup is open would leave the // document listener installed for a popup that no longer renders. if (!this.popup) this.closeSlider(); }); + this.setAvailable(volumeStyleStore.available); + void volumeStyleStore.init(); } @@ -233,7 +263,25 @@ export class VolumeControl extends LitElement { // RENDER // =================================================================== + /** + * `hidden` is set imperatively rather than reflected from the state, + * because it has to be on the *host* and a `@state` does not reflect. + * It is the right attribute besides: it takes the element out of the + * accessibility tree as well as out of the layout. + */ + private setAvailable(available: boolean) { + this.available = available; + this.hidden = !available; + + // A popup left open when the control goes away would keep its + // document click listener installed for markup that no longer + // renders. + if (!available) this.closeSlider(); + } + override render() { + if (!this.available) return nothing; + const muted = this.player.muted; // Inline, the icon is the mute toggle rather than a disclosure: diff --git a/frontend/src/components/now-playing-view/now-playing-view.ts b/frontend/src/components/now-playing-view/now-playing-view.ts index ab71ded..cbf1d1d 100644 --- a/frontend/src/components/now-playing-view/now-playing-view.ts +++ b/frontend/src/components/now-playing-view/now-playing-view.ts @@ -343,6 +343,12 @@ export class NowPlayingView extends LitElement { a media query because the bottom bar wants a different answer at this same viewport. --> + `; diff --git a/frontend/src/store/volume-style-store.ts b/frontend/src/store/volume-style-store.ts index 88ee68a..89ef98a 100644 --- a/frontend/src/store/volume-style-store.ts +++ b/frontend/src/store/volume-style-store.ts @@ -1,5 +1,6 @@ import { EventsOn } from '@runtime/runtime'; import { GetPopupVolume } from '@go/config/config.js'; +import { SystemOwnsVolume } from '@go/player/player.js'; import { Events } from '../events'; type Subscriber = () => void; @@ -28,10 +29,37 @@ type Subscriber = () => void; * becomes one. An install that has chosen the popup sees it swap once * on load, which is the cheaper of the two wrong first frames: the * inline slider occupies the space the popup's button would have. + * + * **`available` is the question one step earlier — whether there is a + * volume of ours to draw at all (#64).** On Android the hardware keys + * are the volume control and the backend pins its own level at + * maximum, so a slider here would move nothing. + * + * It is asked of the *player* rather than of the viewport, and that is + * the whole design decision. Every other stand-down rule in this app + * is a width, because a width is what a browser can answer and what + * every tier can test — but this one is a property of the build. Keyed + * on width instead, an Android tablet at 600px or more would draw the + * bottom bar's slider over a pinned level: a control that cannot act, + * which `library-status-indicator` settled is worse than none. + * + * It lives beside `popup` because both answer "what presentation does + * the volume control get", both readers are the same two components, + * and "none" is a presentation. A second store would be a second + * subscription in the same `connectedCallback` saying the same thing. + * + * The initial value is `true` on the same first-frame rule: there is a + * volume on every platform but one, and the platform that pins it sees + * the control once at boot and never again in the session — the answer + * cannot change while the app runs, so by the time the lazily-mounted + * now-playing view exists it has long been settled by the bar's own + * copy. */ class VolumeStyleStore { private value = false; + private hasVolume = true; + private loaded = false; private subscribers = new Set(); @@ -47,13 +75,21 @@ class VolumeStyleStore { return this.value; } + /** + * Whether this app has a volume of its own to control. False where + * the device owns it; see the class comment. + */ + get available(): boolean { + return this.hasVolume; + } + /** Reads the setting once. Safe to call from every mount. */ async init(): Promise { if (this.loaded) return; this.loaded = true; - await this.refresh(); + await Promise.all([this.refreshAvailability(), this.refresh()]); } subscribe(fn: Subscriber): () => void { @@ -77,6 +113,28 @@ class VolumeStyleStore { } } + /** + * Asked once, not on `GeneralConfigChanged`: this is a property of + * the platform the binary was built for and cannot change while + * the app is running. + */ + private async refreshAvailability(): Promise { + try { + const owned = await SystemOwnsVolume(); + + if (owned === !this.hasVolume) return; + + this.hasVolume = !owned; + this.notify(); + } catch (err) { + // The control renders, which is the answer on every + // platform but one and is the recoverable way to be wrong: + // a working control nobody needs, rather than a missing one + // somebody does. + console.error('failed to ask who owns the volume', err); + } + } + private notify(): void { for (const fn of this.subscribers) fn(); } diff --git a/frontend/test/components/volume-ownership.test.ts b/frontend/test/components/volume-ownership.test.ts new file mode 100644 index 0000000..00fb6a9 --- /dev/null +++ b/frontend/test/components/volume-ownership.test.ts @@ -0,0 +1,93 @@ +/** + * Who owns the volume, and what the control does when it is not us + * (#64). + * + * **This is the tier that can exercise the Android branch**, and it is + * the reason the predicate is a backend answer rather than a build tag + * the frontend cannot see: `SystemOwnsVolume` is a stub here, so the + * "no volume" rendering is checked on an ordinary Linux CI runner with + * no device anywhere. What no tier here can check is the *constant* + * behind it, which `TestPlatformVolumeOwnershipIsDeclaredOncePerPlatform` + * sweeps the Go source for instead. + * + * It is a file of its own because `volumeStyleStore` asks once and + * latches — the answer is a property of the binary and cannot change + * while the app runs, so there is deliberately no event that refreshes + * it. Vitest gives each file its own module registry, which is what + * lets the stub be in place before the singleton is first touched. + * The *available* case is the rest of `transport.test.ts`, which mounts + * the same element under the default stub. + */ +import { describe, expect, it, beforeEach } from 'vitest'; + +import '@components/audio-player/volume-control/volume-control'; +import '@components/now-playing-view/now-playing-view'; +import { Events } from '../../src/events'; +import { emit, resetHarness, stub } from '@test/support/harness'; +import { fixture, shadow, shadowAll } from '@test/support/render'; +import type { TrackInfo } from '@store/player-store'; + +const TRACK: TrackInfo = { + fileName: 'tideline.mp3', + filePath: '/music/tideline.mp3', + trackLength: 245, + seekPosition: 0, + state: 'playing', + title: 'Tideline', + artist: 'Sea Change', + album: 'Ebb', + coverArt: '', + coverArtSmall: '', + coverArtMedium: '', + coverArtLarge: '', + trackChangeId: 1, + artistMbid: '', + releaseGroupMbid: '', + recordingMbid: '', +}; + +describe('a platform whose volume we do not own', () => { + beforeEach(async () => { + resetHarness(); + stub('player.Player.SystemOwnsVolume', true); + stub('config.Config.GetPopupVolume', false); + + // The store latches on the first mount; do it here so every test + // below sees a settled answer rather than the first frame. + const warm = await fixture('volume-control'); + + await warm.updateComplete; + }); + + it('renders no control at all, and no empty shadow root to find', async () => { + const el = await fixture('volume-control'); + + await el.updateComplete; + + // Both halves matter. An empty shadow root is what stops a + // positional or by-role query finding a button that cannot act; + // `hidden` is what stops the host taking a flex item's worth of + // space in the transport it sits in. + expect(shadowAll(el, 'button')).toHaveLength(0); + expect(shadowAll(el, 'wa-slider')).toHaveLength(0); + expect(el.hidden, 'the host is not hidden').toBe(true); + }); + + it('leaves the rest of the phone transport alone', async () => { + emit(Events.TrackChanged, TRACK); + + const view = await fixture('now-playing-view'); + + await view.updateComplete; + + // Seeking and the transport buttons are not volume, and #64 is + // allowed to remove one control, not to thin the screen out. + expect(shadow(view, 'seek-bar')).not.toBeNull(); + expect(shadow(view, 'player-controls')).not.toBeNull(); + + const volume = shadow(view, 'volume-control') as HTMLElement | null; + + expect(volume, 'the element is still mounted').not.toBeNull(); + expect(volume!.hidden, 'a mounted volume-control is not hidden').toBe(true); + }); +});