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.

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

Fixes #21; playlists are loaded in the background #32

Merged
merged 1 commit into from
Sep 12, 2024
Merged

Fixes #21; playlists are loaded in the background #32

merged 1 commit into from
Sep 12, 2024

Conversation

xxxserxxx
Copy link
Collaborator

There are two main components of this fix. The first is trivial: rather than calling GetPlaylists() up front in the GUI init, the playlists page is initialized empty. We then immediately call UpdatePlaylists() which forks off the call in the background and updates the playlists page when the server finally responds with all of the content.

The second component is the progress display in the UI. Since we can not tell from the Subsonic API how long the operation will take, a spinner is implemented on the "playlists" menu item. This runs until the operation is complete, at which point the menu is reset to the standard label.

Reproducing this may require specific conditions. #21 is a pain for me because my Subsonic server is Gonic, and the GetPlaylists call on the server side does a recursive load of all playlists and song metadata on each call, reloading from disk every time. As you can see in the screencast below, on my server with a few dozen playlists, some of which are quite large, this can take over half a minute. Without this patch, stmps is hung pre-UI waiting for the server response.

To reproduce this:

  1. Start with at least a few songs in the music lib
  2. Write a script that creates a few dozen playlists -- the naming doesn't matter -- each containing all of the songs in the library
  3. Run gonic with the library and playlists
  4. Run stmps against that gonic server

Theoretically, this should not interfere with the user using the playlists during a reload; playlist reloading can happen when the user adds songs to a playlist or so forth; any playlist list reload will trigger this new behavior, during which the UI remains responsive. This is visible in the screencast at t:16s, when I switch from the browser to the playlist page -- the menu is appropriately updated with the formatting, although it's difficult to discern in the cast.

I've also run this against an empty gonic interface with no playlists, and confirmed that it doesn't, e.g., leave weird artifacts if there's no server latency.

https://asciinema.org/a/cBH5hCOa2nrzfzqpdcM7oDJ25

There are a couple of considerations -- improvements or possible changes to the implementation:

  1. There's some hardcoding of the playlists menu item, in particular, the code expects the playlists pane to be the third in the menu. Additional code to derive this, or some consts to abstract it would be better proof against future changes; I didn't have a strong opinion about which direction to go, so it's just hard coded. This bugs me the most.
  2. The spinner is based on UTF-8 braille character runes; this will therefore only work if the terminal font includes those code points. It would be safer to use the age-old \|/- sequence, although where do we stop assuming the LCD? There are many other spinner character sequences, several of which would be more obvious -- the spinner I chose is not particularly attention-demanding, IMO, but the braille characters are a fairly safe code point to depend upon, ASCII being the only more reliable (but ugly) option.
  3. I did not make any changes to the README about this feature. But, then, not every little feature is in the README, so maybe this is OK.
  4. I tried to bullet-proof the hell out of the code, since it introduces a fair amount of concurrency; there are mutexes and liberal use of the tcell QueueUpdateDraw function. Most of it is probably unnecessary, but also, none of it is in critical path loops and the overhead is dwarfed by network/server latencies.
  5. The menu label formatting is annoyingly repetitive -- 7 lines of nearly identical code appear in 3 places, and one of those is dangerously in a different file. I was close to creating a convenience function, but it's right on the margin of what I consider overengineered vs cleaner. I could be convinced to pull it out into a convenience function, but it's currently not. The code in question are here, here, and here.

@spezifisch spezifisch merged commit b203a16 into spezifisch:main Sep 12, 2024
@spezifisch
Copy link
Owner

Finally got around to testing it. LGTM, thanks for this!

The second component is the progress display in the UI. Since we can not tell from the Subsonic API how long the operation will take, a spinner is implemented on the "playlists" menu item. This runs until the operation is complete, at which point the menu is reset to the standard label.

I like it but you really have to know it's there to notice it.

2. The spinner is based on UTF-8 braille character runes; this will therefore only work if the terminal font includes those code points. It would be _safer_ to use the age-old `\|/-` sequence, although where do we stop assuming the LCD? There are many other spinner character sequences, several of which would be more obvious -- the spinner I chose is _not_ particularly attention-demanding, IMO, but the braille characters are a fairly safe code point to depend upon, ASCII being the only more reliable (but ugly) option.

I'm fine with this.
We could add a config option like this to be super sure but that might be overkill: https://github.com/nvim-lua/kickstart.nvim/blob/7201dc480134f41dd1be1f8f9b8f8470aac82a3b/init.lua#L93

3. I did not make any changes to the README about this feature. But, then, not every little feature is in the README, so maybe this is OK.

True, I think I'd rather have a better help inside stmps than relying on an external handbook/README. (This could be nice scope creep for my ongoing tview-command integration -- showing documentation for the available commands.)

4. I tried to bullet-proof the hell out of the code, since it introduces a fair amount of concurrency; there are mutexes and liberal use of the **tcell** `QueueUpdateDraw` function. Most of it is probably unnecessary, but also, none of it is in critical path loops and the overhead is dwarfed by network/server latencies.

Ideally I would write tests for this. But at a glance regarding testing in tview, I only found rivo/tview#894.

1. There's some hardcoding of the playlists menu item, in particular, the code **expects** the playlists pane to be the third in the menu. Additional code to derive this, or some `const`s to abstract it would be better proof against future changes; I didn't have a strong opinion about which direction to go, so it's just hard coded. This bugs me the most.
5. The menu label formatting is annoyingly repetitive -- 7 lines of nearly identical code appear in 3 places, and one of those is dangerously in a different file. I was close to creating a convenience function, but it's right on the margin of what I consider overengineered vs cleaner. I could be convinced to pull it out into a convenience function, but it's currently not.  

Yeah, I think my refactoring of stmp left some ugly marks like that here and there, but I don't think the needed refactorings will get into v1.0.0.

@xxxserxxx
Copy link
Collaborator Author

... a spinner is implemented on the "playlists" menu item.

I like it but you really have to know it's there to notice it.

Yeah, I know. Maybe using different glyphs would make it stand out more. I first tried doing a modal dialog tied to just that page, but it defeated me: I couldn't get it to work properly. The spinner was a far easier and less invasive solution.

I suppose we could put some sort of spinner in the list itself, and then clear it before adding items to the list. I really wish Subsonic had a progressive API so we could do a proper progress bar, but without that we're limited to a spinner.

I'm open to other suggestions, and don't mind making a change. Or, if you know how to do a modal tied only to that Playlists page, I could put a more obvious spinner in that. Modals in tview are a PITA.

  1. The spinner is based on UTF-8 braille character runes

I'm fine with this. We could add a config option like this to be super sure but that might be overkill: https://github.com/nvim-lua/kickstart.nvim/blob/7201dc480134f41dd1be1f8f9b8f8470aac82a3b/init.lua#L93

That'd be simple enough, and I'm happy to make that change. Let me know how you want the config to look; really, I'd just put the contents of spinnerText in the config cycle through whatever sequence of characters the user puts in there, however many there are. I don't think the change would require more than a half dozen lines.

  1. I tried to bullet-proof the hell out of the code, since it introduces a fair amount of concurrency; there are mutexes and liberal use of the tcell QueueUpdateDraw function. Most of it is probably unnecessary, but also, none of it is in critical path loops and the overhead is dwarfed by network/server latencies.

Ideally I would write tests for this. But at a glance regarding testing in tview, I only found rivo/tview#894.

Yeah, I agree, test would be good. I didn't see any concurrency warnings from static code analysis, but tcell is skittish about thread locks and I didn't want to take any chances. Belts and suspenders don't hurt anything but performance, and I'm pretty sure I overdid the protections here.

  1. There's some hardcoding of the playlists menu item...
    ...
  2. The menu label formatting is annoyingly repetitive...

Yeah, I think my refactoring of stmp left some ugly marks like that here and there, but I don't think the needed refactorings will get into v1.0.0.

I don't think the refactoring played into this; number 1 was just because I didn't feel like trying to abstract out the page references without talking to you first about how you wanted to do it. And number 5 is literally just me deciding that factoring those lines of code out into a function would be overkill, since it's used exactly twice and I can't foresee a need to use it a third time. If I thought we'd be using it more, I'd have factored it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants