fix(mediacontrols): always send an art URL, even when there is no art
Every other key in the MPRIS metadata map can be omitted safely, 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 whatever the last track had, so playing something without a cover left the previous album's sleeve on screen — which reads as the wrong track playing rather than as missing artwork. The map's construction moves out of UpdateMetadata into a pure metadataMap so it can be asserted on at all: everything else in this file needs a live session bus, which is the same reason the Android contract lives in an untagged file. Closes #41 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -279,17 +279,19 @@ func (h *MPRISHandler) enqueue(fn func()) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// UpdateMetadata pushes track metadata to D-Bus.
|
// metadataMap builds the org.mpris.MediaPlayer2.Player Metadata value
|
||||||
func (h *MPRISHandler) UpdateMetadata(meta Metadata) {
|
// for one track.
|
||||||
h.mu.Lock()
|
//
|
||||||
h.trackID++
|
// It is separated from UpdateMetadata, which needs a live D-Bus
|
||||||
tid := h.trackID
|
// connection, so the map's contents can be asserted on: this file is
|
||||||
h.mu.Unlock()
|
// 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
|
||||||
m := map[string]interface{}{
|
// contract lives in an untagged androidpayload.go.
|
||||||
|
func metadataMap(meta Metadata, trackID uint64) map[string]any {
|
||||||
|
m := map[string]any{
|
||||||
"mpris:trackid": dbus.ObjectPath(
|
"mpris:trackid": dbus.ObjectPath(
|
||||||
fmt.Sprintf(
|
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
|
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 != "" {
|
if meta.ArtFilePath != "" {
|
||||||
m["mpris:artUrl"] = "file://" + meta.ArtFilePath
|
artURL = "file://" + meta.ArtFilePath
|
||||||
}
|
}
|
||||||
|
|
||||||
|
m["mpris:artUrl"] = artURL
|
||||||
|
|
||||||
if meta.DurationSec > 0 {
|
if meta.DurationSec > 0 {
|
||||||
m["mpris:length"] = int64(
|
m["mpris:length"] = int64(
|
||||||
meta.DurationSec,
|
meta.DurationSec,
|
||||||
) * usPerSec
|
) * 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.enqueue(func() {
|
||||||
h.props.SetMust(playerIf, "Metadata", m)
|
h.props.SetMust(playerIf, "Metadata", m)
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -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)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user