-
Notifications
You must be signed in to change notification settings - Fork 514
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
New translation file loading #1795
Conversation
The log messages that output the LANG and LOCALE environment variables were using `QLocale().system().name()` as a fallback if the variable is not found. But we'd want to know if they're unset, so default to "".
PR OpenShot#1332 sought to fix loading of English translations in non-English locales (see OpenShot#1256) by removing a hardcoded `en_US` bypass in the translation file loading. In doing so, it inadvertently broke loading of translation files that match the system locale. This removes the entire "skip loading" clause, because we can never assume that any translation shouldn't be loaded. English translations may need to be loaded and applied, if OpenShot's language is set to English in non-English locales. In contrast, we still need to load our OpenShot translations for the local language even if the selected language is the same as the system locale.
So I have noticed something. Currently (with v2.4.2), if I have set my OpenShot language to 'Default' but have anything other than en_US as my system's default language, OpenShot isn't respecting that but using en_US. But if I myself change the language to be anything particular, it respects that. With this branch, if I set my OpenShot language to 'default' and have my default language (in the the system to be, say ne_NP, for instance) it respects the default language choice. +1 for this PR Thought this would be necessary info. :) |
That is interesting. That may mean that this code, from
# Don't try on default locale, since it fails to load what is the default language
if 'en_US' in locale_name:
continue I thought "current locale" (in the function name) meant the system locale, but if it means the locale being loaded, then there's the same issue that I corrected with the previous change: Even though en_US is the "default language", we may still need to activate it if OpenShot is actually set to English in a non-English locale. However, it's true that loading an en_US translation file isn't technically possible, simply because there isn't one. That's the sense in which it's the "default locale", as that comment says. The intent of the original code, AFAICT, was to avoid loading any translations when en_US was the interface language selected for OpenShot. It just didn't work right, and then the fix made things even worse. CORRECTLY preventing the loading of any translations might work, but since I think Qt also auto-applies its own translations for the system locale, even that may not suffice. Perhaps what we need to do is special-case ETA: This turned out to be a red herring, as I discovered that |
The other option, in thinking about it, would be to just provide a translation file for en_US with OpenShot. It'd be simple to generate, since it would just involve translating every string into itself. But then we could actually load that file when en_US is selected. Anyway, I've asked the Qt forum about querying installed translators. |
This completely reworks the on-disk model for translation files, doing away with the `lang_code/LC_MESSAGES/` path hierarchy entirely in favor of a flat directory structure containing files named with the pattern "OpenShot.lang_code.qm". This allows us to take advantage of QTranslator's fallback code that will, for instance, automatically load `OpenShot.fr.qm` when `OpenShot.fr_ZZ.qm` is not found. The translation files are also indexed in a QResource file named `openshot_lang.qrc`, which is compiled into `openshot_lang.py`. This allows the translations to be loaded from the resource path ":/locale". The translations are stored in the `language/` subdirectory, instead of `locale/`, because `from locale import openshot_lang` conflicts with the Python module named `locale`.
This modifies the translation-loading code to make use of the ":/locale" resource path for OpenShot translations, and greatly simplifies the loading code by making use of Qt's automatic fallback in `QTranslator.load()` instead of parsing the pieces of the locale string directly. The list of supported languages is now built using a QDirIterator over the ":/locale" resource path.
Instead of all the complicated nonsense `get_current_locale()` was performing, have it just return `info.CURRENT_LANGUAGE`. It was only ever being called in one place, `src/classes/metrics.py`, and doing a lot of work for nothing. TBH, I'm not even sure this will work now (`info.CURRENT_LANGUAGE` might not always be set when `get_current_locale()` is called. If it breaks the metrics, we'll fix it some more-sane way than basically **repeating** all of the translation-loading code just to set a single metrics variable.
OK, so I just blew up the world, translations-wise. The set of commits I just pushed in here basically guts the entirety of the previous translation code, including moving all of the source files around for more efficient access. (They're now all stored in one directory, as As a result, you can now launch OpenShot with e.g. #WorksForMe but that's hardly a comprehensive evaluation, especially as I'm in the Testing is very much needed, so please have at it. |
(This does duplicate all of the translation files in the source, but that's meant to be temporary. Once we can coordinate with @jonoomph to update his process for updating and importing translations, we should be able to basically blow away the |
Also, I just realized I'll have to push another commit into this. The rpmfusion packages for Fedora package all of the translation files in a separate RPM, So, I'll have to handle the case where the |
This add standard Python argparse handling for command-line arguments to launch.py, which now responds to `--help` with: ```console src/launch.py --help Loaded modules from current directory: /home/ferd/rpmbuild/REPOS/openshot-qt/src usage: launch.py [-h] [--list-languages] [-V] OpenShot version 2.4.2 optional arguments: -h, --help show this help message and exit --list-languages -V, --version ``` There is also commented out code for another argument, `--lang`, to set the interface language on the command line via language code (as listed using `--list-languages`). That will require the changes from from PR OpenShot#1795 before it can be enabled.
@jonoomph @peanutbutterandcrackers @DylanC @N3WWN (et al):
Those last two are potentially the most controversial/problematic ones, as they mean that new translations can no longer be added simply by dropping a file (or, a directory) into the right location and having it automatically picked up. The file also has to be added to the I decided to go the resource-file route after noting that However, if that's a problem, a supplemental path can be added where OpenShot searches for additional translation files, as a method of adding more/updated translations ad-hoc. The resource file is not required to make the rest of this work, so if anyone objects I can revert the translation-loading logic to scanning As I said in my last comment, I still have to add logic to make sure that a missing With the new command-line argument processing I've submitted in PR #1828, I plan to add a |
Just like `make` in `src/images` builds `openshot_rc.py` from `openshot.qrc`, in `src/language` it will build `openshot_lang.py` from `openshot_lang.qrc`
@peanutbutterandcrackers inexplicably decided to send me results of testing (very much appreciated!) in a direct email. Since that information is vital to this PR, reproducing here:
Screenshots not included, as I'm happy to take anyone's word for what language OpenShot loads in. If someone tells me it didn't load in English when they selected English, then it didn't load in English. I have requested log files from such a session, as those I do need to see. Also, @peanutbutterandcrackers, am I correct in assuming that when OpenShot failed to display Testing now, I am seeing the same as you. Apologies, I honestly thought I'd tested for that condition before opening the PR. But, this is why feedback from other testers is so important. Technical digressionThe other thing I find odd is that even Qt translations are never reported as being loaded, for
With French-Canadian, the following is shown due to the presence of
So it looks like Qt may be doing some special-casing of its own. I do note that Still, I'll look into it. Maybe it would be useful to do something similar with an |
This change _properly_ special-cases for `en_US` as the preference locale, by completely **replacing** the list of possible translations to load. This ensures that no language other than `en_US` is pulled in by the QTranslator. Since we _know_ that we have full support for `en_US` and always will, there's no need to keep a list of other possible languages from the locale/environment to fall back on. Add OpenShot's language preference to the locale-environment logging.
I believe this is now fixed, @peanutbutterandcrackers if you'd be so kind as to re-test? |
Launching with `launch.py --lang <code>` will override all environment and preference language selections, and force OpenShot to use the specified language code. (It must be one of the codes found in the `info.SUPPORTED_LANGUAGES` list, as displayed using `--list-languages`.)
QDirIterator doesn't allow any control over ordering, so we use a QDir to reference ":/locale", and then pull out the `entryList()` with sorting set to `QDir.Name`. This will leave `SUPPORTED_LANGUAGES` sorted English-alphabetically by country code after `en_US`, e.g.: ```console $ src/launch.py --list-lang Loaded modules from current directory: /home/ferd/rpmbuild/REPOS/openshot-qt/src Supported Languages: en_US American English ady af Afrikaans am አማርኛ ar العربية ast Asturianu az Azərbaycan be Беларуская ber bg Български bn বাংলা br Brezhoneg bs Bosanski ca Català ca@valencia Català ckb کوردیی ناوەندی cs Čeština cy Cymraeg da Dansk de Deutsch el Ελληνικά en_AU Australian English en_CA Canadian English en_GB British English eo es Español De España et Eesti eu Euskara fa فارسی fi Suomi fil Filipino fo Føroyskt fr Français fr_CA Français Canadien fy West-Frysk gaa gl Galego he עברית hi हिन्दी hr Hrvatski hu Magyar hy Հայերեն ia id Indonesia is Íslenska it Italiano ja 日本語 jv ka ქართული kab Taqbaylit kk Қазақ Тілі kn ಕನ್ನಡ ko 한국어 ku ky Кыргызча la lt Lietuvių lv Latviešu mk Македонски ml മലയാളം mn Монгол ms Bahasa Melayu my မြန်မာ nap nb Norsk Bokmål ne नेपाली nl Nederlands nn Nynorsk oc pa ਪੰਜਾਬੀ pl Polski pt Português pt_BR Português ro Română ru Русский se Davvisámegiella shn si සිංහල sk Slovenčina sl Slovenščina sq Shqip sr Српски sr@latin Српски sv Svenska ta தமிழ் te తెలుగు th ไทย tr Türkçe tt ug ئۇيغۇرچە uk Українська uz O‘Zbek vi Tiếng Việt zh_CN 简体中文 zh_HK 繁體中文 zh_TW 繁體中文 ```
Neat! It works great now! Just like it ought to. I even changed to two different system locales and it respected the choice for all languages this time around - even en_US. Awesome! 👍 Not attaching logs and stuff, then. 😄 |
Nope, no need.
Cool, glad to hear it! Thanks for looking at that. I got more heavy-handed about just throwing away the system locale information in certain situations, including when the preference is set to The only other workable approach would be to actually distribute a "dummy" I'm going to go through and check over the remaining worries I have about how this will handle missing translation data files, to make sure that can't cause any problems. Once I've got those bases covered, I'll remove the WIP tag and present this as eligible for testing and merging. Though I was really hoping to get a read on how @jonoomph feels about the pathname changes, move to a resource file, and etc. Because if nothing else, this will require changes to:
The things that might be problematic can still be changed, if I'm told they're problematic. But I'm loathe to just assume they aren't purely on the basis of no objections, when that lack of objections means it hasn't even been looked at. And I hate the idea that merging this would amount to pulling the rug out from under him the next time he looks at translations, unless he's been made aware of the changes. |
Oh, it's already solved, as of my last commits. No need for that. *I'm* still idly curious what Qt is doing with its own English "translations", but that's no longer important for OpenShot itself. |
Yeah, that's as far as I'd gotten, too. But it raises more questions than it answers. Those files are the result of the translators' work, but how are the un-filled-out My preference would be to have those |
This was part of my "reduce logspam" initiative, but since the file is changing so radically, I'd rather do this here instead. The commit currently part of the logspam branch will no longer apply after this is merged.
OK! I will delete this comment and I will write in the correct location. |
@ferdnyc - Would you like me to guide @mnwstl into testing the changes from your branch or should I just ask him to attach the logs, sir? |
Neither. @mnwstl please open an Issue for your problem, and we'll look into it. (Logs won't be useful, trust me. Not without getting more information first. This branch will not change anything regarding the language labeling, though it may fix the " |
Actually it looks like this is already reported, in #1793. So, @mnwstl, if you feel that report covers the issues you're having (combined with #1521), rather than opening a new issue you can also just use the Subscribe button at #1793 to follow updates there, and/or add any additional info by leaving a comment there. |
Much like `src/images/Makefile`, the `src/language/Makefile` rules will now also trigger recompilation of `openshot_lang.py` whenever any of the `*.qm` files is updated, not just when `openshot_lang.qrc` is edited. Also checking in recompiled `openshot_lang.py`.
So, I really think this branch is 100% ready, and would solve a lot of i18n problems for our non-English-speaking users. It awaits only @jonoomph 's attention to sort out the build-process side of things. @DylanC , @peanutbutterandcrackers , any hope of moving this forward? |
For the record, the substantive work on this was completed 2018-07-14 @ 8:29am, since then I've just been buffing the fenders, polishing the windscreen, and detailing the interior, simply to keep busy on it and prevent myself forgetting it's here. |
@ferdnyc - I'm happy enough to merge it but I've been holding back due to the possible build system breakage. Maybe its best to just merge and get the build system fixed asap. At least our side of things will be completed. (and you can stop polishing the windscreen) 😆 |
Yeah, honestly I think probably the build system won't break... might not break... but what will definitely break is the translation support in all of the builds. With this merged but the builds not updated, every OpenShot build that's churned out is going to be American English-only. Because even though they'll still contain the (Once that happens, I can delete |
@ferdnyc - Oh that's alright, I think we can cope without the translations in the daily builds for a little while. I'm sure Jonathan will get back on this in a reasonable amount of time. |
Actually, I take it back. Looking at the build scripts I can find, it doesn't appear that specific directories are managed — everything inside The only exception is |
@DylanC @peanutbutterandcrackers Well, good news on the daily builds front: I decided to pull down the most recent Linux AppImage, and near as I can tell it's built with my tutorial code, my new translation-loading code, and even my new command-line processing, all working as expected. The Heck, the AppImage loader even passes the commandline flags along to OpenShot1 — you can run Notes
|
P.S> I guess the next step, in terms of the translations, is now to submit the PR that deletes |
This is now blocking the release of 2.4.3. I might need to revert this PR, if I can't find a sane way of updating and testing languages on all 3 OSes. I also don't understand the moving, and converting all *.qm files into the a python resource file, which seems to simply duplicate our translations files, and add new steps for creating releases? Just confused and trying to sort through this now that it's been merged. |
That part should be unchanged, as I understand it, though you may have to take me through more of the process so I can understand how I've impacted it. I tried to address everything I could see in how translations were managed. My more-recent PR #2103 (the one that did pull the trigger on deleting
The reason for the rename was three-fold:
It does, agreed, but that wasn't the motivator or the sole result. The reason for the resource file and the consolidation of the I actually wish I'd named the files like The compiled resources can be accessed using a path of |
Oh, and of course, the main motivation for doing all this is that translation-file loading was breaking in various corner cases, for various users, and every one of the various attempts to patch it back into correct functionality broke things for a different group of users. The too-much-manual-logic-not-enough-letting-Qt-do-its-job nature of the old code was, in my evaluation, the core of the problems. i18n is hard to do right, and just leaving it to Qt struck me as the better part of valor. This PR, as far as I've been made aware, fixed nearly all of the issues for nearly all of the users, the only remaining one being a problem with the |
Building $ openshot-qt --list-languages
Compiled translation resources missing!
Loading translations from: /usr/lib/python3.6/site-packages/openshot_qt/language
Loaded modules from installed directory: /usr/lib/python3.6/site-packages/openshot_qt
Compiled translation resources missing!
Loading translations from: /usr/lib/python3.6/site-packages/openshot_qt/language
Supported Languages:
en_US American English
ady
af Afrikaans
am አማርኛ
ar العربية
ast Asturianu
az Azərbaycan
be Беларуская
ber
bg Български
bn বাংলা
br Brezhoneg
bs Bosanski
ca Català
ca@valencia Català
ckb کوردیی ناوەندی
cs Čeština
cy Cymraeg
da Dansk
de Deutsch
el Ελληνικά
en_AU Australian English
en_CA Canadian English
en_GB British English
eo
es Español De España
et Eesti
eu Euskara
fa فارسی
fi Suomi
fil Filipino
fo Føroyskt
fr Français
fr_CA Français Canadien
fy West-Frysk
gaa
gl Galego
he עברית
hi हिन्दी
hr Hrvatski
hu Magyar
hy Հայերեն
ia
id Indonesia
is Íslenska
it Italiano
ja 日本語
jv
ka ქართული
kab Taqbaylit
kk Қазақ Тілі
kn ಕನ್ನಡ
ko 한국어
ku
ky Кыргызча
la
lt Lietuvių
lv Latviešu
mk Македонски
ml മലയാളം
mn Монгол
ms Bahasa Melayu
my မြန်မာ
nap
nb Norsk Bokmål
ne नेपाली
nl Nederlands
nn Nynorsk
oc
pa ਪੰਜਾਬੀ
pl Polski
pt Português
pt_BR Português
ro Română
ru Русский
se Davvisámegiella
shn
si සිංහල
sk Slovenčina
sl Slovenščina
sq Shqip
sr Српски
sr@latin Српски
sv Svenska
ta தமிழ்
te తెలుగు
th ไทย
tr Türkçe
tt
ug ئۇيغۇرچە
uk Українська
uz O‘Zbek
vi Tiếng Việt
zh_CN 简体中文
zh_HK 繁體中文
zh_TW 繁體中文 (As you can see, my Fedora packaging doesn't yet include the .qrc file, that's something I have to correct when packaging the 2.4.3 release.) |
Wait, why the hell am I defending code I wrote moths ago, begged you to look at, and heard NOTHING in response? Do what you want, leave me out of it. |
@ferdnyc I really appreciate your work and detailed posts! You are a most impressive coder and communicator! It's true I have my hands full and suck at timely reviews, but I'm working hard to improve. We've merged a ton more PRs in the past 2 months, and this one kind of surprised me with how much changed. Parts of this PR seem unneeded from the outside looking in, but I totally understand your logic and appreciate the response tonight. |
I do intend to keep this merged, but I will need to modify many of my release scripts and processes to correctly update the translations. In the end, I think this is much better than what we did previously... I just need to rework some infrastructure changes around it. |
Update: This has now grown into a full replacement of the code to manage translation-file loading, including changes to the locations and indexing of the translation files themselves (though the individual files have not changed, other than being renamed and moved to a new directory). See comments and commit messages for further details.
While this is no longer marked WIP, one item remains unfinished:
launch.py
to list all supported languages available to OpenShot (--list-languages
), launch OpenShot in a specific language (--lang
<code>
) overriding both the environment and the preferences file for the current session only.--list-lang[uages]
is in PR Add commandline processing to launch.py #1828, and will be done when that merges.--lang
also depends on it, but it depends even more on the logic being written on this side (which it isn't)Original PR
PR #1332 sought to fix loading of English translations in non-English locales (see #1256) by removing a hardcoded
en_US
bypass in the translation file loading. In doing so, it inadvertently broke loading of translation files that match the system locale.This removes the entire "skip loading" clause, because we can never assume that any translation shouldn't be loaded. English translations may need to be loaded and applied, if OpenShot's language is set to English in non-English locales. In contrast, we still need to load our OpenShot translations for the local language even if the selected language is the same as the system locale.Fixes #1521. (I believe. It's not something I can test, since I'm not in a non-English locale.)
Also, the log messages for
$LANG
and$LOCALE
no longer fall back to default strings, sothey will properly show when the corresponding variables are unset or set to the empty string.