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

feat(lyrics-plus): add japanese lyrics conversion tool #1990

Merged
merged 37 commits into from
Dec 26, 2022

Conversation

41pha1
Copy link
Contributor

@41pha1 41pha1 commented Oct 22, 2022

This adds the functionality of switching between different display modes for Japanese lyrics.

@41pha1 41pha1 requested a review from a team as a code owner October 22, 2022 14:41
@41pha1 41pha1 requested review from rxri and afonsojramos October 22, 2022 14:41
@theRealPadster
Copy link
Member

Probably should have the dictionaries included in the same repo

@41pha1
Copy link
Contributor Author

41pha1 commented Oct 22, 2022

Probably should have the dictionaries included in the same repo

Alright I included them in the PR now

@41pha1 41pha1 closed this Oct 22, 2022
@41pha1 41pha1 reopened this Oct 22, 2022
CustomApps/lyrics-plus/index.js Outdated Show resolved Hide resolved
CustomApps/lyrics-plus/manifest.json Outdated Show resolved Hide resolved
@rxri rxri changed the title Japanese Lyric Conversion Tools feat(lyrics-plus): add japense lyrics conversation tool Oct 22, 2022
@rxri rxri changed the title feat(lyrics-plus): add japense lyrics conversation tool feat(lyrics-plus): add japenese lyrics conversation tool Oct 22, 2022
@rxri rxri changed the title feat(lyrics-plus): add japenese lyrics conversation tool feat(lyrics-plus): add japenese lyrics conversion tool Oct 22, 2022
@kyrie25 kyrie25 changed the title feat(lyrics-plus): add japenese lyrics conversion tool feat(lyrics-plus): add japanese lyrics conversion tool Oct 22, 2022
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.

This is really amazing work! 😄 Thanks a lot for your contribution.
I just have a few small suggestions regarding this PR.

CustomApps/lyrics-plus/README.md Outdated Show resolved Hide resolved
CustomApps/lyrics-plus/README.md Outdated Show resolved Hide resolved
CustomApps/lyrics-plus/index.js Outdated Show resolved Hide resolved
@CharlieS1103
Copy link
Member

How about we make it so lyrics plus uses his translation or code or whatever initially with just hotloading but if the user installs the dicts locally via an install script then it uses that method instead? So users who don't need the feature don't need the extra 30mb and the users who often use the feature and don't want the load times can install it

@41pha1
Copy link
Contributor Author

41pha1 commented Oct 23, 2022

The dictionaries will now be dynamically loaded if they are not yet present and the conversion menu will show a loading indicator.

If the dicts are somehow added to the client in any other way, it will use them instead and skip the downloading step.

The download urls are in Translator.js line 6050
(I havent come up with a better place than my personal repo yet)

@rxri
Copy link
Member

rxri commented Nov 1, 2022

Could you provide source code for this then which I will upload to repo?

@41pha1
Copy link
Contributor Author

41pha1 commented Nov 1, 2022

Do you mean the code of the modified kuroshiro / analyzer bundle that currently sits along the Translator class in Translator.js?
I sadly don't have separated files anywhere as I exported everything directly to the bundle with npm.

I am not quite sure if I understand correctly :/

@rxri
Copy link
Member

rxri commented Nov 3, 2022

So, you don't have any source-code for this code? We will not be able to update a code then. We can't extract source from webpack compiled code.

@41pha1
Copy link
Contributor Author

41pha1 commented Nov 5, 2022

Oh yea my bad! I added the unminified code and split it into kuromoji.js and kuroshiro.js If wanted we can move them to the separate repo?

@rxri
Copy link
Member

rxri commented Nov 5, 2022

Revert this commit please. I didn't mean unminified code but source code of it. This is still bundled by webpack which is not a source code

@41pha1
Copy link
Contributor Author

41pha1 commented Nov 5, 2022

I am afraid I cant provide you with what you are looking for in that case

@kyrie25
Copy link
Member

kyrie25 commented Nov 5, 2022

I am afraid I cant provide you with what you are looking for in that case

We are asking for the original code in which this was compiled to, if that clarifies anything for you.

Currently what you have committed here is the minified/compiled code from a Webpack build process, of which we cannot inspect, verify and maintain its functionality.

@41pha1
Copy link
Contributor Author

41pha1 commented Nov 5, 2022

We are asking for the original code in which this was compiled to, if that clarifies anything for you.

Currently what you have committed here is the minified/compiled code from a Webpack build process, of which we cannot inspect, verify and maintain its functionality.

