Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor DownloadStub hash calculation and rename DownloadItemType.song to track #1097

Draft
wants to merge 2 commits into
base: redesign
Choose a base branch
from
+67 −63
Draft

Refactor DownloadStub hash calculation and rename DownloadItemType.song to track #1097

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1a80af9
Separate DownloadStub hash calculation from DownloadItemType enum name
Maxr1998 Mar 8, 2025
475b4c7
Rename DownloadItemType.song to track
Maxr1998 Mar 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/components/AlbumScreen/track_list_tile.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,7 @@ class TrackListItemState extends ConsumerState<TrackListItem>
if (FinampSettingsHelper.finampSettings.isOffline) {
playable = ref.watch(GetIt.instance<DownloadsService>()
.stateProvider(DownloadStub.fromItem(
type: DownloadItemType.song, item: widget.baseItem))
type: DownloadItemType.track, item: widget.baseItem))
.select((value) => value.value?.isComplete ?? false));
} else {
playable = true;
Expand Down Expand Up @@ -717,7 +717,7 @@ class TrackListItemTile extends StatelessWidget {
offset: const Offset(-1.5, 2.5),
child: DownloadedIndicator(
item: DownloadStub.fromItem(
item: baseItem, type: DownloadItemType.song),
item: baseItem, type: DownloadItemType.track),
size: Theme.of(context)
.textTheme
.bodyMedium!
Expand Down
12 changes: 6 additions & 6 deletions lib/components/AlbumScreen/track_menu.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ class _TrackMenuState extends ConsumerState<TrackMenu> {
List<Widget> _menuEntries(BuildContext context) {
final downloadsService = GetIt.instance<DownloadsService>();
final downloadStatus = downloadsService.getStatus(
DownloadStub.fromItem(type: DownloadItemType.song, item: widget.item),
DownloadStub.fromItem(type: DownloadItemType.track, item: widget.item),
null);
var iconColor = Theme.of(context).colorScheme.primary;

Expand All @@ -242,7 +242,7 @@ class _TrackMenuState extends ConsumerState<TrackMenu> {
String? parentTooltip;
if (downloadStatus.isIncidental) {
var parent = downloadsService.getFirstRequiringItem(DownloadStub.fromItem(
type: DownloadItemType.song, item: widget.item));
type: DownloadItemType.track, item: widget.item));
if (parent != null) {
var parentName = AppLocalizations.of(context)!
.itemTypeSubtitle(parent.baseItemType.name, parent.name);
Expand Down Expand Up @@ -410,7 +410,7 @@ class _TrackMenuState extends ConsumerState<TrackMenu> {
enabled: downloadStatus.isRequired,
onTap: () async {
var item = DownloadStub.fromItem(
type: DownloadItemType.song, item: widget.item);
type: DownloadItemType.track, item: widget.item);
await askBeforeDeleteDownloadFromDevice(context, item);
})),
Visibility(
Expand All @@ -425,7 +425,7 @@ class _TrackMenuState extends ConsumerState<TrackMenu> {
downloadStatus == DownloadItemStatus.notNeeded,
onTap: () async {
var item = DownloadStub.fromItem(
type: DownloadItemType.song, item: widget.item);
type: DownloadItemType.track, item: widget.item);
await DownloadDialog.show(context, item, null);
if (context.mounted) {
Navigator.pop(context);
Expand All @@ -446,7 +446,7 @@ class _TrackMenuState extends ConsumerState<TrackMenu> {
enabled: !widget.isOffline && downloadStatus.isIncidental,
onTap: () async {
var item = DownloadStub.fromItem(
type: DownloadItemType.song, item: widget.item);
type: DownloadItemType.track, item: widget.item);
await DownloadDialog.show(context, item, null);
if (context.mounted) {
Navigator.pop(context);
Expand Down Expand Up @@ -598,7 +598,7 @@ class _TrackMenuState extends ConsumerState<TrackMenu> {
enabled: canDeleteFromServer,
onTap: () async {
var item = DownloadStub.fromItem(
type: DownloadItemType.song, item: widget.item);
type: DownloadItemType.track, item: widget.item);
await askBeforeDeleteFromServerAndDevice(context, item);
final BaseItemDto newAlbumOrPlaylist =
await _jellyfinApiHelper.getItemById(widget.parentItem!.id);
Expand Down
2 changes: 1 addition & 1 deletion lib/components/DownloadsScreen/item_file_size.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class ItemFileSize extends ConsumerWidget {
case DownloadItemState.failed:
case DownloadItemState.complete:
case DownloadItemState.needsRedownloadComplete:
if (item!.type == DownloadItemType.song) {
if (item!.type == DownloadItemType.track) {
String codec = "";
String bitrate = "null";
if (item.fileTranscodingProfile == null ||
Expand Down
40 changes: 22 additions & 18 deletions lib/models/finamp_models.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -987,8 +987,8 @@ class DownloadStub {
BaseItemDtoType.fromItem(baseItem!) == baseItemType &&
baseItemType.downloadType == DownloadItemType.collection &&
baseItemType != BaseItemDtoType.noItem;
case DownloadItemType.song:
return baseItemType.downloadType == DownloadItemType.song &&
case DownloadItemType.track:
return baseItemType.downloadType == DownloadItemType.track &&
baseItem != null &&
BaseItemDtoType.fromItem(baseItem!) == baseItemType;
case DownloadItemType.image:
Expand Down Expand Up @@ -1127,7 +1127,7 @@ class DownloadStub {

/// Calculate a DownloadStub's isarId
static int getHash(String id, DownloadItemType type) {
return _fastHash(type.name + id);
return _fastHash(type.isarType + id);
}

@override
Expand Down Expand Up @@ -1314,17 +1314,21 @@ class DownloadItem extends DownloadStub {
/// The primary type of a DownloadItem.
///
/// Enumerated by Isar, do not modify order or delete existing entries.
/// The enum name is used by `DownloadStub.getHash` to calculate the isarId for
/// the downloads system, DO NOT RENAME ANY ENTRIES IN HERE.
/// New entries must be appended at the end of this list.
enum DownloadItemType {
collection(true, false),
song(true, true),
image(true, true),
anchor(false, false),
finampCollection(false, false);
collection("collection", true, false),
track("song", true, true),
image("image", true, true),
anchor("anchor", false, false),
finampCollection("finampCollection", false, false);

const DownloadItemType(this.requiresItem, this.hasFiles);
const DownloadItemType(this.isarType, this.requiresItem, this.hasFiles);

///!!! Used by `DownloadStub.getHash` to calculate the isarId for
///!!! the downloads system, DO NOT EDIT for any existing entries.
///!!! Doing so would invalidate existing downloads
///!!! and cause them to be deleted and re-downloaded.
final String isarType;
Comment on lines +1320 to +1331
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this (and the updated hash calculation) is the juicy bit, right? That looks like it would solve the issue on an API issue at least 👍🏻

And theoretically we could add a proper migration later on, that updates hashes based on the new calculation method or something, but that might be unnecessary.
Using hard-coded strings instead of enum names seems like a good idea anyway, considering that other enum options might be renamed later on as well.

Copy link
Collaborator Author

@Maxr1998 Maxr1998 Mar 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes & yes. The only thing missing for this PR is doing some testing, which I couldn't do last weekend, but I'll hopefully have the time to do that later this week.
Oh, and I need to rebase and fix the merge conflicts.

A migration would definitely be possible, but I'll leave that to someone more knowledgeable with isar 😅

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. Let me know if you want me to test things as well.

And why do you want to rebase-merge? What's the benefit? I've only ever used the two other options...


final bool requiresItem;
final bool hasFiles;
Expand Down Expand Up @@ -1436,16 +1440,16 @@ enum BaseItemDtoType {
artist("MusicArtist", true, [album, track], DownloadItemType.collection),
playlist("Playlist", true, [track], DownloadItemType.collection),
genre("MusicGenre", true, [album, track], DownloadItemType.collection),
track("Audio", false, [], DownloadItemType.song),
track("Audio", false, [], DownloadItemType.track),
library(
"CollectionFolder", true, [album, track], DownloadItemType.collection),
folder("Folder", true, null, DownloadItemType.collection),
musicVideo("MusicVideo", false, [], DownloadItemType.song),
audioBook("AudioBook", false, [], DownloadItemType.song),
tvEpisode("Episode", false, [], DownloadItemType.song),
video("Video", false, [], DownloadItemType.song),
movie("Movie", false, [], DownloadItemType.song),
trailer("Trailer", false, [], DownloadItemType.song),
musicVideo("MusicVideo", false, [], DownloadItemType.track),
audioBook("AudioBook", false, [], DownloadItemType.track),
tvEpisode("Episode", false, [], DownloadItemType.track),
video("Video", false, [], DownloadItemType.track),
movie("Movie", false, [], DownloadItemType.track),
trailer("Trailer", false, [], DownloadItemType.track),
unknown(null, true, null, DownloadItemType.collection);

// All possible types in Jellyfin as of 10.9:
Expand Down
4 changes: 2 additions & 2 deletions lib/models/finamp_models.g.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

46 changes: 23 additions & 23 deletions lib/services/downloads_service.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ class DownloadsService {
.optional(
state != DownloadItemState.syncFailed,
(q) => q
.typeEqualTo(DownloadItemType.song)
.typeEqualTo(DownloadItemType.track)
.or()
.typeEqualTo(DownloadItemType.image))
.filter()
Expand Down Expand Up @@ -410,7 +410,7 @@ class DownloadsService {
_isar.txnSync(() {
downloadCounts["track"] = _isar.downloadItems
.where()
.typeEqualTo(DownloadItemType.song)
.typeEqualTo(DownloadItemType.track)
.filter()
.not()
.stateEqualTo(DownloadItemState.notDownloaded)
Expand Down Expand Up @@ -583,7 +583,7 @@ class DownloadsService {
(q) => q.anyOf([BaseItemDtoType.album, BaseItemDtoType.playlist],
(q, element) => q.baseItemTypeEqualTo(element))
),
(DownloadItemType.song, null),
(DownloadItemType.track, null),
(DownloadItemType.image, null),
];
// Objects matching a require filter cannot require elements matching earlier filters or the current filter.
Expand Down Expand Up @@ -620,12 +620,12 @@ class DownloadsService {
(q, element) => q.baseItemTypeEqualTo(element))
.not()
.info((q) => q.not().group((q) => q
.typeEqualTo(DownloadItemType.song)
.typeEqualTo(DownloadItemType.track)
.or()
.typeEqualTo(DownloadItemType.image))),
// Select required tracks that only link collections or images
(q) => q
.typeEqualTo(DownloadItemType.song)
.typeEqualTo(DownloadItemType.track)
.requiredByIsNotEmpty()
.not()
.info((q) => q.not().group((q) => q
Expand Down Expand Up @@ -660,7 +660,7 @@ class DownloadsService {
downloadCounts[repairStepTrackingName] = 2;
var itemsWithFiles = _isar.downloadItems
.where()
.typeEqualTo(DownloadItemType.song)
.typeEqualTo(DownloadItemType.track)
.or()
.typeEqualTo(DownloadItemType.image)
.findAllSync();
Expand Down Expand Up @@ -694,7 +694,7 @@ class DownloadsService {
_isar.writeTxnSync(() {
var itemsWithFiles = _isar.downloadItems
.where()
.typeEqualTo(DownloadItemType.song)
.typeEqualTo(DownloadItemType.track)
.or()
.typeEqualTo(DownloadItemType.image)
.findAllSync();
Expand All @@ -720,7 +720,7 @@ class DownloadsService {
.where()
.stateNotEqualTo(DownloadItemState.notDownloaded)
.filter()
.typeEqualTo(DownloadItemType.song)
.typeEqualTo(DownloadItemType.track)
.findAllSync();
final JellyfinApiHelper jellyfinApiData =
GetIt.instance<JellyfinApiHelper>();
Expand Down Expand Up @@ -797,7 +797,7 @@ class DownloadsService {
}
for (var item in _isar.downloadItems
.where()
.typeEqualTo(DownloadItemType.song)
.typeEqualTo(DownloadItemType.track)
.or()
.typeEqualTo(DownloadItemType.image)
.filter()
Expand Down Expand Up @@ -882,7 +882,7 @@ class DownloadsService {
_isar.downloadItems.putSync(item);
List<DownloadItem> parents = _isar.downloadItems
.where()
.typeNotEqualTo(DownloadItemType.song)
.typeNotEqualTo(DownloadItemType.track)
.filter()
.requires((q) => q.isarIdEqualTo(item.isarId))
.or()
Expand Down Expand Up @@ -996,7 +996,7 @@ class DownloadsService {
_isar.writeTxnSync(() {
var items = _isar.downloadItems
.where()
.typeEqualTo(DownloadItemType.song)
.typeEqualTo(DownloadItemType.track)
.or()
.typeEqualTo(DownloadItemType.image)
.filter()
Expand Down Expand Up @@ -1111,7 +1111,7 @@ class DownloadsService {
var baseItem = track.track;
baseItem.mediaSources = [track.mediaSourceInfo];
var isarItem =
DownloadStub.fromItem(type: DownloadItemType.song, item: baseItem)
DownloadStub.fromItem(type: DownloadItemType.track, item: baseItem)
.asItem(DownloadProfile(
transcodeCodec: FinampTranscodingCodec.original,
downloadLocationId: track.downloadLocationId));
Expand Down Expand Up @@ -1202,7 +1202,7 @@ class DownloadsService {
// This should only be used for IDs/links and does not need real download items.
List<DownloadItem> required = parent.downloadedChildren.values
.map((e) =>
DownloadStub.fromItem(type: DownloadItemType.song, item: e)
DownloadStub.fromItem(type: DownloadItemType.track, item: e)
.asItem(null))
.toList();
isarItem.orderedChildren = required.map((e) => e.isarId).toList();
Expand Down Expand Up @@ -1300,7 +1300,7 @@ class DownloadsService {
var id = DownloadStub.getHash(item.id, DownloadItemType.collection);
var query = _isar.downloadItems
.where()
.typeEqualTo(DownloadItemType.song)
.typeEqualTo(DownloadItemType.track)
.filter()
.infoFor((q) => q.isarIdEqualTo(id))
.optional(
Expand Down Expand Up @@ -1346,7 +1346,7 @@ class DownloadsService {
}
return _isar.downloadItems
.where()
.typeEqualTo(DownloadItemType.song)
.typeEqualTo(DownloadItemType.track)
.filter()
.group((q) => q
.stateEqualTo(DownloadItemState.complete)
Expand Down Expand Up @@ -1406,7 +1406,7 @@ class DownloadsService {
.optional(
baseTypeFilter == BaseItemDtoType.playlist,
(q) => q.info((q) =>
q.typeEqualTo(DownloadItemType.song).requiredByIsNotEmpty()))
q.typeEqualTo(DownloadItemType.track).requiredByIsNotEmpty()))
.optional(
relatedTo != null,
(q) => q.infoFor((q) => q.info((q) => q.isarIdEqualTo(
Expand Down Expand Up @@ -1434,7 +1434,7 @@ class DownloadsService {
Future<DownloadStub?> getTrackInfo({BaseItemDto? item, String? id}) {
assert((item == null) != (id == null));
return _isar.downloadItems
.get(DownloadStub.getHash(id ?? item!.id, DownloadItemType.song));
.get(DownloadStub.getHash(id ?? item!.id, DownloadItemType.track));
}

/// Get information about a downloaded collection by BaseItemDto or id.
Expand All @@ -1451,7 +1451,7 @@ class DownloadsService {
/// be used instead. Exactly one of the two arguments should be provided.
DownloadItem? getTrackDownload({BaseItemDto? item, String? id}) {
assert((item == null) != (id == null));
return _getDownloadByID(id ?? item!.id, DownloadItemType.song);
return _getDownloadByID(id ?? item!.id, DownloadItemType.track);
}

/// Get an image's DownloadItem by BaseItemDto or id. This method performs file
Expand All @@ -1471,7 +1471,7 @@ class DownloadsService {
Future<DownloadedLyrics?> getLyricsDownload(
{required BaseItemDto baseItem}) async {
var item = _isar.downloadedLyrics
.getSync(DownloadStub.getHash(baseItem.id, DownloadItemType.song));
.getSync(DownloadStub.getHash(baseItem.id, DownloadItemType.track));
return item;
}

Expand Down Expand Up @@ -1504,7 +1504,7 @@ class DownloadsService {
.optional(
state != DownloadItemState.syncFailed,
(q) => q
.typeEqualTo(DownloadItemType.song)
.typeEqualTo(DownloadItemType.track)
.or()
.typeEqualTo(DownloadItemType.image))
.filter()
Expand Down Expand Up @@ -1543,13 +1543,13 @@ class DownloadsService {
info.add(item);
}
}
if (isRequired || item.type == DownloadItemType.song) {
if (isRequired || item.type == DownloadItemType.track) {
var children = item.requires.filter().findAllSync();
for (var child in children) {
_getFileChildren(child, required, info, isRequired);
}
}
if (isRequired || item.type != DownloadItemType.song) {
if (isRequired || item.type != DownloadItemType.track) {
var children = item.info.filter().findAllSync();
for (var child in children) {
_getFileChildren(child, required, info, false);
Expand All @@ -1559,7 +1559,7 @@ class DownloadsService {

/// Recursive subcomponent of [getFileSize].
Future<int> _getFileSize(DownloadItem item, bool required) async {
if (item.type == DownloadItemType.song &&
if (item.type == DownloadItemType.track &&
item.state.isComplete &&
required) {
if (item.fileTranscodingProfile == null ||
Expand Down
Loading