-
Notifications
You must be signed in to change notification settings - Fork 998
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
p/utils: Rewrite install_locale function #1352
base: master
Are you sure you want to change the base?
Conversation
Make the gettext installation platform agnostic and leverage the fact that gettext automatically: * Searches for translations on default system folders * Guesses system language from environment variables Fixes #1321
@DivingDuck @a2k-hanlon if you could please give this a try and confirm it doesn't break Windows / macOS it'd be great. (@neofelis2X you spotted the issue in the first place, please feel free to test this as well. Your case is particularly interesting since your language combination seems more uncommon than ours. In my machine it correctly handles de_AT and it translates to German with no issues) Note that there's a slight behavior change for macOS since now local translation files are preferred over system ones, which was the default behavior for the other platforms. Note that you might need to implement #1351 as well to avoid wx errors. |
I just run the new windows artifact and it don't find the translation files any longer. The should be found under .\locale\ |
Ooops, of course. My bad. Well spotted, could you try with this change see if it finds them correctly now? - local_path = Path('./locale')
+ local_path = Path(".", "locale") Thanks a lot for testing. |
Sorry to be late with the test. This do not to work either. |
Nothing to apologize for :) Sad to hear it doesn't work though, thought I had found a sweet solution. How come the current code: if os.path.exists('./locale'):
translation = gettext.translation(domain, './locale', languages=[lang[0]], fallback= True)
else:
translation = gettext.translation(domain, shared_locale_dir, languages=[lang[0]], fallback= True)
translation.install() works but the code on this PR doesn't? What am I missing here? (Apart from the usual "it works on my machine" :P) |
Honest, I don't know. When I changed it to this solution in the past I tried nearly everything what seems to be possible and nothing worked except this solution. It drove me nuts, because I asked myself exactly the same and at some point I accept that I'm not able to explain it. I think, it have to do with how the path will handled, but I'm really not sure. Do you have a macOS system running as well? Then you maybe can test if we still need this workaround for macOS, because I think the main problem for this solution was that there is no fall back like we have it for Linux and Windows with my old code. For the side note from @neofelis2X regarding locale.getdefaultlocale() deprecated (#1321), I haven't found a solution jet. Something like this:
|
Duh, unfortunately it doesn't fix #1321 :( Maybe I can help with some observations:
I honestly believe it's a bug in wxPyhton. As a start we could ask in their forum. Theres is even a very promising PR, but it was never implemented. |
Ouch, well, it does and it doesn't :P
This analysis you've done here is very useful. Thanks a ton. Two things I take away from it:
|
Well, let's see if we can debug it a little further. Something feels amiss here. Could you run this code on a terminal and post the output? #!/usr/bin/env python3
import pathlib
import locale
import gettext
import os
import sys
print("Detected system locale: ", locale.getlocale())
print("Detected system default locale: ", locale.getdefaultlocale())
path = pathlib.Path('.', 'locale')
print("Local path: ", path)
print("Local path exists? ", path.exists())
DATADIR = os.path.join(sys.prefix, 'share')
shared_path = os.path.join(DATADIR, 'locale')
print("Shared path: ", shared_path)
print("Shared path exists? ", os.path.exists(shared_path))
german_path = path / 'de' / 'LC_MESSAGES' / 'pronterface.mo'
print("Local German translation path: ", german_path)
print("Local German translation exists? ", german_path.exists())
print("gettext finds local translation: ",
gettext.find('pronterface', path, ['de_DE', 'de']))
print("gettext finds shared translation: ",
gettext.find('pronterface', shared_path, ['de_DE', 'de']))
print("gettext finds system translation: ",
gettext.find('pronterface', None, ['de_DE', 'de'])) On my box it outputs the following:
|
Hi, here it is
I would suggest to change:
|
Thanks a lot. I understand from your output here then that:
Good point. Happy with that. I'll push that change. I'm also thinking of pushing this change against the master branch instead since it won't help with #1321 anyway. |
So I've pushed a new proposal, which, IMHO, is even simpler and more elegant. I've tested on my system and it successfully:
I've tested setting my system language to 'fr_FR', 'de_DE', 'de_AT' (which successfully translates to German too) and 'af_ZA' (which, as expected, runs with no translation). Please let me know how it goes on Windows and macOS. I think this time GitHub successfully built the apps so I hope it is an easy test. |
Hi @rockstorm101, |
Ouch hahaha. Sure, take your time, not urgent. Thanks a lot for testing this thing, I know translations a bit of a pain. |
Recently I tested something that involved the locals module. I think I understood now how getdefaultlocale is meant to be replaced. This line... ... is replaced by these two lines: https://docs.python.org/3/library/locale.html#locale.setlocale Idk, maybe you knew this already, but for me it was a realisation. |
Make the gettext installation platform agnostic and leverage the fact that gettext automatically:
Fixes #1321