I will have to get to work on the files from the Kuroshiro repo again, because I was as naive as to modify the compiled code from npm directly and thus am not and never haven been in the possession of the uncompiled code.

@huhridge
Copy link
Contributor

Is there a possibility of bundling the translate function within Spicetify? So maybe it can be used in other extensions and custom apps, etc?

@kyrie25
Copy link
Member

kyrie25 commented Nov 13, 2022

Is there a possibility of bundling the translate function within Spicetify? So maybe it can be used in other extensions and custom apps, etc?

The extension will be released as a standalone package so feel free to use that instead. We have no plans to bundle it into the CLI, mostly because it has a very small use case, scope of usage and is 10 times larger the CLI itself.

@huhridge
Copy link
Contributor

Yeah, but the problem is importing modules in the extensions and making it work with marketplace, unless that was fixed?

@kyrie25
Copy link
Member

kyrie25 commented Nov 13, 2022

Yeah, but the problem is importing modules in the extensions and making it work with marketplace, unless that was fixed?

That would be discussed on the Marketplace side of development. You can always choose to opt out on having users install via Marketplace and do it locally instead. We are also planning to make an option for developers to redirect users to their repository's Readme instead of installing by default on Marketplace (spicetify/marketplace#346)

Either way, we are not forcing users to increase their installation package size by tenfold for a feature that has probably around 1-2% of the userbase using it.

@Pithaya
Copy link
Contributor

Pithaya commented Dec 2, 2022

In case it helps, I've also been trying to integrate Kuroshiro in a spicetify extension without having to download the dictionary files locally, and got it to work without having to host the files in a repo. You can directly get the dictionary files from npm.
There is an issue with loading external urls however (I guess this is why the library's code had to be edited here to use downloaded dictionary files directly?), so I had to add a fix (found here) to get it working.

The repo is private for now so I can't link the file but here is the code I'm using (the extension is built with spicetify creator):

import Kuroshiro from 'kuroshiro';
import KuromojiAnalyzer from '../../node_modules/kuroshiro-analyzer-kuromoji/dist/kuroshiro-analyzer-kuromoji.min.js';

type ExtendedXMLHttpRequest = XMLHttpRequest & {
    patched_open: (method: string, url: string) => void;
};

const dictPath = 'https://cdn.jsdelivr.net/npm/[email protected]/dict';
const dictPathWithoutDoubleSlash =
    'https:/cdn.jsdelivr.net/npm/[email protected]/dict';
/**
 * Fix an issue with kuromoji when loading dict from external urls
 * See: https://github.com/takuyaa/kuromoji.js/issues/37
 * Adapted from: https://github.com/mobilusoss/textlint-browser-runner/pull/7
 */
export function applyKuromojiFix(): void {
    (XMLHttpRequest.prototype as ExtendedXMLHttpRequest).patched_open =
        XMLHttpRequest.prototype.open;
    XMLHttpRequest.prototype.open = function (method: string, url: string) {
        if (url.indexOf(dictPathWithoutDoubleSlash) === 0) {
            (this as ExtendedXMLHttpRequest).patched_open(
                method,
                url.replace(dictPathWithoutDoubleSlash, dictPath)
            );
        } else {
            (this as ExtendedXMLHttpRequest).patched_open(method, url);
        }
    };
}

export async function createKuroshiro(): Promise<Kuroshiro> {
    applyKuromojiFix();

    const kuroshiro: Kuroshiro = new Kuroshiro();
    const analyzer: KuromojiAnalyzer = new KuromojiAnalyzer({
        dictPath: dictPath,
    });

    await kuroshiro.init(analyzer);

    return kuroshiro;
}

@OverRevvv
Copy link

Thank you for your contribution and this fantastic extension. I'd like to know if we can convert Romanji further into English.

@ohitstom
Copy link
Contributor

i really hope this isnt abandoned lol

@41pha1
Copy link
Contributor Author

41pha1 commented Dec 25, 2022

I think I got it to work by loading kuroshiro, kuromoji and the dictionary files via JSDeliver directly from npm using the XMLHttp fix similar to the one suggested by Pithaya.

If you approve these changes I will look into resolving the merge conflicts

I have also seen a lot request for supporting romanization of more languages like Korean or Russian

@rxri
Copy link
Member

rxri commented Dec 25, 2022

It's okay by me, @41pha1

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.

LGTM

@rxri rxri merged commit 8fb666f into spicetify:master Dec 26, 2022
@Tetrax-10
Copy link
Contributor

@41pha1

In line 114 should that be isJapanese or !isJapanese Because if its a japanese song then the menu should show up right?

image

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.

10 participants