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

Engine support for ngettext #21

Merged
merged 1 commit into from Aug 26, 2018
Merged

Engine support for ngettext #21

merged 1 commit into from Aug 26, 2018

Conversation

@gunchleoc
Copy link
Contributor

@gunchleoc gunchleoc commented Aug 26, 2018

Added support for translation with correct plural forms to engine.

Some notes:

  1. Having a macro would probably be better for consistency, but I kept getting compiler errors, so I wrote a function instead
  2. I did not commit the changes from running messages.pot.sh to keep the diff small.
  3. I don't know where to find the affected strings on-screen, so that still needs testing.
Added support for translation with correct plural forms to engine.
@acmepjz
Copy link
Owner

@acmepjz acmepjz commented Aug 26, 2018

Hi,

Thank you for the pull request.

Unfortunately, there are some problems need to tackle before merging it:

  • I'm not so familiar with xgettext parsing source strings with plural forms. I'll find some more information later.
  • The tinygettext doesn't have proper plural forms support it only check the Plural-Forms with a list of predefined plural forms. Maybe I should write a expression parser later...
@gunchleoc
Copy link
Contributor Author

@gunchleoc gunchleoc commented Aug 26, 2018

ngettext is a default keyword for xgettext, so no need to do anything in the tools here. This PR doesn't support levelpacks, but there are no potential plural strings in them yet - no need to fix a problem that doesn't need solving.

I have compared the rules for our current locales from http://docs.translatehouse.org/projects/localization-guide/en/latest/l10n/pluralforms.html with the list in plural_forms.cpp - all our currently shipped locales have their plural forms supported.

We should make another pass with the plural headers shipped from Transifex just to be sure, but I don't have access to them at this point - we might actually need to update Transifex with the new .pot file to make them show up.

@acmepjz
Copy link
Owner

@acmepjz acmepjz commented Aug 26, 2018

I have checked the link you have provided, and it looks like that there are some plural forms not included in the predefined list of plural_forms.cpp, for example Irish language. Never mind since we don't have this translation now... maybe later I will do add a simple exression parser to it.

BTW I have filled a issue on tinygettext repository tinygettext/tinygettext#21 and I have got a reply

Expression parser is a waste of time, just hardcode whatever language you want to add and be done with it. Languages don't change frequently enough to make a parser worth the effort.

Anyway 😶

As for the return value type of _ and ngettext, ideally they should accept const std::string& and return std::string, but in the GNU gettext implementation the argument and the return value are all const char*. For the compatibility reason the _ also returns const char* and in order to prevent potential memory leak or use after delete error, the _ have to be a macro, which enables the compiler to manage the object lifetime automatically. Since the ngettext is a function, after merging changes I'd like to change the argument and return types.

@acmepjz acmepjz merged commit 092cdc0 into acmepjz:master Aug 26, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@acmepjz
Copy link
Owner

@acmepjz acmepjz commented Aug 26, 2018

BTW, the de and gd translation files in the master branch are already from Transifex and they have the correct plural headers. On the other hand some existing old translations doesn't have correct plural headers, for example ru translation.

@gunchleoc
Copy link
Contributor Author

@gunchleoc gunchleoc commented Aug 27, 2018

Did you keep a list of the missing plural forms by any chance? I'd be willing to do the work if we have a list.

Irish (ga) is there, but outdated. I double-checked with the CLDR, which is the authoritative resource. They have changed the format from 3 to 5 entries. This can sometimes happen when rules get refined.

Ping me when you have pushed & pulled with Transifex, then I'll have another look to make sure that all the formulas are up to date. We will then need to keep an eye out when adding new locales. Transifex are good about sending e-mails around when they do anything to their plural implementation, so we're safe if they should change anything in the future.

@acmepjz
Copy link
Owner

@acmepjz acmepjz commented Aug 27, 2018

I have already implemented an expression parser 😄 see https://github.com/acmepjz/tinygettext/tree/plural-forms-expression-parser and tinygettext/tinygettext#21

Usually I only push source & zh_CN translation to Transifex, and pull translations other languages.
Do the plural rules on Transifex get updated automatically?

@gunchleoc gunchleoc deleted the gunchleoc:ngettext branch Aug 28, 2018
@acmepjz
Copy link
Owner

@acmepjz acmepjz commented Aug 28, 2018

I don't know where to find the affected strings on-screen, so that still needs testing.

The %d recordings is in the level editor test play screen, the others are the descriptions of undo/redo commands, you can view them when you do the corresponding commands and move the mouse to the undo/redo button.

Did you keep a list of the missing plural forms by any chance? I'd be willing to do the work if we have a list.

The translations currently we have without plural forms are: es, fi, fr, it, nl, ru (which has wrong plural forms).

@gunchleoc
Copy link
Contributor Author

@gunchleoc gunchleoc commented Aug 28, 2018

I have had a quick look at the parser - I think we should drop support for bitwise operators. They can only be illegal in plural form expressions.

Do the plural rules on Transifex get updated automatically?

Yes, so you need to push the .pot to Transifex, then pull the translations. You should get updated headers then.

@acmepjz
Copy link
Owner

@acmepjz acmepjz commented Aug 29, 2018

I have had a quick look at the parser - I think we should drop support for bitwise operators. They can only be illegal in plural form expressions.

OK, I have checked the official GNU Gettext implementation http://git.savannah.gnu.org/cgit/gettext.git/tree/gettext-runtime/intl/plural.y#n132 http://git.savannah.gnu.org/cgit/gettext.git/tree/gettext-runtime/intl/plural-exp.h#n34, and they doesn't support bitwise operators as well. So I'll remove the bitwise operators.

Yes, so you need to push the .pot to Transifex, then pull the translations. You should get updated headers then.

I see. I only pull the translations which currently is translating on Transifex. I will pull all the translations later before the next beta version (or RC version) is released.

@gunchleoc
Copy link
Contributor Author

@gunchleoc gunchleoc commented Aug 29, 2018

You still need to run the pot update tool - I neglected to do so to keep the diff for my PR small.

Then you will need to push the new version to Transifex. Alternatively, you can set up Transifex to automatically pull any pot updates from https://github.com/acmepjz/meandmyshadow/raw/master/data/locale/messages.pot

This is where you will find the autoupdate function:

autoupdate

@acmepjz
Copy link
Owner

@acmepjz acmepjz commented Aug 29, 2018

I have updated the pot locally, but not yet pushed onto Github nor Transifex. I'll do it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants