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

Added translate function to lyrics #82

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

renatonolo
Copy link

This commit add a translate button to song screen, allowing the users to translate the lyrics.

To this button appear on song screen, you need to enable translate on Settings screen and choose a language to translate.

You can improve this function, creating TranslatePlugins, to get translated songs from another sources..

Please, analyze this and, if you have douts or questions, ask me, then we can solve the problems together.

Thanks.

@emilioastarita
Copy link
Owner

emilioastarita commented Oct 5, 2017

Hello @renatonolo thanks for submiting this PR. I've merged it and resolved conflicts with last master in this branch: https://github.com/emilioastarita/lyricfier/tree/translated-lyrics . I think that needs some work before to be merged on master. Do you want to work on it?

  • Update detectSeries to the same logic that now is using the lyric searcher.
  • Setting option Enable Translate to lyrics has not default on first launch (use Disabled).
  • Would be nice if after pressing Translate the interface scrolls to top (we are doing that on song change)
  • Remove unused vars.
  • If any API_KEY is needed this must be a configurable option for user in Setting. Add a link to the url where our user can obtain it.
  • Translate Lyrics button must not be visible when there is no lyrics yet.

Thanks! If you need any help let me kwnow.

@renatonolo
Copy link
Author

renatonolo commented Oct 5, 2017

Hello man. I'm back.

I saw your suggestions and I liked. Then, I already did the modifications, according your suggestions.
About the API_KEY, I think that the developer of plugin need to add the API_KEY to the code and not the user. For example, in "vagalume" plugin you don't need to put API_KEY, then this variable is empty. What you think?

Can you see and analyze that to merge or not this modifications?

If you have questions, ask me.. 👍
See you...

@emilioastarita
Copy link
Owner

Ok I would check about API_KEY if this is private or not. Usually this could be a problem because multiple users share the same credentials so you hit soon to the rate limits of the APIs. Can you provide a link where I can investigate about TOS and conditions of the provider?

@renatonolo
Copy link
Author

Yeah, you can see the documentation on: https://api.vagalume.com.br/docs/
But it's only on portuguese (PT-BR).. :/

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.

2 participants