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 Localization #2202

Closed
wants to merge 194 commits into from
Closed

i18n Localization #2202

wants to merge 194 commits into from

Conversation

blitzmann
Copy link
Collaborator

@blitzmann blitzmann commented Jun 20, 2020

This PR is to show and discuss current efforts to support basic localization in pyfa.

We will only support languages that the official EVE client supports (it would be weird to have Klingon in the UI but everything in the EVE database is English 😀)

Feel free to contribute to this branch after brushing up on the latest comments to avoid duplication of effort.

Couple of notes:

  • The translations themselves aren't a high priority due to the chance of source strings changing during development. This PR is focused on getting core support for translations in pyfa, and the translations can come later. That being said, we will of course accept PRs with translations, just know that they may need to be re-done as this codebase progresses.
  • At this time we're not looking to get full localization support (for example, 1,234 vs 1.234, date formats, etc). We're focused on GUI text translations. But again, if you know how to do proper localization for digits, etc, PRs are welcome!

TODOs:

  • Complete GUI annotations (_t("")) (also need to switch current _() to _t(), see i18n Localization #2202 (comment) for details DONE)
  • Work out a way for accessing locale settings as soon as pyfa startup (needed for eos initialization)
  • Set eos language based on user's saved preference
  • Database translations. Need input from @DarkFenX on best way to do this since I've been in a hole for the past year and things have changed 😁
    • Phobos supports dumping translations with the data. We'll then need to load eos with the needed translation string and probably alias the translated properties in SQLAlchemy so that we automatically pull the correct column.
    • To expose the language to eos, we'll probably just write eos.config.lang = <lang> in pyfa.py once we get that info.
  • GUI stuff to choose between translations
  • (maybe) First time start up to have the user choose language (and default to user's current language if supported)
  • Some script for automatically generate *.mo and packaging translations. Unsure what we can do here that's cross-platform and not environment-bound. Open to suggestions (@zhaoweny suggests "I think pyfa should rely on shell script for this, because even Git for Windows has required msgfmt, and python's implementation msgfmt.py is hard to detect (e.g. a virtualenv might not have this)")
  • Windows / Mac builds: run translations automatically for each build. I assume we would only want to maintain the uncompiled .po, and then have the build scripts deal with the compiled .mo. This means that folks running from source would have to compile them themselves if they want localization...
  • Ensure helper scripts still work

Bugs:

blitzmann and others added 16 commits May 15, 2020 21:55
# Conflicts:
#	gui/esiFittings.py
1. update `Pyfa_Inspections.xml` to ignore `_`

i18n module `gettext` will do a `gettext.install(...)` to put `_` function into `builtin` module. Currently PyCharm does not recognize this as a function and report as unresolved reference.

2. reformat code to remove padding for vertical dicts

3. update `Pyfa_CodeStyle.xml` to not pad vertical dicts

(cherry picked from commit e5a570a)
@blitzmann blitzmann changed the title I18n Localization i18n Localization Jun 20, 2020
@zhaoweny
Copy link
Contributor

I got a exception when I try to set character skill level in character editor:

pyfa v2.22.1
EVE Data Version: 1751741 (2020-06-18 18:35:35)

OS version: Windows-10-10.0.19041-SP0
Python version: 3.8.0 (tags/v3.8.0:fa919fd, Oct 14 2019, 19:37:50) [MSC v.1916 64 bit (AMD64)]
wxPython version: 4.1.0 (wxWidgets 3.1.4)
SQLAlchemy version: 1.3.17
Logbook version: 1.5.3
Requests version: 2.23.0
Dateutil version: 2.8.1

####################

Traceback (most recent call last):
  File "D:\src\pyfa-org\Pyfa\gui\characterEditor.py", line 665, in changeLevel
    _setTreeSkillLevel(grand, skillID)
  File "D:\src\pyfa-org\Pyfa\gui\characterEditor.py", line 649, in _setTreeSkillLevel
    _("Level {}").format(int(lvl)) if not isinstance(lvl, str) else lvl)
TypeError: 'str' object is not callable

It seems _ = wx.GetTranslation was overrided. Maybe pyfa should use something else for i18n. How about _t = wx.GetTranslation?

During the translation process, There are situation that pyfa should mark a string literal for translation, and do the actual translation later (citing gui/patternEditor.py:184). Maybe pyfa can use _r for GetRaw?

@blitzmann
Copy link
Collaborator Author

I'll have to check to see what you mean about _ getting overridden and using some GetRaw when I get to a computer. Its not immediately clear at the moment

@blitzmann
Copy link
Collaborator Author

blitzmann commented Jun 20, 2020

Ahh, I see the string literal error now. it's due to this:

image

We set the first return value to _ since we don't need it (it's a throw-away). But, since _ is defined globally for the file, this overwrites it for the entire file instead of just the scope.

Hrm... part of me wants to continue to use _ for translations since that's the standard way, but another part of me dreads hunting down all the throw-away _ variables and changing them to something like __.

I think your proposal of using _t() would be a good middle ground 😊 We would want to change everything over to that before we continue with annotations, just so that we can get it out of the way 😀

Still unsure about your proposal on _r(), researching...

@zhaoweny
Copy link
Contributor

@blitzmann for example, say we have these variable for i18n: name, direction

btn.SetToolTip(_("%s patterns %s clipboard") % (name, direction))
btn.Bind(wx.EVT_BUTTON, getattr(self, "{}Patterns".format(name.lower())))

so line 1 both variable should be localized, but name can't be localized on line 2 or it will cause runtime error.

@blitzmann
Copy link
Collaborator Author

blitzmann commented Jun 20, 2020

Aha, I see what you're saying... Hrm... does wxPython have a wx.GetRawString function for translated text?

I'm wondering for those situations if it's better to just add a identifier for these. The way that it's done currently is simply used as a shortcut, but we can do something like

 importExport = ((_("Import"), wx.ART_FILE_OPEN, _("from"), "import"),
                        (_("Export"), wx.ART_FILE_SAVE_AS, _("to"), "export"))

        for name, art, direction, attr_name in importExport:
            ...
            btn.SetToolTip(_("%s patterns %s clipboard") % (name, direction))
            btn.Bind(wx.EVT_BUTTON, getattr(self, "{}Patterns".format(attr_name)))

I think this would be a cleaner solution than to try to manage getting a raw value (unless you think otherwise?)

Nice catch, btw! I bet I screwed up some of the ones I did then. This branch is going to require pretty extensive testing lol

@blitzmann
Copy link
Collaborator Author

blitzmann commented Jun 20, 2020

Or hell, we could just get rid of some of the cruft in the middle:

 importExport = ((_("Import patterns from clipboard"), wx.ART_FILE_OPEN, "import"),
                        (_("Export patterns to clipboard"), wx.ART_FILE_SAVE_AS, "export"))

        for text, art, attr_name in importExport:
            ...
            btn.SetToolTip(text)
            btn.Bind(wx.EVT_BUTTON, getattr(self, "{}Patterns".format(attr_name)))

Let me know which feels more proper to you :) I'm down with any which way

@zhaoweny
Copy link
Contributor

zhaoweny commented Jun 20, 2020

The second one, with full tool tip as a whole string is better for translation. Translated strings would be more natural to read comparing using place holder everywhere :).

I agree it would be better if we can have all literal strings translated before passing around, though it means that source code will require a little more modification.

@blitzmann
Copy link
Collaborator Author

@zhaoweny agreed! :)

@blitzmann
Copy link
Collaborator Author

Noticed a bug:

image

Not Learned shouldn't be prefixed with Level. Will add it to OP as a list of bugs associated with this PR

…new `lang.pot`

Because in-tree virtualenv folder is developer-defined, we can't really predict and provide specific commands for excluding virtualenv.
@blitzmann
Copy link
Collaborator Author

I'm going to have to open a bug report on wxPython's repo, the Discuss site isn't generating any replies. I'm not sure if we can launch this feature with random languages popping up. This can't be the only project that has run into this issue either. :/

@zhaoweny
Copy link
Contributor

zhaoweny commented Aug 1, 2020

I think it's fine. English version of pyfa is still there, the translation is opt-in, there is a indicator of how complete the translation is. They will switch back to English if translation not satisfy their need. 😄

@zhaoweny
Copy link
Contributor

zhaoweny commented Aug 2, 2020

Should we initialize all existing translation with English text? There is a tool called msgen for that.

Edit: the document for wx.GetTranslation suggests wxpython will try EVERY possible translation before falling back to source string.

If the string is not found in any of the loaded message catalogs (see Internationalization ), the original string is returned.

@blitzmann
Copy link
Collaborator Author

If the string is not found in any of the loaded message catalogs (see Internationalization ), the original string is returned.

