Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(lyrics-plus): pick better background color #2221

Merged
merged 4 commits into from Mar 28, 2023
Merged

Conversation

Lseoksee
Copy link
Contributor

@Lseoksee Lseoksee commented Mar 27, 2023

first:
Modified the default musixmatch tokens for lyrics-plus and popupLyrics. The "popupLyrics" token 2005218b74f939209bda92cb633c7380612e14cb7fe92dcd6a780f works, but the token lyrics-plus token 21051986b9886beabe1ce01c3ce94c96319411f8f2c122676365e3 does not seem to work properly.

We applied two tokens, the existing token 2005218b74f939209bda92cb633c7380612e14cb7fe92dcd6a780f and the newly issued token 22111116e5392ce08db76d4ce398786a1f2d7ba5c12a6184f28d46
This reduced the frequency of captcha error.

second:
Modified the default background color code for lyrics-plus to 8747370 If you turn on the colorful background option in "yrics-plus and look at the lyrics of the local music, it's too dark to see. If you have a better color, you can modify it.

before
Desktop Screenshot 2023 03 27 - 15 13 26 33

after
Desktop Screenshot 2023 03 27 - 15 11 15 88

@Lseoksee Lseoksee requested a review from a team as a code owner March 27, 2023 07:37
@Lseoksee Lseoksee requested review from rxri and kyrie25 March 27, 2023 07:37
@kyrie25
Copy link
Member

kyrie25 commented Mar 27, 2023

first:

Modified the default musixmatch tokens for lyrics-plus and popupLyrics. The "popupLyrics" token 2005218b74f939209bda92cb633c7380612e14cb7fe92dcd6a780f works, but the token lyrics-plus token 21051986b9886beabe1ce01c3ce94c96319411f8f2c122676365e3 does not seem to work properly.

We applied two tokens, the existing token 2005218b74f939209bda92cb633c7380612e14cb7fe92dcd6a780f and the newly issued token 22111116e5392ce08db76d4ce398786a1f2d7ba5c12a6184f28d46

This reduced the frequency of captcha error.

This happens partially because the number of users using the same token
hits rate limit. If we were to use the same one as popupLyrics then both extensions would face captcha.

Having another default API key for a couple ten thousands of users would not change anything either.

We already linked a guide on the Spicetify docs and the Lyrics Plus app on how to retrieve their own API key so this change should be reverted.

Copy link
Member

@kyrie25 kyrie25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previous comment

Copy link
Contributor Author

@Lseoksee Lseoksee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kyrie25 Thank you for the good point. However, the existing Lyrics-plus token seems to have a problem. popupLyrics has no captcha error no matter how many songs you listen to, while Lyrics-plus has a captcha error with a probability of almost 90%.
I don't think it's just a matter of the number of users.

@kyrie25
Copy link
Member

kyrie25 commented Mar 27, 2023

@kyrie25 Thank you for the good point. However, the existing Lyrics-plus token seems to have a problem. popupLyrics has no captcha error no matter how many songs you listen to, while Lyrics-plus has a captcha error with a probability of almost 90%.

I don't think it's just a matter of the number of users.

Popup Lyrics has a smaller user base than Lyrics Plus partially because it has a prominent issue that significantly affects UX (#1827), and generally isn't as convenient as an in-app page.

Regardless, it is only one point on the comment I made and with a large influx of users on the scale of thousands using one/two default keys, this would not make any difference.

I also want to point out again that we have a guide on how to get API keys for themselves thus this is still a redundant change.

@kyrie25
Copy link
Member

kyrie25 commented Mar 27, 2023

The changes on fallback color is much appreciated though, so if you were to commit that only/separately I'll accept it.

Copy link
Contributor Author

@Lseoksee Lseoksee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kyrie25 Okay, so I'll leave the second change and the multi-token feature of Popup Lyrics.

@Lseoksee Lseoksee requested review from kyrie25 and removed request for rxri March 27, 2023 08:46
Copy link
Member

@kyrie25 kyrie25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prettify files.

@Lseoksee
Copy link
Contributor Author

Prettify files.

I solved it.

kyrie25
kyrie25 previously approved these changes Mar 27, 2023
Extensions/popupLyrics.js Outdated Show resolved Hide resolved
Copy link
Contributor Author

@Lseoksee Lseoksee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rxri This line has already been a feature in Lyrics Plus. Would you like me to modify the Lyrics Plus as well?

@rxri
Copy link
Member

rxri commented Mar 27, 2023

please do

@Lseoksee
Copy link
Contributor Author

please do

I modified it.

@Lseoksee Lseoksee requested a review from rxri March 28, 2023 06:37
@rxri rxri changed the title feat: lyrics app improvement fix(lyrics-plus): pick better background color Mar 28, 2023
@rxri rxri merged commit 1d11513 into spicetify:master Mar 28, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants