Credits support #526

jacksongoode · 2024-09-24T22:44:44Z

You can also click on the artists to get to their page!

jacksongoode · 2024-09-26T03:01:07Z

@SO9010 Wanted to get this one in but we still need to make sure that clicking credits while there is already a credits window, we don't spawn another.

SO9010 · 2024-09-26T10:20:12Z

@jacksongoode I'll have a look at this, but I also think the UI should be consistent with the preferences window if we have a pop-up open. Otherwise, it looks a bit out of place.

Also, I think we should consider changing the order of the right-click context menu. The contexts in the top section are related to going to the track's artist and the album, so maybe we should place it above or below those two contexts.
I also feel that without doing this, people's muscle memory would get in the way of adding the song to their playlist, as that has always been at the bottom.

Maybe the copy link to track should get moved down to the lower section.

However, it could also be ordered as a list of priority to the user, and maybe we could flip it all the way around so that as soon as the user right-clicks on the track, they can add it to a playlist. This could maybe improve UX? Up to you.

jacksongoode · 2024-10-07T06:15:41Z

I would be happy to go with what you suggest here. I'm indifferent about the location, but it should be logical or follow Spotify? I do feel the default preferences window is a little too big for just the credits, I understand the rational between having them the same size but maybe we can make them the same height and the credits, half the width?

SO9010 · 2024-10-07T12:43:34Z

Yeah, having the same height but half the width sounds good to me.

SO9010 · 2024-10-18T11:03:08Z

Its looking good now!

jacksongoode · 2024-10-18T23:05:53Z

There are still two bugs I'd like it figure out. One is that clicking on the credits to each track doesn't reliably make them appear. It seems sometimes the network gets hung up on fetching the credits. And it requires interacting with some other network connection and triggering some fetch to actually cause the credits to work again.

The other thing is that we should only have one credit window at a time where we set the credit window ID and then replace the title and content if that window exists.

SO9010

I've had a quick look just added my thoughts, I will add some changes that I think would be good.

SO9010 · 2024-10-21T09:25:37Z

psst-gui/src/webapi/client.rs

@@ -38,6 +39,43 @@ use crate::{

 use super::{cache::WebApiCache, local::LocalTrackManager};

+#[derive(Debug, Clone, Data, Lens, Deserialize)]


Do these structures need to be public? They aren't being called more than once, are they? I think it would make more sense and follow the rest of the code more if we had these in the function that calls the API.

SO9010 · 2024-10-21T09:26:50Z

psst-gui/src/webapi/client.rs

+}
+
+#[derive(Debug, Clone, Data, Lens, Deserialize)]
+pub struct CreditArtist {


This could be in the lyrics file, which could then be imported into this file. This is what is usually done.

SO9010 · 2024-10-21T09:31:41Z

psst-gui/src/data/mod.rs

@@ -84,6 +85,8 @@ pub struct AppState {
    pub finder: Finder,
    pub added_queue: Vector<QueueEntry>,
    pub lyrics: Promise<Vector<TrackLines>>,
+    pub credits: Option<TrackCredits>,
+    pub webapi: Arc<WebApi>,


I'm a bit confused about why we need a new webapi module.

SO9010 · 2024-10-21T09:36:46Z

psst-gui/src/webapi/client.rs

+                "client-token",
+                self.session.client_token().unwrap_or_default(),
+            );
+        let response = Self::with_retry(|| Ok(request.clone().call()?))?;


With_retry actually only applies when error 429 is sent back, which means too many requests have been sent. Is this needed?

SO9010 · 2024-10-21T09:55:12Z

I have submitted a pr to the repository for these changes.

jacksongoode · 2024-10-21T19:06:15Z

@SO9010 Still seeing that the response is throttled? It's pretty odd, not sure if this endpoint is more heavily restricted than others so maybe we need to compensate for that?

SO9010 · 2024-10-21T19:47:05Z

It probably is restricted due to being experimental. What happens when it doesn't work? Is there maybe a way to detect it then just block it?

jacksongoode · 2024-10-22T18:30:11Z

When it doesn't work it just stalls and the window doesn't get refreshed. It's actually really finicky... It also might be a consequence of Psst's inability to repair a broken network connection/get a new token?

jacksongoode · 2024-10-23T06:45:52Z

Alright I think I've gotten everything in a good state now.

SO9010 · 2024-10-23T14:32:09Z

Just checked it looks great!

SO9010 · 2024-10-23T14:33:31Z

The only thing that I can think of is perhaps caching this, but I'm not sure it is needed, its quite a fast request anyway with not a lot of information. We could address this later...

SO9010 · 2024-10-23T14:37:54Z

@jacksongoode Something that we should apply to all of our serializing where there may not be any data in the field is by having #[serde(skip_serializing_if = "Option::is_none")].

For example in the playlist one there is:
#[serde(skip_serializing_if = "Option::is_none")]
pub images: Option<Vector>,

This would optimize the requests a lot!

jacksongoode · 2024-10-23T16:14:03Z

Where would you recommend we add that? To all of the credits fields?

Co-authored-by: so9010 <so9010sami@gmail.com>

SO9010 · 2024-10-23T19:41:37Z

psst-gui/src/ui/credits.rs

+
+#[derive(Debug, Clone, Data, Lens, Deserialize)]
+pub struct TrackCredits {
+    #[serde(rename = "trackUri")]


Instead using this we should use: #[serde(rename_all = "camelCase")]

SO9010 · 2024-10-23T19:42:57Z

psst-gui/src/ui/credits.rs

+    #[serde(rename = "roleCredits")]
+    pub role_credits: Arc<Vec<RoleCredit>>,
+    #[serde(rename = "extendedCredits")]
+    pub extended_credits: Arc<Vec<String>>,


I think here would could have #[serde(skip_serializing_if = "Option::is_none")]

Ok I made all the possible credits optional I believe.

jacksongoode · 2024-10-24T20:04:54Z

@SO9010 How does that most recent commit look to you? If good, I'll merge.

SO9010 · 2024-10-24T20:07:13Z

Yes looks good to me!

jacksongoode mentioned this pull request Sep 25, 2024

Credits support #509

Closed

jacksongoode force-pushed the jackson/credits branch from 1880932 to ce81756 Compare October 17, 2024 17:08

SO9010 reviewed Oct 21, 2024

View reviewed changes

jacksongoode force-pushed the jackson/credits branch from 17aa703 to 5f31315 Compare October 21, 2024 19:20

jacksongoode and others added 10 commits October 23, 2024 12:03

Experiment with credits

a877f2d

This works

23cd796

Fix linting

e66c40b

Same size as pref

6fb525b

Suggested Changes (#547)

7a52537

Co-authored-by: so9010 <so9010sami@gmail.com>

Ok this works

8deefd0

Cleanup

5a49194

Fix

d302c88

Revert some mistakes

9e7735b

Fix lint

9aa34ac

jacksongoode force-pushed the jackson/credits branch from e343523 to 9aa34ac Compare October 23, 2024 19:03

SO9010 reviewed Oct 23, 2024

View reviewed changes

Fix to only show credit section if they exist

a0ac59f

jacksongoode merged commit 0292319 into main Oct 24, 2024
8 checks passed

jacksongoode deleted the jackson/credits branch October 24, 2024 20:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Credits support #526

Credits support #526

jacksongoode commented Sep 24, 2024

jacksongoode commented Sep 26, 2024

SO9010 commented Sep 26, 2024

jacksongoode commented Oct 7, 2024 •

edited

Loading

SO9010 commented Oct 7, 2024

SO9010 commented Oct 18, 2024 •

edited

Loading

jacksongoode commented Oct 18, 2024

SO9010 left a comment

SO9010 Oct 21, 2024

SO9010 Oct 21, 2024

SO9010 Oct 21, 2024

SO9010 Oct 21, 2024

SO9010 commented Oct 21, 2024

jacksongoode commented Oct 21, 2024

SO9010 commented Oct 21, 2024

jacksongoode commented Oct 22, 2024

jacksongoode commented Oct 23, 2024

SO9010 commented Oct 23, 2024

SO9010 commented Oct 23, 2024

SO9010 commented Oct 23, 2024

jacksongoode commented Oct 23, 2024

SO9010 Oct 23, 2024

jacksongoode Oct 24, 2024

SO9010 Oct 23, 2024 •

edited

Loading

jacksongoode Oct 24, 2024

jacksongoode commented Oct 24, 2024

SO9010 commented Oct 24, 2024

		@@ -38,6 +39,43 @@ use crate::{

		use super::{cache::WebApiCache, local::LocalTrackManager};

		#[derive(Debug, Clone, Data, Lens, Deserialize)]

Credits support #526

Credits support #526

Conversation

jacksongoode commented Sep 24, 2024

jacksongoode commented Sep 26, 2024

SO9010 commented Sep 26, 2024

jacksongoode commented Oct 7, 2024 • edited Loading

SO9010 commented Oct 7, 2024

SO9010 commented Oct 18, 2024 • edited Loading

jacksongoode commented Oct 18, 2024

SO9010 left a comment

Choose a reason for hiding this comment

SO9010 Oct 21, 2024

Choose a reason for hiding this comment

SO9010 Oct 21, 2024

Choose a reason for hiding this comment

SO9010 Oct 21, 2024

Choose a reason for hiding this comment

SO9010 Oct 21, 2024

Choose a reason for hiding this comment

SO9010 commented Oct 21, 2024

jacksongoode commented Oct 21, 2024

SO9010 commented Oct 21, 2024

jacksongoode commented Oct 22, 2024

jacksongoode commented Oct 23, 2024

SO9010 commented Oct 23, 2024

SO9010 commented Oct 23, 2024

SO9010 commented Oct 23, 2024

jacksongoode commented Oct 23, 2024

SO9010 Oct 23, 2024

Choose a reason for hiding this comment

jacksongoode Oct 24, 2024

Choose a reason for hiding this comment

SO9010 Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

jacksongoode Oct 24, 2024

Choose a reason for hiding this comment

jacksongoode commented Oct 24, 2024

SO9010 commented Oct 24, 2024

jacksongoode commented Oct 7, 2024 •

edited

Loading

SO9010 commented Oct 18, 2024 •

edited

Loading

SO9010 Oct 23, 2024 •

edited

Loading