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

Add Ctrl+Tab and Ctrl+Shift+Tab shortcuts #4146

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

Conversation

winkies
Copy link
Contributor

@winkies winkies commented Jan 12, 2021

Partially Fixes #3359

Description of the Change
Define shortcut on accelerator table and bind it with event table.

Alternate Designs
Tested only on Linux environment. It should work as well on Windows but it should be tested on MacOS X.

Release Notes
Add Ctrl+Tab and Ctrl+Shift+Tab shortcuts for advance view of boinc manager.

@AenBleidd
Copy link
Member

@winkies, it is possible to extend this PR to include all changes that are requested in #3359?
Otherwise I'll ask @RichardHaselgrove to split his ticket into two

@AenBleidd
Copy link
Member

This is not needed on Windows since Ctrl-Tab and Shift-Ctrl-Tab already work fine without this fix. I don't know about Mac, but for now it looks like that this should be a linux-only fix . Also please add Shift-Ctrl-Tab shortcut to iterate tabs backward (and for linux-only too).

@winkies
Copy link
Contributor Author

winkies commented Jan 12, 2021

@AenBleidd, I'm on Ctrl+Shift-Tab shortcut.

About the Tab / Shift-Tab issue, I think that is more complex than just add shortcut on the advanced frame.
Moreover, it need Windows environment to reproduce the issue.

@RichardHaselgrove
Copy link
Contributor

This is not needed on Windows...

Yes, it is: Ctrl-tab still gets stuck on the Notices page, as noted in #3359.

@winkies winkies changed the title Add CTRL+TAB shortcut Add Ctrl+Tab and Ctrl+Shift+Tab shortcuts Jan 12, 2021
@CharlieFenton
Copy link
Contributor

CharlieFenton commented Jan 13, 2021

I object to this PR. WxWidgets translates Ctrl in accelerators to Cmd (the command key) on Macs. Cmd+Tab is a standard keyboard shortcut on Macs to step through open applications. It is equivalent to Alt+Tab on MS Windows. The earlier change to add Ctrl+A as a shortcut for Select All is OK because that is the standard meaning of Cmd+A on Macs.

In addition, there are already keyboard shortcuts to select each of the tabs. I see little need for another shortcut to be able to step through the tabs.

This need to avoid conflicting with standard keyboard shortcuts is the reason most BOINC-specific accelerators include the Shift key.

@AenBleidd
Copy link
Member

So this should be linux-only feature because looks like linux is the only platform that has no these bindings in wxWidget by default

@CharlieFenton
Copy link
Contributor

Please also see this comment in #3359.

@RichardHaselgrove
Copy link
Contributor

Testing with win_manager_cbd87af9f19501ff306d54aeb44e12249f2a56d6: partial success.

Starting with the focus on the tab label row: Ctrl-Tab and Ctrl-Shift-Tab cycle through all pages (as before).

Starting with the focus on another control in the body of a page (default view), Ctrl-Tab and Ctrl-Shift-Tab cycle through all pages without getting stuck on Notices: that's an improvement.

But if you click or tab into the body of the Notices page, there's no way out via the keyboard, except by going through the main menu with accelerator keys. And the accelerators aren't visible until you press 'Alt'.

@codecov
Copy link

codecov bot commented Jan 19, 2021

Codecov Report

Merging #4146 (746d58f) into master (679588d) will decrease coverage by 4.19%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master   #4146      +/-   ##
===========================================
- Coverage      6.26%   2.07%   -4.20%     
===========================================
  Files           275     178      -97     
  Lines         34898   27832    -7066     
  Branches       1492       0    -1492     
===========================================
- Hits           2186     577    -1609     
+ Misses        32596   27255    -5341     
+ Partials        116       0     -116     
Impacted Files Coverage Δ Complexity Δ
...pp/src/main/java/edu/berkeley/boinc/rpc/Project.kt
...c/main/java/edu/berkeley/boinc/mutex/BoincMutex.kt
...in/java/edu/berkeley/boinc/rpc/DeviceStatusData.kt
...va/edu/berkeley/boinc/rpc/ProjectRelatedClasses.kt
...src/main/java/edu/berkeley/boinc/StatusFragment.kt
...main/java/edu/berkeley/boinc/rpc/ProjectsParser.kt
...ava/edu/berkeley/boinc/adapter/ProjectListEntry.kt
...rc/main/java/edu/berkeley/boinc/rpc/VersionInfo.kt
...rkeley/boinc/adapter/NoticesRecyclerViewAdapter.kt
...edu/berkeley/boinc/rpc/ProjectAttachReplyParser.kt
... and 90 more

@winkies
Copy link
Contributor Author

winkies commented Feb 6, 2021

Please see the lastest status about Notices page issue.

IMO, you can split the ticket in two except if someone wants to give it a shot on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Manager: missing/inconsistent keyboard handlers
4 participants