Skip to content
This repository has been archived by the owner on Apr 27, 2024. It is now read-only.

Functionality to disable specific lyrics provider #54

Merged
merged 13 commits into from Sep 15, 2022
Merged

Functionality to disable specific lyrics provider #54

merged 13 commits into from Sep 15, 2022

Conversation

SurajBhari
Copy link
Contributor

kinda closes #53

@NyaomiDEV
Copy link
Owner

A couple points to this:

  • Please respect my code formatting. I am using camel case and not snake case. Also there are missing spaces around tokens such as : and =;
  • Your description in the configuration file is rushed; please use proper English and formatting also there;
  • src/main/integrations/lyrics.ts: Imports must not specify .js at the end, as per code style choice;
  • src/main/integrations/lyrics.ts: Please assign Config as close as possible to the checks; there's no reason to get it before as it's not used elsewhere in the function body;
  • src/main/integrations/lyrics.ts: The use of Object.assign is not proper, meaning that you did not test this code. You should assign an object containing the provider if you want to go that route; however, a simpler way to do this could just be to use regular assignations:
      	// Line 26 onwards
      	let providers: any = {}; // marking providers as 'any' avoids TS issues
      	if(Config.lyricsProvider.Musixmatch) // accounting for the first suggestion
      		providers.Musixmatch = Musixmatch;
      	
      	if(Config.lyricsProvider.NetEase)
      		providers.NetEase = NetEase;
      	// and so on...

The PR is not mergeable in this state; please fix all those issues and then I will review it once more.

@SurajBhari
Copy link
Contributor Author

Imports must not specify .js at the end, as per code style choice;
I can see other files using this like seekbar.ts, screen.ts, nowplaying.ts, lyrics.ts, event.ts, buttons.ts, appicon.ts under src/www/lib

@NyaomiDEV
Copy link
Owner

NyaomiDEV commented Sep 15, 2022

seekbar.ts, screen.ts, nowplaying.ts, lyrics.ts, event.ts, buttons.ts, appicon.ts

The lib parts compiles into ES6 modules for use in browsers; what you're modifying compiles into CommonJS for use in the main Electron context.

Please take a look at the review I started.

src/main/integrations/lyrics.ts Outdated Show resolved Hide resolved
src/types.ts Show resolved Hide resolved
assets/config.json5 Outdated Show resolved Hide resolved
assets/config.json5 Outdated Show resolved Hide resolved
src/main/integrations/lyrics.ts Outdated Show resolved Hide resolved
@SurajBhari
Copy link
Contributor Author

me using snakecases as default make sense. cuz I come from Python Background. and we only use snakecase there.

@NyaomiDEV
Copy link
Owner

I want to know if this PR works as advertised in this state...

@SurajBhari
Copy link
Contributor Author

it does. tested and worked.
There can be a little more addition.
like removing the huge "No Lyrics Avaialable" its just something that annoys me.

@NyaomiDEV
Copy link
Owner

it does. tested and worked. There can be a little more addition. like removing the huge "No Lyrics Avaialable" its just something that annoys me.

WHY? It is meant to inform the user that lyrics aren't available!

@NyaomiDEV NyaomiDEV merged commit 0e648c2 into NyaomiDEV:master Sep 15, 2022
@NyaomiDEV
Copy link
Owner

So, IDK if that was my fault or not, but the metadata query code was faulty lol.

Anyway, I fixed it now, and also the list now reorders the services' priority.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Language Mismatch when no lyrics are found.
2 participants