mirror of
https://github.com/zarzet/SpotiFLAC-Mobile.git
synced 2026-10-12 10:47:31 +02:00
fix(playlists): thread the source provider id through to recent playlists
Fixes #368. Reopening a Spotify playlist from "recent access" showed no tracks, while the first view (right after pasting the URL) worked fine. Root cause traced across two layers: 1. Go: ExtURLHandleResult (the parsed shape of an extension's handleUrl() return value) never captured a top-level `id` for the handled resource. Track/album/artist results carry their own id inside their nested metadata, but a plain playlist result has no such object, so its id was silently dropped everywhere from the goja parser through to the JSON the Dart side receives. 2. Dart: TrackState had nowhere to put that id even if it existed (only playlistName), so recording a "recent" playlist access stored the playlist's *name* as if it were its id. Reopening it later fed that name into PlaylistScreen's provider-guessing logic (legacyProviderIdFromResourceId, which only recognizes legacy "provider:id" prefixes), which naturally failed and fell through to a hardcoded Deezer metadata fetch using a Spotify playlist's name as the resource id - guaranteed to return nothing. Fix, matching the "generic API, not per-provider checks" architecture in CONTRIBUTING.md: - go_backend/extension_provider_wrapper.go: add ExtURLHandleResult.ID. - go_backend/extension_goja_convert.go: parse it from the handler's return value. - go_backend/exports_extensions.go: surface it in the JSON response. - lib/providers/track_provider.dart: add TrackState.playlistId, populated from the response's `id` field for playlist results. - lib/screens/home_tab.dart: record the real playlist id (falling back to the name only if a provider never supplies one) and pass both the id and the already-known provider id forward to PlaylistScreen. - lib/screens/playlist_screen.dart: PlaylistScreen gains a metadataProviderId param that takes priority over guessing from the id's shape. - lib/screens/home_tab_recent.dart: pass the recent-access entry's stored providerId through when reopening a playlist. - lib/utils/provider_resource_ids.dart: extract the known-id-vs-guessed-id preference into a small, directly testable resolvePreferredMetadataProviderId helper. Note: this fixes the common case (an extension already reported its own id as the source provider for the URL it handled). If a provider never supplies an id for its handleUrl() playlist result, playlistId stays null and behavior is unchanged from before this fix - no regression, just not a complete fix for that narrower case, since that would require changes in extension-side JS code outside this repo. Tests: - go_backend/extension_goja_convert_url_handle_test.go: the new ID field round-trips from a handler's return value, and stays empty when the handler doesn't supply one. - test/provider_resource_ids_test.dart: resolvePreferredMetadataProviderId prefers a known provider id, falls back to the legacy-prefix guess, treats a blank known id as unknown, and returns null for the unprefixed-id-with-no-known-provider case from #368 itself. Verification: go build/vet/test and flutter analyze/test all green.
This commit is contained in:
1 parent
555fd77cc8
commit
bed95efb29
10 files changed
+173
-4
No files matched your search
@@ -805,6 +805,7 @@ func HandleURLWithExtensionJSON(url string) (string, error) {
|
|||||||
|
|
||||||
response := map[string]any{
|
response := map[string]any{
|
||||||
"type": result.Type,
|
"type": result.Type,
|
||||||
|
"id": result.ID,
|
||||||
"extension_id": extensionID,
|
"extension_id": extensionID,
|
||||||
"name": result.Name,
|
"name": result.Name,
|
||||||
"cover_url": result.CoverURL,
|
"cover_url": result.CoverURL,
|
||||||
|
|||||||
@@ -522,6 +522,7 @@ func parseExtensionURLHandleValue(vm *goja.Runtime, value goja.Value) (ExtURLHan
|
|||||||
obj := value.ToObject(vm)
|
obj := value.ToObject(vm)
|
||||||
handleResult := ExtURLHandleResult{
|
handleResult := ExtURLHandleResult{
|
||||||
Type: gojaObjectString(obj, "type"),
|
Type: gojaObjectString(obj, "type"),
|
||||||
|
ID: gojaObjectString(obj, "id"),
|
||||||
Name: gojaObjectString(obj, "name"),
|
Name: gojaObjectString(obj, "name"),
|
||||||
CoverURL: gojaObjectString(obj, "cover_url", "coverUrl"),
|
CoverURL: gojaObjectString(obj, "cover_url", "coverUrl"),
|
||||||
HeaderImage: gojaObjectString(obj, "header_image", "headerImage"),
|
HeaderImage: gojaObjectString(obj, "header_image", "headerImage"),
|
||||||
|
|||||||
@@ -0,0 +1,53 @@
|
|||||||
|
package gobackend
|
||||||
|
|
||||||
|
import (
|
||||||
|
"testing"
|
||||||
|
|
||||||
|
"github.com/dop251/goja"
|
||||||
|
)
|
||||||
|
|
||||||
|
func TestParseExtensionURLHandleValueCapturesID(t *testing.T) {
|
||||||
|
vm := goja.New()
|
||||||
|
value, err := vm.RunString(`({
|
||||||
|
type: "playlist",
|
||||||
|
id: "37i9dQZF1DXcBWIGoYBM5M",
|
||||||
|
name: "Discover Weekly",
|
||||||
|
tracks: []
|
||||||
|
})`)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("RunString: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
result, err := parseExtensionURLHandleValue(vm, value)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("parseExtensionURLHandleValue: %v", err)
|
||||||
|
}
|
||||||
|
if result.Type != "playlist" {
|
||||||
|
t.Fatalf("Type = %q", result.Type)
|
||||||
|
}
|
||||||
|
if result.ID != "37i9dQZF1DXcBWIGoYBM5M" {
|
||||||
|
t.Fatalf("ID = %q, want the playlist id from the handler result", result.ID)
|
||||||
|
}
|
||||||
|
if result.Name != "Discover Weekly" {
|
||||||
|
t.Fatalf("Name = %q", result.Name)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestParseExtensionURLHandleValueOmitsIDWhenAbsent(t *testing.T) {
|
||||||
|
vm := goja.New()
|
||||||
|
// Track/album/artist results carry their own ID inside their nested
|
||||||
|
// metadata, so a handler that omits the top-level "id" (as these do
|
||||||
|
// today) must not surface a stale or zero-value ID.
|
||||||
|
value, err := vm.RunString(`({ type: "track", name: "A Track" })`)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("RunString: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
result, err := parseExtensionURLHandleValue(vm, value)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("parseExtensionURLHandleValue: %v", err)
|
||||||
|
}
|
||||||
|
if result.ID != "" {
|
||||||
|
t.Fatalf("ID = %q, want empty when the handler result has none", result.ID)
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -724,7 +724,11 @@ func (p *extensionProviderWrapper) customSearch(query string, options map[string
|
|||||||
}
|
}
|
||||||
|
|
||||||
type ExtURLHandleResult struct {
|
type ExtURLHandleResult struct {
|
||||||
Type string `json:"type"`
|
Type string `json:"type"`
|
||||||
|
// ID identifies the handled resource itself (e.g. a playlist ID). Track,
|
||||||
|
// album, and artist results already carry their own ID inside their
|
||||||
|
// nested metadata; this covers result types with no such nested object.
|
||||||
|
ID string `json:"id,omitempty"`
|
||||||
Track *ExtTrackMetadata `json:"track,omitempty"`
|
Track *ExtTrackMetadata `json:"track,omitempty"`
|
||||||
Tracks []ExtTrackMetadata `json:"tracks,omitempty"`
|
Tracks []ExtTrackMetadata `json:"tracks,omitempty"`
|
||||||
Album *ExtAlbumMetadata `json:"album,omitempty"`
|
Album *ExtAlbumMetadata `json:"album,omitempty"`
|
||||||
|
|||||||
@@ -19,6 +19,7 @@ class TrackState {
|
|||||||
final String? albumId;
|
final String? albumId;
|
||||||
final String? albumName;
|
final String? albumName;
|
||||||
final String? playlistName;
|
final String? playlistName;
|
||||||
|
final String? playlistId;
|
||||||
final String? artistId;
|
final String? artistId;
|
||||||
final String? artistName;
|
final String? artistName;
|
||||||
final String? coverUrl;
|
final String? coverUrl;
|
||||||
@@ -40,6 +41,7 @@ class TrackState {
|
|||||||
this.albumId,
|
this.albumId,
|
||||||
this.albumName,
|
this.albumName,
|
||||||
this.playlistName,
|
this.playlistName,
|
||||||
|
this.playlistId,
|
||||||
this.artistId,
|
this.artistId,
|
||||||
this.artistName,
|
this.artistName,
|
||||||
this.coverUrl,
|
this.coverUrl,
|
||||||
@@ -64,6 +66,7 @@ class TrackState {
|
|||||||
String? albumId,
|
String? albumId,
|
||||||
String? albumName,
|
String? albumName,
|
||||||
String? playlistName,
|
String? playlistName,
|
||||||
|
String? playlistId,
|
||||||
String? artistId,
|
String? artistId,
|
||||||
String? artistName,
|
String? artistName,
|
||||||
String? coverUrl,
|
String? coverUrl,
|
||||||
@@ -87,6 +90,7 @@ class TrackState {
|
|||||||
albumId: albumId ?? this.albumId,
|
albumId: albumId ?? this.albumId,
|
||||||
albumName: albumName ?? this.albumName,
|
albumName: albumName ?? this.albumName,
|
||||||
playlistName: playlistName ?? this.playlistName,
|
playlistName: playlistName ?? this.playlistName,
|
||||||
|
playlistId: playlistId ?? this.playlistId,
|
||||||
artistId: artistId ?? this.artistId,
|
artistId: artistId ?? this.artistId,
|
||||||
artistName: artistName ?? this.artistName,
|
artistName: artistName ?? this.artistName,
|
||||||
coverUrl: coverUrl ?? this.coverUrl,
|
coverUrl: coverUrl ?? this.coverUrl,
|
||||||
@@ -243,6 +247,7 @@ class TrackNotifier extends Notifier<TrackState> {
|
|||||||
collectionName ??
|
collectionName ??
|
||||||
(result['album'] as Map<String, dynamic>?)?['name'] as String?,
|
(result['album'] as Map<String, dynamic>?)?['name'] as String?,
|
||||||
playlistName: type == 'playlist' ? collectionName : null,
|
playlistName: type == 'playlist' ? collectionName : null,
|
||||||
|
playlistId: type == 'playlist' ? result['id'] as String? : null,
|
||||||
coverUrl: normalizeCoverReference(result['cover_url']?.toString()),
|
coverUrl: normalizeCoverReference(result['cover_url']?.toString()),
|
||||||
headerVideoUrl: normalizeRemoteHttpUrl(
|
headerVideoUrl: normalizeRemoteHttpUrl(
|
||||||
result['header_video']?.toString(),
|
result['header_video']?.toString(),
|
||||||
|
|||||||
@@ -598,13 +598,14 @@ class _HomeTabState extends ConsumerState<HomeTab>
|
|||||||
}
|
}
|
||||||
|
|
||||||
if (trackState.playlistName != null && trackState.tracks.isNotEmpty) {
|
if (trackState.playlistName != null && trackState.tracks.isNotEmpty) {
|
||||||
|
final playlistProviderId = trackState.searchExtensionId ?? 'spotify';
|
||||||
ref
|
ref
|
||||||
.read(recentAccessProvider.notifier)
|
.read(recentAccessProvider.notifier)
|
||||||
.recordPlaylistAccess(
|
.recordPlaylistAccess(
|
||||||
id: trackState.playlistName!,
|
id: trackState.playlistId ?? trackState.playlistName!,
|
||||||
name: trackState.playlistName!,
|
name: trackState.playlistName!,
|
||||||
imageUrl: trackState.coverUrl,
|
imageUrl: trackState.coverUrl,
|
||||||
providerId: trackState.searchExtensionId ?? 'spotify',
|
providerId: playlistProviderId,
|
||||||
);
|
);
|
||||||
|
|
||||||
Navigator.push(
|
Navigator.push(
|
||||||
@@ -614,6 +615,8 @@ class _HomeTabState extends ConsumerState<HomeTab>
|
|||||||
playlistName: trackState.playlistName!,
|
playlistName: trackState.playlistName!,
|
||||||
coverUrl: trackState.coverUrl,
|
coverUrl: trackState.coverUrl,
|
||||||
tracks: trackState.tracks,
|
tracks: trackState.tracks,
|
||||||
|
playlistId: trackState.playlistId,
|
||||||
|
metadataProviderId: playlistProviderId,
|
||||||
recommendedService:
|
recommendedService:
|
||||||
trackState.searchExtensionId ?? trackState.searchSource,
|
trackState.searchExtensionId ?? trackState.searchSource,
|
||||||
),
|
),
|
||||||
|
|||||||
@@ -342,6 +342,7 @@ extension _HomeTabRecentUI on _HomeTabState {
|
|||||||
coverUrl: item.imageUrl,
|
coverUrl: item.imageUrl,
|
||||||
tracks: const [],
|
tracks: const [],
|
||||||
playlistId: item.id,
|
playlistId: item.id,
|
||||||
|
metadataProviderId: item.providerId,
|
||||||
),
|
),
|
||||||
),
|
),
|
||||||
);
|
);
|
||||||
|
|||||||
@@ -31,6 +31,7 @@ class PlaylistScreen extends ConsumerStatefulWidget {
|
|||||||
final String? headerVideoUrl;
|
final String? headerVideoUrl;
|
||||||
final List<Track> tracks;
|
final List<Track> tracks;
|
||||||
final String? playlistId;
|
final String? playlistId;
|
||||||
|
final String? metadataProviderId;
|
||||||
final String? recommendedService;
|
final String? recommendedService;
|
||||||
|
|
||||||
const PlaylistScreen({
|
const PlaylistScreen({
|
||||||
@@ -40,6 +41,7 @@ class PlaylistScreen extends ConsumerStatefulWidget {
|
|||||||
this.headerVideoUrl,
|
this.headerVideoUrl,
|
||||||
required this.tracks,
|
required this.tracks,
|
||||||
this.playlistId,
|
this.playlistId,
|
||||||
|
this.metadataProviderId,
|
||||||
this.recommendedService,
|
this.recommendedService,
|
||||||
});
|
});
|
||||||
|
|
||||||
@@ -126,7 +128,10 @@ class _PlaylistScreenState extends ConsumerState<PlaylistScreen>
|
|||||||
}
|
}
|
||||||
|
|
||||||
String? _metadataProviderId(String playlistId) {
|
String? _metadataProviderId(String playlistId) {
|
||||||
final providerId = legacyProviderIdFromResourceId(playlistId);
|
final providerId = resolvePreferredMetadataProviderId(
|
||||||
|
widget.metadataProviderId,
|
||||||
|
playlistId,
|
||||||
|
);
|
||||||
if (providerId == null) return null;
|
if (providerId == null) return null;
|
||||||
final effective = resolveEffectiveMetadataProvider(
|
final effective = resolveEffectiveMetadataProvider(
|
||||||
providerId,
|
providerId,
|
||||||
|
|||||||
@@ -18,3 +18,20 @@ String stripPrefixedResourceId(String value) {
|
|||||||
}
|
}
|
||||||
return value.substring(colonIndex + 1);
|
return value.substring(colonIndex + 1);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Resolves the metadata provider id to fetch a resource from, preferring a
|
||||||
|
/// caller-supplied [knownProviderId] (e.g. from a recent-access entry that
|
||||||
|
/// already recorded its source) over guessing from [resourceId]'s shape.
|
||||||
|
///
|
||||||
|
/// [legacyProviderIdFromResourceId] only recognizes legacy-prefixed ids
|
||||||
|
/// ("deezer:...", "spotify:...", etc.); anything else - including a plain,
|
||||||
|
/// unprefixed id from a provider that never used that scheme - resolves to
|
||||||
|
/// null unless the caller already knows the provider.
|
||||||
|
String? resolvePreferredMetadataProviderId(
|
||||||
|
String? knownProviderId,
|
||||||
|
String resourceId,
|
||||||
|
) {
|
||||||
|
final known = knownProviderId?.trim();
|
||||||
|
if (known != null && known.isNotEmpty) return known;
|
||||||
|
return legacyProviderIdFromResourceId(resourceId);
|
||||||
|
}
|
||||||
@@ -0,0 +1,79 @@
|
|||||||
|
import 'package:flutter_test/flutter_test.dart';
|
||||||
|
import 'package:spotiflac_android/constants/music_services.dart';
|
||||||
|
import 'package:spotiflac_android/utils/provider_resource_ids.dart';
|
||||||
|
|
||||||
|
void main() {
|
||||||
|
group('legacyProviderIdFromResourceId', () {
|
||||||
|
test('maps known legacy prefixes to their provider id', () {
|
||||||
|
expect(
|
||||||
|
legacyProviderIdFromResourceId('deezer:123'),
|
||||||
|
MusicServices.deezer,
|
||||||
|
);
|
||||||
|
expect(legacyProviderIdFromResourceId('qobuz:123'), MusicServices.qobuz);
|
||||||
|
expect(legacyProviderIdFromResourceId('tidal:123'), MusicServices.tidal);
|
||||||
|
expect(
|
||||||
|
legacyProviderIdFromResourceId('spotify:123'),
|
||||||
|
MusicServices.spotify,
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('returns null for an id with no known prefix', () {
|
||||||
|
expect(legacyProviderIdFromResourceId('37i9dQZF1DXcBWIGoYBM5M'), isNull);
|
||||||
|
expect(legacyProviderIdFromResourceId(''), isNull);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
group('stripPrefixedResourceId', () {
|
||||||
|
test('strips a leading provider prefix', () {
|
||||||
|
expect(stripPrefixedResourceId('deezer:123'), '123');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('leaves an id without a prefix unchanged', () {
|
||||||
|
expect(
|
||||||
|
stripPrefixedResourceId('37i9dQZF1DXcBWIGoYBM5M'),
|
||||||
|
'37i9dQZF1DXcBWIGoYBM5M',
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('leaves a bare colon or trailing colon unchanged', () {
|
||||||
|
expect(stripPrefixedResourceId(':123'), ':123');
|
||||||
|
expect(stripPrefixedResourceId('deezer:'), 'deezer:');
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
group('resolvePreferredMetadataProviderId', () {
|
||||||
|
test('prefers a known provider id over guessing from the resource id', () {
|
||||||
|
expect(
|
||||||
|
resolvePreferredMetadataProviderId(
|
||||||
|
'apple-music-ext',
|
||||||
|
'37i9dQZF1DXcBWIGoYBM5M',
|
||||||
|
),
|
||||||
|
'apple-music-ext',
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('falls back to the legacy prefix guess when nothing is known', () {
|
||||||
|
expect(
|
||||||
|
resolvePreferredMetadataProviderId(null, 'deezer:123'),
|
||||||
|
MusicServices.deezer,
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('treats a blank known provider id as unknown', () {
|
||||||
|
expect(
|
||||||
|
resolvePreferredMetadataProviderId(' ', 'deezer:123'),
|
||||||
|
MusicServices.deezer,
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
test(
|
||||||
|
'returns null for an unprefixed id with no known provider (the #368 case)',
|
||||||
|
() {
|
||||||
|
expect(
|
||||||
|
resolvePreferredMetadataProviderId(null, '37i9dQZF1DXcBWIGoYBM5M'),
|
||||||
|
isNull,
|
||||||
|
);
|
||||||
|
},
|
||||||
|
);
|
||||||
|
});
|
||||||
|
}
|
||||||
Reference in new issue
Block a user