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/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. */