diff --git a/cmd/soundtouch-cli/cmd_events.go b/cmd/soundtouch-cli/cmd_events.go index 86df8a1..baedb9b 100644 --- a/cmd/soundtouch-cli/cmd_events.go +++ b/cmd/soundtouch-cli/cmd_events.go @@ -413,9 +413,11 @@ func handlePresetEvent(event *models.PresetUpdatedEvent, verbose bool) { for _, preset := range presets.Preset { fmt.Printf(" 📻 Preset %d:", preset.ID) - if preset.ContentItem != nil { - fmt.Printf(" %s", preset.ContentItem.ItemName) - fmt.Printf(" (%s)", preset.ContentItem.Source) + // IsEmpty catches both and INVALID_SOURCE + // placeholders; using the nil-safe helpers below means the + // inner Printf never dereferences a nil ContentItem. + if !preset.IsEmpty() { + fmt.Printf(" %s (%s)", preset.GetDisplayName(), preset.GetSource()) } fmt.Println() diff --git a/cmd/soundtouch-cli/cmd_info.go b/cmd/soundtouch-cli/cmd_info.go index 18174d0..5d80d0a 100644 --- a/cmd/soundtouch-cli/cmd_info.go +++ b/cmd/soundtouch-cli/cmd_info.go @@ -177,23 +177,36 @@ func getPresets(c *cli.Context) error { fmt.Printf("Device Presets:\n") - if len(presets.Preset) == 0 { + // Filter out placeholder presets the firmware emits for unconfigured + // slots (issue #308): self-closing after factory reset, + // or on healthy devices. + // IsEmpty covers both shapes; accessing fields like ContentItem.Source + // directly on the first shape panics. + configured := make([]models.Preset, 0, len(presets.Preset)) + + for _, p := range presets.Preset { + if !p.IsEmpty() { + configured = append(configured, p) + } + } + + if len(configured) == 0 { fmt.Printf(" No presets configured\n") return nil } fmt.Printf(" Configured Presets:\n") - for _, preset := range presets.Preset { + for _, preset := range configured { fmt.Printf(" %d. %s\n", preset.ID, preset.GetDisplayName()) - fmt.Printf(" Source: %s\n", preset.ContentItem.Source) + fmt.Printf(" Source: %s\n", preset.GetSource()) - if preset.ContentItem.SourceAccount != "" && preset.ContentItem.SourceAccount != preset.ContentItem.Source { - fmt.Printf(" Account: %s\n", preset.ContentItem.SourceAccount) + if account := preset.GetSourceAccount(); account != "" && account != preset.GetSource() { + fmt.Printf(" Account: %s\n", account) } - if preset.ContentItem.Location != "" { - fmt.Printf(" Location: %s\n", preset.ContentItem.Location) + if location := preset.GetLocation(); location != "" { + fmt.Printf(" Location: %s\n", location) } // Show preset creation time if available diff --git a/cmd/websocket-demo/main.go b/cmd/websocket-demo/main.go index 4ddd32e..13a4eaa 100644 --- a/cmd/websocket-demo/main.go +++ b/cmd/websocket-demo/main.go @@ -355,9 +355,11 @@ func handlePreset(event *models.PresetUpdatedEvent, verbose bool) { for _, preset := range presets.Preset { fmt.Printf(" 📻 Preset %d:", preset.ID) - if preset.ContentItem != nil { - fmt.Printf(" %s", preset.ContentItem.ItemName) - fmt.Printf(" (%s)", preset.ContentItem.Source) + // IsEmpty catches both and INVALID_SOURCE + // placeholders; using the nil-safe helpers below means the + // inner Printf never dereferences a nil ContentItem. + if !preset.IsEmpty() { + fmt.Printf(" %s (%s)", preset.GetDisplayName(), preset.GetSource()) } fmt.Println() diff --git a/examples/preset-management/main.go b/examples/preset-management/main.go index d935e33..2e18f03 100644 --- a/examples/preset-management/main.go +++ b/examples/preset-management/main.go @@ -96,22 +96,38 @@ func showCurrentPresets(c *client.Client) error { return err } - if len(presets.Preset) == 0 { + // Filter out placeholder presets (issue #308): self-closing + // entries from a factory-reset device and + // INVALID_SOURCE placeholders from healthy devices both panic if + // their fields are dereferenced directly. + configured := make([]models.Preset, 0, len(presets.Preset)) + + for _, p := range presets.Preset { + if !p.IsEmpty() { + configured = append(configured, p) + } + } + + if len(configured) == 0 { fmt.Println(" 📭 No presets configured") return nil } - fmt.Printf(" 📻 Found %d configured presets:\n", len(presets.Preset)) - for _, preset := range presets.Preset { + fmt.Printf(" 📻 Found %d configured presets:\n", len(configured)) + + for _, preset := range configured { fmt.Printf(" %d. %s\n", preset.ID, preset.GetDisplayName()) - fmt.Printf(" Source: %s\n", preset.ContentItem.Source) - if preset.ContentItem.Location != "" { - fmt.Printf(" Location: %s\n", preset.ContentItem.Location) + fmt.Printf(" Source: %s\n", preset.GetSource()) + + if location := preset.GetLocation(); location != "" { + fmt.Printf(" Location: %s\n", location) } + if preset.CreatedOn != nil && *preset.CreatedOn != 0 { createdTime := time.Unix(*preset.CreatedOn, 0) fmt.Printf(" Created: %s\n", createdTime.Format("2006-01-02 15:04:05")) } + fmt.Println() } diff --git a/pkg/models/presets.go b/pkg/models/presets.go index 0723f76..000af75 100644 --- a/pkg/models/presets.go +++ b/pkg/models/presets.go @@ -71,9 +71,25 @@ func (p *Preset) IsSpotifyPreset() bool { return p.ContentItem != nil && p.ContentItem.Source == "SPOTIFY" } -// IsEmpty returns true if the preset has no content +// IsEmpty returns true if the preset has no playable content. Two +// placeholder shapes are observed in the wild and both count as empty: +// +// - (or ) — no ContentItem child at all. +// Emitted by some firmware after a factory reset (issue #308). +// - +// — a placeholder ContentItem the firmware uses for unconfigured +// slots, observed on FW 27.0.6 even on devices that were never +// reset. +// +// Treating both as empty keeps GetEmptyPresetSlots, GetUsedPresetSlots +// and HasPresets honest, and lets callers safely skip placeholders +// before formatting a preset for display. func (p *Preset) IsEmpty() bool { - return p.ContentItem == nil + if p.ContentItem == nil { + return true + } + + return p.ContentItem.Source == "" || p.ContentItem.Source == "INVALID_SOURCE" } // GetSource returns the source of the preset content diff --git a/pkg/models/presets_test.go b/pkg/models/presets_test.go new file mode 100644 index 0000000..721150c --- /dev/null +++ b/pkg/models/presets_test.go @@ -0,0 +1,188 @@ +package models + +import ( + "encoding/xml" + "testing" +) + +// reporterXML is the /presets response captured from the speaker that +// crashed the CLI in issue #308 (ST10 post factory reset, FW 27.0.6). +// Two configured presets followed by three self-closing +// placeholders. The original crash happened on the first : +// GetDisplayName() handled the nil ContentItem, but the very next +// line dereferenced ContentItem.Source unconditionally. +const reporterXML = ` + + +MDR JUMP + + + + + +SUNSHINE LIVE + +http://cdn-profiles.tunein.com/s10637/images/logog.png?t=637791086340000000 + + + + + + +` + +// invalidSourceXML is the second placeholder shape observed in the +// wild (gesellix's ST10/ST20 on FW 27.0.6, never factory-reset). The +// firmware here populates ContentItem with source="INVALID_SOURCE" +// for unconfigured slots — non-nil but useless, so the old IsEmpty +// (== nil only) returned false and the placeholders polluted listings. +const invalidSourceXML = `` + + `` + + `` + + `` + + `Sand Castle Tapes` + + `Unpluggedhttps://example.com/art.jpg` + + `SMOOTH JAZZhttps://example.com/logo.png` + + `` + +func TestIsEmpty_NoContentItem(t *testing.T) { + // Shape A: — ContentItem == nil. This is the shape + // behind the issue #308 crash. + p := Preset{} + if !p.IsEmpty() { + t.Error("IsEmpty() should be true when ContentItem is nil") + } +} + +func TestIsEmpty_InvalidSourcePlaceholder(t *testing.T) { + // Shape B: ContentItem present but Source == "INVALID_SOURCE". + // Observed on devices that never had a factory reset. + p := Preset{ + ContentItem: &ContentItem{Source: "INVALID_SOURCE", IsPresetable: true}, + } + if !p.IsEmpty() { + t.Error("IsEmpty() should be true when ContentItem has INVALID_SOURCE") + } +} + +func TestIsEmpty_EmptySource(t *testing.T) { + // A ContentItem with no Source can't drive playback. Treat it + // as empty too — defensive, not tied to a single observed shape. + p := Preset{ContentItem: &ContentItem{}} + if !p.IsEmpty() { + t.Error("IsEmpty() should be true when ContentItem.Source is empty") + } +} + +func TestIsEmpty_RealPreset(t *testing.T) { + p := Preset{ + ContentItem: &ContentItem{ + Source: "TUNEIN", + ItemName: "MDR JUMP", + }, + } + if p.IsEmpty() { + t.Error("IsEmpty() should be false for a configured preset") + } +} + +func TestReporterXML_DoesNotPanicAndFiltersEmpty(t *testing.T) { + // Reproducer for issue #308: simulate the loop that crashed the + // CLI. The fix is two-fold: IsEmpty now recognises , + // and callers use the nil-safe Get* accessors. Walking every + // preset through the same paths the CLI uses must not panic on + // any entry. + var presets Presets + + if err := xml.Unmarshal([]byte(reporterXML), &presets); err != nil { + t.Fatalf("Failed to unmarshal reporter XML: %v", err) + } + + if got := len(presets.Preset); got != 5 { + t.Fatalf("Expected 5 preset entries (2 configured + 3 empty), got %d", got) + } + + emptyCount := 0 + configuredCount := 0 + + for _, p := range presets.Preset { + // The CLI now skips empty presets before dereferencing + // anything on ContentItem. The IsEmpty call must catch all + // three entries. + if p.IsEmpty() { + emptyCount++ + continue + } + + configuredCount++ + + // These calls would have panicked pre-fix on the empty + // entries; here they exercise the still-printed paths for + // the real ones. + _ = p.GetDisplayName() + _ = p.GetSource() + _ = p.GetSourceAccount() + _ = p.GetLocation() + } + + if emptyCount != 3 { + t.Errorf("Expected 3 empty presets, got %d", emptyCount) + } + + if configuredCount != 2 { + t.Errorf("Expected 2 configured presets, got %d", configuredCount) + } + + // HasPresets should reflect "there are real presets" — not + // confused by the placeholders. + if !presets.HasPresets() { + t.Error("HasPresets() should be true (2 real presets present)") + } + + if got := presets.GetUsedPresetSlots(); len(got) != 2 { + t.Errorf("GetUsedPresetSlots() = %v; want 2 entries", got) + } +} + +func TestInvalidSourceXML_PlaceholdersFilteredOut(t *testing.T) { + // Second-shape reproducer: three INVALID_SOURCE placeholders + // preceding three real presets. Before the IsEmpty extension, + // listings printed "0. Preset 0 / Source: INVALID_SOURCE" three + // times before the real entries — annoying, not crashing. + var presets Presets + + if err := xml.Unmarshal([]byte(invalidSourceXML), &presets); err != nil { + t.Fatalf("Failed to unmarshal invalid-source XML: %v", err) + } + + if got := len(presets.Preset); got != 6 { + t.Fatalf("Expected 6 preset entries, got %d", got) + } + + configured := 0 + + for _, p := range presets.Preset { + if !p.IsEmpty() { + configured++ + } + } + + if configured != 3 { + t.Errorf("Expected 3 configured presets (after filtering INVALID_SOURCE placeholders), got %d", + configured) + } + + // The three placeholders all carry id="0", so used-slot + // reporting should ignore them and show only the real ids. + used := presets.GetUsedPresetSlots() + if len(used) != 3 { + t.Fatalf("GetUsedPresetSlots() = %v; want 3 entries", used) + } + + wantIDs := map[int]bool{1: true, 2: true, 6: true} + for _, id := range used { + if !wantIDs[id] { + t.Errorf("Unexpected used slot id %d; want one of %v", id, []int{1, 2, 6}) + } + } +} diff --git a/pkg/service/setup/setup.go b/pkg/service/setup/setup.go index 8cd00be..7d799b2 100644 --- a/pkg/service/setup/setup.go +++ b/pkg/service/setup/setup.go @@ -2588,7 +2588,12 @@ func (m *Manager) syncPresets(deviceIP, accountID, deviceID string) { var servicePresets []models.ServicePreset for _, p := range ps.Preset { - if p.ContentItem == nil { + // IsEmpty catches both placeholder shapes a SoundTouch device + // can emit: self-closing (issue #308) and + // . Neither carries + // real playable data and persisting them would surface as + // junk entries in the admin web UI. + if p.IsEmpty() { continue }