From 6ad993e27df60c443279b0e5a158a5b25f5d9777 Mon Sep 17 00:00:00 2001 From: zarzet Date: Tue, 11 Aug 2026 15:46:33 +0700 Subject: [PATCH] fix(lyrics): respect extension provider selection #511 --- go_backend/lyrics.go | 109 +++++++++++------- go_backend/lyrics_config.go | 41 ++++++- go_backend/lyrics_supplement_test.go | 33 +++++- .../lyrics_provider_priority_page.dart | 39 +++++-- .../settings/lyrics_settings_page.dart | 18 ++- 5 files changed, 182 insertions(+), 58 deletions(-) diff --git a/go_backend/lyrics.go b/go_backend/lyrics.go index 5241f756..d669ca59 100644 --- a/go_backend/lyrics.go +++ b/go_backend/lyrics.go @@ -192,6 +192,7 @@ func (c *LyricsClient) durationMatches(lrcDuration, targetDuration float64) bool func (c *LyricsClient) FetchLyricsAllSources(spotifyID, trackName, artistName string, durationSec float64) (*LyricsResponse, error) { primaryArtist := normalizeArtistName(artistName) fetchOptions := GetLyricsFetchOptions() + configuredProviderOrder := GetLyricsProviderOrder() if isLikelyInstrumentalTrack(trackName) { GoLog("[Lyrics] Track marked instrumental by title heuristic, skipping lyrics search: %s - %s\n", artistName, trackName) @@ -203,62 +204,57 @@ func (c *LyricsClient) FetchLyricsAllSources(spotifyID, trackName, artistName st return instrumental, nil } + extensionProviders := make(map[string]*extensionProviderWrapper) extManager := getExtensionManager() - var extensionProviders []*extensionProviderWrapper if extManager != nil { - extensionProviders = extManager.GetLyricsProviders() + for _, provider := range extManager.GetLyricsProviders() { + providerName := "extension:" + strings.ToLower(strings.TrimSpace(provider.extension.ID)) + extensionProviders[providerName] = provider + } + } + + providerOrder := resolveLyricsProviderOrder(configuredProviderOrder, extensionProviders) + selectedExtensionCount := 0 + for _, providerName := range providerOrder { + if strings.HasPrefix(providerName, "extension:") { + selectedExtensionCount++ + } } var cachedNonExtension *LyricsResponse if cached, found := globalLyricsCache.Get(artistName, trackName, durationSec); found { isExtensionCache := strings.HasPrefix(cached.Source, "Extension:") - if len(extensionProviders) == 0 || isExtensionCache { + cachedProviderSelected := false + if isExtensionCache { + cachedProviderName := "extension:" + strings.ToLower(strings.TrimSpace(strings.TrimPrefix(cached.Source, "Extension:"))) + for _, providerName := range providerOrder { + if providerName == cachedProviderName { + cachedProviderSelected = true + break + } + } + } + if (!isExtensionCache && selectedExtensionCount == 0) || cachedProviderSelected { fmt.Printf("[Lyrics] Cache hit for: %s - %s\n", artistName, trackName) cachedCopy := *cached cachedCopy.Source = cached.Source + " (cached)" return &cachedCopy, nil } - // If extension providers are currently enabled, don't let stale built-in cache - // mask newly installed/activated extensions. - cachedNonExtension = cached - GoLog("[Lyrics] Ignoring cached non-extension lyrics because extension providers are available\n") + if !isExtensionCache { + // If extension providers are currently selected, don't let stale built-in + // cache mask them. It remains available as a fallback if they fail. + cachedNonExtension = cached + GoLog("[Lyrics] Ignoring cached non-extension lyrics because selected extension providers are available\n") + } else { + GoLog("[Lyrics] Ignoring cached lyrics from an unselected extension provider\n") + } } isValidResult := func(l *LyricsResponse) bool { return lyricsHasUsableText(l) } - if len(extensionProviders) > 0 { - for _, provider := range extensionProviders { - providerName := "extension:" + provider.extension.ID - if skip, remaining, reason := shouldSkipLyricsProvider(providerName); skip { - GoLog("[Lyrics] Skipping unavailable extension lyrics provider %s for %s: %s\n", provider.extension.ID, remaining.Round(time.Second), reason) - continue - } - GoLog("[Lyrics] Trying extension lyrics provider: %s\n", provider.extension.ID) - lyrics, err := provider.FetchLyrics(trackName, artistName, "", durationSec) - if err == nil && isValidResult(lyrics) { - GoLog("[Lyrics] Got lyrics from extension: %s\n", provider.extension.ID) - markLyricsProviderAvailable(providerName) - globalLyricsCache.Set(artistName, trackName, durationSec, lyrics) - return lyrics, nil - } - if err != nil { - GoLog("[Lyrics] Extension %s failed: %v\n", provider.extension.ID, err) - markLyricsProviderUnavailable(providerName, err) - } - } - } - - if cachedNonExtension != nil { - cachedCopy := *cachedNonExtension - cachedCopy.Source = cachedNonExtension.Source + " (cached fallback)" - GoLog("[Lyrics] Extension providers unavailable for this track, using cached built-in lyrics\n") - return &cachedCopy, nil - } - - providerOrder := GetLyricsProviderOrder() simplifiedTrack := simplifyTrackName(trackName) request := lyricsProviderSearchRequest{ spotifyID: spotifyID, @@ -272,16 +268,48 @@ func (c *LyricsClient) FetchLyricsAllSources(spotifyID, trackName, artistName st GoLog("[Lyrics] Searching for: %s - %s (providers: %v)\n", artistName, trackName, providerOrder) - lyrics, err := fetchBuiltInLyricsProviders(providerOrder, request, c.fetchBuiltInLyricsProvider) + fetchProvider := func(providerName string, request lyricsProviderSearchRequest) (*LyricsResponse, error, bool) { + if provider, ok := extensionProviders[providerName]; ok { + lyrics, err := provider.FetchLyrics(request.trackName, request.artistName, "", request.durationSec) + return lyrics, err, true + } + return c.fetchBuiltInLyricsProvider(providerName, request) + } + + lyrics, err := fetchLyricsProviders(providerOrder, request, fetchProvider) if err == nil && isValidResult(lyrics) { globalLyricsCache.Set(artistName, trackName, durationSec, lyrics) return lyrics, nil } + if cachedNonExtension != nil { + cachedCopy := *cachedNonExtension + cachedCopy.Source = cachedNonExtension.Source + " (cached fallback)" + GoLog("[Lyrics] Selected extension providers unavailable for this track, using cached built-in lyrics\n") + return &cachedCopy, nil + } + return nil, fmt.Errorf("lyrics not found from any source") } -func fetchBuiltInLyricsProviders( +func resolveLyricsProviderOrder( + configuredOrder []string, + extensionProviders map[string]*extensionProviderWrapper, +) []string { + providerOrder := make([]string, 0, len(configuredOrder)) + for _, providerName := range configuredOrder { + if isKnownBuiltInLyricsProvider(providerName) { + providerOrder = append(providerOrder, providerName) + continue + } + if _, available := extensionProviders[providerName]; available { + providerOrder = append(providerOrder, providerName) + } + } + return providerOrder +} + +func fetchLyricsProviders( providerOrder []string, request lyricsProviderSearchRequest, fetchProvider func(string, lyricsProviderSearchRequest) (*LyricsResponse, error, bool), @@ -302,7 +330,8 @@ func fetchBuiltInLyricsProviders( continue } - knownProvider := isKnownBuiltInLyricsProvider(providerName) + knownProvider := isKnownBuiltInLyricsProvider(providerName) || + (strings.HasPrefix(providerName, "extension:") && len(providerName) > len("extension:")) if !knownProvider { GoLog("[Lyrics] Unknown provider: %s, skipping\n", providerName) continue diff --git a/go_backend/lyrics_config.go b/go_backend/lyrics_config.go index ab39714f..88696089 100644 --- a/go_backend/lyrics_config.go +++ b/go_backend/lyrics_config.go @@ -118,11 +118,15 @@ var ( func SetLyricsProviderOrder(providers []string) { lyricsProvidersMu.Lock() - defer lyricsProvidersMu.Unlock() if len(providers) == 0 { + changed := len(lyricsProviders) != 0 lyricsProviders = nil + lyricsProvidersMu.Unlock() clearLyricsProviderHealth() + if changed { + globalLyricsCache.ClearAll() + } return } @@ -140,19 +144,44 @@ func SetLyricsProviderOrder(providers []string) { LyricsProviderLyricsPlus: true, } - var valid []string + valid := make([]string, 0, len(providers)) + seen := make(map[string]struct{}, len(providers)) for _, p := range providers { normalized := strings.ToLower(strings.TrimSpace(p)) - if validNames[normalized] { - valid = append(valid, normalized) + isExtension := strings.HasPrefix(normalized, "extension:") && + strings.TrimSpace(strings.TrimPrefix(normalized, "extension:")) != "" + if !validNames[normalized] && !isExtension { + continue } + if _, exists := seen[normalized]; exists { + continue + } + seen[normalized] = struct{}{} + valid = append(valid, normalized) } + changed := !equalLyricsProviderOrders(lyricsProviders, valid) lyricsProviders = valid + lyricsProvidersMu.Unlock() clearLyricsProviderHealth() + if changed { + globalLyricsCache.ClearAll() + } GoLog("[Lyrics] Provider order set to: %v\n", valid) } +func equalLyricsProviderOrders(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} + func clearLyricsProviderHealth() { lyricsProviderHealthMu.Lock() defer lyricsProviderHealthMu.Unlock() @@ -242,7 +271,9 @@ func GetLyricsProviderOrder() []string { defer lyricsProvidersMu.RUnlock() if len(lyricsProviders) == 0 { - return DefaultLyricsProviders + result := make([]string, len(DefaultLyricsProviders)) + copy(result, DefaultLyricsProviders) + return result } result := make([]string, len(lyricsProviders)) diff --git a/go_backend/lyrics_supplement_test.go b/go_backend/lyrics_supplement_test.go index 3598d502..27b0a400 100644 --- a/go_backend/lyrics_supplement_test.go +++ b/go_backend/lyrics_supplement_test.go @@ -16,8 +16,8 @@ func TestLyricsCacheParsingAndLRCLibClient(t *testing.T) { if ua := appUserAgent(); !strings.Contains(ua, "4.5.0") { t.Fatalf("user agent = %q", ua) } - SetLyricsProviderOrder([]string{"LRCLIB", "bad", "netease"}) - if providers := GetLyricsProviderOrder(); len(providers) != 2 || providers[0] != LyricsProviderLRCLIB { + SetLyricsProviderOrder([]string{"LRCLIB", "bad", "extension:Apple-Music", "netease", "extension:apple-music"}) + if providers := GetLyricsProviderOrder(); len(providers) != 3 || providers[0] != LyricsProviderLRCLIB || providers[1] != "extension:apple-music" { t.Fatalf("providers = %#v", providers) } SetLyricsProviderOrder(nil) @@ -228,7 +228,7 @@ func TestConcurrentLyricsProvidersReturnFastFallback(t *testing.T) { defer clearLyricsProviderHealth() start := time.Now() - lyrics, err := fetchBuiltInLyricsProviders( + lyrics, err := fetchLyricsProviders( []string{LyricsProviderLRCLIB, LyricsProviderAppleMusic}, lyricsProviderSearchRequest{}, func(providerName string, _ lyricsProviderSearchRequest) (*LyricsResponse, error, bool) { @@ -250,11 +250,36 @@ func TestConcurrentLyricsProvidersReturnFastFallback(t *testing.T) { } } +func TestResolveLyricsProviderOrderOnlyIncludesSelectedAvailableExtensions(t *testing.T) { + availableExtensions := map[string]*extensionProviderWrapper{ + "extension:apple-music": nil, + "extension:future-provider": nil, + } + providers := resolveLyricsProviderOrder( + []string{ + LyricsProviderLRCLIB, + "extension:future-provider", + "extension:not-installed", + LyricsProviderNetease, + }, + availableExtensions, + ) + + want := []string{ + LyricsProviderLRCLIB, + "extension:future-provider", + LyricsProviderNetease, + } + if !equalLyricsProviderOrders(providers, want) { + t.Fatalf("providers = %#v, want %#v", providers, want) + } +} + func TestConcurrentLyricsProvidersPreferEarlierProviderWithinGrace(t *testing.T) { clearLyricsProviderHealth() defer clearLyricsProviderHealth() - lyrics, err := fetchBuiltInLyricsProviders( + lyrics, err := fetchLyricsProviders( []string{LyricsProviderLRCLIB, LyricsProviderAppleMusic}, lyricsProviderSearchRequest{}, func(providerName string, _ lyricsProviderSearchRequest) (*LyricsResponse, error, bool) { diff --git a/lib/screens/settings/lyrics_provider_priority_page.dart b/lib/screens/settings/lyrics_provider_priority_page.dart index 0d06ade0..e13a1238 100644 --- a/lib/screens/settings/lyrics_provider_priority_page.dart +++ b/lib/screens/settings/lyrics_provider_priority_page.dart @@ -1,6 +1,7 @@ import 'package:flutter/material.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:spotiflac_android/l10n/l10n.dart'; +import 'package:spotiflac_android/providers/extension_provider.dart'; import 'package:spotiflac_android/providers/settings_provider.dart'; import 'package:spotiflac_android/utils/adaptive_layout.dart'; import 'package:spotiflac_android/widgets/discard_changes_dialog.dart'; @@ -19,7 +20,7 @@ class LyricsProviderPriorityPage extends ConsumerStatefulWidget { class _LyricsProviderPriorityPageState extends ConsumerState { - static const _allProviderIds = [ + static const _builtInProviderIds = [ 'lrclib', 'netease', 'musixmatch', @@ -37,9 +38,6 @@ class _LyricsProviderPriorityPageState late List _initialProviders; bool _hasChanges = false; - List get _disabledProviders => - _allProviderIds.where((id) => !_enabledProviders.contains(id)).toList(); - @override void initState() { super.initState(); @@ -61,7 +59,19 @@ class _LyricsProviderPriorityPageState @override Widget build(BuildContext context) { - final disabled = _disabledProviders; + final extensions = ref.watch(extensionProvider).extensions; + final extensionNames = { + for (final extension in extensions) + if (extension.enabled && extension.hasLyricsProvider) + 'extension:${extension.id.toLowerCase()}': extension.displayName, + }; + final allProviderIds = [ + ..._builtInProviderIds, + ...extensionNames.keys.where((id) => !_builtInProviderIds.contains(id)), + ]; + final disabled = allProviderIds + .where((id) => !_enabledProviders.contains(id)) + .toList(); return PrioritySettingsScaffold( hasChanges: _hasChanges, @@ -91,7 +101,11 @@ class _LyricsProviderPriorityPageState itemCount: _enabledProviders.length, itemBuilder: (context, index) { final id = _enabledProviders[index]; - final info = _getLyricsProviderInfo(id, context); + final info = _getLyricsProviderInfo( + id, + context, + extensionNames[id], + ); return ReorderablePriorityItem( key: ValueKey(id), index: index, @@ -136,7 +150,11 @@ class _LyricsProviderPriorityPageState sliver: SliverList( delegate: SliverChildBuilderDelegate((context, index) { final id = disabled[index]; - final info = _getLyricsProviderInfo(id, context); + final info = _getLyricsProviderInfo( + id, + context, + extensionNames[id], + ); return _DisabledProviderItem( key: ValueKey(id), providerId: id, @@ -183,6 +201,7 @@ class _LyricsProviderPriorityPageState static _LyricsProviderInfo _getLyricsProviderInfo( String id, BuildContext context, + String? extensionDisplayName, ) { switch (id) { case 'lrclib': @@ -253,7 +272,11 @@ class _LyricsProviderPriorityPageState ); default: return _LyricsProviderInfo( - name: id, + name: + extensionDisplayName ?? + (id.startsWith('extension:') + ? id.substring('extension:'.length) + : id), description: context.l10n.lyricsProviderExtensionDesc, icon: Icons.extension, ); diff --git a/lib/screens/settings/lyrics_settings_page.dart b/lib/screens/settings/lyrics_settings_page.dart index 9fbfc6cb..0fafb17d 100644 --- a/lib/screens/settings/lyrics_settings_page.dart +++ b/lib/screens/settings/lyrics_settings_page.dart @@ -1,6 +1,7 @@ import 'package:flutter/material.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:spotiflac_android/l10n/l10n.dart'; +import 'package:spotiflac_android/providers/extension_provider.dart'; import 'package:spotiflac_android/providers/settings_provider.dart'; import 'package:spotiflac_android/screens/settings/lyrics_provider_priority_page.dart'; import 'package:spotiflac_android/widgets/settings_group.dart'; @@ -12,6 +13,10 @@ class LyricsSettingsPage extends ConsumerWidget { @override Widget build(BuildContext context, WidgetRef ref) { final settings = ref.watch(settingsProvider); + final extensionProviderNames = { + for (final extension in ref.watch(extensionProvider).extensions) + 'extension:${extension.id.toLowerCase()}': extension.displayName, + }; return PopScope( canPop: true, @@ -59,6 +64,7 @@ class LyricsSettingsPage extends ConsumerWidget { subtitle: _getLyricsProvidersSubtitle( context, settings.lyricsProviders, + extensionProviderNames, ), onTap: () => Navigator.push( context, @@ -187,9 +193,19 @@ class LyricsSettingsPage extends ConsumerWidget { String _getLyricsProvidersSubtitle( BuildContext context, List providers, + Map extensionProviderNames, ) { if (providers.isEmpty) return context.l10n.downloadProvidersNoneEnabled; - return providers.map((p) => _providerDisplayNames[p] ?? p).join(' > '); + return providers + .map( + (provider) => + _providerDisplayNames[provider] ?? + extensionProviderNames[provider] ?? + (provider.startsWith('extension:') + ? provider.substring('extension:'.length) + : provider), + ) + .join(' > '); } void _showLyricsModePicker(