search

jpochyla / psst

Fast and multi-platform Spotify client with native GUI
MIT License
8.53k stars 218 forks source link

Tracks page doesn't properly reflect changes #348

Open Insprill opened 2 years ago

Insprill commented 2 years ago

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

This also applies when unsaving songs.

Expected behavior The list will be updated properly.

Environment

Additional context Add any other context about the problem here.

oleggtro commented 1 year ago

Same thing for me, this is kind of annoying. I'm probably going to look into this in the next few days.

jacksongoode commented 1 year ago

Can confirm this on latest/macOS as well.

oleggtro commented 1 year ago

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 💀

Insprill commented 1 year ago

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.

jacksongoode commented 1 year ago

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.

jacksongoode commented 1 year ago

Maybe because TrackId's implement copy instead of clone which Albums do? Eh maybe this is a frontend refresh thing.

literallyjustroy commented 9 months ago

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

jacksongoode commented 9 months ago

@literallyjustroy If you're interested in submitted a PR to revise this, I'd be more than happy to look over it!

literallyjustroy commented 9 months ago

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.

literallyjustroy commented 9 months ago

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...

jacksongoode commented 9 months ago

Ah so it doesn't need to be a promise? We need to immediately update that object?