Skip to content

Commit

Permalink
review findings 2
Browse files Browse the repository at this point in the history 
- removed unneeded code, refactoring
- use `AsyncValue`s for checking if metadata is loading
- reset/dispose provider when settings are changed (e.g. offline mode is toggled)
  • Loading branch information
@Chaphasilor
Chaphasilor committed Apr 26, 2024
1 parent 89926f1 commit de3bd6a
Show file tree
Hide file tree
Showing 7 changed files with 50 additions and 80 deletions.
8 changes: 6 additions & 2 deletions lib/components/now_playing_bar.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -509,8 +509,12 @@ class NowPlayingBar extends ConsumerWidget {
final queueService = GetIt.instance<QueueService>();
var imageTheme =
ref.watch(playerScreenThemeProvider(Theme.of(context).brightness));
ref.listen(currentTrackMetadataProvider, (metadataOrNull, metadata) {
// print("metadata changed: $metadataOrNull");

// reset the metadata provider when any settings (e.g. offline mode) change
FinampSettingsHelper.finampSettingsListener.addListener(() {
// don't keep the metadata provider alive any longer, so that once the last listener detaches, the provider is disposed
// when a new listener attaches, a new provider is created, and that one is kept alive until the settings change again
metadataKeepAliveLink?.close();
});

return Hero(
Expand Down
34 changes: 17 additions & 17 deletions lib/screens/lyrics_screen.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ class _LyricsViewState extends ConsumerState<LyricsView> with WidgetsBindingObse

@override
Widget build(BuildContext context) {
final metadata = ref.watch(currentTrackMetadataProvider).value;
final metadata = ref.watch(currentTrackMetadataProvider);

final _audioHandler = GetIt.instance<MusicPlayerBackgroundTask>();

Expand Down Expand Up @@ -248,19 +248,19 @@ class _LyricsViewState extends ConsumerState<LyricsView> with WidgetsBindingObse
);
}

if (metadata == null || (metadata.hasLyrics && metadata.lyrics == null && metadata.state != MetadataState.loaded)) {
if (metadata.isLoading || metadata.isRefreshing) {
return getEmptyState(
message: "Loading lyrics...",
icon: TablerIcons.microphone_2,
);
} else if (!metadata.hasLyrics) {
} else if (metadata.value == null || metadata.value!.hasLyrics && metadata.value!.lyrics == null && !metadata.isLoading) {
return getEmptyState(
message: "No lyrics available.",
message: "Couldn't load lyrics!",
icon: TablerIcons.microphone_2_off,
);
} else if (metadata.hasLyrics && metadata.lyrics == null && metadata.state == MetadataState.loaded) {
} else if (!metadata.value!.hasLyrics) {
return getEmptyState(
message: "Couldn't load lyrics!",
message: "No lyrics available.",
icon: TablerIcons.microphone_2_off,
);
} else {
Expand All @@ -269,15 +269,15 @@ class _LyricsViewState extends ConsumerState<LyricsView> with WidgetsBindingObse
progressStateStreamSubscription = progressStateStream.listen((state) async {
currentPosition = state.position;

if (metadata.lyrics!.lyrics?.first.start == null || !_isInForeground) {
if (metadata.value!.lyrics!.lyrics?.first.start == null || !_isInForeground) {
return;
}

// find the closest line to the current position, clamping to the first and last lines
int closestLineIndex = -1;
for (int i = 0; i < metadata.lyrics!.lyrics!.length; i++) {
for (int i = 0; i < metadata.value!.lyrics!.lyrics!.length; i++) {
closestLineIndex = i;
final line = metadata.lyrics!.lyrics![i];
final line = metadata.value!.lyrics!.lyrics![i];
if ((line.start ?? 0) ~/ 10 > (currentPosition?.inMicroseconds ?? 0)) {
closestLineIndex = i - 1;
break;
Expand All @@ -289,10 +289,10 @@ class _LyricsViewState extends ConsumerState<LyricsView> with WidgetsBindingObse
setState(() {}); // Rebuild to update the current line
if (autoScrollController.hasClients) {
int clampedIndex = currentLineIndex ?? 0;
if (clampedIndex >= metadata.lyrics!.lyrics!.length) {
clampedIndex = metadata.lyrics!.lyrics!.length - 1;
if (clampedIndex >= metadata.value!.lyrics!.lyrics!.length) {
clampedIndex = metadata.value!.lyrics!.lyrics!.length - 1;
}
// print("currentPosition: ${currentPosition?.inMicroseconds}, currentLineIndex: $currentLineIndex, line: ${metadata.lyrics!.lyrics![clampedIndex].text}");
// print("currentPosition: ${currentPosition?.inMicroseconds}, currentLineIndex: $currentLineIndex, line: ${metadata.value!.lyrics!.lyrics![clampedIndex].text}");
if (clampedIndex < 0) {
await autoScrollController.scrollToIndex(
-1,
Expand All @@ -301,7 +301,7 @@ class _LyricsViewState extends ConsumerState<LyricsView> with WidgetsBindingObse
);
} else {
unawaited(autoScrollController.scrollToIndex(
clampedIndex.clamp(0, metadata.lyrics!.lyrics!.length - 1),
clampedIndex.clamp(0, metadata.value!.lyrics!.lyrics!.length - 1),
preferPosition: AutoScrollPosition.middle,
duration: const Duration(milliseconds: 500),
));
Expand All @@ -320,10 +320,10 @@ class _LyricsViewState extends ConsumerState<LyricsView> with WidgetsBindingObse
child: LyricsListMask(
child: ListView.builder(
controller: autoScrollController,
itemCount: metadata.lyrics!.lyrics?.length ?? 0,
itemCount: metadata.value!.lyrics!.lyrics?.length ?? 0,
itemBuilder: (context, index) {
final line = metadata.lyrics!.lyrics![index];
final nextLine = index < metadata.lyrics!.lyrics!.length - 1 ? metadata.lyrics!.lyrics![index + 1] : null;
final line = metadata.value!.lyrics!.lyrics![index];
final nextLine = index < metadata.value!.lyrics!.lyrics!.length - 1 ? metadata.value!.lyrics!.lyrics![index + 1] : null;

final isCurrentLine = (currentPosition?.inMicroseconds ?? 0) >= (line.start ?? 0) ~/ 10 &&
(nextLine == null || (currentPosition?.inMicroseconds ?? 0) < (nextLine.start ?? 0) ~/ 10 );
Expand Down Expand Up @@ -364,7 +364,7 @@ class _LyricsViewState extends ConsumerState<LyricsView> with WidgetsBindingObse
}
),
),
if (index == metadata.lyrics!.lyrics!.length - 1)
if (index == metadata.value!.lyrics!.lyrics!.length - 1)
SizedBox(height: constraints.maxHeight * 0.2),
],
);
Expand Down
6 changes: 3 additions & 3 deletions lib/screens/player_screen.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -282,10 +282,10 @@ class _PlayerScreenContent extends StatelessWidget {
return Consumer(
builder: (context, ref, child) {

final metadata = ref.watch(currentTrackMetadataProvider).value;
final metadata = ref.watch(currentTrackMetadataProvider);

final isLyricsLoading = metadata?.item.mediaStreams?.any((e) => e.type == "Lyric") == null;
final isLyricsAvailable = (metadata?.hasLyrics ?? false) && (metadata?.lyrics != null || metadata?.state == MetadataState.loading);
final isLyricsLoading = metadata.isLoading || metadata.isRefreshing;
final isLyricsAvailable = (metadata.value?.hasLyrics ?? false) && (metadata.value?.lyrics != null || metadata.isLoading) && !metadata.hasError;
IconData getLyricsIcon() {
if (!isLyricsLoading && !isLyricsAvailable) {
return TablerIcons.microphone_2_off;
Expand Down
11 changes: 5 additions & 6 deletions lib/services/current_track_metadata_provider.dart
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import 'dart:async';

import 'package:flutter/material.dart';
import 'package:finamp/services/finamp_settings_helper.dart';
import 'package:flutter_riverpod/flutter_riverpod.dart';
import 'package:get_it/get_it.dart';

Expand All @@ -9,6 +9,8 @@ import 'package:finamp/models/jellyfin_models.dart';
import 'package:finamp/services/queue_service.dart';
import 'metadata_provider.dart';

KeepAliveLink? metadataKeepAliveLink; // this can be used to reset the provider

/// Provider to handle pre-fetching metadata for upcoming tracks
final currentTrackMetadataProvider =
AutoDisposeFutureProvider<MetadataProvider?>((ref) async {
Expand All @@ -29,11 +31,8 @@ final currentTrackMetadataProvider =
item: currentTrack,
includeLyrics: true,
);
var out = ref.watch(metadataProvider(request)).valueOrNull;
if (out != null) {
ref.keepAlive();
}
return out;
metadataKeepAliveLink = ref.keepAlive();
return ref.watch(metadataProvider(request).future);
}
return null;
});
Expand Down
27 changes: 12 additions & 15 deletions lib/services/downloads_service.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -680,35 +680,32 @@ class DownloadsService {
await resyncAll();
forceFullSync = false;

// Step 4 - Make sure there are no unanchored nodes in metadata.
// Step 4 - Fetch all missing lyrics
_downloadsLogger.info("Starting downloads repair step 4");
downloadCounts[repairStepTrackingName] = 4;
final Map<int, LyricDto?> idsWithLyrics = HashMap();
var allItems = _isar.downloadItems.where().findAllSync();
var allItems = _isar.downloadItems
.where()
.stateNotEqualTo(DownloadItemState.notDownloaded)
.filter()
.typeEqualTo(DownloadItemType.song)
.findAllSync();
final JellyfinApiHelper _jellyfinApiData = GetIt.instance<JellyfinApiHelper>();
for (var item in allItems) {
if (item.baseItem?.mediaStreams?.any((stream) => stream.type == "Lyric") ?? false) {
// check if lyrics are already downloaded
if (_isar.downloadedLyrics.where().isarIdEqualTo(item.isarId).countSync() > 0) {
continue;
}
idsWithLyrics[item.isarId] = null;
}
}
//TODO move this into a separate function in the background service
final JellyfinApiHelper _jellyfinApiData = GetIt.instance<JellyfinApiHelper>();
for (var id in idsWithLyrics.keys) {
var canonItem = _isar.downloadItems.getSync(id);
if (canonItem?.baseItem != null) {
LyricDto? lyrics;
try {
lyrics = await _jellyfinApiData.getLyrics(itemId: canonItem!.baseItem!.id);
_downloadsLogger.finer("Fetched lyrics for ${canonItem!.baseItem!.name}");
idsWithLyrics[id] = lyrics;
lyrics = await _jellyfinApiData.getLyrics(itemId: item.id);
_downloadsLogger.finer("Fetched lyrics for ${item.name}");
idsWithLyrics[item.isarId] = lyrics;
} catch (e) {
_downloadsLogger.warning("Failed to fetch lyrics for ${canonItem!.baseItem!.name}.");
rethrow;
_downloadsLogger.warning("Failed to fetch lyrics for ${item.name}.");
}

}
}
_isar.writeTxnSync(() {
Expand Down
12 changes: 0 additions & 12 deletions lib/services/downloads_service_backend.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1485,18 +1485,6 @@ class DownloadsSyncService {
}
canonItem.path = path_helper.join(subDirectory, fileName);
canonItem.fileTranscodingProfile = canonItem.syncTranscodingProfile;
if (canonItem.baseItem!.mediaSources == null && mediaSources != null) {
// Deep copy BaseItemDto as they are not expected to be modified
var newBaseItem = BaseItemDto.fromJson(canonItem.baseItem!.toJson());
newBaseItem.mediaSources = mediaSources;
canonItem = canonItem.copyWith(item: newBaseItem)!;
}
if (canonItem.baseItem!.mediaStreams == null && mediaStreams != null) {
// Deep copy BaseItemDto as they are not expected to be modified
var newBaseItem = BaseItemDto.fromJson(canonItem.baseItem!.toJson());
newBaseItem.mediaStreams = mediaStreams;
canonItem = canonItem.copyWith(item: newBaseItem)!;
}
if (canonItem.state != DownloadItemState.notDownloaded) {
_syncLogger.severe(
"Song ${canonItem.name} changed state to ${canonItem.state} while initiating download.");
Expand Down
32 changes: 7 additions & 25 deletions lib/services/metadata_provider.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,8 @@ class MetadataRequest {
int get hashCode => Object.hash(item.id, includeLyrics);
}

enum MetadataState {
loading,
loaded,
}

class MetadataProvider {

MetadataState state = MetadataState.loading;
final BaseItemDto item;
LyricDto? lyrics;

Expand All @@ -64,19 +58,15 @@ final AutoDisposeFutureProviderFamily<MetadataProvider?, MetadataRequest>
//!!! only use offline metadata if the app is in offline mode
// Finamp should always use the server metadata when online, if possible
if (FinampSettingsHelper.finampSettings.isOffline) {
try {
final downloadItem = await downloadsService.getSongInfo(id: request.item.id);
if (downloadItem != null) {
metadataProviderLogger.fine("Got offline metadata for '${request.item.name}'");
itemInfo = downloadItem.baseItem!;
}
} catch (e) {
metadataProviderLogger.warning("Couldn't get the offline metadata for track '${request.item.name}'");
final downloadItem = await downloadsService.getSongInfo(id: request.item.id);
if (downloadItem != null) {
metadataProviderLogger.fine("Got offline metadata for '${request.item.name}'");
itemInfo = downloadItem.baseItem!;
}
} else {
final requiredAttributes = [request.item.mediaStreams];
// try to fetch from server if not found in downloads
if (itemInfo == null || requiredAttributes.any((e) => e == null)) {
// fetch from server in online mode if needed attributes are missing
if (requiredAttributes.any((e) => e == null)) {
metadataProviderLogger.fine("Fetching metadata for '${request.item.name}' (${request.item.id}) from server due to missing attributes");
try {
itemInfo = await jellyfinApiHelper.getItemById(request.item.id);
Expand Down Expand Up @@ -104,21 +94,14 @@ final AutoDisposeFutureProviderFamily<MetadataProvider?, MetadataRequest>
// Finamp should always use the server metadata when online, if possible
if (FinampSettingsHelper.finampSettings.isOffline) {
DownloadedLyrics? downloadedLyrics;
try {
downloadedLyrics = await downloadsService.getLyricsDownload(baseItem: request.item);
if (downloadedLyrics != null) {
metadata.lyrics = downloadedLyrics.lyricDto;
metadataProviderLogger.fine("Got offline lyrics for '${request.item.name}'");
}
else {
metadataProviderLogger.fine("No offline lyrics for '${request.item.name}'");
}
} catch (e) {
metadataProviderLogger.warning("Couldn't get the offline lyrics for track '${request.item.name}'");
}

if (downloadedLyrics != null) {
metadata.lyrics = downloadedLyrics.lyricDto;
}
} else {
metadataProviderLogger.fine("Fetching lyrics for '${request.item.name}' (${request.item.id})");
try {
Expand All @@ -133,7 +116,6 @@ final AutoDisposeFutureProviderFamily<MetadataProvider?, MetadataRequest>
}

metadataProviderLogger.fine("Fetched metadata for '${request.item.name}' (${request.item.id}): ${metadata.lyrics} ${metadata.hasLyrics}");
metadata.state = MetadataState.loaded;

return metadata;

Expand Down

0 comments on commit de3bd6a

Please sign in to comment.