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

p/utils: Rewrite install_locale function #1352

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

rockstorm101
Copy link
Collaborator

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

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
@rockstorm101
Copy link
Collaborator Author

@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.

@DivingDuck
Copy link
Collaborator

I just run the new windows artifact and it don't find the translation files any longer. The should be found under .\locale\

@rockstorm101
Copy link
Collaborator Author

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.

@DivingDuck
Copy link
Collaborator

Sorry to be late with the test. This do not to work either.

@rockstorm101
Copy link
Collaborator Author

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)

@DivingDuck
Copy link
Collaborator

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:

def install_locale(domain):
    shared_locale_dir = os.path.join(DATADIR, 'locale')
    translation = None
    lang = locale.getdefaultlocale() 
    
    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()

@neofelis2X
Copy link
Contributor

Duh, unfortunately it doesn't fix #1321 :(

Maybe I can help with some observations:
error_cyprus
The error message originates from wxPythons Locals module. It shows up when self.locale = wx.Locale(lang) is initiated with certain languages. Here is a bunch of languages I tested:

#lang = wx.LANGUAGE_ENGLISH_CANADA # no error
X #lang = wx.LANGUAGE_ENGLISH_AUSTRIA # error message

#lang = wx.LANGUAGE_GERMAN_GERMANY # no error      
#lang = wx.LANGUAGE_GERMAN_AUSTRIAN # no error
X #lang = wx.LANGUAGE_GERMAN_LUXEMBOURG # error message

#lang = wx.LANGUAGE_GREEK # no error
#lang = wx.LANGUAGE_GREEK_GREECE # no error
X #lang = wx.LANGUAGE_GREEK_CYPRUS # error message

X #lang = wx.LANGUAGE_GERMAN_BELGIUM # error message
X #lang = wx.LANGUAGE_ENGLISH_BELGIUM # error message
#lang = wx.LANGUAGE_FRENCH_BELGIAN # no error
#lang = wx.LANGUAGE_DUTCH_BELGIAN # no error

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.

@rockstorm101
Copy link
Collaborator Author

Duh, unfortunately it doesn't fix #1321 :(

Ouch, well, it does and it doesn't :P

The error message originates from wxPythons Locals module. It shows up when [...] is initiated with certain languages.

This analysis you've done here is very useful. Thanks a ton. Two things I take away from it:

  1. I'm finally (yey! \o/) able to trigger that message. However it occurs on different language combinations from those you listed (see below). But, I suspected it had to do with the language package installed on my system. As you see I got the error on all Greek, French, Hebrew, Chinese,... (for which I don't have a language package installed on my system) but no errors for any German or English combinations (which I had installed). Uninstalling German and installing French made the error appear on German combinations but not on French ones. So it works as expected on my system I guess? The fact that it doesn't work right for you makes me think that a) a bug in wxPython or b) a bug in macOS or c) you don't have your system languages correctly configured (correctly as in the way wxPython expects them to be).

  2. The error is not related to printrun.utils.install_locale function. Therefore this PR could be withdrawn. However it'd be great to clean up that bit of code anyway.

        #lang = wx.LANGUAGE_ENGLISH_CANADA # no error
        #lang = wx.LANGUAGE_ENGLISH_AUSTRIA # no error

        #lang = wx.LANGUAGE_GERMAN # no error      
        #lang = wx.LANGUAGE_GERMAN_GERMANY # no error      
        #lang = wx.LANGUAGE_GERMAN_AUSTRIAN # no error
        #lang = wx.LANGUAGE_GERMAN_LUXEMBOURG # no error

        #lang = wx.LANGUAGE_GREEK # error message
        #lang = wx.LANGUAGE_GREEK_GREECE # error message
        #lang = wx.LANGUAGE_GREEK_CYPRUS # error message
        
        #lang = wx.LANGUAGE_GERMAN_BELGIUM # no error
        #lang = wx.LANGUAGE_ENGLISH_BELGIUM # no error
        #lang = wx.LANGUAGE_FRENCH_BELGIAN # error message
        #lang = wx.LANGUAGE_DUTCH_BELGIAN # error message

        #lang = wx.LANGUAGE_FRENCH # error message
        #lang = wx.LANGUAGE_HEBREW # error message
        #lang = wx.LANGUAGE_CHINESE # error message

@rockstorm101
Copy link
Collaborator Author

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.

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:

