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

Add tooltips to playback controls #352

Merged
merged 2 commits into from
Oct 27, 2021
Merged

Add tooltips to playback controls #352

merged 2 commits into from
Oct 27, 2021

Conversation

rmg-x
Copy link
Contributor

@rmg-x rmg-x commented Oct 24, 2021

Small change to add tool tips to the playback controls.

@xou816
Copy link
Owner

xou816 commented Oct 25, 2021

Hi there, thanks for the PR! Quick review from my phone: I think the tooltips for play and pause are not translated?

@rmg-x
Copy link
Contributor Author

rmg-x commented Oct 25, 2021

That's correct, I guess I should've asked before adding the "translatable" attribute.

Does text like this need to support a certain set of languages (other than English) before being merged, if any?
If not, should the translatable attribute be removed?

@rmg-x
Copy link
Contributor Author

rmg-x commented Oct 25, 2021

Oh, and I forgot to ask about the code portion of the text generation (which I think you were referring to originally):
What method/pattern is typically used to get translated text via the code if applicable?

@xou816
Copy link
Owner

xou816 commented Oct 25, 2021

This project uses GNU gettext; anytime you need a string from source code to be translated, wrap it in a call to the gettext macro or fn, you'll see multiple instances of it throughout the code.

Then you can run the meson commands mentioned in the README to extract these strings (as well as the strings marked as translatable in the ui files): they will be added to all currently supported languages but will be blank initially and will fallback English, but people will be able to contribute translations from there :)

@rmg-x
Copy link
Contributor Author

rmg-x commented Oct 26, 2021

Ah okay, that makes sense, thanks!
I've made some code changes and included screenshots below. Let me know if everything looks alright.

image
image
image

@xou816
Copy link
Owner

xou816 commented Oct 27, 2021

LGTM, thanks for your work, merging! :)

@xou816 xou816 merged commit 200498b into xou816:master Oct 27, 2021
@xou816
Copy link
Owner

xou816 commented Oct 27, 2021

On second thought, I merged this a bit fast, maybe the tooltip for the Repeat button could use some improvement since it switches between three different "modes", but no big deal obviously :)

@rmg-x
Copy link
Contributor Author

rmg-x commented Oct 27, 2021

Ah, good point. I completely forgot about that as I don't use that button often.
It looks like the current functionality Spotify has is to display the next "state" in the tooltip text.

Example:

  • Disabled -> "Enable repeat"
  • Enabled -> "Enable repeat one"
  • Repeat(1) -> "Disable repeat"

If I find some more time, I will open a PR if someone else hasn't already.

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

2 participants