psst
psst copied to clipboard
jpochyla
Tracks page doesn't properly reflect changes
Describe the bug When adding or removing saved tracks, the tracks page does not reflect those changes properly until the app is restarted.
To Reproduce
- Load the tracks page.
- Save a song that isn't already saved.
- The last saved song will now be duplicated at the bottom of the list, and the new song will not be there.
This also applies when unsaving songs.
- Go to the tracks page.
- Unsave a song.
- The last song in the list will be removed, and the song you just unsaved will still be there.
Expected behavior The list will be updated properly.
Environment
- OS: Windows 10
- Version: 21H2 19044.2130
Additional context Add any other context about the problem here.
Same thing for me, this is kind of annoying. I'm probably going to look into this in the next few days.
Yeah, pretty sure thats a problem with us not invalidating the cached api response/altering the local copy on updates
Wasn't able to look at it till now, though. Maybe in the next couple days 💀
I'm not sure exactly what the issue is, but it's not that.
https://github.com/jpochyla/psst/blob/64362ea4e3fc636febd95c5405b9c3cbf9f034c8/psst-gui/src/data/mod.rs#L293-L305
Here is where the local state is updated. If you print out the contents of the Vector after it's modified, the contents represent the correct state. In add_track, if Vector::push_front is replaced with Vector::push_back, the correct track gets added to the end of the list and is visible in the UI. Because of this, I would suspect it's related to Druid's handling of Vectors, however, albums use basically the same code and don't have this issue.
It seems the way the Track and Album object are handled is different. I'm looking into this now. My first thought is that SavedAlbum is stored as a hashset of str while SavedTracks are a hashset of the custom TrackId.
Maybe because TrackId's implement copy instead of clone which Albums do? Eh maybe this is a frontend refresh thing.
This appears to not to affect playlists when removing a song because we are specifically resubmitting the playlist data load command: https://github.com/jpochyla/psst/blob/38422b1795c98d8d0e3bc8dc479d12f8d5bd7154/psst-gui/src/ui/playlist.rs#L172-L173
As well, adding a song to a playlist doesn't have issues because playlist navigation always reloads the PlaylistDetails. It looks like it shouldn't, but the LOAD_DETAIL command always gets sent here because data.playlist_detail.playlist is ALWAYS Empty.
https://github.com/jpochyla/psst/blob/38422b1795c98d8d0e3bc8dc479d12f8d5bd7154/psst-gui/src/controller/nav.rs#L47-L53
@literallyjustroy If you're interested in submitted a PR to revise this, I'd be more than happy to look over it!
Still digging around on the internal logic here. A quick and dirty fix would just be force a full reload on all track lists when selected (like playlists do now). But then accessing your library would be slower than it is today, so it's really not ideal.
The saved_tracks is actually updated correctly during a remove track/add track, it's just that the tracks list widget doesn't use the new data. This is especially noticeable if you remove the top track from your saved songs, then try to play it. It'll start playing the one below it (since internally that is the first song in the list, even though the ui doesn't show it).
Not familiar with druid/not sure how to tell the page to reload the widget. Ideally we do a combination. We should show the correct data we have saved locally right on navigation, then we can reload the tracks in the background and if there's an update we'll see it then.
Okay so the problem with the lack of ui updating is (I'd guess this is pretty obvious if you're familiar at all with druid) is centered around Lenses. In particular I think the problem is that the saved_tracks_widget lens is looking at the Library::savedTrack field, which is actually a promise. So the only time the ui will update is when the entire object is changed. For example, the ui will actually update if you call clear() on the saved_tracks promise.
Interestingly, Saved Albums correctly updates instantly when getting removed...
Ah so it doesn't need to be a promise? We need to immediately update that object?