-
Notifications
You must be signed in to change notification settings - Fork 7
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
Conversation
…show the user the ongoing operation.
Finally got around to testing it. LGTM, thanks for this!
I like it but you really have to know it's there to notice it.
I'm fine with this.
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.)
Ideally I would write tests for this. But at a glance regarding testing in tview, I only found rivo/tview#894.
Yeah, I think my refactoring of |
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.
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.
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.
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. |
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 callUpdatePlaylists()
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:
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:
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.\|/-
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.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.