Bug 412184

Summary: Focused "New screenshot" doesn't work
Product: [Applications] Spectacle Reporter: damiano38
Component: GeneralAssignee: Boudhayan Gupta <me>
Severity: normal CC: antonio.prcela, kde, nate
Priority: NOR Keywords: usability
Version: 19.08.1   
Target Milestone: ---   
Platform: Arch Linux   
OS: Linux   
Latest Commit: Version Fixed In: 19.08.3

Description damiano38 2019-09-22 09:13:05 UTC
After open Spectacle "New screenshot" button always is focused but now after click Enter always "Help" opens up instead of "New screenshot".

1. Open Spectacle
2. Focus on "New screenshot" with Tab
3. Click Enter

Help opens up.

Take a new screenshot.
Comment 1 Antonio Prcela 2019-09-22 10:10:42 UTC
Looks like almost everything, except Tools & Export, doesn't get proper focus with Tab.
Comment 2 Nate Graham 2019-10-07 03:36:54 UTC
Yep, I've noticed this too. That's because "Help" is the default button (which is really hard to tell with the default Breeze color scheme; see Bug 386158).

This window should not have a default button; it's not that kind of dialog. That should make the currently-focused button always  respond to the return key as we all expect.
Comment 3 Nate Graham 2019-10-07 16:14:01 UTC
Looks like we're hitting a Qt bug: https://bugreports.qt.io/browse/QTBUG-4021

We may need to execute an ugly workaround or else port the main window away from using QDialogButtonBox, which *really* wants to have a default button.
Comment 4 Nate Graham 2019-10-07 16:19:24 UTC
Darn, the workaround in that QT bug report doesn't work anymore. Guess we gotta do this the hard-but-correct way. :)
Comment 5 Nate Graham 2019-10-07 16:38:09 UTC
Git bisect says this was broken by https://cgit.kde.org/spectacle.git/commit/?id=da30d66cc7fd483af8600125da08eddceb02f11e.

Looks like that commit changed the "Take a New Screenshot" button from a PushButton to a ToolButton. That button was previously being the default button automatically by virtue of being a PushButton (which was masked by the fact that we explicitly gave it focus too). Being a ToolButton now, default button status devolves to the next PushButton, which happens to be the Help button on the bottom row.

David, could you take a look?
Comment 6 Antonio Prcela 2019-10-07 20:04:44 UTC
nice find Nate! I'll also take a look.
Comment 7 David Redondo 2019-10-08 08:27:33 UTC
(In reply to damiano38 from comment #0)
> After open Spectacle "New screenshot" button always is focused but now after
> click Enter always "Help" opens up instead of "New screenshot".
A workaround for you if you want to take screenshotsLA with a button press until this bug is fixed. Instead of using Enter you can use ctrl+n or alt+t (Might be different if you are using another Language).
Comment 8 David Redondo 2019-10-08 13:50:55 UTC
After debugging I am quite sure that the problem is not QDialogButtonBox but the fact that KSMainWindow is a QDialog. Which sets the default property of all pushbuttons with autoDefault==true to true in setVisible.
Because the button was also a pushbutton and had focus this worked before.

So we either set autoDefault to false for the three pushbuttons or change KSMainWindow's base to something else. I don't know if it currently uses some QDialog specific feature or if something else would do.
Comment 9 Nate Graham 2019-10-08 13:52:35 UTC
IIRC one of the reasons is so the main window closes when the Escape key is pressed, which is a dialog behavior.
Comment 10 David Redondo 2019-10-08 14:04:06 UTC
Quick test changing QDialog to QWidget confirms you're right. Escape to quit doesn't work anymore. Maybe we could implement this then ourselves. The drawback would be that no button would be activatable by Enter anymore. But quickly cross checking with 19.04 the toolbuttons (Configure, Copy  to Clipboard and Save) were apparently never and pressing enter when they had focused also opened the help menu. 
The best would be if we figure out a way to make always the button (tool or push) that has focus react to the key press.
Comment 11 David Redondo 2019-10-10 10:17:43 UTC
Git commit 34484c935c5c6ea91f11bc69791ca43e18022016 by David Redondo.
Committed on 10/10/2019 at 10:17.
Pushed by davidre into branch 'Applications/19.08'.

Make all buttons in the main window activatable with enter

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.
FIXED-IN: 19.08.3

Reviewers: #spectacle, ngraham

Reviewed By: #spectacle, ngraham

Subscribers: ngraham, aprcela, #spectacle

Tags: #spectacle

Differential Revision: https://phabricator.kde.org/D24508

M  +24   -0    src/Gui/KSMainWindow.cpp
M  +2    -0    src/Gui/KSMainWindow.h