Improve keyboard handling for focused buttons in Crop and Reduce Red Eye tools
ClosedPublic

Authored by rkflx on Aug 19 2018, 6:30 AM.

Details

Summary

Both Crop and Reduce Red Eye feature a Cancel button.
While normally pressing Enter or Space while a button has
keyboard focus should trigger the button's action, in this case
incorrect actions were executed: Enter accepted the modifications,
i.e. it did the opposite of what the label said, and Space even
jumped to the next image.

For Enter this can be solved by explicitely checking for keyboard
focus, which avoids the underlying problem, i.e. that
ViewMainPage::slotEnterPressed calls *Tool::keyPressEvent before
QDialogButtonBox had a chance to handle the event. For Space
forwarding in ViewMainPage::eventFilter is enough to override the
default shortcut handler, since *Tool::keyPressEvent will implicitely
call event->accept().

Note that this also prevents Space from navigating to the next
image even when the buttons are not focused. We could bring that back,
but it would actually be more consistent if we did not, since the arrow
keys are also blocked when a tool is active. To still navigate away
while a tool is active, users can set a secondary shortcut for
Next to avoid getting in conflict with how Space is supposed
to work on buttons.

FIXED-IN: 18.08.1

Test Plan

Use to focus any of the buttons in the Crop and
Reduce Red Eye tools, and press Enter and Space. Both
shortcuts should trigger the appropriate action, i.e. either OK or
Cancel depending on the button.

Press the same shortcuts while the image is focused. Space should
do nothing, and Enter should accept.

Diff Detail

Repository
R260 Gwenview
Branch
arcpatch-D14925
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 2019
Build 2037: arc lint + arc unit
rkflx requested review of this revision.Aug 19 2018, 6:30 AM
rkflx created this revision.
rkflx added a comment.Aug 19 2018, 6:31 AM

Another idea I had (more suited for master, though): In addition to Enter and double-clicking I could also make Space accept a tool. Instead of doing nothing this could be handy for two-handed use (left hand on the keyboard, right hand on the mouse). Thoughts?

Also, it's now apparent that the CropAdvanced settings checkbox is unable to gain keyboard focus. It's a trivial fix, but I'll commit that separately.

Works mostly good, except for the red eye Close (see inline comment).
Btw. Backspace still goes to the previous image. But not sure if we should change this too. It's not used otherwise and could be assigned to any other action by the user.

Another idea I had (more suited for master, though): In addition to Enter and double-clicking I could also make Space accept a tool. Instead of doing nothing this could be handy for two-handed use (left hand on the keyboard, right hand on the mouse). Thoughts?

For me Enter is the optimum - left hand on mouse, right hand on keyboard... ;)
But I agree otherwise Space is easier to reach. Though maybe it's a bit unusual for accepting.

app/viewmainpage.cpp
787–788

Any reason why you use auto instead of int?

lib/redeyereduction/redeyereductiontool.cpp
178–183

The red eye tool uses QDialogButtonBox::Close. This results in a crash when pressing one of the buttons with Return.
(Attention: In master both are used!)

rkflx marked 2 inline comments as done.Aug 20 2018, 11:17 AM
rkflx added a subscriber: ngraham.

Btw. Backspace still goes to the previous image. But not sure if we should change this too. It's not used otherwise and could be assigned to any other action by the user.

I agree. The point of the patch was to enable keyboard handling for the buttons, not disable shortcuts in general (of which there are many more) when a tool is used.

Another idea I had (more suited for master, though): In addition to Enter and double-clicking I could also make Space accept a tool. Instead of doing nothing this could be handy for two-handed use (left hand on the keyboard, right hand on the mouse). Thoughts?

For me Enter is the optimum - left hand on mouse, right hand on keyboard... ;)
But I agree otherwise Space is easier to reach. Though maybe it's a bit unusual for accepting.

@ngraham Any opinion on that? Should I add Space in addition to Enter to easily accept from the keyboard, or is this too awkward?

app/viewmainpage.cpp
787–788

I kinda wanted to be flexible to whatever enum Key might be changed to in a very distant future. Nevertheless, using int is also okay, in particular since on the right-hand side int is not mentioned explicitly yet.

lib/redeyereduction/redeyereductiontool.cpp
178–183

Ah, I only tested with the master branch and kinda missed that lonely Close button. Yay for reviews!

I'll do this in a more generic way.

rkflx updated this revision to Diff 40045.Aug 20 2018, 11:18 AM
rkflx marked 2 inline comments as done.
  • Fix crash
  • Use int

I could get behind using the space bar as an accelerator. No objection there. Seems like a nice hidden power user feature.

muhlenpfordt accepted this revision.Aug 21 2018, 6:34 AM

Nice idea to use the role! 👍
Works perfect now.

This revision is now accepted and ready to land.Aug 21 2018, 6:34 AM
Restricted Application added a project: Gwenview. · View Herald TranscriptAug 21 2018, 6:34 AM
This revision was automatically updated to reflect the committed changes.

The trouble with the red eye Close button creeped into master now (stable is still fine).
Pressing Close (not Cancel) with Return does not work. This needs some adjustment for the new widget helpDialogButtonBox.

rkflx added a comment.Aug 23 2018, 6:12 AM

Oh well, the joys of diverging branches. Only the "close" vs "cancel" button types were accounted for, but not the (now) differing button boxes themselves. Patch in D15014. Glad you checked again!