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

AddCatalog doesn't fail with wxLANGUAGE_DEFAULT #18227

Open
wxtrac opened this issue Sep 22, 2018 · 10 comments
Open

AddCatalog doesn't fail with wxLANGUAGE_DEFAULT #18227

wxtrac opened this issue Sep 22, 2018 · 10 comments
Milestone

Comments

@wxtrac
Copy link
Collaborator

wxtrac commented Sep 22, 2018

Issue migrated from trac ticket # 18227

component: wxMSW | priority: normal | keywords: addcatalog translations locale

2018-09-22 17:47:19: @lanurmi created the issue


When wxLocale is inited with wxLANGUAGE_DEFAULT, and the default language is non-English, wxTranslations::AddCatalog returns true even if the requested catalog in the preferred language was not found. This is specific to Windows, possibly also Mac, but *nix is not affected.

What happens is that on non-English Windows (for example, Windows 7 in Finnish), GetPreferredUILanguage returns two languages: fi_FI and en_US. Now, if the catalog for Finnish was not found, the original msgid language English is still considered to satisfy what the caller asked for, so no error is returned. This is however not what the caller expected, and on *nix an error would be returned.

I also tested this on a Windows 10 installed in German, and again two preferred languages are returned. On neither machine has the user (me) done anything to choose English as a secondary language.

In wx 2.8 AddCatalog used to fail when a catalog in the default language was not found. On *nix it still does.

Steps to reproduce in the unmodified internat sample:
0. Have something else than English as the preferred language.

  1. Remove all *.mo files, so no catalog exists.
  2. Select System default as the language in internat.
  3. Observe there is no error message about failing to load the catalog.
@wxtrac
Copy link
Collaborator Author

wxtrac commented Sep 23, 2018

2018-09-23 01:32:31: @vadz commented


I'm not sure what should we be doing here as having en_US in preferred languages implicitly seems to go completely against all the logic in src/common/translation.cpp which explicitly tries, AFAICS, to honour all the preferred languages.

Vaclav, would you have any thoughts about this?

@wxtrac
Copy link
Collaborator Author

wxtrac commented Nov 18, 2018

2018-11-18 01:47:58: @vadz changed status from new to closed

2018-11-18 01:47:58: @vadz set resolution to fixed

2018-11-18 01:47:58: @vadz commented

In 2d784da:
Load catalogs for all preferred languages, if they exist

This way the first and only fallback language isn't necessarily the
msgid language (which is English most often). This is how GNU gettext
works -- it uses multiple fallback languages when multiple preferred
languages are set.

As a side effect, fixes #18227 in one possible way.

@wxtrac
Copy link
Collaborator Author

wxtrac commented Dec 17, 2018

2018-12-17 02:28:12: slodki commented


In wx 2.8 AddCatalog used to fail when a catalog in the default language was not found.

Above statement is not true - AddCatalog used to fail when a catalog in the default language was not found AND separate message catalog file was required (default language <> msgIdLanguage):

https://github.com/wxWidgets/wxWidgets/blob/v2.8.12/src/common/intl.cpp#L2830

Now in 3.1.2 it fails if the most preferred language is the source code language - we leave loop with break and success = false BEFORE we reach calculation of success value for the first element of the array.

See #18297

@wxtrac
Copy link
Collaborator Author

wxtrac commented Dec 18, 2018

2018-12-18 05:33:31: @vadz commented


I feel that perhaps we should revert 2d784da (as well as the recent fixes for the regressions introduced by it), finally, as in addition to the already fixed #18297 and #18299 we still have #18300 (not sure if everything there will be fixed by reverting though) and #18302 (which will be, I think).

I still have no idea how to fix the original problem, however. But it's arguably less bad than all the problems that were caused by fixing it.

@wxtrac
Copy link
Collaborator Author

wxtrac commented Dec 18, 2018

2018-12-18 23:32:41: @lanurmi commented


Replying to [comment:3 slodki]:

In wx 2.8 AddCatalog used to fail when a catalog in the default language was not found.

Above statement is not true - AddCatalog used to fail when a catalog in the default language was not found AND separate message catalog file was required

Fair enough, it is not true in the strictly logical sense, but it is true as a statement expressed in everyday English.

Replying to [comment:4 vadz]:

I feel that perhaps we should revert 2d784da (as well as the recent fixes for the regressions introduced by it), finally, as in addition to the already fixed #18297 and #18299 we still have #18300 (not sure if everything there will be fixed by reverting though) and #18302 (which will be, I think).

I still have no idea how to fix the original problem, however. But it's arguably less bad than all the problems that were caused by fixing it.

I agree that at this point it seems reasonable to revert the change, as it turned out to be more complex to handle all use cases correctly than it seemed at first. Further work is needed apparently. It was unfortunate to introduce such problems to an actual release, but on the other hand I wonder if we would have received all the feedback if the change were only present in unreleased master.

