Album info #268
Album info #268
Conversation
Alright. will do. Also I thought this only applied to when I would change the potfiles |
|
I'll have a proper look/review ASAP, thank you!!! |
src/api/cached_client.rs
Outdated
| @@ -35,6 +35,8 @@ pub trait SpotifyApiClient { | |||
|
|
|||
| fn get_album(&self, id: &str) -> BoxFuture<SpotifyResult<AlbumDescription>>; | |||
|
|
|||
| fn get_album_info(&self, id: &str) -> BoxFuture<SpotifyResult<AlbumDescriptionInfo>>; | |||
There was a problem hiding this comment.
Sorry, I should have been clearer; I stand by what I said earlier, there is no need to duplicate the endpoint here; there is no reason get_album and get_album_info would return different things
What I meant when I said that you could have multiple API models was that other models that depend on Album could depend on a different model, for instance (model names are just an example):
- RawSearchResults could have a Option<Page> because we don't get copyrights, label, etc
- the regular endpoint would return a FullAlbum
And then those two would be converted to our local model (AlbumDescription) or another one
src/app/models.rs
Outdated
| @@ -96,6 +100,35 @@ impl PartialEq for AlbumDescription { | |||
|
|
|||
| impl Eq for AlbumDescription {} | |||
|
|
|||
| #[derive(Clone, Debug)] | |||
| pub struct AlbumDescriptionInfo { | |||
There was a problem hiding this comment.
good idea to compose :)
naming suggestion: AlbumFullDescription ?
src/app/models.rs
Outdated
| } | ||
|
|
||
| #[derive(Clone, Debug)] | ||
| pub struct AlbumInfoRef { |
There was a problem hiding this comment.
a note on naming: those models I suffixed with Ref where meant to act like a "reference" to some resource; for instance ArtistRef would hold an identifier that would allow having a full Artist struct, and a name to help display that reference
In that case, maybe AlbumReleaseDetails?
src/app/models.rs
Outdated
| pub struct AlbumInfoRef { | ||
| pub label: String, | ||
| pub release_date: String, | ||
| pub copyrights: Vec<CopyrightRef>, |
src/api/cached_client.rs
Outdated
| Ok(album) | ||
| let info = self | ||
| .cache_get_or_write(SpotCacheKey::Album(&id), None, |etag| { | ||
| self.client.get_album_info(&id).etag(etag).send() |
There was a problem hiding this comment.
Almost there :)
See the issue here is that we end up calling two times the same album endpoint within this function -- it's alright because of the cache but it should not happen regardless (actually not fine, it writes two different models to the same cache entry but anyway)
Ask yourself, what does the v1/album endpoint actually return? a "full" album as defined by Spotify, so the union of your current AlbumInfo + the previous Album model
Just update the get_album fn from the client to return the full API model, and then you're good I think, this line is no longer needed :) nor is get_album_info needed
src/app/models.rs
Outdated
| #[derive(Clone, Debug)] | ||
| pub struct AlbumFullDescription { | ||
| pub description: AlbumDescription, | ||
| pub info: AlbumReleaseDetails, |
There was a problem hiding this comment.
(nitpick: this field could be named release_details)
|
Thank you!! Looks like we're done :) I'll give it a quick final test to see if nothing broke after all the changes from review but it should be good to merge |
|
Hi, GNOME 3.36.8 Do you want me to create an issue for this? |
|
Hi! Yes, an issue will be easier to track, thank you! :) |
This pull request focuses on adding a button and ui to albums to display some album info.
The layout is as follows:
