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.

Sign up for GitHub

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

Jump to bottom

[twine] Plural localizations #8390

Closed
wants to merge 2 commits into from
Closed

[twine] Plural localizations #8390

wants to merge 2 commits into from

Conversation

greshilov
Copy link
Contributor

@greshilov greshilov commented Mar 27, 2018 •
edited
Loading

Usage example:

[bookmarks_places]
    en:one = %d bookmark
    en:other = %d bookmarks
    ru:one = %d метка
    ru:few = %d метки
    ru:other = %d меток
    ar = %d من الإشارات المرجعية
    cs = %d záložek
    da = %d bogmærker
...

Generating rules:

  1. Placeholder must be %d.
  2. If string is plural for any language code then all values without plural modificator (:one, :two, ...) automatically transforms to:
    language_code:other = value
  3. For formatters that don't support plural strings value is taken from :other key
  4. If :other key is not set, but :one, :two or any other is present, :other is getting from default language translation. Example:
[bookmarks_places]
    en:one = %d bookmark
    en:other = %d bookmarks
    ru:one = %d метка
    ru:few = %d метки
...
Transforms to:
    ru:other -> %d bookmarks

Please, don't forget set en or en:other .

alexzatsepin
alexzatsepin reviewed Mar 27, 2018
View reviewed changes
android/src/com/mapswithme/maps/bookmarks/Holders.java Outdated
@@ -101,7 +101,7 @@ void setName(@NonNull String name)

void setSize(int size)
{
mSize.setText(mSize.getResources().getString(R.string.bookmarks_places, String.valueOf(size)));
mSize.setText(mSize.getResources().getQuantityString(R.plurals.bookmarks_places, size, String.valueOf(size)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Too long string. The max values is 100 symbols. Please transfer 'String.valueOf(size)' on the next line.

 mSize.setText(mSize.getResources().getQuantityString(R.plurals.bookmarks_places, size, 
                                                      String.valueOf(size)));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

String.valueOf(size) was replaced with size (because of %d usage). Thus, fixed :)

@greshilov greshilov force-pushed the plural-localizations branch from 7e281f9 to 010eb30 Compare March 27, 2018 14:22
@Zverik
Copy link
Contributor

Zverik commented Mar 27, 2018

Меня немного смущает, что ты меняешь код twine. Это означает, что мы не сможем его обновить. Сейчас смотрю форки и тикеты в twine, чтобы узнать, насколько это всё стандартно.

@greshilov
Copy link
Contributor Author

@Zverik я вдохновлялся вот этим issue. Можно форкнуть оригинальный твайн, сделать ветку с изменениями и обновлять его по мере надобности.

Zverik
Zverik suggested changes Mar 29, 2018
View reviewed changes
tools/twine/lib/twine/formatters/abstract.rb Outdated
@@ -143,10 +150,23 @@ def format_key_value(definition, lang)
key_value_pattern % { key: format_key(definition.key.dup), value: format_value(value.dup) }
end

def format_plural(definition, lang)
if definition.is_plural?
Copy link
Contributor

Choose a reason for hiding this comment

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

Сюда передаются уже is_plural?, незачем проверять второй раз. Это даже хуже, потому что возникают вопросы, почему все остальные проверки и вызов format_key_value не перенести.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

tools/twine/lib/twine/formatters/abstract.rb Outdated
if definition.is_plural?
plural_hash = definition.plural_translation_for_lang(lang)
if plural_hash
format_plural_keys(definition.key.dup, plural_hash)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ты вызываешь с двумя параметрами, а в сигнатуре ниже — один.

Copy link
Contributor

Choose a reason for hiding this comment

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

Также, почему в format_key_value вызывается одним способом, а тут — другим?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Сигнатуру починил! Мне кажется проще в данном случае использовать функцию, а не подстановку в паттерн, потому что плюральная строка гораздо сложнее выглядит.
Android:
<string name=""bookmarks_places">%d меток</string>
vs

<plurals name="bookmarks_places">
	  <item quantity="one">%d метка</item>
	  <item quantity="few">%d метки</item>
	  <item quantity="other">%d меток</item>
</plurals>

tools/twine/lib/twine/formatters/abstract.rb
@@ -46,11 +47,11 @@ def set_translation_for_key(key, lang, value)
end
current_definition = TwineDefinition.new(key)
current_section.definitions << current_definition

Copy link
Contributor

Choose a reason for hiding this comment

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

Я понимаю, что неаккуратненько, но давай косяки исходного кода не исправлять, чтобы проще было обновлять?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

tools/twine/lib/twine/twine_file.rb Outdated
@@ -45,11 +50,29 @@ def matches_tags?(tags, include_untagged)

def translation_for_lang(lang)
translation = [lang].flatten.map { |l| @translations[l] }.first

Copy link
Contributor

Choose a reason for hiding this comment

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

Здесь тоже незачем убирать пустые строки.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

tools/twine/lib/twine/twine_file.rb Outdated
end

def add_plural_translation(lang, plural, value)
if PLURAL_KEYS.include? plural
Copy link
Contributor

Choose a reason for hiding this comment

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

Эту проверку нужно перенести ниже, где вызывается метод, и писать Warning, если plural не входит в массив.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

tools/twine/lib/twine/twine_file.rb Outdated
@@ -137,11 +160,12 @@ def read(path)
parsed = true
end
else
match = /^([^=]+)=(.*)$/.match(line)
match = /^([^:=]+):?([^:=]+)=(.*)$/.match(line)
Copy link
Contributor

Choose a reason for hiding this comment

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

Если я правильно понимаю, этот регэксп откусывает по символу от всех ключей. Может, сделать проще?
/^([^:=]+)(?::([^=]+))?=(.*)$/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Спасибо, поправил :)

tools/twine/lib/twine/twine_file.rb Outdated
current_definition.translations[key] = value
# Providing backward compatibility
# for formatters without plural support
if plural_key.empty? || plural_key == 'other'
Copy link
Contributor

Choose a reason for hiding this comment

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

Что-то мне не очень это нравится. В format_definition это не запихнуть?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Можно, но тогда это решение не будет работать для форматеров с переопределенным format_definition. В данном случае мы на этапе создания объекта definition обеспечиваем наличие ключа в @translations, если вдруг пользователь указал en:other, но не указал en.

tools/twine/lib/twine/twine_file.rb Outdated

def plural_translation_for_lang(lang)
if @plural_translations.has_key? lang
@plural_translations[lang].clone
Copy link
Contributor

Choose a reason for hiding this comment

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

Почему .clone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Поменял на .dup :)