#18300 seems partially unrelated to this very change.

@wxtrac
Copy link
Collaborator Author

wxtrac commented Dec 23, 2018

2018-12-23 17:34:06: @vadz commented


In 270ad54:
Revert all recent changes to wxTranslations

The latest changes to wxTranslations::AddCatalog() behaviour were not
backwards-compatible and also had other problem, so revert them for now,
even if this means that #18227 has to be reopened.

This is a combination of the following commits:


Revert "Fix regression in wxTranslations::AddCatalog()"

This reverts commit 14e9058.

See #18297.


Revert "Fix crash in translations code when no translations are found"

This reverts commit 80904d1.

See #18299.


Revert "Rename new wxTranslations method to GetAcceptableTranslations()"

This reverts commit 20b02d6.


Revert "Load catalogs for all preferred languages, if they exist"

This reverts commit 2d784da.


Revert "Allow getting all usable translations languages"

This reverts commit 5d08e40.


See #18227, #18300.

Closes #18302.

@wxtrac
Copy link
Collaborator Author

wxtrac commented Dec 23, 2018

2018-12-23 17:34:56: @vadz changed status from closed to reopened

2018-12-23 17:34:56: @vadz removed resolution (was fixed)

@vadz vadz added this to the 3.3.0 milestone Sep 30, 2023
vadz added a commit to vadz/wxWidgets that referenced this issue Sep 30, 2023
The new function only returns true if the catalog could be really
loaded and not if it is considered not to be needed because the message
ID language (which is typically "en-US") happens to be present in the
preferred UI languages list (which seems to always include "en-US" in at
least Western European MSW).

This allows to ditinguish, albeit in a rather awkward (but
backwards-compatible) way between having a translation for the given
language and not needed such translation.

It is still not clear if it is really correct to return "en-US" from the
list of preferred languages even if the user has never intentionally
configured the OS to indicate that English is acceptable, but at least
now we can work around this issue and use AddAvailableCatalog() in
AddStdCatalog() to make sure we only skip loading unversioned wxstd.mo
if the versioned wxstd-x.y.mo file is really found instead of never
doing it, as was the case until now (see wxWidgets#23886).

Also add GetBestAvailableTranslation() helper which seems more useful
than the existing GetBestTranslation() one and is similarly related to
it.

See wxWidgets#18227.
@vadz
Copy link
Contributor

vadz commented Sep 30, 2023

After returning to this, I realize that I actually have a problem with adding msgIdLanguage to the "available translations" which was done back in 01f953e (Add wxTranslations::GetBestTranslation()., 2012-09-08). This seems wrong because this translation is not actually available -- it is fine not to use it at all, but this is not quite the same thing.

Unfortunately to solve this we'd need to change AddCatalog() to return a 3-state enum with possible values of "Loaded", "Not loaded" and "Not needed" which is going to be difficult to implement in a backwards compatible way. So I guess the only thing we can do is to add yet another function behaving like this.

For the record, I've toyed with the idea of returning false from AddCatalog(domain) overload in the case when there is no translation but the language is still considered to be acceptable, while continuing to return true from AddCatalog(domain, msgIdLanguage) overload, but decided this would be too confusing, finally.

So I've created #23918 which is supposed to address this. Please let me know what do you think!

@vslavik
Copy link
Member

vslavik commented Oct 2, 2023

After returning to this, I realize that I actually have a problem with adding msgIdLanguage to the "available translations" which was done back in 01f953e (Add wxTranslations::GetBestTranslation()., 2012-09-08). This seems wrong because this translation is not actually available -- it is fine not to use it at all, but this is not quite the same thing.

Would it feel OK to you if the function was called GetBestUILanguage() instead? That is, I think, the rationale behind that behavior: it is meant to return the language available for use, which is usually "translation", but not in the msgid/English case. But omitting msgIdLanguage from the list would require special-casing that one language everywhere.

@vadz
Copy link
Contributor

vadz commented Oct 2, 2023

I agree that calling it GetBestUILanguage() would have been much better/more clear but I'm not sure that adding yet another synonym now, while still keeping the current name for compatibility, is worth it.

But mostly, renaming it is still not going to solve the problem of en-US being -- apparently -- always present in the preferred UI languages list and so AddCatalog() always succeed, so the other changes would still be needed, AFAICS.

vadz added a commit that referenced this issue Oct 2, 2023
The new function only returns true if the catalog could be really
loaded and not if it is considered not to be needed because the message
ID language (which is typically "en-US") happens to be present in the
preferred UI languages list (which seems to always include "en-US" in at
least Western European MSW).

This allows to distinguish, albeit in a rather awkward (but
backwards-compatible) way between having a translation for the given
language and not needed such translation.

It is still not clear if it is really correct to return "en-US" from the
list of preferred languages even if the user has never intentionally
configured the OS to indicate that English is acceptable, but at least
now we can work around this issue and use AddAvailableCatalog() in
AddStdCatalog() to make sure we only skip loading unversioned wxstd.mo
if the versioned wxstd-x.y.mo file is really found instead of never
doing it, as was the case until now (see #23886).

Also add GetBestAvailableTranslation() helper which seems more useful
than the existing GetBestTranslation() one and is similarly related to
it.

See #18227.
vadz added a commit to vadz/wxWidgets that referenced this issue Oct 5, 2023
The new function only returns true if the catalog could be really
loaded and not if it is considered not to be needed because the message
ID language (which is typically "en-US") happens to be present in the
preferred UI languages list (which seems to always include "en-US" in at
least Western European MSW).

This allows to distinguish, albeit in a rather awkward (but
backwards-compatible) way between having a translation for the given
language and not needed such translation.

It is still not clear if it is really correct to return "en-US" from the
list of preferred languages even if the user has never intentionally
configured the OS to indicate that English is acceptable, but at least
now we can work around this issue and use AddAvailableCatalog() in
AddStdCatalog() to make sure we only skip loading unversioned wxstd.mo
if the versioned wxstd-x.y.mo file is really found instead of never
doing it, as was the case until now (see wxWidgets#23886).

Also add GetBestAvailableTranslation() helper which seems more useful
than the existing GetBestTranslation() one and is similarly related to
it.

See wxWidgets#18227, wxWidgets#23930.

(cherry picked from commit 94b1a17)
vadz added a commit to vadz/wxWidgets that referenced this issue Oct 6, 2023
The new function only returns true if the catalog could be really
loaded and not if it is considered not to be needed because the message
ID language (which is typically "en-US") happens to be present in the
preferred UI languages list (which seems to always include "en-US" in at
least Western European MSW).

This allows to distinguish, albeit in a rather awkward (but
backwards-compatible) way between having a translation for the given
language and not needed such translation.

It is still not clear if it is really correct to return "en-US" from the
list of preferred languages even if the user has never intentionally
configured the OS to indicate that English is acceptable, but at least
now we can work around this issue and use AddAvailableCatalog() in
AddStdCatalog() to make sure we only skip loading unversioned wxstd.mo
if the versioned wxstd-x.y.mo file is really found instead of never
doing it, as was the case until now (see wxWidgets#23886).

Also add GetBestAvailableTranslation() helper which seems more useful
than the existing GetBestTranslation() one and is similarly related to
it.

See wxWidgets#18227, wxWidgets#23930.

(cherry picked from commit 94b1a17)
vadz added a commit to vadz/wxWidgets that referenced this issue Oct 6, 2023
The new function only returns true if the catalog could be really
loaded and not if it is considered not to be needed because the message
ID language (which is typically "en-US") happens to be present in the
preferred UI languages list (which seems to always include "en-US" in at
least Western European MSW).

This allows to distinguish, albeit in a rather awkward (but
backwards-compatible) way between having a translation for the given
language and not needed such translation.

It is still not clear if it is really correct to return "en-US" from the
list of preferred languages even if the user has never intentionally
configured the OS to indicate that English is acceptable, but at least
now we can work around this issue and use AddAvailableCatalog() in
AddStdCatalog() to make sure we only skip loading unversioned wxstd.mo
if the versioned wxstd-x.y.mo file is really found instead of never
doing it, as was the case until now (see wxWidgets#23886).

Also add GetBestAvailableTranslation() helper which seems more useful
than the existing GetBestTranslation() one and is similarly related to
it.

See wxWidgets#18227, wxWidgets#23930.

(cherry picked from commit 94b1a17)
vadz added a commit to vadz/wxWidgets that referenced this issue Nov 5, 2023
When the original messages language matches the language of the locale
being used, these strings can be used directly and so AddCatalog()
should still return true for them but it didn't do it any more after the
changes of 94b1a17 (Add AddAvailableCatalog() and use it in
AddStdCatalog(), 2023-09-30).

See wxWidgets#18227.

Closes wxWidgets#24019.
vadz added a commit that referenced this issue Nov 7, 2023
When the original messages language matches the language of the locale
being used, these strings can be used directly and so AddCatalog()
should still return true for them but it didn't do it any more after the
changes of 94b1a17 (Add AddAvailableCatalog() and use it in
AddStdCatalog(), 2023-09-30).

See #18227.

Closes #24019.

Closes #24037.
vadz added a commit that referenced this issue Nov 7, 2023
When the original messages language matches the language of the locale
being used, these strings can be used directly and so AddCatalog()
should still return true for them but it didn't do it any more after the
changes of 94b1a17 (Add AddAvailableCatalog() and use it in
AddStdCatalog(), 2023-09-30).

See #18227.

Closes #24019.

Closes #24037.

(cherry picked from commit 39079cb)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants