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

list major common keyboard layouts first #2788

Merged
merged 1 commit into from
Sep 8, 2020
Merged

list major common keyboard layouts first #2788

merged 1 commit into from
Sep 8, 2020

Conversation

sdp5
Copy link
Contributor

@sdp5 sdp5 commented Aug 14, 2020

Refer: rhbz#1158370
Depends: mike-fabian/langtable#11

This is an attempt to determine major keyboard layouts and place them first in the keyboard layouts selection list.
Hence lesser scroll for users.

To Do:

  • determine better approach, if any
  • few UI changes: include a separator
  • code cleanups

@pep8speaks
Copy link

pep8speaks commented Aug 14, 2020

Hello @sundeep-anand! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 25:1: E402 module level import not at top of file

Line 43:1: E402 module level import not at top of file
Line 46:1: E402 module level import not at top of file

Comment last updated at 2020-09-02 12:20:59 UTC

Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, nice work!

Just a small nitpick right now. First, let's wait until the langtable change will be decided and then we can start with the review and suggestions here.

pyanaconda/ui/gui/xkl_wrapper.py Outdated Show resolved Hide resolved
@jkonecny12
Copy link
Member

Jenkins, it's ok to test.

@jkonecny12 jkonecny12 added master Please, use the `f39` label instead. notable change Important changes like API change, behavior change... labels Aug 17, 2020
@sdp5
Copy link
Contributor Author

sdp5 commented Aug 19, 2020

Langtable changes are released and built as well.

Not sure why Rawhide tests failed.
Looking forward for suggestions/improvements.

Meanwhile I'll try to add a separator and clean up spokes/keyboard.py changes a bit.

@sdp5 sdp5 requested a review from jkonecny12 August 19, 2020 11:24
@VladimirSlavik
Copy link
Contributor

The gist of unit tests failure was AttributeError: module 'langtable' has no attribute 'list_common_keyboards' so that could be resolved now. Additional failures were reported by pylint:

E1101(no-member):/anaconda/pyanaconda/localization.py:384,11: get_common_keyboard_layouts: Module 'langtable' has no 'list_common_keyboards' member
W0612(unused-variable):/anaconda/pyanaconda/ui/gui/spokes/keyboard.py:101,8: AddLayoutDialog.compare_layouts: Unused variable 'show_str1'
W0612(unused-variable):/anaconda/pyanaconda/ui/gui/spokes/keyboard.py:102,8: AddLayoutDialog.compare_layouts: Unused variable 'show_str2'
W0611(unused-import):/anaconda/pyanaconda/ui/gui/spokes/keyboard.py:42,0: : Unused locale imported as locale_mod

Theoretically only the pylint stuff should remain now... if the langtable build already propagated all the way to our PR test workers. Let's see?

Jenkins, test this please.

@jkonecny12
Copy link
Member

jenkins, test this please

@jkonecny12
Copy link
Member

Langtable is now in the repositories so the main problem is gone the rest seems to be valid issue:

W0612(unused-variable):/anaconda/pyanaconda/ui/gui/spokes/keyboard.py:101,8: AddLayoutDialog.compare_layouts: Unused variable 'show_str1'
W0612(unused-variable):/anaconda/pyanaconda/ui/gui/spokes/keyboard.py:102,8: AddLayoutDialog.compare_layouts: Unused variable 'show_str2'
W0611(unused-import):/anaconda/pyanaconda/ui/gui/spokes/keyboard.py:42,0: : Unused locale imported as locale_mod

@jkonecny12
Copy link
Member

Please ignore the test result now. New pylint version was released so we need to make adjustments to fix this.

@sdp5 sdp5 requested a review from poncovka September 1, 2020 17:27
@sdp5 sdp5 changed the title wip: list major common keyboard layouts first list major common keyboard layouts first Sep 2, 2020
@sdp5
Copy link
Contributor Author

sdp5 commented Sep 2, 2020

kindly review ..

Copy link
Contributor

@VladimirSlavik VladimirSlavik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know enough about this to approve, but code LGTM.

Copy link
Contributor

@poncovka poncovka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good to me. Thanks!

@poncovka poncovka added the manual testing required This issue can't be merged without manual testing label Sep 7, 2020
Copy link
Contributor

@VladimirSlavik VladimirSlavik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM as far as I can tell...

@M4rtinK
Copy link
Contributor

M4rtinK commented Sep 8, 2020

Made a couple screenshots of how this looks like in practice, sorted top of the list in the "ADD A KEYBOARD LAYOUT" dialog:
sorted_kayboard_layouts_top
The divider between the major/common layouts and the less common ones:
sorted_kayboard_layouts_divider

Copy link
Contributor

@M4rtinK M4rtinK left a 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 as well - thanks a lot! :)

Copy link
Member

@jkonecny12 jkonecny12 left a 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. Even thought this test is not really testing Anaconda code but let's fix that later.

Thanks for your contribution and for fixing this old bug!

@M4rtinK M4rtinK merged commit 44b5048 into rhinstaller:master Sep 8, 2020
@sdp5
Copy link
Contributor Author

sdp5 commented Sep 9, 2020

welcome, and thanks for merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
manual testing required This issue can't be merged without manual testing master Please, use the `f39` label instead. notable change Important changes like API change, behavior change...
Development

Successfully merging this pull request may close these issues.

6 participants