-
Notifications
You must be signed in to change notification settings - Fork 349
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
rhbz#1888697 list important major languages #2935
Conversation
Hello @sundeep-anand! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2020-11-13 15:18:13 UTC |
/packit build |
Hello @sundeep-anand thanks for your PR. Yes, this seems something which should be part of the langtable. What do you think @mike-fabian ? Is it appropriate to have list of these languages in langtable or is there a better solution? |
Yes, we can add this to langtable. I must say that I don't like such a list of common languages very much, it always annoys me a bit when I open gnome-control-center. My own native language (German) is in the list of common languages, but I feel that it might annoy people that their language isn't. But anyway, I am not against adding something like that to langtable, it is probably better to have it there than in anaconda. |
Just to comment, I believe Gnome used number of speakers as the main criteria for the top listed languages. |
As to the technical matters:
Apart from that, there is the "soft" side of this. It's a potentially loaded decision / change.
At any rate, I am told @M4rtinK already had some discussions about this, so switching to him... |
depends mike-fabian/langtable#12 |
pyanaconda/ui/gui/spokes/welcome.py
Outdated
@@ -162,6 +162,10 @@ def initialize(self): | |||
# get language codes for the locales | |||
langs = [localization.get_language_id(locale) for locale in locales] | |||
|
|||
# get common languages and append them here | |||
common_locales = localization.get_common_locales() | |||
langs += [localization.get_language_id(locale) for locale in common_locales] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this duplicate languages on territories where a common language is a territory language too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no.. the language will appear once.. in territory and be omitted from common.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise, the code looks good to me. Thank you!
pyanaconda/localization.py
Outdated
@@ -72,6 +72,11 @@ def get_language_id(locale): | |||
return langtable.parse_locale(locale).language | |||
|
|||
|
|||
def get_common_locales(): | |||
"""Return common locales to prioritize them""" | |||
return langtable.list_common_locales() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, update the required langtable
version in the anaconda.spec.in
file.
/packit build |
Failure of the tests is expected here:
|
/packit build |
Only users with write or admin permissions to the repository can trigger Packit-as-a-Service |
/packit build Sorry about that. |
/packit build |
Seems that the langtable is still not in the Fedora yet. |
/packit build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks a lot! :)
Sorry for taking me so long to review this. :P The RPM test is failing, but this might be fixed by rebasing the PR on latest master. Cold you please do that ? :) I also did some manual testing and everything seems to work fine: See the 7 languages under the selected language and Afrikaans - these are the major languages this PR is about. :) In comparison this is how it looks like at the moment without the PR being applied: As you can there is just an alphabetical list of all languages Anaconda knowns under the currently selected language. I've also checked with geolocation enabled & all seems to be fine as well: One thing I have noticed is, that the separators we use in the language listbox are really hard to see. I don't think it used to be like this & it's possibly due to some theming changes. In any case, this is not really something this PR needs to fix & I'll make a note for myself to look into fixing all the Anaconda separators to be more visible. |
Also one more thing - the Packit test failure seems to be related to ongoing packit test infra issues & not an issue with this PR. |
sorry - not sure how the review got dismissed .. |
/packit build |
This is expected it's change of behavior in the project. Reason is push to the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me too
I did a new Anaconda build yesterday, so this change should be now hitting the Fedora Rawhide nightly composes as part of Anaconda 34.14-1. :) |
I did a new Anaconda build yesterday, so this change should be now hitting the Fedora Rawhide nightly composes as part of Anaconda 34.14-1. :)
thank you!
|
Listing the most important major languages make them super-easy to find for international users. (lesser scroll)
Identification of those languages is a challenge, however we may refer gnome-control-center.
Implementation
One option is to join (prepend) those identified languages to
locales
inpyanaconda/ui/gui/spokes/welcome.py
. Here, we may have separators to avoid confusion: separate local/national languages to common/major ones. So,major languages | national languages | other languages
. However, in a many cases the count of national languages remain less than 4 which may leave the separators very close! And may not look nice.Another option could be
major languages + national languages | other languages
. I feel this may create confusion.As we are identifying local/national languages already to help ease user's search (for their languages), it makes sense to populate the first list with identified common languages in the event when
territory lookup
fails instead of justen_US
.Should we try moving
get_common_languages
to langtable instead?Please feel free, to suggest better implementation way.