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

i18n/ru: Added Russian language #740

Merged
merged 2 commits into from
Aug 30, 2020
Merged

i18n/ru: Added Russian language #740

merged 2 commits into from
Aug 30, 2020

Conversation

tesla1856
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Aug 21, 2020

Codecov Report

Merging #740 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #740   +/-   ##
=======================================
  Coverage   88.47%   88.47%           
=======================================
  Files         457      457           
  Lines       15031    15031           
  Branches     1317     1317           
=======================================
  Hits        13298    13298           
  Misses       1649     1649           
  Partials       84       84           

Copy link
Member

@bastimeyer bastimeyer left a comment

Choose a reason for hiding this comment

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

Thank you very much for the PR! I know translating is a lot of work, so this is as always very much appreciated. 👍🏻

Before I comment on the changes, I just quickly wanted to mention that there's been another PR (#733) from two months ago with added Russian translations, but it's incomplete and hasn't been finished and also has some invalid YAML which I didn't want to fix on my own due to its incompleteness. If I'm going to merge your translations, this will unfortunately mean that I'll have to drop the work of @dEN5-tech's pull request. 😞


Since your changes seem to be complete, I'll give it a quick review. I can't comment on the translations themselves because I don't speak Russian, so all I can do here is check for formatting errors, missing stuff and how well the text rendering fits in the GUI's components. Once everything has been resolved, I'll keep the PR open for a bit for potential review. Btw, if there's anything unclear due to missing context, please ask.

  • It looks like you've added/kept the English translations as a comment. The other translation files don't have that, but I'm okay with this.
  • After building the app with the new translations, I've noticed that some translations are too long and break the GUI's layout at certain places. One of them is the settings menu's submenu and a couple of rows of individual settings titles are a bit too long as well. There also seems to be a layout issue with some checkboxes, but I think that I can fix this in the component's stylesheets instead. The individual settings titles should also be fixable. About the submenu, I'm not so sure. I've had similar issues with the German translations and had to choose different wording to make it fit. A fix would mean a rewrite of the submenu's layout. I'll have to figure out what's the best solution here. If you could find shorter translations for the settings submenu, that would be ideal for now.

Comment on lines +8 to +12
codes:
ArrowLeft: Влево #Left
ArrowRight: Вправо #Right
ArrowUp: Вверх #Up
ArrowDown: Вниз #Down
Copy link
Member

Choose a reason for hiding this comment

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

If you feel that it's necessary, you could add more translations here, as these translation keys are dynamic. See the translations wiki or the de locale's hotkeys translations as an example:
https://github.com/streamlink/streamlink-twitch-gui/wiki/Translating#28-hotkeys

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The arrow buttons do not have an English name on the keyboard in Russia, so I decided to translate them. The rest of the keys are named and they match the US.

Comment on lines 1 to 17
twitch:
channel:
followers: "Подписчиков: {{count}}"
#one: "Один человек подписан" #"One person is following"
#other: "Подписчиков: {{count}}" #"{{count}} people are following"
views: "Просмотров: {{count}}"
#one: "Один просмотр" #"One channel view"
#other: "Просмотров: {{count}}" #"{{count}} channel views"
stream:
created-at:
# new Moment(stream.created_at).format(...)
less-than-24h: "[Запущен в] LTS" #"[Online since] LTS"
# new Moment(stream.created_at).format(...)
more-than-24h: "[Запущен в] llll" #"[Online since] llll"
viewers: "Зрителей: {{count}}"
#one: "Один зритель" #"One person is watching"
#other: "Зрителей: {{count}}" #"{{count}} people are watching"
Copy link
Member

Choose a reason for hiding this comment

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

The yarn run grunt webpack:i18n task says that these translations are missing

>> ru: models.twitch.channel.followers.one
>> ru: models.twitch.channel.followers.other
>> ru: models.twitch.channel.views.one
>> ru: models.twitch.channel.views.other
>> ru: models.twitch.stream.viewers.one
>> ru: models.twitch.stream.viewers.other

and it looks like you've commented out the translation keys. Did you do that because of the pluralization? Russian has different pluralization rules, right? See here:
https://github.com/streamlink/streamlink-twitch-gui/wiki/Translating#26-pluralization

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it looks like a bug in the program.

First I tried to translate like this:

twitch:
    channel:
        followers:
            one: "Один человек подписан" #"One person is following"
            other: "Подписчиков: {{count}}" #"{{count}} people are following"
        views:
            one: "Один просмотр" #"One channel view"
            other: "Просмотров: {{count}}" #"{{count}} channel views"
    stream:
        created-at:
            # new Moment(stream.created_at).format(...)
            less-than-24h: "[Запущен в] LTS" #"[Online since] LTS"
            # new Moment(stream.created_at).format(...)
            more-than-24h: "[Запущен в] llll" #"[Online since] llll"
        viewers:
            one: "Один зритель" #"One person is watching"
            other: "Зрителей: {{count}}" #"{{count}} people are watching"

But, I saw this error:
image

I decided to add the values ​​that the program required. But, I had to comment out these:

>> ru: models.twitch.channel.followers.one
>> ru: models.twitch.channel.followers.other
>> ru: models.twitch.channel.views.one
>> ru: models.twitch.channel.views.other
>> ru: models.twitch.stream.viewers.one
>> ru: models.twitch.stream.viewers.other

You can leave only those values ​​that are really required by the program.

Copy link
Member

Choose a reason for hiding this comment

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

But, I saw this error:

When there's a {{count}} variable, it needs all the translation/inflection keys of your locale's pluralization rules. For ru, that would be one, few, many and other, and according to the rules, it's looking for many when the value is 900:
https://github.com/jamesarosen/ember-i18n/blob/v5.2.0/addon/config/ru.js

The "missing translation key" error is unfortunately a bit misleading in this case, because it doesn't include the inflection key name.

The reason why it's working when not using any inflection keys is that it has a fallback mechanic for that, and it tries to look up the static translation key (needs to be a simple string):
https://github.com/jamesarosen/ember-i18n/blob/v5.2.0/addon/utils/locale.js#L76-L83

If it works for all numbers, then using that fallback should be fine. The webpack:i18n analyzer task doesn't understand this though, and I'll need to implement this exceptional case. But since it's just a warning and not an error, you don't have to worry about that.

This however means that you should take a look at all the other translations with {{count}} variables, too. Either add all the inflection keys, or make the translation static. Please add a note/comment above the translation key when making it static, thank you.

@bastimeyer
Copy link
Member

Looking good now, thanks!

As said, I'll leave this open for a bit for potential reviews. The remaining minor layout issues will be fixed on the master branch after merging (or prior to that if, we'll see). If you have further improvements, just push them to the PR.

Once again thank you for taking the time and submitting the translations!

@dEN5-tech
Copy link

( well it's good that the translation was completed)

@bastimeyer bastimeyer merged commit f72ee78 into streamlink:master Aug 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants