Conversation
|
We should make a placeholder icon for playlists with out cover images |
|
@jacksongoode I would like some input on the functionality please, currently, it just loads the first 10 for the artist and playlists, but we can get all of them, should we just load all of them in a scroll thing, or should we make it so we can say clikco nthe title to bring it to all of them? Edit: I can change how many is gotten i think its cleaner if we leave it just showing 20 of them, but we should have a way to view all |
|
Ok so I have set the unloadable playlist to the default playlist icon, I will do the same for the users. I will add (un)follow next, then I will request a review!! |
|
Alright, I'm quite happy with where this is at now, @jacksongoode could you please look over the code as there is a lot of it XD |
jacksongoode
left a comment
jacksongoode
left a comment
There was a problem hiding this comment.
Nice it looks great! Here's my first pass of comments.
| public_user_detail: PublicUserDetail { | ||
| info: Promise::Empty, | ||
| }, |
There was a problem hiding this comment.
Was there a reason to create a struct to hold only the info? Otherwise this could be just a Promise?
There was a problem hiding this comment.
This is in case we wanted to add the ability to view all for different elements, so for example you can have all of the user playlists or artists or friends there, and that imo would be better split into different parts?
psst-gui/src/data/nav.rs
Outdated
| Nav::SavedAlbums => "Saved Albums".to_string(), | ||
| Nav::Shows => "Podcasts".to_string(), | ||
| Nav::SearchResults(query) => query.to_string(), | ||
| Nav::PublicUserDetail(usr) => usr.get_display_name().to_string(), |
There was a problem hiding this comment.
We can probably use user :) just one more char.
There was a problem hiding this comment.
Also any reason we are using a getter here? Compared to just having it be a field like .name (like the others?)
There was a problem hiding this comment.
The issue is the inconsistencies with the serialisation of public user, sometimes it is display_name and sometimes its name...
| // Update the is_following state in the cached data | ||
| if let Some(cached) = data.public_user_detail.info.resolved_mut() { | ||
| let info = Arc::make_mut(&mut cached.data); | ||
| info.is_following = Some(true); | ||
| info.followers_count += 1; | ||
| } | ||
| } | ||
| }, | ||
| ) | ||
| .on_command_async( | ||
| UNFOLLOW_USER, | ||
| |username| WebApi::global().unfollow_user(&username), | ||
| |_, _, _| {}, | ||
| |_, data, (_username, r)| { | ||
| if let Err(err) = r { | ||
| data.error_alert(err); | ||
| } else { | ||
| data.info_alert("User unfollowed."); | ||
| // Update the is_following state in the cached data | ||
| if let Some(cached) = data.public_user_detail.info.resolved_mut() { | ||
| let info = Arc::make_mut(&mut cached.data); | ||
| info.is_following = Some(false); | ||
| info.followers_count -= 1; |
There was a problem hiding this comment.
I guess it makes sense that we update these values, but do we ever actually refresh these values elsewhere? Not that it matters, just curious. I'm trying to think about how Psst might cache things, but some fields shouldn't really need to be cached if they are dynamic, like follower counts, listener counter perhaps.
There was a problem hiding this comment.
No we don't refresh them elsewhere :P, not actually sure why I did this like this anymore... Do you think I should change it?
| #[serde(default)] | ||
| pub recently_played_artists: Vector<PublicUserArtist>, | ||
| #[serde(default)] | ||
| pub public_playlists: Vector<PublicUserPlaylist>, |
There was a problem hiding this comment.
Like perhaps this should just be a vec of playlists?
| }) | ||
| } | ||
|
|
||
| pub fn get_public_user_followers(&self, id: Arc<str>) -> Result<Vector<PublicUser>, Error> { |
There was a problem hiding this comment.
I think some of these UI widgets can be abstracted here too.
There was a problem hiding this comment.
I guess what I meant here is that get_public_user_followers, get_public_user_following are identical except for the request url and could be abstracted.
Co-authored-by: Jackson <54308792+jacksongoode@users.noreply.github.com>
|
Thanks for the review, I will go through this in a bit! |
|
Annoyingly psst isnt loading at all anymore, the program runs for me on all builds, but then the whole ui crashes... I hope your not getting the same issue... Edit: Just needed a reboot XD |
|
@jacksongoode I was looking into lazy scrolling with druid, and unfortunately, it be possible, at least in a clean way, there is no way I belive to append onto a list view... |
2 participants
This is the PR for the feature #616.
This is doing a similar thing to how the homepage works and should have essentially the same functionality as the Spotify one. Something that needs discussing is how can we should all the playlists or followers?