Skip to content
This repository has been archived by the owner on Aug 23, 2024. It is now read-only.

Enable resuming of queue #293

Merged
merged 4 commits into from
Apr 12, 2022
Merged

Enable resuming of queue #293

merged 4 commits into from
Apr 12, 2022

Conversation

kgarner7
Copy link
Contributor

This PR seeks to address #130.

To support saving/restoring the queue state, there are two new reducers. They store the following data (without pre-processing), compressed using brotli:

  • the queue entries. This includes the base entry, but also shuffledEntry and sortedEntry (useful if the user had shuffled/sorted)
  • the sort state (to restore if the user sorted)
  • the current song state
  • the player state

This is a draft PR because there were a few questions I had:

  1. Should we do some pre-processing of the shuffled/sorted entries? Since they are just alternate versions of the main entry, we could do a simple manual compression mapping album ids to their location(s) in the other lists.
  2. How should it be handled when both sortColumn and shuffle are set? When I have both, it seems like the sort takes precedence, but not before sometimes switching rapidly between shuffled and sorted version
  3. The before quit event is not sent in Windows if the app is closed due to a shutdown/restart of the system or a user logout. I'm not sure if/how we could handle this edge case
  4. This should probably be a configuration option, but I'd be worried about adding something that would require additional translations.

Finally, as an aside, the reason why I ended up with brotli (out of brotli, gzip, and deflate), was that on a sample queue of ~10k songs (with sorted and shuffled entries), it had the best compression at between 3.1-3.5 MB (variance depending on OS), but was the fastest. Unfortunately this compression has to be synchronous, so there is a delay in shutdown.

- on before-quit, save the queue state (entries, sorting, current song/player)
- state is compressed using brotli
- when the window is ready, restore the queue
@kgarner7
Copy link
Contributor Author

...I was hoping that a draft PR wouldn't trigger builds. Obviously, I'm incorrect .-.

@jeffvli
Copy link
Owner

jeffvli commented Apr 10, 2022

...I was hoping that a draft PR wouldn't trigger builds. Obviously, I'm incorrect .-.

No worries about builds running. Public repos have unlimited build minutes :)

Thanks for taking on this issue. In regards to your questions:

  1. I'd say preprocessing for the entry vs shuffledEntry is optional. It's up to you if you want to do it or not. My reason for not having a preference is because in this case there is only one queue saved. The filesize is probably at the point where it's a non-factor due to modern disk capacities.
  2. Don't save the sortedEntry. The column sort state isn't persisted between sessions so if the user wanted to view their queue in the sorted state they could just toggle it again on startup.
  3. I don't think we need to work around it. Just add a notice that improper shutdowns may result in the queue not saving.
  4. There's quite a few strings that are untranslated for the languages, so don't worry about adding something new (especially a setting description). I still need to set up weblate for easier translations.

Unfortunately this compression has to be synchronous, so there is a delay in shutdown.

How long is the shutdown delay? Could possibly send an ipcMain event to the frontend and have it open a modal notice until the save is complete.

@kgarner7
Copy link
Contributor Author

  1. If that's the case, then I think I'll just save both.
  2. Fair enough! I just wanted to resume in the exact same state, but makes sense.
  3. Seems good.
  4. I can add the warning as part of the configuration

When I was working on the dev version, I was seeing delays of a few seconds to close, but the reported time for decompression was ~180ms (brotli) versus ~450ms for gzip/deflate. With a production version, I can barely notice the difference now. Must've just been the delay of tearing down webpack dev server

@jeffvli jeffvli linked an issue Apr 10, 2022 that may be closed by this pull request
@kgarner7
Copy link
Contributor Author

Applied the changes! Now I can see that, for large queues (~10k), it adds 2-3 seconds on window close. Unfortunately, because the renderer process freezes, I couldn't figure out a way to send a toast/other notice (the toast would fire, but never render).

Tested on windows/linux/mac (sort of). Please test both the dev and prod version, because the production version gave me more headaches than I expected (among other things, why the saveState dispatch is not forward to the main process).

@kgarner7 kgarner7 marked this pull request as ready for review April 11, 2022 03:41
src/__tests__/App.test.tsx Outdated Show resolved Hide resolved
@jeffvli
Copy link
Owner

jeffvli commented Apr 11, 2022

Besides needing to remove the resume property from the playQueue state, I think looks good to merge!

@kgarner7
Copy link
Contributor Author

Addressed.
Also, async saving! And using a compression level of 1. As it turns out, deflate of 1 seems to do just fine (much better than brotli). This also removes the need to do dispatching for saving the state.

We can add a notice that says 'now saving, please wait', but this would require deferring the actual saving slightly (in some simple testing, a delay of 100ms was enough for the toast to be shown). For queue = 10k songs, the time was somewhere around 500ms across OSX and Windows.

With 100k songs (admittedly many, many repeats), it compressed down to ~32MB in 3-4 seconds.
Decompression at this size causes the UI to freeze, but short of having less state in the queue, this is probably to be expected (as we need to update the UI state as well).

@jeffvli
Copy link
Owner

jeffvli commented Apr 12, 2022

The changes look good, and is extremely fast! 3-4s for 100k songs is a good benchmark especially since I'd wager 99% of users will have less than 10k songs at any given time. One thing before I merge, I noticed the default setting value is not yet.

Can you add a value in the setDefaultSettings.ts file that follows the existing setting styles?

@jeffvli jeffvli merged commit b3de3be into jeffvli:main Apr 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remember last song/playlist on startup
2 participants