tools/twine/lib/twine/output_processor.rb Outdated
@@ -42,6 +42,18 @@ def process(language)
new_definition = definition.dup
new_definition.translations[language] = value

current_lang_plural = definition.plural_translation_for_lang(language)
Copy link
Contributor

Choose a reason for hiding this comment

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

Что-то меня смущает, что выполнение до сюда не дойдёт, если для языка есть только plural. См. выше next unless value.

Copy link
Contributor

Choose a reason for hiding this comment

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

А, понял, other же добавляем безусловно. Странно это. Видимо, нужно потом проверять на is_plural и это значение запихивать в правильный ключ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Даже если только plural, но указан other, то по идее должно дойти благодаря вот этой строчке:

if plural_key.empty? || plural_key == 'other'
    current_definition.translations[key] = value
end

Логика запутанная, конечно, получилась :(

tools/twine/lib/twine/output_processor.rb Outdated
new_definition.plural_translations[language] = {'other' => value }
# When user forget set 'other' key
elsif !current_lang_plural.key? 'other'
new_definition.plural_translations[language]['other'] = value
Copy link
Contributor

Choose a reason for hiding this comment

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

Здесь, видимо, нужен цикл по current_lang_plural. А зачем other исключать?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

current_lang_plural это словарик вида:
{"one"=>"%d bookmark", "other"=>"%d bookmarks"},
Соответственно блок кода:

elsif !current_lang_plural.key? 'other'
    new_definition.plural_translations[language]['other'] = value
end

проверяет, есть ли для данного языка other и если нет, то насильно запихивает value из обычной локализации.

@greshilov greshilov force-pushed the plural-localizations branch 3 times, most recently from b341d14 to 442b5a6 Compare March 30, 2018 09:58
@greshilov greshilov force-pushed the plural-localizations branch from 442b5a6 to c656125 Compare March 30, 2018 10:55
@greshilov greshilov closed this Apr 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zverik Zverik Zverik requested changes

@alexzatsepin alexzatsepin alexzatsepin approved these changes

@vmihaylenko vmihaylenko vmihaylenko approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@greshilov @Zverik @vmihaylenko @alexzatsepin