Add sync button to playlist screen #534

shayaantx · 2023-11-13T19:33:30Z

Downloads playlists on the playlist screen (will add anything missing and remove anything that doesn't exist in playlist anymore that is downloaded)

Chaphasilor · 2023-11-14T12:37:15Z

Thanks for the PR!
Is this similar to #460?
Maybe you could add a small description or a short video that clarifies how this works :)

shayaantx · 2023-11-14T15:42:34Z

added summary (lemme know if you want some screenshots or a video)

@Chaphasilor it is similar, however my understanding of #460 is that it will sync every single parent (which i think could be an album or playlist) that exists on the downloads screen.

The difference here is this lives solely on the playlist screen (not albums), this also removes any songs that are no longer part of the playlist anymore (need to test some edge cases with this tho). This also only syncs whatever playlist you are currently viewing (not all the playlists like the other button)

Chaphasilor · 2023-11-14T17:27:07Z

@shayaantx okay, got it. Would there be a way to re-use some of the code from the other PR to keep things organized? Or are you already doing that?

shayaantx · 2023-11-15T17:14:14Z

@Chaphasilor the only clean way I see to reuse that code would to not reuse it and replace that code with this code (since this removes items too)

I can try to refactor it and see what I can come up with it

Chaphasilor · 2023-11-16T16:02:21Z

Okay, then maybe you could split the logic into a separate file and function(s) so that it's easier to replace the existing code later on. Once I get around to testing this out, we can try to get this merged 😁

Chaphasilor · 2023-11-17T20:07:34Z

Not sure why the build it failing. I'll take a look soon!

shayaantx · 2023-11-22T23:21:57Z

@Chaphasilor I was looking into the case where you have a downloaded item shared across playlists/album

so it won't remove the downloaded item as long as the user synced both album and playlist (or playlist and playlist or album and album etc etc)

So the user would have to had to press sync on the album/playlist with the song, then also sync on the other, then the DownloadSong::requiredBy won't be empty.

I also wasn't passing "deleteFor" to DownloadsHelper::deleteDownloads() which was making it always remove it everytime (I fixed this in my recent push).

I refactored it out as requested, still looking into possibly reusing this logic for the SyncDownloads stuff (just been busy - will try to look more this weekend)

Chaphasilor · 2023-11-23T00:02:13Z

That sounds great! My own big PR got merged in recently so I'll be taking a look at the other open PRs soon, including yours. Should I wait until you looked into the reusability?

shayaantx · 2023-11-23T00:40:01Z

That sounds great! My own big PR got merged in recently so I'll be taking a look at the other open PRs soon, including yours. Should I wait until you looked into the reusability?

Yea I'll repost once once I've figured out the reusability part

shayaantx · 2023-11-27T20:14:28Z

@Chaphasilor so I refactored a few things locally:

Replaced download button on album/playlist with just a delete button if the album has been downloaded (so I removed the download button in lieu of the sync button I added)
Then replaced the underlying logic sync downloads button on downloads page with my sync logic

However I found one issue, specifically with the delete part of the logic. If you recall before I mentioned I wasn't using "deletedFor" attribute when DownloadsHelper::deleteDownloads()

This particular function always removes whatever the deletedFor value is from the parent caches (_downloadedParentsBox for example). This results in every sync removing the parent item (which basically makes the app look like nothing was downloaded)

I'm investigating the best way to address this, basically seems like I might need to refactor some of the delete logic to be separate so I don't delete parent unless the user asks to delete the entire downloaded album/playlist and in the sync logic just delete items (unless they sync and the album/playlist is empty)

Chaphasilor · 2023-11-27T21:41:59Z

Ah I see, that makes sense. I haven't looked at the code yet, but maybe adding a removeParentItem flag would help, or even better detecting if the parent has any downloaded children and not deleting the parent if that is the case.
But I guess that depends on how the downloading logic handles deleting a parent (including any children), for example if yoi want to get rid of a previously downloaded album.

I'm not familiar with the downloading stuff myself, maybe @jmshrv has some additional input...

shayaantx · 2023-11-28T03:45:47Z

@Chaphasilor yea I did something like that locally, need to test it a bit, then ill repush

Specifically I renamed the existing deleteDownloads to deleteParentAndChildDownloads since that really attempts to force delete all the downloads (including parent if you give)
So I split out deleting children and deleting parent, then in my sync delete function I explicitly check if the parent has anymore children left, and delete it if it does not.

Chaphasilor · 2023-11-28T21:25:07Z

Awesome! Looking forward to testing it out 😁

shayaantx · 2023-12-01T03:22:29Z

@Chaphasilor i tested out the changes with my own usage (seems fine), let me know if you find anything weird

Behavior now:

Now there is a delete and sync button (delete wipes out all offline downloads), sync will add/remove any changes to playlist/album
If you have never synced the playlist/album, the delete button doesn't show, but the sync button will always show
If you remove all items from a playlist (in jellyfin), then sync, it will remove all downloaded items, but leave behind the playlist (which can be manually removed) - the entries for each downloaded item will be gone.
If an item is shared across offline downloaded album/playlists, that item should not be removed (at least in my testing it was not). The caveat to this is, you must actually sync that offline item for both album/playlists (i.e., if you never click sync on playlistB but did on playlistA, the requiredBy field for the downloaded item has no reference to playlistB - which makes sense I think)

Chaphasilor · 2023-12-01T12:42:06Z

@shayaantx I'd prefer that in the case where the playlist/album has not been downloaded and the delete icon is not shown, you show the download icon instead of the sync icon. The sync icon could be mistaken for a refresh icon, leaving users confused about how they can download the playlist :)

Regarding the outdate requiredBy reference, that is expected behavior I think.
In the end, the "Sync All" button in the settings will probably be the preferred way to sync downloads anyway, because it handles all the playlists and albums, but now with your improved logic.

Only thing I'm wondering about for the "Sync All" button, say that Playlist A removes a requiredBy for a certain song and Playlist B adds one for the same song, while syncing we should try to first update all "requiredBy"s and only then start deleting song.
Otherwise we end up deleting the song because it's not required by Playlist A anymore and that one is checked first, and then re-downloading it for Playlist B. What do you think? I guess we could address this in a follow-up PR once this one has been merged :)

Chaphasilor · 2023-12-01T18:08:11Z

Did some quick testing, so far everything works 😁
Also merged the main branch to fix some build errors due to conflicting dependencies for me.
I'll look into merging this once #197 is merged. I'm guessing there will be a few merge conflicts, as both PRs deal with the download button...

shayaantx · 2023-12-01T20:33:27Z

Did some quick testing, so far everything works 😁 Also merged the main branch to fix some build errors due to conflicting dependencies for me. I'll look into merging this once #197 is merged. I'm guessing there will be a few merge conflicts, as both PRs deal with the download button...

Nice, I still need to change those buttons that you mentioned

Only thing I'm wondering about for the "Sync All" button, say that Playlist A removes a requiredBy for a certain song and Playlist B adds one for the same song, while syncing we should try to first update all "requiredBy"s and only then start deleting song.

This might be hard to do, need to think about it

shayaantx · 2023-12-03T00:41:30Z

@Chaphasilor ok I made a slight modification to show the download button only if album/playlist hasn't been downloaded yet

then if you click download, the buttons state updates, and you get a delete/sync button

… yet (for album/playlist

Chaphasilor · 2023-12-05T06:30:02Z

Does your implementation add the new downloads to the downloads "queue"? So that it shows up on the "Downloads" screen as enqueued or running?
Because I noticed that the "Retry" button that attempts to re-download failed items does not reliably increase the "enqueued" amount...

shayaantx · 2023-12-05T15:04:57Z

Does your implementation add the new downloads to the downloads "queue"? So that it shows up on the "Downloads" screen as enqueued or running? Because I noticed that the "Retry" button that attempts to re-download failed items does not reliably increase the "enqueued" amount...

I would assume this issue has always existed, cause looking at original code there is nothing explicit for that (that looks to all be within the downloads helper). I've changed the stuff above that (i.e., what to download and what to remove)

I will also mention that button (sync button on downloads screen), doesn't really do anything with UI when you click it, however I see it sync-ing my downloads if I leave the screen and come back.

Chaphasilor · 2023-12-05T20:18:21Z

Yeah, the UI part also needs some work. But that's a different story...

Is this PR ready to merge from your side?

Chaphasilor · 2023-12-05T21:15:09Z

Just fixed the conflicts after merging #197, please let me know if I made a mistake anywhere!

Chaphasilor · 2023-12-13T14:48:28Z

@shayaantx I noticed that if I download an album and then enable offline mode, it doesn't show up in the albums list until I restart the app. Can you reproduce?

Chaphasilor · 2023-12-13T14:54:23Z

Another thing I noticed: if I delete an album but a track from that album is still in a playlist, the track stays downloaded on the device, but the album image does not. So the track is missing the album image, which isn't ideal. Not sure if that is in-scope for this PR though...

Edit: this also results in a null check exception when trying to go to the album

shayaantx · 2023-12-13T16:20:20Z

I noticed that if I download an album and then enable offline mode, it doesn't show up in the albums list until I restart the app. Can you reproduce?

Thats the thing I mentioned earlier about refreshing a bit more aggressively. So if you do following:

Sync fresh album
Enable offline mode
I do see it doesn't show up, but if you tap the arrow button, it will appear.

So when the tab is in focus, its not doing a fresh load (offline enabled or not) - at least this is what I've seen.

Another thing I noticed: if I delete an album but a track from that album is still in a playlist, the track stays downloaded on the device, but the album image does not. So the track is missing the album image, which isn't ideal. Not sure if that is in-scope for this PR though...

Lemme investigate this

Chaphasilor · 2023-12-13T18:19:09Z

Ahh okay I see, yeah that sounds like it could be improved. But I think we can get this merged first, since it's not a regression!

shayaantx · 2023-12-13T18:57:38Z

Ahh okay I see, yeah that sounds like it could be improved. But I think we can get this merged first, since it's not a regression!

Up to you boss, I can still look into the images removing kerfuffle too, doesn't matter to me.

Chaphasilor · 2023-12-14T08:24:58Z

Yeah the album covers fix would be nice. But no need to look into the refreshing :)

shayaantx · 2023-12-15T03:15:53Z

@Chaphasilor

Another thing I noticed: if I delete an album but a track from that album is still in a playlist, the track stays downloaded on the device, but the album image does not. So the track is missing the album image, which isn't ideal. Not sure if that is in-scope for this PR though...

How are you reproducing it? Are you

Create 2 playlists with one shared song that has an image
Go to playlist A, sync
Go to playlist B, sync
Delete playlist A
Then see missing image on playlist B

If I follow above steps I can't seem to reproduce the behavior you described, however when I debug the code, it seems to delete the image, but the UI doesn't reflect it - dunno, but it def seems like it is executing the remove download image future erroneously


  Future<void> addDownloads({
    required List<BaseItemDto> items,
    required BaseItemDto parent,
    required bool useHumanReadableNames,
    required DownloadLocation downloadLocation,

    /// The view that this download is in. Used for sorting in offline mode.
    required String viewId,
  }) async {
....
      for (final item in items) {
        _downloadsLogger.info("Attempting to download ${item.id}");
        if (_downloadedItemsBox.containsKey(item.id)) {
          // If the item already exists, add the parent item to its requiredBy field and skip actually downloading the song.
          // We also add the item to the downloadedChildren of the parent that we're downloading.
          _downloadsLogger.info(
              "Item ${item.id} already exists in downloadedItemsBox, adding requiredBy to DownloadedItem and adding to ${parent.id}'s downloadedChildren");

          // This is technically nullable but we check if it contains the key
          // in order to get to this point.
          DownloadedSong itemFromBox = _downloadedItemsBox.get(item.id)!;

          itemFromBox.requiredBy.add(parent.id);
          addDownloadedSong(itemFromBox);
          _addItemToDownloadedAlbum(parent.id, item);
          continue;
        }
....

This is the download jellyfin items function snippet, basically if the song already exists, it will basically put the song in all the cache/collections/requiredBy fields then continues to next item. What's missing here is adding requiredby updates to the downloaded image itself (if it exists) for the item we are downloading (which already exists). So this seems like an existing bug cause I didn't change this logic - but I can push a commit to fix 2moro or sometime this weekend.

Chaphasilor · 2023-12-15T09:25:46Z

What I did (iirc) was:

Download playlist
Download album of song in playlist
Delete album

Could also be that I did 1 and 2 in reverse order...

If you could figure out how to fix it, that would be awesome! 😁

shayaantx · 2023-12-16T03:38:15Z

@Chaphasilor I think I need to rewrite this entire download/remove function to make sense of this, like the way downloadImage gets managed is with requiredBy collection as well (however the ids we pass into this are song ids themselves). This is behavior I'm seeing so far:

Sync both playlists
Delete "te1" playlist
Switch to offline mode

You can see above the playlist loses parent image, but retains song images. I saw some more weird logic (not sure if its from debugging), where after deleting first playlist, the downloaded song was missing - but I can't reproduce that without breakpoints (maybe the futures weren't executing properly cause of my breakpoints)

Now if you modify steps to reproduce:

Sync both playlists
Delete one song from "te1" - "Inner Assassins"
Then come back to downloads page

You will get the entire shared song missing (this doesn't happen with the main sync button), but more cause the downloads helper does a hard delete in deleteDownloadChildren (which I kind of workaround in the download sync helper), the lil bit below

        if (downloadedSong.requiredBy.isEmpty || deletedFor == null) {

I dunno honestly, things get weird deleting, and all these collections used randomly everywhere to maintain persistent state isn't great. There seems like a greater need for a simpler model to store this data imo, but I'm super lazy :)

shayaantx · 2023-12-16T21:16:59Z

Lemme debug it some more and see what I come up with

…delete it, cause its shared...

shayaantx · 2023-12-17T03:02:50Z

@Chaphasilor ok I fixed the 2nd bug I mentioned by taking out the logic out of deleteDownloadChildren() that just deletes the song not including the deleteFor parameter (as that parameter causes the issue in this case and caused me some other problems down the line). The only annoying part is I duplicated that logic in a new method deleteSong(), cause the existing method honestly should be dismantled.

I'm still not able to reproduce the first issue you mentioned, maybe I can add push some logging changes, and you reproduce and post logs?

Chaphasilor · 2023-12-17T20:05:45Z

I'll try to repro it with your changes first, I'll take a look at those tomorrow! Sorry for not responding earlier...

shayaantx · 2023-12-18T03:21:41Z

I'll try to repro it with your changes first, I'll take a look at those tomorrow! Sorry for not responding earlier...

fyi the recent change isn't for your reported issue (although I think* that happens with this entire PR), its for when someone manually deletes a single shared song

Chaphasilor · 2023-12-18T07:59:41Z

Repro is here:

2023-12-18-08-53-25_1.mp4

It really is just:

Download playlist
Download album of track in playlist
Delete album again
Track is now missing the cover

shayaantx · 2023-12-18T16:10:39Z

@Chaphasilor oh you meant the cover image of the playlist/album not the individual track right? Cause I can reproduce the cover image disappearing with these steps, but the individual song still retains its image

Chaphasilor · 2023-12-18T16:17:32Z

Actually no, at the end on the songs tab you can see the song is missing the cover. The album is also missing the cover, but since it's deleted it disappears right after refreshing anyway...

shayaantx · 2023-12-20T04:37:36Z

Actually no, at the end on the songs tab you can see the song is missing the cover. The album is also missing the cover, but since it's deleted it disappears right after refreshing anyway...

FYI I've gone a different direction with this one, basically trying to decouple the download storage (which I'm calling the download store or whatever) from the downloads helper while trying to refactor how all this storage works and hopefully in the end these type of cases become easier (I hope)

It is also possible I give up and just fix this specific use case :), but it really seems like all the Hive boxes shouldn't be all over the downloads helper cause then its always a case of did I put the right object in the right collection in new or existing methods (just makes seeing the downloaded songs and assets relationships much nicer I think).

Lemme know if this is overkill too

Chaphasilor · 2023-12-20T20:53:34Z

@shayaantx are you aware of the planned changes to the download functionality? Also discussed here: #213

Best read through it and make sure you're not putting in too much effort for something that will be completely re-built eventually :)

shayaantx · 2023-12-21T02:05:46Z

@shayaantx are you aware of the planned changes to the download functionality? Also discussed here: #213

Best read through it and make sure you're not putting in too much effort for something that will be completely re-built eventually :)

Lol agree with all those bullet points, yea maybe I won't go into this I guess - I'll go back to trying to fix this specific edge case

Chaphasilor · 2023-12-21T16:01:06Z

Also, #568 just dropped, take a look at this. That is a bigger project though and will probably take a bit longer to hit stable, so don't worry about your changes becoming obsolete :)

We can get this PR merged as soon as you're happy with it!

shayaantx · 2023-12-22T04:33:26Z

Also, #568 just dropped, take a look at this. That is a bigger project though and will probably take a bit longer to hit stable, so don't worry about your changes becoming obsolete :)

We can get this PR merged as soon as you're happy with it!

Yea that PR completely removes the downloads helper, which this PR hinges on

Looking at that PR I see _syncDownload() does what appears to be a much easier to grok way of downloading, no idea how it would affect this PR

So I guess you could merge it in this current state and Komodo5197 could merge in these changes into his or we could wait till he is done and merge this?

Chaphasilor · 2023-12-22T09:34:12Z

I'd merge this first. The other PR should be rebased anyway.

So we'll leave out the missing covers for now? It's not a huge deal anyway...

shayaantx · 2023-12-23T03:41:17Z

fyi @Chaphasilor merged dec8e85 to resolve conflicts

Chaphasilor · 2023-12-24T14:48:41Z

Merged. Merry Christmas 🎄

Chaphasilor self-assigned this Nov 29, 2023

Add sync button to playlist screen

178e4b0

shayaantx force-pushed the main branch from 096041f to 178e4b0 Compare December 1, 2023 03:15

Chaphasilor mentioned this pull request Dec 1, 2023

Download sync not working (or PEBKAC) #539

Closed

Chaphasilor linked an issue Dec 1, 2023 that may be closed by this pull request

Download sync not working (or PEBKAC) #539

Closed

Merge branch 'main' into pr/shayaantx/534

71e1025

Show download button (instead of sync) if nothing has been downloaded…

9348262

… yet (for album/playlist

Merge remote-tracking branch 'origin/main' into pr/shayaantx/534

98091e0

Chaphasilor mentioned this pull request Dec 11, 2023

Download all missing songs on playlist/album #328

Closed

Chaphasilor mentioned this pull request Dec 13, 2023

Playlists tab is not refreshed when switching to offline mode #533

Closed

Fix bug where deleting one shared song in downloads screen shouldn't …

c3bbeb6

…delete it, cause its shared...

Komodo5197 mentioned this pull request Dec 22, 2023

[Redesign] Downloads Storage Rewrite #568

Merged

Merge remote-tracking branch 'upstream/main'

dec8e85

Chaphasilor merged commit cc2c208 into jmshrv:main Dec 24, 2023
2 checks passed

Add sync button to playlist screen #534

Add sync button to playlist screen #534

Conversation

shayaantx commented Nov 13, 2023 • edited Loading

Chaphasilor commented Nov 14, 2023

shayaantx commented Nov 14, 2023

Chaphasilor commented Nov 14, 2023

shayaantx commented Nov 15, 2023

Chaphasilor commented Nov 16, 2023

Chaphasilor commented Nov 17, 2023

shayaantx commented Nov 22, 2023

Chaphasilor commented Nov 23, 2023

shayaantx commented Nov 23, 2023

shayaantx commented Nov 27, 2023

Chaphasilor commented Nov 27, 2023

shayaantx commented Nov 28, 2023

Chaphasilor commented Nov 28, 2023

shayaantx commented Dec 1, 2023

Chaphasilor commented Dec 1, 2023

Chaphasilor commented Dec 1, 2023

shayaantx commented Dec 1, 2023

shayaantx commented Dec 3, 2023

Chaphasilor commented Dec 5, 2023 • edited Loading

shayaantx commented Dec 5, 2023

Chaphasilor commented Dec 5, 2023

Chaphasilor commented Dec 5, 2023

Chaphasilor commented Dec 13, 2023

Chaphasilor commented Dec 13, 2023 • edited Loading

shayaantx commented Dec 13, 2023 • edited Loading

Chaphasilor commented Dec 13, 2023

shayaantx commented Dec 13, 2023

Chaphasilor commented Dec 14, 2023

shayaantx commented Dec 15, 2023

Chaphasilor commented Dec 15, 2023

shayaantx commented Dec 16, 2023

shayaantx commented Dec 16, 2023

shayaantx commented Dec 17, 2023

Chaphasilor commented Dec 17, 2023

shayaantx commented Dec 18, 2023

Chaphasilor commented Dec 18, 2023

shayaantx commented Dec 18, 2023

Chaphasilor commented Dec 18, 2023

shayaantx commented Dec 20, 2023

Chaphasilor commented Dec 20, 2023

shayaantx commented Dec 21, 2023

Chaphasilor commented Dec 21, 2023

shayaantx commented Dec 22, 2023

Chaphasilor commented Dec 22, 2023

shayaantx commented Dec 23, 2023

Chaphasilor commented Dec 24, 2023

shayaantx commented Nov 13, 2023 •

edited

Loading

Chaphasilor commented Dec 5, 2023 •

edited

Loading

Chaphasilor commented Dec 13, 2023 •

edited

Loading

shayaantx commented Dec 13, 2023 •

edited

Loading