I noticed that as well in my search, but I find it hard to believe that's what most programs would tolerate. If you set language to Russian, and it doesn't find it, I would think the vast majority of applications would want to see the source string, and not some other random language. So that leads me to think that there may be some technique that others have done to avoid that issue (like loading a very specific catalog and then ignoring the rest, but then I don't know how we would be able to dynamically generate the list of available).

As for pre-loading them with English, that's absolutely something that we can probably do. I'm not exactly sure how Crowdin would work with that tho. I guess if we use it on the .pot file, that would default all the msgstr to the English version (assuming). Crowdin might get confused those and think those are translations, thus the % complete wouldn't be correct. That's something we could try tho, and if that's the case then we can switch to showing percentage approved instead and consider "approved" strings to be translated

As for releasing with this issue, I'm just generally iffy with the idea. I get that it's opt-in, but I feel like if folks opt in and the translation isn't there, they would expect to see English and that should be fine with them- they understand it's incomplete. But if they start seeing their language along with 3 or 4 other random languages, they may think it's completely broken and not bother with it in the future. It's a perception thing.

I saw we try the msgen route and go from there to see if the issue persists and how Crowdin deals with it

I18n: more string annotations (mostly tooltips)
@zhaoweny
Copy link
Contributor

zhaoweny commented Aug 2, 2020

How about make it a step in the build? We don't have to commit these not-translated-at-all translations into pyfa's code base. 🤔

@blitzmann
Copy link
Collaborator Author

Well that would make too much sense! 😋

Good idea though, that would solve everything I think. I'll do some research tonight and try to get something worked out for the builds. 👍

@blitzmann
Copy link
Collaborator Author

I have it working with appveyor builds, works pretty well 😁 Now I've got to get it to work with the travis builds.

Not really sure what to do about linux. This is using gettext tools directly, not something that's been converted to python. So linux folks will have to make sure to download gettext and run msgen against the po files, and then run the mo conversion

# Conflicts:
#	staticdata/fsd_binary/typedogma.json
#	staticdata/fsd_lite/evetypes.json
#	staticdata/phobos/metadata.0.json
#	staticdata/phobos/traits.json
@blitzmann
Copy link
Collaborator Author

kk, everything should be fairly situated now. Last thing I have to do is ensure all our helper scripts still function correctly and poke DarkFenX for a quick rundown on the changes. But I think we're in a good spot now that we've got that empty translation thing figured out,

@blitzmann blitzmann marked this pull request as ready for review August 18, 2020 01:33
@zhaoweny
Copy link
Contributor

@blitzmann @DarkFenX is there anything I can help to get this PR merged?

@blitzmann
Copy link
Collaborator Author

I poked @DarkFenX last week to take a look as I think it's ready. As this PR changes how static files are generated and he's the one that's been doing updates, I would like for him to sign off on it. I actually haven't checked slack since then (and for some reason it just simply refuses to auto start when I login to the desktop 😣), and I've been busy with other projects recently.

I'll check it tonight and try to keep it moving. I'm hoping we can get it merged in this weekend, and maybe a new pre-release out as well :)

@DarkFenX
Copy link
Member

DarkFenX commented Sep 9, 2020

FYI i got past huge workload I had at work. Will check this PR and how localization works in general after today's pyfa releases.

@zhaoweny
Copy link
Contributor

zhaoweny commented Sep 9, 2020

@DarkFenX great 😄 . Happy to answer any question you might have :).

@blitzmann
Copy link
Collaborator Author

@DarkFenX don't remember if I ever gave you this, but to dump the data I've been running:

dump_data.py --eve E:\EVE --json ..\staticdata -p C:\path\to\Phobos

Should help in checking it out. It imports phobos files and uses a custom class to split the filess, avoiding the filesize limitations on Github.

Other than that, most of the codebase changes are simply GUI annotations. I added some logic that switches which columns are used in the database based on the language selected that you may be more interested in.

@DarkFenX
Copy link
Member

FYI yesterday I've looked into changes to phobos and adjusted them, so that:

  • by default, no grouping is used
  • maps are sorted before splitting them into groups to reduce diffs between staticdata updates

I grabbed EVE client data dump today using --group=10000, all files are sub-50M. So, that's all for the phobos part, now I need to go through pyfa part - see what the changes are in db generation and UI are, then we should be able to make a release.

@zhaoweny
Copy link
Contributor

Ping 😄 .

I understand there might be something else that need to be taken care of other than Pyfa. If there is anything I can do to help this PR go through, please let me know.

@DarkFenX
Copy link
Member

I am looking into it now. Want to combine sisi release with localization as another beta release

@DarkFenX
Copy link
Member

DarkFenX commented Oct 22, 2020

FYI i am in process of merging i18n and singularity branches. There were quite a few conflicts, I am yet to resolve gui/builtinContextMenus/envEffectAdd.py. Also, some message IDs changed (the text in the code), some were added during the merge, I am yet to figure out what else has to be changed to accomodate to that. Hopefully will make a release tomorrow, if everything works alright.

@DarkFenX
Copy link
Member

DarkFenX commented Oct 23, 2020

I merged it manually into singularity branch. It works at 1st glance, but i will thoroughly test it today and tomorrow, and add latest sisi updates as well. Noticed a few quirks which look strange.

@DarkFenX DarkFenX closed this Oct 23, 2020
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

4 participants