Make all buttons in the main window activatable with enter
ClosedPublic

Authored by davidre on Oct 9 2019, 9:23 AM.

Details

Summary

In a QDialog QPushbuttons will have autoDefault==true and the Dialog will call
setDefault(true) on them. This allows the user to activate the Buttons with the
enter key. However we also use QToolButtons in the main window with no visible
difference to the user. This caused unexpected activations of the help button
(the first default button) when a tool button was focused. In a custom event
handler we can check if the current focused widget is tool or push button when
the enter key is pressed and activate them accordingly.

BUG: 412184
FIXED-IN: 19.08.3

Diff Detail

Repository
R166 Spectacle
Branch
buttons (branched from Applications/19.08)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17467
Build 17485: arc lint + arc unit
davidre created this revision.Oct 9 2019, 9:23 AM
Restricted Application added a project: Spectacle. · View Herald TranscriptOct 9 2019, 9:23 AM
Restricted Application added a subscriber: Spectacle. · View Herald Transcript
davidre requested review of this revision.Oct 9 2019, 9:23 AM
aprcela added a subscriber: aprcela.Oct 9 2019, 9:49 AM

Works fine. All buttons get proper focus on TAB now.

src/Gui/KSMainWindow.cpp
520

This one got here by accident? :)

davidre updated this revision to Diff 67555.Oct 9 2019, 2:19 PM

remove stray qdebug

davidre marked an inline comment as done.Oct 9 2019, 2:23 PM
ngraham added a subscriber: ngraham.EditedOct 9 2019, 4:22 PM

This works, though I'll admit that the custom event filter makes me a bit queasy. Feels kind of like hacking around something that wasn't implemented the right way in the first place (i.e. using a QDialog and QDialogButtonBox). Is porting away from that stuff even worse? Also is there a reason why we can't use only PushButtons in the main window? I was surprised to discover that half of them were ToolButtons when I went digging into the code, because they all *look* like PushButtons.

This works, though I'll admit that the custom event filter makes me a bit queasy. Feels kind of like hacking around something that wasn't implemented the right way in the first place (i.e. using a QDialog and QDialogButtonBox). Is porting away from that stuff even worse?

I tried making it a QWidget but then no button was reacting to enter and we would lose close-on-esc which we would then have to implement ourselves. Hey it's, not *that* bad ;). It's an only an handler and not a filter for another widget.
Maybe someone with more experience has another idea? Or we concede that no button can be activated with Enter only with Space.

Also is there a reason why we can't use only PushButtons in the main window? I was surprised to discover that half of them were ToolButtons when I went digging into the code, because they all *look* like PushButtons.

I didn't wrote the code but the obvious reason would be that one can assign actions to ToolButtons. Here it is used with KStandardActions and I use it for the screenshot button to switch easily between states.

ngraham accepted this revision.Oct 9 2019, 5:14 PM

All right, fair enough! Let's maybe add some comments explaining the reason for the event handler, then ship it.

This revision is now accepted and ready to land.Oct 9 2019, 5:14 PM
davidre updated this revision to Diff 67593.Oct 10 2019, 10:16 AM

add comment

This revision was automatically updated to reflect the committed changes.