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:
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,
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`
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
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
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
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
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
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
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.
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
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,
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
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
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
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
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.
ngraham added a reviewer: VDG.
REVISION DETAIL
https://phabricator.kde.org/D7828
To: emateli, #frameworks, ngraham, aacid, #vdg
Cc: ngraham, aacid, #frameworks
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
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
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
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
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".
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
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:
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
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,
ngraham added a comment.
@aacid, all good now?
REVISION DETAIL
https://phabricator.kde.org/D7828
To: emateli, #frameworks
Cc: ngraham, aacid, #frameworks
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
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
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
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
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
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
34 matches
Mail list logo
