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

[lyrics] Romanization toggle for Genius plugin #1039

Merged
merged 19 commits into from
Mar 14, 2023
Merged

Conversation

DereC4
Copy link
Contributor

@DereC4 DereC4 commented Feb 22, 2023

  • Adds a side menu to the genius-lyrics plugin

  • Adds a romanization toggle to the side menu allowing song lyrics in East Asian languages to be converted to their romanized forms

  • I've been exploring other methods right now and the one implemented is non invasive (doesn't interfere with other lyrics) which I believe is the best result

Screenshots:

Prior behavior
image

With romanization toggle
image

@DereC4 DereC4 changed the title [in-app-menu] Added a side menu for Genius Lyrics plugin to toggle song Romanization [in-app-menu] Added a side menu for Genius Lyrics plugin Feb 22, 2023
Copy link
Collaborator

@Araxeus Araxeus left a comment

Choose a reason for hiding this comment

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

Left some code review, note that I didn't actually test it yet.

also you should change the name of the PR to be more like [lyrics] Added romanization option

since this has nothing to do with either in-app-menu or a "side menu"

thanks 👍🏼

plugins/lyrics-genius/back.js Outdated Show resolved Hide resolved
plugins/lyrics-genius/menu.js Outdated Show resolved Hide resolved
plugins/lyrics-genius/menu.js Outdated Show resolved Hide resolved
plugins/lyrics-genius/back.js Outdated Show resolved Hide resolved
plugins/lyrics-genius/back.js Outdated Show resolved Hide resolved
plugins/lyrics-genius/back.js Outdated Show resolved Hide resolved
plugins/lyrics-genius/back.js Outdated Show resolved Hide resolved
@DereC4 DereC4 changed the title [in-app-menu] Added a side menu for Genius Lyrics plugin [lyrics] Romanization toggle for Genius plugin Feb 23, 2023
@DereC4
Copy link
Contributor Author

DereC4 commented Feb 25, 2023

Currently reworking detection for east Asian language songs!

@Zo-Bro-23
Copy link
Contributor

Currently reworking detection for east Asian language songs!

Will Indian languages be possible 🤓 I'm not really sure if Genius/Musixmatch has a good database of lyrics for Indian songs though. Lyrics are either in native script or in romanized script, but there doesn't seem to be a pattern, and it just seems to be the fancy of whoever uploaded the lyrics 😅

@DereC4
Copy link
Contributor Author

DereC4 commented Feb 25, 2023

Currently reworking detection for east Asian language songs!

Will Indian languages be possible 🤓 I'm not really sure if Genius/Musixmatch has a good database of lyrics for Indian songs though. Lyrics are either in native script or in romanized script, but there doesn't seem to be a pattern, and it just seems to be the fancy of whoever uploaded the lyrics 😅

Honestly I'd love to take a look at that after I finish implementing this for East Asian songs! (I'm not sure how Indian language scripts work with Unicode and stuff but if it exists there's probably a way)

Also love your profile widgets lol

@Zo-Bro-23
Copy link
Contributor

Zo-Bro-23 commented Feb 25, 2023

Honestly I'd love to take a look at that after I finish implementing this for East Asian songs! (I'm not sure how Indian language scripts work with Unicode and stuff but if it exists there's probably a way)

Yeah, me neither. I mostly listen to Malayalam songs, and I only know to read Hindi besides Malayalam. Since we are a relatively small state, the lyrics available are sort of random, like I mentioned before. I did a bit of digging on Genius, and first of all, very few Malayalam songs have lyrics available. The ones that I did find had romanized lyrics already, so that's that. (See this, this, and this). Musixmatch has some lyrics in native script though, in case you want to take a look (See this and this; I'm not able to view these lyrics on the Musixmatch website because it's apparently "restricted"). I'm wondering, would it be possible to convert romanized lyrics back to native script if Genius/Musixmatch has the romanized version 🤔

Also love your profile widgets lol

You mean my README? Thanks; took me plenty of time to set up 🤭

Edit: I realize now that the romanization is an option inherent in Genius, and not something that you implemented. In this case it probably won't be possible to support Malayalam, and also won't be possible to convert romanized lyrics back to native script. Just hoping Genius implements a good Malayalam database soon (Genius doesn't support community contributions right?)!

Edit 2: Musixmatch has a good implementation of Malayalam lyrics with romanization support. See this. Would it be possible to add a similar toggle for Musixmatch or is it too much work? Also, and this is really awkward, but I don't exactly understand how the Genius plugin works. How does it decide when to replace the Musixmatch lyrics with the Genius one? And is there any benefit to using Genius over Musixmatch or is it just a fallback for when Musixmatch doesn't have lyrics for any one song? I suspect Musixmatch will soon become more popular due to the amazing integrations with Spoify, YTM, Amazon Music, Google Search, etc.

@Araxeus
Copy link
Collaborator

Araxeus commented Feb 25, 2023

Musixmatch

they have a free api from what I see https:/musixmatch/musixmatch-sdk
but it wasn't updated in 6 years?

EDIT:
actually, the free plan is limited to 2k calls per day https://developer.musixmatch.com/plans and other bs

2k calls is absolutely nothing since this app has alot of downloads

@Zo-Bro-23
Copy link
Contributor

Musixmatch

they have a free api from what I see https:/musixmatch/musixmatch-sdk but it wasn't updated in 6 years?

EDIT: actually, the free plan is limited to 2k calls per day https://developer.musixmatch.com/plans and other bs

2k calls is absolutely nothing since this app has alot of downloads

No, but YTM natively supports Musixmatch. Would it be hard to intercept those API calls? I haven't worked with this before, so I'm really not sure if it's like relatively easy to implement or near impossible.

@DereC4
Copy link
Contributor Author

DereC4 commented Feb 25, 2023

It does seem that in some cases the lyrics are overrided with other sites like Musixmatch and I can't do much in those scenarios, but for the instances where Genius Lyrics is used it works

@DereC4
Copy link
Contributor Author

DereC4 commented Feb 25, 2023

I'll be pushing my updated version soon

@DereC4
Copy link
Contributor Author

DereC4 commented Feb 25, 2023

Alright this is it; take a look at the updated screenshots I've included in the description!

@DereC4 DereC4 requested a review from Araxeus February 26, 2023 04:20
plugins/lyrics-genius/back.js Outdated Show resolved Hide resolved
plugins/lyrics-genius/back.js Outdated Show resolved Hide resolved
plugins/lyrics-genius/back.js Outdated Show resolved Hide resolved
plugins/lyrics-genius/back.js Outdated Show resolved Hide resolved
plugins/lyrics-genius/back.js Outdated Show resolved Hide resolved
@DereC4 DereC4 requested a review from Araxeus February 28, 2023 18:40
plugins/discord/menu.js Outdated Show resolved Hide resolved
plugins/lyrics-genius/back.js Outdated Show resolved Hide resolved
@DereC4 DereC4 requested a review from Araxeus February 28, 2023 21:08
Copy link
Collaborator

@Araxeus Araxeus left a comment

Choose a reason for hiding this comment

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

LGTM (codewise, haven't actually tested the functionality)

plugins/lyrics-genius/menu.js Outdated Show resolved Hide resolved
@DereC4 DereC4 requested a review from Araxeus March 3, 2023 06:10
Copy link
Collaborator

@Araxeus Araxeus left a comment

Choose a reason for hiding this comment

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

@DereC4 The merge conflict should be simple to resolve,
use your incoming changes but replace encodeURI with encodeURIComponent

- `https://genius.com/api/search/multi?per_page=5&q=${encodeURI(queryString)}`
+ `https://genius.com/api/search/multi?per_page=5&q=${encodeURIComponent(queryString)}`

@DereC4
Copy link
Contributor Author

DereC4 commented Mar 9, 2023

@DereC4 The merge conflict should be simple to resolve, use your incoming changes but replace encodeURI with encodeURIComponent

- `https://genius.com/api/search/multi?per_page=5&q=${encodeURI(queryString)}`
+ `https://genius.com/api/search/multi?per_page=5&q=${encodeURIComponent(queryString)}`

will do

@DereC4 DereC4 requested a review from Araxeus March 10, 2023 02:59
@Araxeus
Copy link
Collaborator

Araxeus commented Mar 11, 2023

I meant you should update from the main branch and resolve the merge conflict, not make a new commit 😅

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-on-github

Could you update from master now?
btw I'm now a collaborator so I can merge the PR myself 😉

@DereC4
Copy link
Contributor Author

DereC4 commented Mar 12, 2023

I meant you should update from the main branch and resolve the merge conflict, not make a new commit 😅

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-on-github

Could you update from master now? btw I'm now a collaborator so I can merge the PR myself 😉

ohh

@DereC4 DereC4 requested a review from Araxeus March 12, 2023 18:33
@Araxeus Araxeus merged commit 69cd5ed into th-ch:master Mar 14, 2023
@DereC4
Copy link
Contributor Author

DereC4 commented Mar 29, 2023

Musixmatch

they have a free api from what I see https:/musixmatch/musixmatch-sdk but it wasn't updated in 6 years?
EDIT: actually, the free plan is limited to 2k calls per day https://developer.musixmatch.com/plans and other bs
2k calls is absolutely nothing since this app has alot of downloads

No, but YTM natively supports Musixmatch. Would it be hard to intercept those API calls? I haven't worked with this before, so I'm really not sure if it's like relatively easy to implement or near impossible.

Made some progress #1092 regarding Musixmatch interception

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.

3 participants