D7828: createKMessageBox tries to focus a default button when available

[Henrik Fehlauer]

rkflx accepted this revision.
rkflx added a comment.


  In https://phabricator.kde.org/D7828#160998, @emateli wrote:
  
  > If this turns out to be okay, I'll edit the summary and title later
  
  
  Playing around with Dolphin's usage of `createKMessageBox`, I made the 
following observations:
  
  - Focus: Before on checkbox, now on first button.
  - Default: Before on some random button (changing with app restart), now 
consistently on first button.
  - `setFocus` to a different button was broken and is still broken.
  - `setDefault` to a different button was broken and is still broken.
  
  This is a great improvement and already fits Dolphin's case. As this does not 
regress any other usage (remember, this was broken already), I would be in 
favour of committing this now (after adapting the commit message).
  
  @aacid As you "requested changes", would you be okay with that?
  
  That said, a followup patch would be marvelous (add autotests, fix `setFocus` 
and `setDefault`).
  
  In https://phabricator.kde.org/D7828#161102, @elvisangelaccio wrote:
  
  > Does this mean that apps should always pass a parent? (if that's the case, 
we should mention it in the KMessageBox documentation)
  
  
  If at all, apps should //never// pass a parent, because it gets overwritten 
anyway. @emateli is just moving the code for this around, not changing it. But 
as the docs already state "This function will take care of connecting to it.", 
adding something about a parent will probably only cause more confusion.

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham, aacid, #vdg, rkflx
Cc: elvisangelaccio, rkflx, abetts, subdiff, ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Elvis Angelaccio]

elvisangelaccio added a comment.


  Does this mean that apps should always pass a parent? (if that's the case, we 
should mention it in the KMessageBox documentation)

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham, aacid, #vdg
Cc: elvisangelaccio, rkflx, abetts, subdiff, ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Emirald Mateli]

emateli updated this revision to Diff 21453.
emateli added a comment.


  Well, the diff of this patch took a sizeable reduction.
  
  I took a second look at this and here is what happened in the end and what 
caused the inconsistency with Dolphin.
  
  The issue was that the `QDialogButtonBox` argument, which contains the 
buttons needs to have a not null parent argument. If it does, then everything 
works as expected, if it doesn't (which Dolphin does not suppliy) then the 
focus is all messed up and will be given in order to the widgets created that 
can be focused.
  
  From what I dug out the order seems to be:
  
  1. Label (if the `AllowLink` flag is present)
  2. The string list
  3. Checkbox
  4. Left most action button
  
  Moving the call of `setParent` before any of the above mentioned widgets is 
created seems to fix this (I built this system-wide so all the KDE apps running 
can use it and no issues so far, although it's only been a couple of hours)
  
  If this turns out to be okay, I'll edit the summary and title later, since it 
is re-purposed in a way by not involving widget focus change.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7828?vs=20497=21453

REVISION DETAIL
  https://phabricator.kde.org/D7828

AFFECTED FILES
  src/kmessagebox.cpp

To: emateli, #frameworks, ngraham, aacid, #vdg
Cc: rkflx, abetts, subdiff, ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Henrik Fehlauer]

rkflx added a comment.


  In general, having a default button while setting the initial focus elsewhere 
is mainly useful for larger dialogs like our various standard configuration 
dialogs, where this provides a way to accept without tabbing forever. For 
smaller dialogs like a messagebox this is not that important, but at least 
consistent. For messageboxes my general expectation is that the default button 
should also have focus to more clearly guide the user in the best decision 
regarding what button choose.
  
  That said, I agree with @subdiff to at least allow deviations from this, 
while at the same time fixing Dolphin. BTW, Dolphin's inconsistency is the 
reason why I mentioned this Differential in https://phabricator.kde.org/T7022.
  
  > a fix for this change in behaviour when a parent widget is present or not
  
  This sounds reasonable and would be great if this fixed Dolphin. There is 
another odd behaviour in Dolphin's tab close confirmation dialog compared to 
e.g. Konsole's: First the default button is Quit, after switching focus around 
with [⇥] in every possible direction suddenly Close is default. Maybe this is 
related to not specifying a parent window and could help solving this?

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham, aacid, #vdg
Cc: rkflx, abetts, subdiff, ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Albert Astals Cid]

aacid added a comment.


  In https://phabricator.kde.org/D7828#160029, @ngraham wrote:
  
  > In other words, it seems intentional that the button marked default will 
always receive focus.
  
  
  No, open kate->settings->configure kate
  
  Ok is the default button and it is not supposed to have the focus, default == 
should have focus is simply not what default means.

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham, aacid, #vdg
Cc: abetts, subdiff, ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Emirald Mateli]

emateli added a comment.


  I think this is getting a bit out of hand here. Please try to read my last 
message where I explain why I decided to submit this patch and why I think it's 
a bug (the whole parent widget thing).
  
  Given the new information on why this occurs the whole focus by force thing 
might not be optimal (although I don't see it as being wrong either), so we can 
check for a solution that perhaps simply fixes the inconsistency when being 
called with a parent and without one.
  This could be easily solvable in Dolphin's repository where this would need 
only one additional parameter to the `createKMessageBox` call, but I still 
thing it belongs to be fixed in the frameworks.

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham, aacid, #vdg
Cc: abetts, subdiff, ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Nathaniel Graham]

ngraham added a comment.


  In https://phabricator.kde.org/D7828#160028, @aacid wrote:
  
  > In https://phabricator.kde.org/D7828#160018, @ngraham wrote:
  >
  > > Once you press tab the focus moves elsewhere and hitting return or enter 
will press whatever's selected rather than the original default button. So 
really the concept of a "Default Button" in our world seems to mostly be 
synonymous with "the button that receives focus by default." In this case, then 
there should only be one focus, and that focus should be on whatever is marked 
as a default button. Makes sense to me.
  >
  >
  > Hmmm, that's not really what it happens, is it?
  >  otherwise  "i am focused on the checkbox but press enter and the button 
gets pressed" would never happen?
  
  
  It seems that regardless, the condition with this Dolphin dialog box where 
the checkbox receives focus but another buttin is marked as Default is actually 
unintentional, and is a bug that should be fixed.
  
  Try this in Kate: Open a new document, type some stuff, and close it. In the 
resulting dialog box, he "Save" button is focused by default, but if you hit 
tab or the right arrow key, the focus will shift to another button and then 
hitting return or enter will press that button. This is a really nice feature.
  
  In other words, it seems intentional that the button marked default will 
always receive focus.

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham, aacid, #vdg
Cc: abetts, subdiff, ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Albert Astals Cid]

aacid added a comment.


  In https://phabricator.kde.org/D7828#160018, @ngraham wrote:
  
  > Once you press tab the focus moves elsewhere and hitting return or enter 
will press whatever's selected rather than the original default button. So 
really the concept of a "Default Button" in our world seems to mostly be 
synonymous with "the button that receives focus by default." In this case, then 
there should only be one focus, and that focus should be on whatever is marked 
as a default button. Makes sense to me.
  
  
  Hmmm, that's not really what it happens, is it?
  otherwise  "i am focused on the checkbox but press enter and the button gets 
pressed" would never happen?

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham, aacid, #vdg
Cc: abetts, subdiff, ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Nathaniel Graham]

ngraham added a comment.


  Once you press tab the focus moves elsewhere and hitting return or enter will 
press whatever's selected rather than the original default button. So really 
the concept of a "Default Button" in our world seems to mostly be synonymous 
with "the button that receives focus by default." In this case, then there 
should only be one focus, and that focus should be on whatever is marked as a 
default button. Makes sense to me.

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham, aacid, #vdg
Cc: abetts, subdiff, ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Albert Astals Cid]

aacid added a comment.


  In https://phabricator.kde.org/D7828#159598, @ngraham wrote:
  
  > In https://phabricator.kde.org/D7828#159556, @abetts wrote:
  >
  > > If you have two highlights, how do you know what's going to be activated 
when hitting enter?
  >
  >
  > My thoughts exactly.
  
  
  But it does not really solve that, once you press tab and then you still have 
the same problem, right?

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham, aacid, #vdg
Cc: abetts, subdiff, ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Emirald Mateli]

emateli added a comment.


  Hey @subdiff, thanks for your input on this. Whether this patch goes in or 
not, I still think that this "odd" behaviour is something that the frameworks 
should fix or change rather than relying on the developer of each application 
to do this.
  
  However I never expected this to stir up such a conversation given that I 
thought this was the normal behaviour in Plasma, given that all(?) the other 
dialogs created by the framework have the default button focused (See: Konsole 
close dialog w/ multiple tabs open (the exact same dialog as Dolphin's), or 
Konversation after having joined a channel. )
  
  Since these functions internally all rely on `createKMessageBox` and work 
properly so to say, I dug a bit deeper into this and found out the following 
why Dolphin's close dialog is the odd one out: If no parent window is specified 
when calling this function then the checkbox will be focused, if one is passed 
then the focus shifts onto the buttons.
  
  Current call: `QDialogButtonBox* buttons = new 
QDialogButtonBox(QDialogButtonBox::Yes | QDialogButtonBox::No | 
QDialogButtonBox::Cancel);`
  "Fixed" call: `QDialogButtonBox* buttons = new 
QDialogButtonBox(QDialogButtonBox::Yes | QDialogButtonBox::No | 
QDialogButtonBox::Cancel, dialog);`
  
  This leads me to believe that the checkbox having the focus is an 
irregularity, whether one would consider it a bug is a matter of prespective. 
But i would expect the frameworks to be consistent in what it does and whether 
the dialog has a parent or not should not affect what widget is focused.
  
  But just like @aacid mentioned, we should not look to break existing code, so 
given the new information on why this occurs, then perhaps a new implementation 
might be needed or simply a fix for this change in behaviour when a parent 
widget is present or not.

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham, aacid, #vdg
Cc: abetts, subdiff, ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Nathaniel Graham]

ngraham added a comment.


  In https://phabricator.kde.org/D7828#159556, @abetts wrote:
  
  > If you have two highlights, how do you know what's going to be activated 
when hitting enter?
  
  
  My thoughts exactly.

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham, aacid, #vdg
Cc: abetts, subdiff, ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Andres Betts]

abetts added a comment.


  I am leaning toward implementing the change. The reason being that the user 
gets a duplicate indication of focus when likely they might feel confused, I 
have been at times too, by not knowing clearly where the highlight is. If you 
have two highlights, how do you know what's going to be activated when hitting 
enter?

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham, aacid, #vdg
Cc: abetts, subdiff, ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Roman Gilg]

subdiff added a comment.


  Hi Emirald,
  
  first let me tell you that I totally get where you're coming from. To be 
honest the selected checkbox in this specific dialog of Dolphin has annoyed me 
already for a long time. So thank you for looking into this.
  
  But we have to dissect the situation a bit more: A button with focus and the 
default button in a KMessageBox are two different things per definition as 
Albert pointed out.
  
  That said I can only think of one sensible case where this distinction makes 
sense: When //no// element has focus, but you still want a default action to 
trigger on Enter. In any case having focus on some control element but then 
pressing Enter triggering an action related to some other button doesn't seem 
like good Ui to me. So this sounds similar to what you say.
  
  But in the end it should be up to the app developer to decide if the focus 
and default action element are the same one or different ones in his 
KMessageBox. I would still advise him in general to not make them different, 
but don't forget that your change is a change in Frameworks restricting it to 
our presumed best practice for //all// apps using a KMessageBox. And there 
might be corner cases we haven't thought about, where different focus and 
default action make sense.
  
  That said the focused checkbox in Dolphin should go away (as I said in the 
beginning it annoyed me already for a longer time). The rationale behind this 
was maybe, so a user could press Space for activating the checkbox and Enter 
for triggering the default action, but checking a focused checkbox with Space 
and not Enter isn't obvious and this checkbox also isn't something a user 
totally needs to have one button keyboard access to. So just set in this 
specific Dolphin dialog the focus item to the Ok button. By the way this is not 
something totally new to Dolphin. The "Confirm delete" dialog has its default 
button focused.

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham, aacid, #vdg
Cc: subdiff, ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Nathaniel Graham]

ngraham added a comment.


  In https://phabricator.kde.org/D7828#159511, @aacid wrote:
  
  > > Without this patch, the *actual* default button is virtually 
indistinguishable from other buttons when it doesn't have focus. It looks like 
this, with only a slightly lighter background than the other buttons:
  >
  > This is a bug of the breeze style, go fix it there.
  
  
  Sure: https://bugs.kde.org/show_bug.cgi?id=386158
  
  If the Breeze folks indicate that their preferred solution is to implement 
this patch, can we go for it?

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham, aacid, #vdg
Cc: ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Albert Astals Cid]

aacid added a comment.


  > You can't interact with the list so it is ruled out for being focused. The 
check box acts as a confirmation and thus is a secondary element which you also 
do not want to toggle by mistake, so it rests upon the buttons to have it.
  
  This makes some sense, but aren't you are breaking existing code?
  
  see detailedErrorInternal, detailedSorryInternal, they call setFocus on a 
button which you are probably then overriding?

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham, aacid, #vdg
Cc: ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Albert Astals Cid]

aacid added a comment.


  > Without this patch, the *actual* default button is virtually 
indistinguishable from other buttons when it doesn't have focus. It looks like 
this, with only a slightly lighter background than the other buttons:
  
  This is a bug of the breeze style, go fix it there.

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham, aacid, #vdg
Cc: ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Nathaniel Graham]

ngraham added a reviewer: VDG.

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham, aacid, #vdg
Cc: ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Emirald Mateli]

emateli added a comment.


  @aacid the issue at hand is that in this particular scenario the default 
button more or less equals to the one that has focus.
  
  There are only 3 types of widgets that can be created here: buttons, a list 
or a check box.
  
  You can't interact with the list so it is ruled out for being focused. The 
check box acts as a confirmation and thus is a secondary element which you also 
do not want to toggle by mistake, so it rests upon the buttons to have it.
  If any button is to have focus then it might as well be the default one,  
giving it to another button will break the enter button since the focused 
button's handler will be invoked over the default's. 
  This also has the added benefit of allowing to invoke the default action via 
space bar.

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham, aacid
Cc: ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Nathaniel Graham]

ngraham added a comment.


  @aacid sorry to be unclear.
  
  To most people, the default button is the one that's visually distinct. In 
Breeze, that generally is the one with a blue background.
  
  Without this patch, the *actual* default button is virtually 
indistinguishable from other buttons when it doesn't have focus. It looks like 
this, with only a slightly lighter background than the other buttons:
  
  F5446387: default wrong.png 
  
  With the patch, the default button gets focus, so it looks like a default 
button should (IMHO):
  
  F5446389: default correct.png 
  
  IMHO it's more important to put the focus where it will cause the button to 
look correct than on an unrelated UI element.

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham, aacid
Cc: ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Albert Astals Cid]

aacid added a comment.


  In https://phabricator.kde.org/D7828#159054, @ngraham wrote:
  
  > There actually is a bug being fixed here in that currently many buttons 
marked default aren't actually getting made default buttons in that they turn 
blue and pressing enter will click them. This fixes that.
  
  
  I don't understand this sentence, can you rewrite it and/or give an example?

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham, aacid
Cc: ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Albert Astals Cid]

aacid requested changes to this revision.
aacid added a comment.
This revision now requires changes to proceed.


  Marking as request changes since i'd like someone with UI expertise to 
confirm this is what we want since from the code point of view default = focus 
is not what the default property for buttons means.

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham, aacid
Cc: ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Albert Astals Cid]

aacid added a comment.


  Sorry to be blunt, but your opinion doesn't matter.
  
  Here is the definition of the default property for a button.
  
  http://doc.qt.io/qt-5/qpushbutton.html#default-prop
  
  Nowhere in it says "this is the button that should have focus when the dialog 
pops up".

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham
Cc: ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Emirald Mateli]

emateli added a comment.


  In my opinion a "Default" widget, should also have focus. Makes little sense 
to have some widget focused and then pressing enter to trigger the action of 
another one. 
  By focusing, the default action is also made visible and allows to invoke the 
it action by pressing space bar. And no, I haven't spoken to the usability 
team. What is the team name so we can tag them here perhaps?
  
  After running some tests even when manually setting some other action to 
default, enter always invokes the same action. Eg: in the case of a Yes, No, 
Cancel dialog, the yes will always be called regardless.

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham
Cc: ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Nathaniel Graham]

ngraham added a comment.


  There actually is a bug being fixed here in that currently many buttons 
marked default aren't actually getting made default buttons in that they turn 
blue and pressing enter will click them. This fixes that.

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham
Cc: ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Albert Astals Cid]

aacid added a comment.


  Sorry but after reading what the patch does i'm not sure this makes sense.
  
  Default button just means "the button that is triggered when pressing Enter", 
it has nothing to do with "this is the button that should have focus".
  
  Have you talked with the usability people about this?

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham
Cc: ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


  +1, this works great for me. Any other objections, or can I land this?

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks, ngraham
Cc: ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Nathaniel Graham]

ngraham added a comment.


  @aacid, all good now?

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks
Cc: ngraham, aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Emirald Mateli]

emateli updated this revision to Diff 20497.
emateli added a comment.


  Added autotests

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7828?vs=19543=20497

REVISION DETAIL
  https://phabricator.kde.org/D7828

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kmessageboxautotest.cpp
  autotests/kmessageboxautotest.h
  src/kmessagebox.cpp

To: emateli, #frameworks
Cc: aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Albert Astals Cid]

aacid added a comment.


  In https://phabricator.kde.org/D7828#152060, @emateli wrote:
  
  > @aacid just to make clear, you're saying to add another case to the 
`testMessageBox` on `kmessageboxtest.cpp` right?
  
  
  No, look in the autotest folder not in the test folder.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks
Cc: aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Emirald Mateli]

emateli added a comment.


  @aacid just to make clear, you're saying to add another case to the 
`testMessageBox` on `kmessageboxtest.cpp` right?

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks
Cc: aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Albert Astals Cid]

aacid added a comment.


  This should come with an autotest

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks
Cc: aacid, #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Emirald Mateli]

emateli edited the summary of this revision.
emateli edited the test plan for this revision.
emateli added a reviewer: Frameworks.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D7828

To: emateli, #frameworks
Cc: #frameworks

D7828: createKMessageBox tries to focus a default button when available

[Emirald Mateli]

emateli created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  createKMessageBox-btn-focus

REVISION DETAIL
  https://phabricator.kde.org/D7828

AFFECTED FILES
  src/kmessagebox.cpp

To: emateli
Cc: #frameworks