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/frontend/src/components/explore-album-details/explore-album-details.ts b/frontend/src/components/explore-album-details/explore-album-details.ts index 45a02d5..de44a52 100644 --- a/frontend/src/components/explore-album-details/explore-album-details.ts +++ b/frontend/src/components/explore-album-details/explore-album-details.ts @@ -720,14 +720,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(); 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/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/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);