$ python3 test.py 
Detected system locale:  ('en_US', 'UTF-8')
[... deprecation warning ...]
Detected system default locale:  ('en_US', 'UTF-8')
Local path:  locale
Local path exists?  True
Shared path:  /usr/share/locale
Shared path exists?  True
Local German translation path:  locale/de/LC_MESSAGES/pronterface.mo
Local German translation exists?  True
gettext finds local translation:  locale/de/LC_MESSAGES/pronterface.mo
gettext finds shared translation:  None
gettext finds system translation:  None

@DivingDuck
Copy link
Collaborator

Hi, here it is

Detected system locale:  ('de_DE', 'cp1252')
Detected system default locale:  ('de_DE', 'cp1252')
Local path:  locale
Local path exists?  True
Shared path:  C:\Users\Armin\Source\Repos\DivingDuck\Printrun\v3\share\locale
Shared path exists?  False
Local German translation path:  locale\de\LC_MESSAGES\pronterface.mo
Local German translation exists?  True
gettext finds local translation:  locale\de\LC_MESSAGES\pronterface.mo
gettext finds shared translation:  None
gettext finds system translation:  None
Press any key to continue . . .

I would suggest to change:

path = pathlib.Path('.', 'locale')

path = pathlib.Path.cwd().joinpath(".", "locale")
for accessing the actual working directory

Detected system locale:  ('de_DE', 'cp1252')
Detected system default locale:  ('de_DE', 'cp1252')
Local path:  C:\Users\Armin\Source\Repos\DivingDuck\Printrun\locale
Local path exists?  True
Shared path:  C:\Users\Armin\Source\Repos\DivingDuck\Printrun\v3\share\locale
Shared path exists?  False
Local German translation path:  C:\Users\Armin\Source\Repos\DivingDuck\Printrun\locale\de\LC_MESSAGES\pronterface.mo
Local German translation exists?  True
gettext finds local translation:  C:\Users\Armin\Source\Repos\DivingDuck\Printrun\locale\de\LC_MESSAGES\pronterface.mo
gettext finds shared translation:  None
gettext finds system translation:  None
Press any key to continue . . .

@rockstorm101
Copy link
Collaborator Author

rockstorm101 commented May 12, 2023

Hi, here it is

Thanks a lot. I understand from your output here then that:

  1. The local translation files are correctly found by gettext. Therefore, the "new" code (meaning the one suggested by this PR) should work and find the local translation files first. Does it not?

  2. The folder constructed with os.path.join(DATADIR, 'locale') does not exist on Windows systems. (It doesn't exist on my Linux either). In the "old" code (current one in 2.0.0 release), in the case of Windows, the shared_locale_dir had no effect?

I would suggest to change:

path = pathlib.Path('.', 'locale')

path = pathlib.Path.cwd().joinpath(".", "locale")

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.

@rockstorm101
Copy link
Collaborator Author

rockstorm101 commented May 14, 2023

So I've pushed a new proposal, which, IMHO, is even simpler and more elegant. I've tested on my system and it successfully:

  1. Finds local translations at ./locale/fr/LC_MESSAGES/pronterface.mo.
  2. If I remove those, it finds them at system directory /usr/share/locale/fr/LC_MESSAGES/pronterface.mo.
    • That is the directory that was previously defined by variable shared_locale_dir. Therefore this definition is redundant, at least for Debian/Ubuntu systems. My understanding is that it points to a non-existent place on Windows, so no need to define it for Windows either? Happy to add it back if required.
  3. If I remove the system '.mo' file then it successfully runs with no translation at all.
  4. In all cases gettext detects my defined system language without having to specify it via languages variable.

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.

@DivingDuck
Copy link
Collaborator

Hi @rockstorm101,
sorry to say it still do not find the translation on Windows. I try looking tomorrow on this.

@rockstorm101
Copy link
Collaborator Author

sorry to say it still do not find the translation on Windows. I try looking tomorrow on this.

Ouch hahaha. Sure, take your time, not urgent. Thanks a lot for testing this thing, I know translations a bit of a pain.

Base automatically changed from patches to master May 29, 2023 14:56
@neofelis2X
Copy link
Contributor

regarding locale.getdefaultlocale() deprecated (#1321), I haven't found a solution jet.

Recently I tested something that involved the locals module. I think I understood now how getdefaultlocale is meant to be replaced.

This line...
lang = locale.getdefaultlocale()

... is replaced by these two lines:
locale.setlocale(locale.LC_ALL, '') »This sets the locale for all categories to the user’s default setting...«
lang = locale.getlocale()

https://docs.python.org/3/library/locale.html#locale.setlocale

Idk, maybe you knew this already, but for me it was a realisation.

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.

Can't set locale Macbook with macOS 12.6.3
3 participants