diff --git a/backend/download/reconcile.go b/backend/download/reconcile.go index 1e209c3..1984171 100644 --- a/backend/download/reconcile.go +++ b/backend/download/reconcile.go @@ -310,15 +310,35 @@ func (r *Reconciler) run(ctx context.Context, force bool) (Summary, error) { summary.Synced = r.syncExternalLists(ctx) - attempted, started, err := r.attemptDue(ctx, force) - if err != nil { - return summary, err + // Nothing is searched for when there is nothing to search with, and + // the point is what that *does not* do to the list. + // + // Attempting anyway is not merely wasted work: every request comes + // back "no download clients are enabled", which RecordAttempt writes + // down as an attempt and schedules a retry for -- so a user who has + // deliberately built a wanted list with no client watched their + // requests accrue failures and announce "next check in 6 hours" + // about a check that cannot happen. Wanting something without a way + // to fetch it is a supported thing to do; being told it is being + // looked for is a lie. + // + // Everything above this line still runs: an artist subscription + // still expands, and a request the user satisfied by some other + // route -- ripped, bought, copied in -- is still retired, because + // neither needs a provider. + summary.NoProviders = len(r.manager.enabledProviders()) == 0 + + if !summary.NoProviders { + attempted, started, err := r.attemptDue(ctx, force) + if err != nil { + return summary, err + } + + summary.Attempted = attempted + summary.Started = started } - summary.Attempted = attempted - summary.Started = started summary.Waiting = r.countWaiting(ctx) - summary.NoProviders = len(r.manager.enabledProviders()) == 0 r.logger.Info( "reconciled request list", diff --git a/backend/download/reconcile_test.go b/backend/download/reconcile_test.go index 23eef8c..114ff4b 100644 --- a/backend/download/reconcile_test.go +++ b/backend/download/reconcile_test.go @@ -453,6 +453,15 @@ func TestReconcileRespectsBatchSize(t *testing.T) { f := newReconcileFixture(t) ctx := context.Background() + // A client that searches and finds nothing. The batch size is about + // how many requests one pass *searches for*, which only means + // anything when there is something to search with -- a pass with no + // provider now attempts nothing at all, deliberately. + f.manager.installProvider( + Config{ID: 1, Priority: 50}, + NewFakeProvider(1, "finds-nothing", Caps{CanSearch: true}), + ) + f.reconciler.SetBatch(2) for _, mbid := range []string{"rg-1", "rg-2", "rg-3", "rg-4"} { @@ -593,3 +602,72 @@ func TestSummaryReportsNoProviders(t *testing.T) { t.Error("summary did not report that no download client is enabled") } } + +// ...and it does not search, which is the part the user sees. +// +// Attempting with no provider fails every request with "no download +// clients are enabled", and RecordAttempt writes that down as an +// attempt and schedules a retry -- so a wanted list built deliberately +// without a client accrued failures and announced "next check in 6 +// hours" about a check that cannot happen. Wanting something with no +// way to fetch it is supported; being told it is being looked for is +// a lie. +func TestNoProvidersMeansNoAttempt(t *testing.T) { + t.Parallel() + + f := newReconcileFixture(t) + ctx := context.Background() + + id, err := f.store.AddRequest(ctx, Request{ + MBID: "rg-1", + Entity: EntityReleaseGroup, + LibraryID: 1, + Title: "OK Computer", + }) + if err != nil { + t.Fatalf("AddRequest: %v", err) + } + + f.catalog.tracklists["rg-1"] = fourTrackDownload().Expected + + summary, err := f.reconciler.RunNow(ctx) + if err != nil { + t.Fatalf("RunNow: %v", err) + } + + if summary.Attempted != 0 { + t.Errorf("attempted %d requests with no client to search with, want 0", + summary.Attempted) + } + + // The list still knows what is on it: "nothing happened" has to be + // reportable as "nothing was searched for, of the one thing you + // want" rather than as silence. + if summary.Waiting != 1 { + t.Errorf("summary reported %d waiting, want 1", summary.Waiting) + } + + req, err := f.store.GetRequest(ctx, id) + if err != nil { + t.Fatalf("GetRequest: %v", err) + } + + if req.Attempts != 0 { + t.Errorf("attempts = %d, want 0: a pass that could not search did not", + req.Attempts) + } + + if req.LastError != "" { + t.Errorf("lastError = %q, want empty: the request did not fail, it "+ + "was never tried", req.LastError) + } + + // A new request is due immediately (next_try_at is set to now on + // insert), so the fault is not the presence of a time -- it is a + // time pushed into the future by a failed attempt, which is what the + // UI renders as "next check in 6 hours". + if req.NextTryAt.After(time.Now().Add(time.Minute)) { + t.Errorf("next try scheduled for %v: a check that cannot happen was "+ + "put on the clock", req.NextTryAt) + } +} diff --git a/backend/mediacontrols/mpris_linux.go b/backend/mediacontrols/mpris_linux.go index b74f6d2..06e0d2f 100644 --- a/backend/mediacontrols/mpris_linux.go +++ b/backend/mediacontrols/mpris_linux.go @@ -279,17 +279,19 @@ func (h *MPRISHandler) enqueue(fn func()) { } } -// UpdateMetadata pushes track metadata to D-Bus. -func (h *MPRISHandler) UpdateMetadata(meta Metadata) { - h.mu.Lock() - h.trackID++ - tid := h.trackID - h.mu.Unlock() - - m := map[string]interface{}{ +// metadataMap builds the org.mpris.MediaPlayer2.Player Metadata value +// for one track. +// +// It is separated from UpdateMetadata, which needs a live D-Bus +// connection, so the map's contents can be asserted on: this file is +// behind a build tag and everything in it that touches h is reachable +// only from a session bus, which is the same reason the Android +// contract lives in an untagged androidpayload.go. +func metadataMap(meta Metadata, trackID uint64) map[string]any { + m := map[string]any{ "mpris:trackid": dbus.ObjectPath( fmt.Sprintf( - "/org/yellowjacket/Track/%d", tid, + "/org/yellowjacket/Track/%d", trackID, ), ), } @@ -306,16 +308,45 @@ func (h *MPRISHandler) UpdateMetadata(meta Metadata) { m["xesam:album"] = meta.Album } + // Always present, even with nothing to point at. + // + // Every other key here can be omitted safely because a client + // reading the map sees a track with no title or no album and + // renders it that way. Art is different: KDE's applet (and + // others) treat an *absent* mpris:artUrl as "no news about the + // art" and keep drawing whatever the last track had, so playing + // something with no cover left the previous album's sleeve on + // screen — which reads as the wrong track playing rather than as + // missing artwork. + // + // An empty string is the honest answer and is what the spec's + // "URI" type degrades to; a client that cannot load it falls back + // to its own placeholder, which is the behaviour wanted. + artURL := "" if meta.ArtFilePath != "" { - m["mpris:artUrl"] = "file://" + meta.ArtFilePath + artURL = "file://" + meta.ArtFilePath } + m["mpris:artUrl"] = artURL + if meta.DurationSec > 0 { m["mpris:length"] = int64( meta.DurationSec, ) * usPerSec } + return m +} + +// UpdateMetadata pushes track metadata to D-Bus. +func (h *MPRISHandler) UpdateMetadata(meta Metadata) { + h.mu.Lock() + h.trackID++ + tid := h.trackID + h.mu.Unlock() + + m := metadataMap(meta, tid) + h.enqueue(func() { h.props.SetMust(playerIf, "Metadata", m) }) diff --git a/backend/mediacontrols/mpris_linux_test.go b/backend/mediacontrols/mpris_linux_test.go new file mode 100644 index 0000000..195fb2d --- /dev/null +++ b/backend/mediacontrols/mpris_linux_test.go @@ -0,0 +1,86 @@ +//go:build linux && !android + +package mediacontrols + +import "testing" + +// The one key that must be present even when it is empty. +// +// Everything else in the map may be omitted, because a client reading +// it renders a track with no title as a track with no title. Art is +// different: KDE's applet treats an *absent* mpris:artUrl as no news +// about the art and keeps drawing the last one it saw, so a track with +// no cover wore the previous album's sleeve — which reads as the wrong +// track playing rather than as missing artwork. +func TestMetadataMapAlwaysCarriesArtURL(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + meta Metadata + want string + }{ + { + name: "no art at all", + meta: Metadata{Title: "Blue in Green"}, + want: "", + }, + { + name: "art on disk", + meta: Metadata{ + Title: "Blue in Green", + ArtFilePath: "/covers/kind-of-blue_lg.jpg", + }, + want: "file:///covers/kind-of-blue_lg.jpg", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + m := metadataMap(tt.meta, 1) + + got, ok := m["mpris:artUrl"] + if !ok { + t.Fatal("mpris:artUrl is absent; it must always be sent") + } + + if got != tt.want { + t.Errorf("mpris:artUrl = %v, want %q", got, tt.want) + } + }) + } +} + +// The trackid has to change between tracks or a client is entitled to +// treat the metadata as describing the same track it already has. +func TestMetadataMapTrackIDVaries(t *testing.T) { + t.Parallel() + + first := metadataMap(Metadata{Title: "A"}, 1)["mpris:trackid"] + second := metadataMap(Metadata{Title: "B"}, 2)["mpris:trackid"] + + if first == second { + t.Errorf("trackid did not change: %v", first) + } +} + +// The optional keys stay optional — this is what makes artUrl's +// always-present treatment a deliberate exception rather than drift. +func TestMetadataMapOmitsEmptyOptionalFields(t *testing.T) { + t.Parallel() + + m := metadataMap(Metadata{}, 1) + + for _, key := range []string{ + "xesam:title", + "xesam:artist", + "xesam:album", + "mpris:length", + } { + if _, ok := m[key]; ok { + t.Errorf("%s is present for an empty Metadata", key) + } + } +} diff --git a/backend/queue/emit.go b/backend/queue/emit.go index 3b0bb4d..d503b8d 100644 --- a/backend/queue/emit.go +++ b/backend/queue/emit.go @@ -81,6 +81,7 @@ func (q *Queue) emitTracksModified( Index: index, Positions: positions, CurrentIndex: q.currentIndex, + Source: q.source, }, ) } diff --git a/backend/queue/emit_test.go b/backend/queue/emit_test.go index 4e7a978..81fcaae 100644 --- a/backend/queue/emit_test.go +++ b/backend/queue/emit_test.go @@ -219,6 +219,56 @@ func TestEmit_AddTrackSendsDeltaNotSnapshot(t *testing.T) { } } +// The append clears the source, and the delta is the only event those +// paths emit — so if it does not carry the source, the frontend keeps +// the label it was last given and goes on offering a link back to an +// album the queue no longer holds until something forces a full state. +func TestEmit_AppendDeltaCarriesClearedSource(t *testing.T) { + t.Parallel() + + q, db, rec := setupRecordedQueue(t) + paths := seedAudioFiles(t, db, 4) + + q.SetQueue( + paths[:3], 0, false, + Source{Type: "album", ID: 1, Label: "Abbey Road"}, + ) + + if _, ok := rec.Wait(events.QueueChanged, waitFor); !ok { + t.Fatalf("no QueueChanged after SetQueue; got %v", rec.Names()) + } + + rec.Reset() + q.AddTrack(paths[3]) + + if got := modifiedOf(t, rec).Source; got != (Source{}) { + t.Errorf("delta source = %+v, want zero value", got) + } +} + +// And a delta that did not clear it still reports the source it has, +// or the frontend would drop a perfectly good label on every removal. +func TestEmit_NonAppendDeltaCarriesSource(t *testing.T) { + t.Parallel() + + q, db, rec := setupRecordedQueue(t) + paths := seedAudioFiles(t, db, 4) + + album := Source{Type: "album", ID: 1, Label: "Abbey Road"} + q.SetQueue(paths, 0, false, album) + + if _, ok := rec.Wait(events.QueueChanged, waitFor); !ok { + t.Fatalf("no QueueChanged after SetQueue; got %v", rec.Names()) + } + + rec.Reset() + q.RemoveTrack(3) + + if got := modifiedOf(t, rec).Source; got != album { + t.Errorf("delta source = %+v, want %+v", got, album) + } +} + func TestEmit_RemoveTracksReportsPositions(t *testing.T) { t.Parallel() diff --git a/backend/queue/queue.go b/backend/queue/queue.go index 00ab9c2..8a29c28 100644 --- a/backend/queue/queue.go +++ b/backend/queue/queue.go @@ -156,12 +156,21 @@ type PlaybackFailure struct { } // TracksModified is the payload for the QueueTracksModified event. +// +// Source is carried because an append is exactly what can *invalidate* +// it: a queue built from one album stops being that album the moment a +// track from somewhere else is added to it. The delta is the only event +// those paths emit, so without this the frontend would keep the label +// it was last given and go on saying "Playing from" an album that is no +// longer what is queued — an event carrying what its consumer needs, so +// nothing has to invalidate anything. type TracksModified struct { Action string `json:"action"` Tracks []Track `json:"tracks,omitempty"` Index int `json:"index"` Positions []int `json:"positions,omitempty"` CurrentIndex int `json:"currentIndex"` + Source Source `json:"source"` } // Queue manages an ordered list of tracks for playback. @@ -455,6 +464,8 @@ func (q *Queue) AddTrack(filePath string) { q.generateShuffleOrder() } + q.dropSource() + q.persistAddTrack(track) q.persistState() q.emitTracksModified( @@ -505,6 +516,8 @@ func (q *Queue) AddTracks(filePaths []string) { q.generateShuffleOrder() } + q.dropSource() + q.persistAddTracks(newTracks) q.persistState() q.emitTracksModified( @@ -563,6 +576,8 @@ func (q *Queue) InsertNextTracks(filePaths []string) { q.generateShuffleOrder() } + q.dropSource() + q.persistInsertTracks(newTracks, insertPos) q.persistState() q.emitTracksModified( @@ -613,6 +628,8 @@ func (q *Queue) InsertNext(filePath string) { q.generateShuffleOrder() } + q.dropSource() + q.persistInsertTracks([]Track{track}, insertPos) q.persistState() q.emitTracksModified( @@ -680,6 +697,8 @@ func (q *Queue) InsertTracksAt(filePaths []string, index int) { q.generateShuffleOrder() } + q.dropSource() + q.persistInsertTracks(newTracks, index) q.persistState() q.emitTracksModified( @@ -1537,6 +1556,31 @@ func (q *Queue) reindexPositions() { } } +// dropSource forgets which collection the queue was built from. +// +// A Source is a claim that everything queued came from one album, +// playlist, genre or artist, and the frontend renders it as a +// "Playing from X" link back to that page. Adding or inserting a track +// makes the claim false — the queue is now that album *plus* something +// else — so every path that does so calls this. +// +// It was set by SetQueue and cleared in exactly one place, Clear, so a +// label survived every append. It is persisted too (source_type / +// source_id / source_label on the queue state row), which is what made +// a wrong label outlive the session that earned it: an album queued on +// Monday, added to on Tuesday, still offered a link back to that album +// on Friday. +// +// Removing, reordering and shuffling deliberately do not call this. A +// queue with a track taken out of it, or played in another order, is +// still that album — the link still goes somewhere true. Only the +// arrival of a track from elsewhere makes it a lie. +// +// The caller must hold q.mu. +func (q *Queue) dropSource() { + q.source = Source{} +} + // commitMutation persists the current queue state after a mutation. // When reindex is true, track positions are renumbered first. // The caller must hold q.mu. diff --git a/backend/queue/queue_test.go b/backend/queue/queue_test.go index 11c89bb..c062b6f 100644 --- a/backend/queue/queue_test.go +++ b/backend/queue/queue_test.go @@ -126,6 +126,117 @@ func TestClear_ResetsSource(t *testing.T) { } } +// A queue built from one album stops being that album the moment a +// track from somewhere else joins it, so every path that adds one +// drops the source. Before this, SetQueue was the only writer and +// Clear the only clearer, so "Playing from Abbey Road" outlived every +// append — and, being persisted, every restart too. +func TestAppendPathsDropSource(t *testing.T) { + t.Parallel() + + album := Source{Type: "album", ID: 1, Label: "Abbey Road"} + + tests := []struct { + name string + append func(q *Queue, paths []string) + }{ + { + name: "AddTrack", + append: func(q *Queue, paths []string) { + q.AddTrack(paths[5]) + }, + }, + { + name: "AddTracks", + append: func(q *Queue, paths []string) { + q.AddTracks(paths[5:7]) + }, + }, + { + name: "InsertNext", + append: func(q *Queue, paths []string) { + q.InsertNext(paths[5]) + }, + }, + { + name: "InsertNextTracks", + append: func(q *Queue, paths []string) { + q.InsertNextTracks(paths[5:7]) + }, + }, + { + name: "InsertTracksAt", + append: func(q *Queue, paths []string) { + q.InsertTracksAt(paths[5:7], 1) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + q, db := setupTestQueue(t) + paths := seedAudioFiles(t, db, 8) + + q.SetQueue(paths[:5], 0, false, album) + + if got := q.GetState().Source; got != album { + t.Fatalf("source before append: got %+v, want %+v", got, album) + } + + tt.append(q, paths) + + if got := q.GetState().Source; got != (Source{}) { + t.Errorf( + "source after %s: got %+v, want zero value", + tt.name, got, + ) + } + }) + } +} + +// Removing and reordering deliberately do not drop it: a queue with a +// track taken out of it is still that album, and the link still goes +// somewhere true. +func TestRemoveAndMoveKeepSource(t *testing.T) { + t.Parallel() + + album := Source{Type: "album", ID: 1, Label: "Abbey Road"} + + t.Run("RemoveTrack", func(t *testing.T) { + t.Parallel() + + q, db := setupTestQueue(t) + paths := seedAudioFiles(t, db, 5) + + q.SetQueue(paths, 0, false, album) + q.RemoveTrack(3) + + if got := q.GetState().Source; got != album { + t.Errorf("source after RemoveTrack: got %+v, want %+v", got, album) + } + }) + + t.Run("MoveQueueTracks", func(t *testing.T) { + t.Parallel() + + q, db := setupTestQueue(t) + paths := seedAudioFiles(t, db, 5) + + q.SetQueue(paths, 0, false, album) + q.MoveQueueTracks([]int{0}, 3) + + if got := q.GetState().Source; got != album { + t.Errorf( + "source after MoveQueueTracks: got %+v, want %+v", + got, album, + ) + } + }) +} + func TestSetQueue_WithStartIndex(t *testing.T) { t.Parallel() diff --git a/backend/smartplaylist/smartplaylist.go b/backend/smartplaylist/smartplaylist.go index f3030d5..9705419 100644 --- a/backend/smartplaylist/smartplaylist.go +++ b/backend/smartplaylist/smartplaylist.go @@ -28,6 +28,7 @@ var ( errUnsupportedOp = errors.New("unsupported operator") errInvalidSortField = errors.New("invalid sort field: not in allowed field list") errNotNumeric = errors.New("value must be numeric") + errInvalidMatch = errors.New("match must be \"all\" or \"any\"") ) // Rule represents a single filter condition for a smart playlist. @@ -37,13 +38,45 @@ type Rule struct { Value string `json:"value"` } +// MatchType decides how a rule set's conditions combine. +// +// The rules used to be joined with " AND " and nothing else, so a +// playlist could only ever narrow: "jazz released after 1960" was +// expressible and "jazz or blues" was not, which is most of what +// anyone reaches for a second rule to say. +type MatchType string + +const ( + // MatchAll requires every rule to hold — the historical behaviour, + // and what an empty match means so that every rule set written + // before this existed keeps the meaning it was saved with. + MatchAll MatchType = "all" + // MatchAny requires at least one rule to hold. + MatchAny MatchType = "any" +) + +// joiner returns the SQL keyword that combines two conditions. +// An unrecognised value cannot reach here — ParseRuleSet rejects one +// — so the default is about the empty string, which is every rule set +// saved before this field existed. +func (m MatchType) joiner() string { + if m == MatchAny { + return " OR " + } + + return " AND " +} + // RuleSet holds the complete filter configuration for a smart // playlist, including optional sort and limit. type RuleSet struct { - Rules []Rule `json:"rules"` - Limit int `json:"limit,omitempty"` - SortField string `json:"sort_field,omitempty"` - SortDir string `json:"sort_dir,omitempty"` + Rules []Rule `json:"rules"` + // Match is "all" or "any"; empty means "all". It is omitempty so + // an untouched playlist's stored JSON does not change shape. + Match MatchType `json:"match,omitempty"` + Limit int `json:"limit,omitempty"` + SortField string `json:"sort_field,omitempty"` + SortDir string `json:"sort_dir,omitempty"` } // fieldMap maps user-facing rule field names to track_metadata column @@ -116,7 +149,12 @@ const genreDelimiter = "||" // slice of rules. It is a pure function — no database access needed. // Returns the clause (without the leading "WHERE"), the parameter // args, and any validation error. -func BuildWhereClause(rules []Rule) (string, []any, error) { +// +// match decides how the conditions combine; an empty match is MatchAll, +// which is what every rule set saved before the field existed means. +func BuildWhereClause( + rules []Rule, match MatchType, +) (string, []any, error) { if len(rules) == 0 { return "", nil, nil } @@ -179,7 +217,28 @@ func BuildWhereClause(rules []Rule) (string, []any, error) { args = append(args, condArgs...) } - return strings.Join(conditions, " AND "), args, nil + // Under OR, each condition is parenthesised; under AND it is not. + // + // The asymmetry is deliberate rather than an omission. AND is the + // tighter operator in SQL, so an OR-join has to protect any + // condition that contains a top-level AND of its own or the halves + // come apart: `days_since_played less_than` is + // `last_played IS NOT NULL AND < ?`, which read without + // brackets under an OR-join happens to still parse correctly and + // would stop doing so the moment a condition grows a top-level OR. + // Bracketing under AND would be a no-op semantically and would + // rewrite the clause every existing test pins, so the brackets go + // exactly where they change something. + if match == MatchAny { + bracketed := make([]string, len(conditions)) + for i, cond := range conditions { + bracketed[i] = "(" + cond + ")" + } + + conditions = bracketed + } + + return strings.Join(conditions, match.joiner()), args, nil } // validateOperator checks that the operator is valid for the field @@ -599,7 +658,7 @@ func Evaluate( start := time.Now() logger := db.Logger() - where, args, err := BuildWhereClause(ruleSet.Rules) + where, args, err := BuildWhereClause(ruleSet.Rules, ruleSet.Match) if err != nil { return nil, fmt.Errorf( "smart playlist rule error: %w", err, @@ -1036,6 +1095,16 @@ func ParseRuleSet(jsonStr string) (RuleSet, error) { ) } + // A match nobody recognises would otherwise fall through to AND, + // which is a playlist quietly returning the wrong tracks rather + // than refusing to be saved. This is the only place a rule set + // enters the backend, so it is the only place that has to ask. + if rs.Match != "" && rs.Match != MatchAll && rs.Match != MatchAny { + return RuleSet{}, fmt.Errorf( + "%w: %q", errInvalidMatch, rs.Match, + ) + } + return rs, nil } diff --git a/backend/smartplaylist/smartplaylist_test.go b/backend/smartplaylist/smartplaylist_test.go index 5c2efa0..7259a60 100644 --- a/backend/smartplaylist/smartplaylist_test.go +++ b/backend/smartplaylist/smartplaylist_test.go @@ -1,6 +1,7 @@ package smartplaylist import ( + "errors" "strings" "testing" @@ -170,7 +171,7 @@ func TestBuildWhereClause_TextIs(t *testing.T) { clause, args, err := BuildWhereClause([]Rule{ {Field: "artist", Operator: "is", Value: "Queen"}, - }) + }, MatchAll) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -189,7 +190,7 @@ func TestBuildWhereClause_TextIsNot(t *testing.T) { clause, args, err := BuildWhereClause([]Rule{ {Field: "artist", Operator: "is_not", Value: "Queen"}, - }) + }, MatchAll) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -209,7 +210,7 @@ func TestBuildWhereClause_TextContains(t *testing.T) { clause, args, err := BuildWhereClause([]Rule{ {Field: "title", Operator: "contains", Value: "Black"}, - }) + }, MatchAll) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -231,7 +232,7 @@ func TestBuildWhereClause_TextDoesNotContain(t *testing.T) { Field: "title", Operator: "does_not_contain", Value: "Black", }, - }) + }, MatchAll) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -251,7 +252,7 @@ func TestBuildWhereClause_TextStartsWith(t *testing.T) { clause, args, err := BuildWhereClause([]Rule{ {Field: "title", Operator: "starts_with", Value: "Back"}, - }) + }, MatchAll) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -270,7 +271,7 @@ func TestBuildWhereClause_TextEndsWith(t *testing.T) { clause, args, err := BuildWhereClause([]Rule{ {Field: "title", Operator: "ends_with", Value: "Black"}, - }) + }, MatchAll) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -292,7 +293,7 @@ func TestBuildWhereClause_TextIsAnyOf(t *testing.T) { Field: "artist", Operator: "is_any_of", Value: `["Queen","AC/DC"]`, }, - }) + }, MatchAll) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -312,7 +313,7 @@ func TestBuildWhereClause_NumericIs(t *testing.T) { clause, args, err := BuildWhereClause([]Rule{ {Field: "year", Operator: "is", Value: "1980"}, - }) + }, MatchAll) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -331,7 +332,7 @@ func TestBuildWhereClause_NumericIsNot(t *testing.T) { clause, args, err := BuildWhereClause([]Rule{ {Field: "year", Operator: "is_not", Value: "1980"}, - }) + }, MatchAll) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -350,7 +351,7 @@ func TestBuildWhereClause_NumericGreaterThan(t *testing.T) { clause, args, err := BuildWhereClause([]Rule{ {Field: "year", Operator: "greater_than", Value: "2000"}, - }) + }, MatchAll) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -369,7 +370,7 @@ func TestBuildWhereClause_NumericLessThan(t *testing.T) { clause, args, err := BuildWhereClause([]Rule{ {Field: "year", Operator: "less_than", Value: "1980"}, - }) + }, MatchAll) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -391,7 +392,7 @@ func TestBuildWhereClause_NumericBetween(t *testing.T) { Field: "year", Operator: "between", Value: "1975,1985", }, - }) + }, MatchAll) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -414,7 +415,7 @@ func TestBuildWhereClause_NumericBetweenJSON(t *testing.T) { Field: "year", Operator: "between", Value: `["1975","1985"]`, }, - }) + }, MatchAll) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -434,7 +435,7 @@ func TestBuildWhereClause_GenreIsProducesSubquery(t *testing.T) { clause, args, err := BuildWhereClause([]Rule{ {Field: "genre", Operator: "is", Value: "Rock"}, - }) + }, MatchAll) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -466,7 +467,7 @@ func TestBuildWhereClause_GenreIsNotProducesSubquery(t *testing.T) { clause, args, err := BuildWhereClause([]Rule{ {Field: "genre", Operator: "is_not", Value: "Rock"}, - }) + }, MatchAll) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -495,7 +496,7 @@ func TestBuildWhereClause_GenreIsAnyOfProducesSubquery(t *testing.T) { Field: "genre", Operator: "is_any_of", Value: `["Rock","Pop"]`, }, - }) + }, MatchAll) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -524,7 +525,7 @@ func TestBuildWhereClause_GenreContainsUsesSubquery(t *testing.T) { clause, args, err := BuildWhereClause([]Rule{ {Field: "genre", Operator: "contains", Value: "Rock"}, - }) + }, MatchAll) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -557,7 +558,7 @@ func TestBuildWhereClause_MultipleRulesAND(t *testing.T) { clause, args, err := BuildWhereClause([]Rule{ {Field: "artist", Operator: "is", Value: "Queen"}, {Field: "year", Operator: "greater_than", Value: "1975"}, - }) + }, MatchAll) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -572,6 +573,110 @@ func TestBuildWhereClause_MultipleRulesAND(t *testing.T) { } } +func TestBuildWhereClause_MultipleRulesOR(t *testing.T) { + t.Parallel() + + clause, args, err := BuildWhereClause([]Rule{ + {Field: "artist", Operator: "is", Value: "Queen"}, + {Field: "year", Operator: "greater_than", Value: "1975"}, + }, MatchAny) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + want := "(artist_name = ? COLLATE NOCASE) OR (year > ?)" + if clause != want { + t.Errorf("clause = %q, want %q", clause, want) + } + + if len(args) != 2 || args[0] != "Queen" || args[1] != int64(1975) { + t.Errorf("args = %v, want [Queen 1975]", args) + } +} + +// An empty match is what every rule set saved before the field existed +// carries, and it has to keep meaning AND — a playlist silently +// widening to OR on upgrade is the whole risk of adding this field. +func TestBuildWhereClause_EmptyMatchIsAll(t *testing.T) { + t.Parallel() + + rules := []Rule{ + {Field: "artist", Operator: "is", Value: "Queen"}, + {Field: "year", Operator: "greater_than", Value: "1975"}, + } + + empty, _, err := BuildWhereClause(rules, "") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + all, _, err := BuildWhereClause(rules, MatchAll) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if empty != all { + t.Errorf("empty match = %q, want the same as MatchAll %q", + empty, all) + } +} + +// A condition carrying its own top-level AND is what makes the +// bracketing under OR load-bearing: `days_since_played less_than` +// is two predicates, and both belong to the same rule. +func TestBuildWhereClause_ORBracketsCompoundCondition(t *testing.T) { + t.Parallel() + + clause, _, err := BuildWhereClause([]Rule{ + {Field: "artist", Operator: "is", Value: "Queen"}, + { + Field: "days_since_played", + Operator: "less_than", + Value: "30", + }, + }, MatchAny) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if !strings.Contains(clause, "(last_played IS NOT NULL AND") { + t.Errorf( + "compound condition is not bracketed under OR: %q", + clause, + ) + } +} + +func TestParseRuleSet_RejectsUnknownMatch(t *testing.T) { + t.Parallel() + + _, err := ParseRuleSet(`{"rules":[],"match":"either"}`) + if err == nil { + t.Fatal("expected an error for an unknown match type") + } + + if !errors.Is(err, errInvalidMatch) { + t.Errorf("err = %v, want errInvalidMatch", err) + } +} + +func TestParseRuleSet_AcceptsAnyAndAll(t *testing.T) { + t.Parallel() + + for _, want := range []MatchType{MatchAll, MatchAny} { + rs, err := ParseRuleSet( + `{"rules":[],"match":"` + string(want) + `"}`, + ) + if err != nil { + t.Fatalf("match %q: unexpected error: %v", want, err) + } + + if rs.Match != want { + t.Errorf("match = %q, want %q", rs.Match, want) + } + } +} + func TestBuildWhereClause_SameFieldMultipleTimes(t *testing.T) { t.Parallel() @@ -581,7 +686,7 @@ func TestBuildWhereClause_SameFieldMultipleTimes(t *testing.T) { Field: "genre", Operator: "does_not_contain", Value: "Punk", }, - }) + }, MatchAll) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -609,7 +714,7 @@ func TestBuildWhereClause_SameFieldMultipleTimes(t *testing.T) { func TestBuildWhereClause_EmptyRules(t *testing.T) { t.Parallel() - clause, args, err := BuildWhereClause(nil) + clause, args, err := BuildWhereClause(nil, MatchAll) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -631,7 +736,7 @@ func TestBuildWhereClause_InvalidField(t *testing.T) { Field: "nonexistent", Operator: "is", Value: "anything", }, - }) + }, MatchAll) if err == nil { t.Fatal("expected error for invalid field, got nil") } @@ -654,7 +759,7 @@ func TestBuildWhereClause_InvalidOperatorForNumeric(t *testing.T) { _, _, err := BuildWhereClause([]Rule{ {Field: "year", Operator: "contains", Value: "1980"}, - }) + }, MatchAll) if err == nil { t.Fatal( "expected error for text operator on numeric field", @@ -676,7 +781,7 @@ func TestBuildWhereClause_InvalidOperatorForText(t *testing.T) { Field: "artist", Operator: "greater_than", Value: "Queen", }, - }) + }, MatchAll) if err == nil { t.Fatal( "expected error for numeric operator on text field", @@ -723,6 +828,80 @@ func TestEvaluate_TextIs(t *testing.T) { } } +// Two rules that share no track at all: under AND this is empty, and +// under OR it is the union. Before Match existed only the first was +// expressible, so a playlist could only ever narrow — "jazz or blues" +// had no way to be said. +func TestEvaluate_MatchAnyUnionsWhereMatchAllIntersects(t *testing.T) { + t.Parallel() + + db := database.NewTestDB(t) + seedSmartPlaylistData(t, db) + + // Queen has two tracks; Beyoncé has one; no track is by both. + rules := []Rule{ + {Field: "artist", Operator: "is", Value: "Queen"}, + {Field: "artist", Operator: "is", Value: "Beyoncé"}, + } + + all, err := Evaluate(db, RuleSet{Rules: rules, Match: MatchAll}) + if err != nil { + t.Fatalf("Evaluate(all): %v", err) + } + + if len(all) != 0 { + t.Errorf("match=all returned %d tracks, want 0", len(all)) + } + + either, err := Evaluate(db, RuleSet{Rules: rules, Match: MatchAny}) + if err != nil { + t.Fatalf("Evaluate(any): %v", err) + } + + if len(either) != 3 { + t.Fatalf("match=any returned %d tracks, want 3", len(either)) + } + + for _, tr := range either { + if tr.ArtistName != "Queen" && tr.ArtistName != "Beyoncé" { + t.Errorf( + "track %q has artist %q, want Queen or Beyoncé", + tr.TrackName, tr.ArtistName, + ) + } + } +} + +// An empty match is what every playlist saved before the field existed +// carries, and it has to keep meaning AND all the way through Evaluate +// — a stored playlist silently widening on upgrade is the only real +// risk in adding this. +func TestEvaluate_EmptyMatchStillIntersects(t *testing.T) { + t.Parallel() + + db := database.NewTestDB(t) + seedSmartPlaylistData(t, db) + + tracks, err := Evaluate(db, RuleSet{ + Rules: []Rule{ + {Field: "artist", Operator: "is", Value: "Queen"}, + {Field: "year", Operator: "greater_than", Value: "1979"}, + }, + }) + if err != nil { + t.Fatalf("Evaluate: %v", err) + } + + // Only "Another One Bites the Dust" (Queen, 1980) satisfies both. + if len(tracks) != 1 { + t.Fatalf("got %d tracks, want 1", len(tracks)) + } + + if want := "Another One Bites the Dust"; tracks[0].TrackName != want { + t.Errorf("got %q, want %q", tracks[0].TrackName, want) + } +} + // TestEvaluate_ArtworkEnrichment verifies the presentation-only // cover-art and MusicBrainz-ID fields are attached to matched tracks // by the batched fetchArtwork pass (they are no longer part of the @@ -1340,7 +1519,7 @@ func TestSQLInjection_FieldName(t *testing.T) { Field: "title; DROP TABLE playlists", Operator: "is", Value: "x", }, - }) + }, MatchAll) if err == nil { t.Fatal( "expected error for injected field name, got nil", diff --git a/e2e/specs/queue-toggle-state.spec.ts b/e2e/specs/queue-toggle-state.spec.ts new file mode 100644 index 0000000..78ac663 --- /dev/null +++ b/e2e/specs/queue-toggle-state.spec.ts @@ -0,0 +1,66 @@ +import { test, expect } from '../support/fixtures.js'; + +/** + * The queue button says whether the queue is open. + * + * It used to look identical in both states, so the only way to tell + * what pressing it would do was to look at the other side of the window + * and infer it — and for anyone not looking at all there was nothing to + * infer from: no `aria-expanded`, no `aria-controls`, no pressed state. + * + * The state is reflected *from the panel*, not kept beside the click, + * because the button is not the only thing that opens the queue — + * `now-playing-view` sets the same attribute, since it hides the bar + * this button lives in. A flag maintained by the click handler would be + * right until something else opened the panel and then quietly wrong, + * which is the second test here. + */ +test.describe('the queue toggle', () => { + test('reports open and closed, and names what it controls', async ({ + app, + }) => { + const toggle = app.locator('#queue-button'); + + await expect(toggle).toHaveAttribute('aria-controls', 'queue-panel'); + await expect(toggle).toHaveAttribute('aria-expanded', 'false'); + + await toggle.click(); + await expect(toggle).toHaveAttribute('aria-expanded', 'true'); + + // The state is not only in the accessibility tree: a control that + // announces a state it does not draw is half a fix. + // + // Background rather than colour, because the pointer is still on + // the button after the click and `:hover` paints it the same accent + // the open state does -- so a colour comparison here passes on the + // broken build and proves nothing. + const [open, closed] = await toggle.evaluate((el) => { + const now = getComputedStyle(el).backgroundColor; + + el.setAttribute('aria-expanded', 'false'); + const shut = getComputedStyle(el).backgroundColor; + + el.setAttribute('aria-expanded', 'true'); + + return [now, shut]; + }); + + expect(open).not.toBe(closed); + + await toggle.click(); + await expect(toggle).toHaveAttribute('aria-expanded', 'false'); + }); + + test('follows the panel when something else opens it', async ({ app }) => { + const toggle = app.locator('#queue-button'); + + await expect(toggle).toHaveAttribute('aria-expanded', 'false'); + + // Exactly what `now-playing-view`'s queue button does. + await app.evaluate(() => + document.getElementById('queue-panel')?.setAttribute('open', ''), + ); + + await expect(toggle).toHaveAttribute('aria-expanded', 'true'); + }); +}); diff --git a/frontend/index.css b/frontend/index.css index 5d5fe0a..7549e03 100644 --- a/frontend/index.css +++ b/frontend/index.css @@ -207,6 +207,17 @@ body div.sidebar { color: var(--yj-accent, #ffd43b); } + /* An open queue is a state this button can be in, and it used to + look exactly like the closed one -- so the only way to tell what + pressing it would do was to look at the other side of the window + and infer it. `aria-expanded` is the same fact for anyone not + looking at all, and it points at the panel it controls. */ + #queue-button[aria-expanded='true'] { + color: var(--yj-accent, #ffd43b); + background: var(--yj-bg-overlay, #404040); + border-radius: 4px; + } + #queue-button.drag-over { color: var(--yj-accent, #ffd43b); outline: 2px dashed var(--yj-accent, #ffd43b); diff --git a/frontend/index.html b/frontend/index.html index 7af95aa..14b3786 100644 --- a/frontend/index.html +++ b/frontend/index.html @@ -37,7 +37,8 @@ diff --git a/frontend/index.ts b/frontend/index.ts index 0b05564..2431050 100644 --- a/frontend/index.ts +++ b/frontend/index.ts @@ -521,6 +521,28 @@ if (queueButton && queuePanel) { } }); + // The button says whether the panel is open, and it learns that + // from the panel rather than from its own click handler. + // + // It is not the only thing that opens the queue -- `now-playing-view` + // sets the same attribute, because it hides the bar this button + // lives in -- so a state kept beside the click would be right until + // something else opened the panel and then quietly wrong. The panel's + // `open` attribute is the one fact; this reflects it. + const reflectQueueState = () => { + queueButton.setAttribute( + 'aria-expanded', + String(queuePanel.hasAttribute('open')), + ); + }; + + new MutationObserver(reflectQueueState).observe(queuePanel, { + attributes: true, + attributeFilter: ['open'], + }); + + reflectQueueState(); + // --------------------------------------------------------------- // Queue button as drop target (when queue panel is closed) // --------------------------------------------------------------- diff --git a/frontend/src/components/audio-player/seekbar/seek-bar.ts b/frontend/src/components/audio-player/seekbar/seek-bar.ts index 95f38aa..5ef3ea6 100644 --- a/frontend/src/components/audio-player/seekbar/seek-bar.ts +++ b/frontend/src/components/audio-player/seekbar/seek-bar.ts @@ -68,6 +68,29 @@ export class SeekBar extends LitElement { align-items: center; } + /* The clocks must not resize as they count. + + Two things move them, and they need different answers. Digits in + a proportional font are different widths, so 1:11 is narrower + than 4:08 and the bar breathed once a second -- that is what + tabular figures fix. The character *count* changes too, at the + hundredth minute and whenever the right-hand clock is toggled to + remaining and grows a minus sign, and a figure width cannot fix + that -- so each clock also reserves the widest string this track + can put in it. The budget is per track rather than a constant + because reserving six characters on every track would push the + slider in by a character at each end for nothing. */ + #seek-bar-container small, + .time-toggle { + font-variant-numeric: tabular-nums; + flex: 0 0 auto; + min-width: calc(var(--yj-clock-chars, 5) * 1ch); + } + + #seek-bar-container small { + text-align: left; + } + .time-toggle { background: none; border: none; @@ -76,6 +99,9 @@ export class SeekBar extends LitElement { font: inherit; font-size: var(--wa-font-size-s, 0.875rem); cursor: pointer; + /* One more for the minus sign the remaining form carries. */ + min-width: calc((var(--yj-clock-chars, 5) + 1) * 1ch); + text-align: right; } .time-toggle:hover, @@ -219,8 +245,19 @@ export class SeekBar extends LitElement { : formatSeconds(this.trackLength); const rightTime = this.hasTrack ? rightLabel : '--:--'; + // The widest string either clock can hold for *this* track. The + // duration is the longest elapsed value there can be, so its length + // is the budget; `--:--` is five, which is also the floor. + const clockChars = Math.max( + 5, + this.hasTrack ? formatSeconds(this.trackLength).length : 0, + ); + return html` -
+
${elapsedTime} - ${album.Name}${album.Year - ? html` - - (${album.Year})${album.Name}${album.Year + ? html`(${album.Year})` : nothing}
diff --git a/frontend/src/components/downloads-view/downloads-view.ts b/frontend/src/components/downloads-view/downloads-view.ts index 8047e34..43441eb 100644 --- a/frontend/src/components/downloads-view/downloads-view.ts +++ b/frontend/src/components/downloads-view/downloads-view.ts @@ -512,7 +512,9 @@ export class DownloadsView extends ViewLifecycleMixin(LitElement) { ${request.artist ? `${request.artist} — ` : ''}${request.title || request.mbid}
-
${requestDetail(request, this.nowMs)}
+
+ ${requestDetail(request, this.nowMs, this.canDownload)} +
${request.state === 'satisfied' @@ -706,10 +708,20 @@ export class DownloadsView extends ViewLifecycleMixin(LitElement) { * looked for rather than as an error, because that is what it is — the * retry is already scheduled and there is nothing for the user to do. */ -function requestDetail(request: Request, nowMs: number): string { +function requestDetail( + request: Request, + nowMs: number, + canDownload: boolean, +): string { if (request.state === 'satisfied') return 'In your library'; if (request.state === 'paused') return 'Paused — not being looked for'; + // With no client there is no search and no retry clock — the + // backend stopped scheduling one — so a row must not imply either. + // "Queued" and "next check in 6 hours" are both promises nothing is + // in a position to keep. + if (!canDownload) return 'On your list — no download client to search with'; + if (request.attempts === 0) return 'Queued — not searched for yet'; const tries = `Searched ${request.attempts} time${request.attempts === 1 ? '' : 's'}`; diff --git a/frontend/src/components/explore-album-details/explore-album-details.ts b/frontend/src/components/explore-album-details/explore-album-details.ts index 45a02d5..f447e96 100644 --- a/frontend/src/components/explore-album-details/explore-album-details.ts +++ b/frontend/src/components/explore-album-details/explore-album-details.ts @@ -2,6 +2,7 @@ import { LitElement, html, css, nothing } from 'lit'; import { customElement, property, state, query } from 'lit/decorators.js'; import { classMap } from 'lit/directives/class-map.js'; import { designTokens } from '../../styles/tokens.css'; +import { srOnly } from '../../styles/sr-only.css'; import { LookupReleaseGroup, BrowseReleases, @@ -289,6 +290,7 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost { designTokens, exploreLinkStyles, contextMenuStyles, + srOnly, css` :host { display: flex; @@ -662,34 +664,21 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost { font-weight: 400; } - /* The request control is only offered where there is - * something to request, and only when the row is being - * attended to — a column of plus signs down a mostly-owned - * album is the clutter the green ticks were. + /* The request control is offered on every row that has + * something to request, and is not revealed on hover. * - * Hidden with opacity, never display:none or visibility, - * so it keeps its place in the layout (rows do not reflow - * as the pointer moves) and stays in the tab order and the - * accessibility tree. focus-within is what makes it - * reachable without a mouse: tabbing to the button reveals - * it, and the row's own focus reveals it before you get - * there. */ + * It used to be transparent until the row was hovered or + * focused, on the reasoning that a column of plus signs + * down a mostly-owned album is clutter. That reasoning was + * inherited from the green ticks it replaced and does not + * survive the rule those were removed for: a tick marked + * the *common* case, while this marks the rows that are + * **not** here. A mark on the exception is the information + * on this page — and one that appears only under the + * pointer cannot be seen, counted, or reached by anyone + * driving this with a finger. */ .track-row .track-request { flex-shrink: 0; - opacity: 0; - transition: opacity 0.12s ease; - } - - .track-row:hover .track-request, - .track-row:focus-within .track-request, - .track-row .track-request:focus-visible { - opacity: 1; - } - - @media (prefers-reduced-motion: reduce) { - .track-row .track-request { - transition: none; - } } `, ]; @@ -720,14 +709,28 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost { // The download button only appears once a client is connected, // so this tracks the provider list rather than assuming. + // + // The `requestUpdate` is what makes the *tracklist's* badges + // move. Both assignments below are reactive fields, so Lit + // repaints when either changes — but a track request changes + // neither: `canDownload` is about providers and `isRequested` + // is about this album's own release group. Each row's badge + // reads `libraryStatusFor(false, track.mbid)` at render time, + // which is a dependency on the store that Lit cannot see, so + // clicking one filed the request and left the plus exactly + // where it was. The other three hosts rendering these badges + // (`explore-artist-details`, `explore-view`, `top-results-row`) + // have always asked for the repaint here; this one did not. this.downloadUnsub = downloadStore.subscribe(() => { this.canDownload = downloadStore.available; this.syncRequested(); + this.requestUpdate(); }); void downloadStore.init().then(() => { this.canDownload = downloadStore.available; this.syncRequested(); + this.requestUpdate(); }); void this.resolveTargetLibraryId(); @@ -3033,11 +3036,21 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost { /* ── Tracklist ── */ + /** + * The heading is there and is not drawn. + * + * A list of numbered titles with durations under an album's cover + * does not need a word above it saying what it is — it was the + * only thing on this page labelling something already obvious. But + * the section is a landmark and the page's heading structure runs + * through it, so what goes is the *ink*, not the element: a reader + * jumping by heading still finds the tracklist. + */ private renderTracklist() { if (this.loadingReleases) { return html`
-

Tracklist

+

Tracklist

Loading tracks\u2026
`; @@ -3050,7 +3063,7 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost { if (!current) { return html`
-

Tracklist

+

Tracklist

No release data available. @@ -3063,7 +3076,7 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost { if (tracks.length === 0) { return html`
-

Tracklist

+

Tracklist

@@ -3079,7 +3092,7 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost { return html`
-

Tracklist

+

Tracklist

${discNumbers.map((discNum) => { const discTracks = discMap.get(discNum) ?? []; diff --git a/frontend/src/components/smart-playlist-editor/smart-playlist-editor.ts b/frontend/src/components/smart-playlist-editor/smart-playlist-editor.ts index 5e723ec..c74d668 100644 --- a/frontend/src/components/smart-playlist-editor/smart-playlist-editor.ts +++ b/frontend/src/components/smart-playlist-editor/smart-playlist-editor.ts @@ -193,6 +193,10 @@ export class SmartPlaylistEditor extends LitElement { // ── Internal state ────────────────────────────────────────────── @state() private ruleRows: RuleRow[] = [emptyRule()]; + /** Whether every rule must hold or any one of them. Mirrors the + * backend's `match`; 'all' is the default and the only thing a + * playlist saved before this existed can have meant. */ + @state() private matchType: 'all' | 'any' = 'all'; @state() private limit = 0; @state() private sortField = 'random'; @state() private sortDir = ''; @@ -224,6 +228,19 @@ export class SmartPlaylistEditor extends LitElement { flex-shrink: 0; } + .match-row { + display: flex; + align-items: center; + gap: 6px; + font-size: var(--yj-text-sm); + color: var(--yj-text-secondary, #b3b3b3); + flex-wrap: wrap; + } + + .match-select { + min-width: 72px; + } + .rule-row { display: grid; grid-template-columns: 160px 140px 1fr 28px; @@ -511,6 +528,10 @@ export class SmartPlaylistEditor extends LitElement { ); this.ruleRows = rows.length > 0 ? rows : [emptyRule()]; + // A playlist saved before this field existed has no match + // and means "all" — the backend reads an empty match the + // same way, so an upgrade cannot widen anyone's playlist. + this.matchType = parsed.match === 'any' ? 'any' : 'all'; this.limit = parsed.limit ?? 0; this.sortField = parsed.sort_field || 'random'; this.sortDir = parsed.sort_dir ?? ''; @@ -551,6 +572,7 @@ export class SmartPlaylistEditor extends LitElement { return JSON.stringify({ rules, + match: this.matchType, limit: this.limit || 0, sort_field: this.sortField || '', sort_dir: this.sortDir || '', @@ -630,6 +652,11 @@ export class SmartPlaylistEditor extends LitElement { this.onRulesChanged(); } + private updateMatchType(value: string) { + this.matchType = value === 'any' ? 'any' : 'all'; + this.onRulesChanged(); + } + private updateSortField(value: string) { this.sortField = value; if (!value) this.sortDir = ''; @@ -731,6 +758,7 @@ export class SmartPlaylistEditor extends LitElement { override render() { return html`
+ ${this.renderMatchType()} ${this.ruleRows.map((row, index) => this.renderRuleRow(row, index), )} @@ -743,6 +771,46 @@ export class SmartPlaylistEditor extends LitElement { `; } + /** + * Whether every rule has to hold, or any one of them. + * + * It is a sentence with a control in the middle rather than a + * labelled field, because the two readings differ by one word and + * that word is the whole of the setting — "Match **all** of the + * following rules" says what the list below it means in a way a + * select labelled "Match" beside a list does not. + * + * Hidden while there is one rule: with nothing to combine, all and + * any are the same query, and a control whose two settings cannot + * differ is a question the user has no way to answer wrongly and + * no reason to answer at all. + */ + private renderMatchType() { + if (this.ruleRows.length < 2) return nothing; + + return html` +
+ Match + + of the following rules +
+ `; + } + private renderRuleRow(row: RuleRow, index: number) { const isBetween = row.operator === 'between'; const operators = row.field ? getOperatorsForField(row.field) : []; diff --git a/frontend/src/store/queue-store.ts b/frontend/src/store/queue-store.ts index ed62dbb..63c1874 100644 --- a/frontend/src/store/queue-store.ts +++ b/frontend/src/store/queue-store.ts @@ -57,6 +57,12 @@ interface TracksModified { index: number; positions?: number[]; currentIndex: number; + /** The queue's source *after* the mutation. An append clears it + * backend-side — a queue built from one album is not that album + * once a track from elsewhere joins it — and this delta is the + * only event those paths emit, so the label would otherwise keep + * pointing at a collection the queue no longer holds. */ + source?: QueueSource; } type Subscriber = () => void; @@ -198,6 +204,7 @@ class QueueStore { } this.state.currentIndex = delta.currentIndex; + this.state.source = delta.source ?? EMPTY_QUEUE_SOURCE; } // =================================================================== diff --git a/frontend/src/utils/drag-image.ts b/frontend/src/utils/drag-image.ts index d0dff31..6002dcb 100644 --- a/frontend/src/utils/drag-image.ts +++ b/frontend/src/utils/drag-image.ts @@ -29,11 +29,23 @@ export function createDragImage(count: number): HTMLElement { } /** - * Creates a drag image showing an album cover art thumbnail. - * Falls back to the track-count badge if the image fails to load. + * Creates a drag image showing an album cover art thumbnail, with a + * corner badge saying how many tracks are on the way. + * + * The count is not decoration. The cover says *what* is being dragged + * and nothing said *how much* — an album is 1 track or 30 and the + * thumbnail is identical either way, so the one number the drop is + * about was the one thing the drag did not show. Every other drag in + * the app says it (`createDragImage` is a count and nothing else); + * this one was the exception because it had a picture to show instead. + * + * A count of 1 draws no badge: "1" over a single album cover is noise, + * and the absence is unambiguous next to a badge that only ever + * appears when there is more than one. */ export function createAlbumArtDragImage( coverUrl: string, + count = 1, ): HTMLElement { const size = 64; const wrapper = document.createElement('div'); @@ -44,6 +56,12 @@ export function createAlbumArtDragImage( 'left: -1000px', 'pointer-events: none', 'z-index: 9999', + // The badge is positioned against this box, and the box stays + // exactly the cover's size: anything outside it risks being + // clipped out of the snapshot the browser takes, and padding + // it instead would move the cover away from the cursor. + `width: ${size}px`, + `height: ${size}px`, ].join(';'); const img = document.createElement('img'); @@ -61,11 +79,44 @@ export function createAlbumArtDragImage( ].join(';'); wrapper.appendChild(img); + + if (count > 1) { + wrapper.appendChild(countBadge(count)); + } + document.body.appendChild(wrapper); return wrapper; } +/** The corner badge on a multi-track drag image. */ +function countBadge(count: number): HTMLElement { + const badge = document.createElement('span'); + + badge.className = 'drag-count-badge'; + badge.textContent = String(count); + badge.style.cssText = [ + 'position: absolute', + 'top: 3px', + 'right: 3px', + 'min-width: 20px', + 'height: 20px', + 'padding: 0 5px', + 'box-sizing: border-box', + 'border-radius: 10px', + 'background: #ffd43b', + 'color: #000', + 'font-size: 12px', + 'font-weight: 600', + 'font-family: inherit', + 'line-height: 20px', + 'text-align: center', + 'box-shadow: 0 1px 4px rgba(0,0,0,0.5)', + ].join(';'); + + return badge; +} + /** * Creates a drag image styled like a queue track card showing the * track title and artist. Used when dragging a single track. diff --git a/frontend/test/components/album-card-year.test.ts b/frontend/test/components/album-card-year.test.ts new file mode 100644 index 0000000..ce57a48 --- /dev/null +++ b/frontend/test/components/album-card-year.test.ts @@ -0,0 +1,87 @@ +/** + * The year on an album card survives a long album name. + * + * The year used to be part of the same run of text as the title, inside + * one `text-overflow: ellipsis` box — so it was the first thing the + * ellipsis ate. A card wide enough for a long name never showed its + * year at all, which means sorting the grid *by year* showed years only + * for the albums with short names: the sort said one thing and the + * cards showed another. + * + * The fix is a flex row in which only the title truncates, rather than + * a second line, because the card's height is what the virtualizer + * measures rows by. + */ +import { describe, expect, it, beforeEach } from 'vitest'; +import type { LitElement } from 'lit'; + +import '@components/cover-grid/cover-grid'; +import { emit, stub, flush, resetHarness } from '@test/support/harness'; +import { Events } from '../../src/events'; +import { fixture, shadowAll } from '@test/support/render'; + +const LONG = + 'The Rise and Fall of a Midwest Princess in the Key of Everything'; + +/** Long names throughout: the fault only shows on a card under + * pressure, and a grid of "Album 3" proves nothing. */ +const ALBUMS = Array.from({ length: 12 }, (_, i) => ({ + ID: i + 1, + Name: `${LONG} ${i + 1}`, + ArtistName: 'Aurora Fields', + Year: 2019 + (i % 5), +})); + +/** Give the virtualizer a viewport; a zero-height host renders nothing. */ +function sized(el: HTMLElement): void { + el.style.display = 'block'; + el.style.height = '600px'; + el.style.width = '900px'; +} + +async function settle(el: LitElement): Promise { + await flush(); + await el.updateComplete; + await new Promise((r) => setTimeout(r, 80)); +} + +describe('the album card’s year', () => { + beforeEach(() => { + resetHarness(); + stub('library.Library.GetAlbums', ALBUMS); + stub('library.Library.GetTracks', []); + emit(Events.LibraryScanComplete); + }); + + it('is rendered on every card, however long the name', async () => { + const el = await fixture('cover-grid'); + + sized(el); + await settle(el); + + const cards = shadowAll(el, '.album-card'); + const years = shadowAll(el, '.album-year'); + + expect(cards.length).toBeGreaterThan(0); + expect(years).toHaveLength(cards.length); + expect(years.every((y) => /^\(\d{4}\)$/.test(y.textContent!.trim()))).toBe( + true, + ); + }); + + it('is not what the ellipsis eats', async () => { + const el = await fixture('cover-grid'); + + sized(el); + await settle(el); + + const year = shadowAll(el, '.album-year')[0]!; + const title = shadowAll(el, '.album-title')[0]!; + + // The title is the box that gives way... + expect(title.scrollWidth).toBeGreaterThan(title.clientWidth); + // ...and the year keeps every pixel it asked for. + expect(year.clientWidth).toBeGreaterThan(0); + expect(year.scrollWidth).toBeLessThanOrEqual(year.clientWidth + 1); + }); +}); diff --git a/frontend/test/components/album-request-badge-visibility.test.ts b/frontend/test/components/album-request-badge-visibility.test.ts new file mode 100644 index 0000000..71b032e --- /dev/null +++ b/frontend/test/components/album-request-badge-visibility.test.ts @@ -0,0 +1,97 @@ +/** + * The request badge on an unowned row is there without being hovered. + * + * It used to be transparent until the row was hovered or focused, on + * the reasoning that a column of plus signs down a mostly-owned album + * is clutter. That reasoning came from the green ticks it replaced and + * does not survive the rule those were removed for: a tick marked the + * **common** case, while this marks the rows that are *not* here. A + * mark on the exception is the information on this page, and one that + * exists only under the pointer cannot be seen, counted, or reached by + * anyone driving the app with a finger. + * + * That the badge *repaints* when clicked is the other half of #33 and + * is covered by `album-track-request.test.ts`. + */ +import { describe, expect, it, beforeEach } from 'vitest'; +import type { LitElement } from 'lit'; + +import '@components/explore-album-details/explore-album-details'; +import { stub, flush, resetHarness } from '@test/support/harness'; +import { fixture, shadowAll } from '@test/support/render'; + +function track(n: number, owned: boolean) { + return { + position: n, + discNumber: 1, + title: `Track ${n}`, + length: 200000, + mbid: `mbid-${n}`, + inLibrary: owned, + }; +} + +/** An album with one owned track and one that is not here. */ +async function albumWithAnUnownedTrack(): Promise { + const el = await fixture('explore-album-details', { + albumName: 'Glass Harbour', + releaseGroupMBID: 'rg-1', + }); + + stub('library.Library.GetFilePathsByRecordingMBIDs', { + 'mbid-1': ['/music/mbid-1.mp3'], + }); + + Object.assign(el, { + versionEntries: [ + { + key: 'v1', + label: '2019', + sublabel: '2 tracks', + tracks: [track(1, true), track(2, false)], + }, + ], + selectedVersionKey: 'v1', + loadingReleases: false, + loadingInfo: false, + }); + el.requestUpdate(); + await flush(); + await el.updateComplete; + + return el; +} + +const badges = (el: LitElement) => + shadowAll(el, 'library-status-indicator.track-request'); + +describe('the tracklist’s request badge', () => { + beforeEach(() => { + resetHarness(); + stub('library.Library.GetFilePathsByRecordingMBIDs', {}); + stub('library.Library.GetFilePathsByAlbums', {}); + stub('library.Library.GetAlbumTracks', []); + stub('library.Library.GetAllLibrariesWithTrackCounts', []); + stub('download.Service.ProviderKinds', []); + stub('download.Service.ListProviders', []); + stub('download.Service.ListDownloads', []); + stub('download.Service.ListRequests', []); + }); + + it('is visible without a pointer anywhere near it', async () => { + const el = await albumWithAnUnownedTrack(); + const [badge] = badges(el); + + expect(badge).toBeTruthy(); + // Computed opacity rather than the absence of a rule, because the + // rule could come back under a different selector. + expect(getComputedStyle(badge!).opacity).toBe('1'); + }); + + it('is still only on the rows with something to request', async () => { + // Always-visible is not the same as everywhere: an owned track has + // nothing left to ask for, and a badge on it would be the column of + // green ticks this page deliberately stopped drawing. + expect(badges(await albumWithAnUnownedTrack())).toHaveLength(1); + }); +}); diff --git a/frontend/test/components/album-track-request.test.ts b/frontend/test/components/album-track-request.test.ts new file mode 100644 index 0000000..28af27f --- /dev/null +++ b/frontend/test/components/album-track-request.test.ts @@ -0,0 +1,149 @@ +/** + * Issue #33: "Want track" filed the request and left the badge alone. + * + * The tracklist's badges read `libraryStatusFor(false, track.mbid)` at + * render time, which is a dependency on `downloadStore` that Lit cannot + * see. `explore-album-details` did subscribe to that store, but its + * callback only assigned `canDownload` and `isRequested` — neither of + * which a *track* request changes — so nothing in the component's + * reactive state moved and the page never re-rendered. The request was + * real, the plus stayed a plus, and clicking again cancelled it. + * + * The other three hosts rendering these badges (`explore-artist- + * details`, `explore-view`, `top-results-row`) have always asked for + * the repaint in the same place, which is what made this one look + * correct on inspection. + */ +import { describe, expect, it, beforeEach } from 'vitest'; +import type { LitElement } from 'lit'; + +import '@components/explore-album-details/explore-album-details'; +import type { Request } from '@store/download-store'; +import { Events } from '../../src/events'; +import { stub, flush, resetHarness, emit } from '@test/support/harness'; +import { fixture, shadowAll } from '@test/support/render'; + +type RequestOverrides = Partial> & { + state?: `${Request['state']}`; + entity?: `${Request['entity']}`; +}; + +function request(overrides: RequestOverrides): Request { + return { + id: 1, + mbid: 'mbid-1', + entity: 'recording', + libraryId: 1, + artist: 'An Artist', + title: 'Track 1', + scope: 'future', + secondary: false, + state: 'wanted', + attempts: 0, + ...overrides, + } as Request; +} + +/** Put a request list into the store the way the backend does. */ +async function withRequests(rows: Request[]): Promise { + stub('download.Service.ListRequests', rows); + emit(Events.RequestsChanged); + await flush(); +} + +function track(n: number) { + return { + position: n, + discNumber: 1, + title: `Track ${n}`, + length: 200000, + mbid: `mbid-${n}`, + inLibrary: false, + }; +} + +/** An unowned catalog tracklist, which is the only case with badges: + * an owned row renders none, there being nothing left to ask for. */ +async function withTracklist(count: number): Promise { + const el = await fixture('explore-album-details', { + albumName: 'Glass Harbour', + }); + + Object.assign(el, { + versionEntries: [ + { + key: 'v1', + label: '2019', + sublabel: `${count} tracks`, + tracks: Array.from({ length: count }, (_, i) => track(i + 1)), + }, + ], + selectedVersionKey: 'v1', + loadingReleases: false, + loadingInfo: false, + }); + el.requestUpdate(); + await flush(); + await el.updateComplete; + + return el; +} + +/** The status of each track badge, in tracklist order. */ +function badgeStatuses(el: LitElement): string[] { + return shadowAll(el, 'library-status-indicator.track-request').map( + (b) => b.getAttribute('status') ?? '', + ); +} + +describe('the album tracklist’s request badges', () => { + beforeEach(async () => { + resetHarness(); + stub('library.Library.GetFilePathsByRecordingMBIDs', {}); + stub('library.Library.GetFilePathsByAlbums', {}); + stub('library.Library.GetAlbumTracks', []); + stub('library.Library.GetAllLibrariesWithTrackCounts', []); + await withRequests([]); + }); + + it('starts as a plus on every unowned row', async () => { + const el = await withTracklist(3); + + expect(badgeStatuses(el)).toEqual([ + 'not-in-library', + 'not-in-library', + 'not-in-library', + ]); + }); + + it('repaints the row whose track has been requested', async () => { + const el = await withTracklist(3); + + await withRequests([request({ mbid: 'mbid-2' })]); + await el.updateComplete; + + // Only the requested row moves. A request is by MBID, so the two + // rows either side of it are still a plus. + expect(badgeStatuses(el)).toEqual([ + 'not-in-library', + 'queued', + 'not-in-library', + ]); + }); + + it('repaints again when the request is cancelled', async () => { + const el = await withTracklist(3); + + await withRequests([request({ mbid: 'mbid-2' })]); + await el.updateComplete; + + await withRequests([]); + await el.updateComplete; + + expect(badgeStatuses(el)).toEqual([ + 'not-in-library', + 'not-in-library', + 'not-in-library', + ]); + }); +}); diff --git a/frontend/test/components/album-tracklist-heading.test.ts b/frontend/test/components/album-tracklist-heading.test.ts new file mode 100644 index 0000000..ce1e633 --- /dev/null +++ b/frontend/test/components/album-tracklist-heading.test.ts @@ -0,0 +1,89 @@ +/** + * The tracklist's own heading. + * + * A list of numbered titles with durations, under the album's cover, is + * the one thing on this page that did not need a word above it saying + * what it was — "TRACKLIST" labelled the only thing already obvious. + * + * What goes is the *ink*, not the element. The section is a landmark + * and the page's heading structure runs through it, so a reader moving + * by heading still has to be able to find it, and it is hidden the way + * `sr-only` hides things: `clip-path`, never `display: none`, which + * would take it out of the accessibility tree along with the layout. + */ +import { describe, expect, it, beforeEach } from 'vitest'; +import type { LitElement } from 'lit'; + +import '@components/explore-album-details/explore-album-details'; +import { stub, flush, resetHarness } from '@test/support/harness'; +import { fixture, shadowAll } from '@test/support/render'; + +function track(n: number) { + return { + position: n, + discNumber: 1, + title: `Track ${n}`, + length: 200000, + mbid: `mbid-${n}`, + inLibrary: true, + }; +} + +async function albumPage(): Promise { + const el = await fixture('explore-album-details', { + albumName: 'Glass Harbour', + releaseGroupMBID: 'rg-1', + }); + + Object.assign(el, { + versionEntries: [ + { + key: 'v1', + label: '2019', + sublabel: '2 tracks', + tracks: [track(1), track(2)], + }, + ], + selectedVersionKey: 'v1', + loadingReleases: false, + loadingInfo: false, + }); + el.requestUpdate(); + await flush(); + await el.updateComplete; + + return el; +} + +const tracklistHeading = (el: LitElement) => + shadowAll(el, 'h3').find((h) => h.textContent?.trim() === 'Tracklist'); + +describe('the album tracklist heading', () => { + beforeEach(() => { + resetHarness(); + stub('library.Library.GetFilePathsByRecordingMBIDs', {}); + stub('library.Library.GetFilePathsByAlbums', {}); + stub('library.Library.GetAlbumTracks', []); + stub('library.Library.GetAllLibrariesWithTrackCounts', []); + stub('download.Service.ProviderKinds', []); + stub('download.Service.ListProviders', []); + stub('download.Service.ListDownloads', []); + stub('download.Service.ListRequests', []); + }); + + it('is still in the tree', async () => { + expect(tracklistHeading(await albumPage())).toBeTruthy(); + }); + + it('takes up no room on the page', async () => { + const heading = tracklistHeading(await albumPage())!; + const box = heading.getBoundingClientRect(); + + expect(box.width).toBeLessThanOrEqual(1); + expect(box.height).toBeLessThanOrEqual(1); + // Hidden by clipping, not by removal: display:none and + // visibility:hidden both take it out of the accessibility tree. + expect(getComputedStyle(heading).display).not.toBe('none'); + expect(getComputedStyle(heading).visibility).not.toBe('hidden'); + }); +}); diff --git a/frontend/test/components/downloads-no-client.test.ts b/frontend/test/components/downloads-no-client.test.ts new file mode 100644 index 0000000..5f88275 --- /dev/null +++ b/frontend/test/components/downloads-no-client.test.ts @@ -0,0 +1,90 @@ +/** + * What a wanted list says when there is nothing to search with. + * + * Wanting something without a download client is a supported thing to + * do — the list is kept, and it starts moving when a client is added. + * What was not supported was the app *claiming to be looking*: every + * pass attempted each request, failed it with "no download clients are + * enabled", recorded that as an attempt and scheduled a retry, so a row + * read "Searched 3 times, no download clients are enabled · next check + * in 6 hours" about a check that could not happen. + * + * The backend half is `TestNoProvidersMeansNoAttempt`. This is the row. + */ +import { describe, expect, it, beforeEach } from 'vitest'; +import type { LitElement } from 'lit'; + +import '@components/downloads-view/downloads-view'; +import { stub, emit, flush, resetHarness } from '@test/support/harness'; +import { Events } from '../../src/events'; +import { fixture, shadowAll } from '@test/support/render'; + +/** A request that has been tried and is waiting on a retry — the shape + * a list with a client in it produces. */ +const WAITING = { + id: 1, + mbid: 'rg-1', + entity: 'release-group', + libraryId: 1, + artist: 'Aurora Fields', + title: 'Glass Harbour', + state: 'wanted', + attempts: 3, + lastError: 'no source has it yet', + nextTryAt: new Date(Date.now() + 6 * 3600_000).toISOString(), +}; + +const PROVIDER = { + id: 1, + kind: 'slskd', + name: 'Sound', + enabled: true, + priority: 50, +}; + +/** + * The download store is a singleton whose `init()` runs once per + * session, so a second mount does not re-read the provider list. The + * event is how the app itself learns a client was added, and is what + * makes this test independent of which case ran first. + */ +async function view(providers: unknown[]): Promise { + stub('download.Service.ListProviders', providers); + + const el = await fixture('downloads-view'); + + emit(Events.DownloadProvidersChanged); + await flush(); + await el.updateComplete; + + return el; +} + +const details = (el: LitElement) => + shadowAll(el, '.detail').map((d) => d.textContent!.trim()); + +describe('a request row with no download client', () => { + beforeEach(() => { + resetHarness(); + stub('download.Service.ProviderKinds', []); + stub('download.Service.ListProviders', []); + stub('download.Service.ListDownloads', []); + stub('download.Service.ListRequests', [WAITING]); + }); + + it('does not promise a check that cannot happen', async () => { + const el = await view([]); + + expect(details(el)).toHaveLength(1); + expect(details(el)[0]).toBe( + 'On your list — no download client to search with', + ); + expect(details(el)[0]).not.toMatch(/next check/); + }); + + it('reports the retry schedule again once a client exists', async () => { + const el = await view([PROVIDER]); + + expect(details(el)[0]).toMatch(/next check/); + }); +}); diff --git a/frontend/test/components/drag-image.test.ts b/frontend/test/components/drag-image.test.ts new file mode 100644 index 0000000..c436e5c --- /dev/null +++ b/frontend/test/components/drag-image.test.ts @@ -0,0 +1,71 @@ +/** + * A drag says how much it is carrying. + * + * Every drag in the app already did — `createDragImage` is a count and + * nothing else — except the one with a picture to show instead. An + * album dragged to the queue put its cover under the cursor and said + * nothing about how many tracks that was, and an album is 1 track or 30 + * with the same thumbnail either way. The number is the thing the drop + * is about. + * + * A count of 1 draws no badge: "1" over a single cover is noise, and + * the absence reads unambiguously beside a badge that only ever appears + * when there is more than one. + */ +import { describe, expect, it, afterEach } from 'vitest'; + +import { + createAlbumArtDragImage, + removeDragImage, +} from '@utils/drag-image'; + +const made: HTMLElement[] = []; + +function dragImage(count?: number): HTMLElement { + const el = + count === undefined + ? createAlbumArtDragImage('data:image/gif;base64,R0lGODlhAQABAAAAACw=') + : createAlbumArtDragImage( + 'data:image/gif;base64,R0lGODlhAQABAAAAACw=', + count, + ); + + made.push(el); + + return el; +} + +const badge = (el: HTMLElement) => + el.querySelector('.drag-count-badge'); + +describe('the album drag image', () => { + afterEach(() => { + while (made.length > 0) removeDragImage(made.pop()!); + }); + + it('says how many tracks are being dragged', () => { + expect(badge(dragImage(12))?.textContent).toBe('12'); + }); + + it('says nothing when there is only one track', () => { + expect(badge(dragImage(1))).toBeNull(); + }); + + it('still draws a bare cover for a caller that gives no count', () => { + // The count is optional so the helper stays usable from a call site + // that has a cover and no list; it must not badge such a drag "1". + expect(badge(dragImage())).toBeNull(); + }); + + it('keeps the badge inside the cover', () => { + // setDragImage snapshots the element, and anything outside its box + // risks being clipped out of that snapshot — while padding the box + // instead would move the cover away from the cursor. + const el = dragImage(30); + const outer = el.getBoundingClientRect(); + const mark = badge(el)!.getBoundingClientRect(); + + expect(mark.right).toBeLessThanOrEqual(outer.right); + expect(mark.top).toBeGreaterThanOrEqual(outer.top); + }); +}); diff --git a/frontend/test/components/transport.test.ts b/frontend/test/components/transport.test.ts index 3312fb5..f3e97c5 100644 --- a/frontend/test/components/transport.test.ts +++ b/frontend/test/components/transport.test.ts @@ -227,6 +227,41 @@ describe('', () => { expect(text(el, '[data-testid="remaining-time"]')).toBe('01:30'); }); + it('keeps the slider still as the clocks count', async () => { + const el = await fixture('seek-bar'); + + emit(Events.TrackChanged, { ...TRACK, trackChangeId: 31 }); + await flush(); + await el.updateComplete; + + const slider = () => + shadow(el, 'wa-slider')!.getBoundingClientRect(); + const before = slider(); + + // 1:11 against 4:08 is the reported jitter: different digits, and + // in a proportional font different widths. Toggling the right-hand + // clock is the other half -- the minus sign is a whole character. + for (const positionSeconds of [8, 71, 88]) { + emit(Events.PlaybackPositionChanged, { + positionSeconds, + trackLength: 90, + trackChangeId: 31, + seq: positionSeconds, + playing: true, + }); + await flush(); + await el.updateComplete; + + expect(slider().width).toBeCloseTo(before.width, 1); + expect(slider().left).toBeCloseTo(before.left, 1); + } + + await click(el, '[data-testid="remaining-time"]'); + await el.updateComplete; + + expect(slider().width).toBeCloseTo(before.width, 1); + }); + it('renders the position the backend reports rather than its own count', async () => { const el = await fixture('seek-bar'); diff --git a/frontend/test/stores/queue-store.test.ts b/frontend/test/stores/queue-store.test.ts index 4af2b87..77a24ec 100644 --- a/frontend/test/stores/queue-store.test.ts +++ b/frontend/test/stores/queue-store.test.ts @@ -198,6 +198,61 @@ describe('queue store: move', () => { }); }); +/** + * "Playing from X" is a claim that everything queued came from X, and + * appending a track from anywhere else makes it false. The backend + * clears the source on every add/insert path — but the delta is the + * only event those paths emit, so the label corrects itself here or + * not at all. + */ +describe('queue store: the source travels on the delta', () => { + function syncWithAlbum(): void { + emit(Events.QueueChanged, { + tracks: [track(1), track(2)], + currentIndex: 0, + shuffleMode: false, + repeatMode: 'off', + source: { type: 'album', id: 7, label: 'Abbey Road' }, + }); + } + + beforeEach(() => { + syncWithAlbum(); + }); + + it('drops the label when an append clears it backend-side', () => { + emit(Events.QueueTracksModified, { + action: 'add', + tracks: [track(3)], + index: 2, + currentIndex: 0, + source: { type: '', id: 0, label: '' }, + }); + + expect(queueStore.getState().source).toEqual({ + type: '', + id: 0, + label: '', + }); + }); + + it('keeps a label the backend still reports, as on a removal', () => { + emit(Events.QueueTracksModified, { + action: 'remove', + positions: [1], + index: 0, + currentIndex: 0, + source: { type: 'album', id: 7, label: 'Abbey Road' }, + }); + + expect(queueStore.getState().source).toEqual({ + type: 'album', + id: 7, + label: 'Abbey Road', + }); + }); +}); + describe('queue store: mode deltas', () => { beforeEach(() => { sync([track(1)], 0);