Bug 385158 - QDialogButtonBox::Discard is overridden to use "edit-clear" icon; upstream setting ("edit-delete" icon) is correct
Summary: QDialogButtonBox::Discard is overridden to use "edit-clear" icon; upstream se...
Status: RESOLVED FIXED
Alias: None
Product: frameworks-kwidgetsaddons
Classification: Frameworks and Libraries
Component: general (show other bugs)
Version: unspecified
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Christoph Feck
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-09-27 23:22 UTC by Nate Graham
Modified: 2017-10-18 20:04 UTC (History)
4 users (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Graham 2017-09-27 23:22:56 UTC
When you create a QDialogButtonBox with a Discard button via QDialogButtonBox::Discard, it uses the edit-clear icon from the theme. An example is Spectacle's main window.

This is incorrect: the Discard action is destructive, so it should use the edit-delete icon. In fact, upstream Qt uses this icon by default: https://github.com/qt/qtbase/blob/6d699d08200b1fe3a616dfbc275d46c98b77fcbd/src/widgets/styles/qcommonstyle.cpp#L5780

Somewhere we're overriding this, and we should probably stop, because edit-delete is more correct.
Comment 1 Kai Uwe Broulik 2017-09-28 08:21:55 UTC
In frameworkintegration/src/kstyle/kstyle.cpp it is set to dialog-cancel
It is kwidgetsaddons KGuiItem::discard() that Spectacle is using that is set to edit-clear
Comment 2 Nate Graham 2017-09-28 13:11:48 UTC
Thanks for the info, Kai! I've submitted a patch for this: https://phabricator.kde.org/D8029
Comment 3 Nate Graham 2017-09-28 16:44:06 UTC
Git commit 6f30aad51452583d5bfc6815acae099b3f0e017c by Nathaniel Graham.
Committed on 28/09/2017 at 16:43.
Pushed by ngraham into branch 'master'.

Use edit-delete icon for destructive discard action

Summary:

Use edit-delete icon for buttons that execute destructive discard actions. This is what Upstream Qt originally had, but we were overriding it to use a less-appropriate icon (edit-clear)

Test Plan:
Tested in KDE Neon. An example of the change can be seen in Spectacle's main window:
{F4233176}

Reviewers: #frameworks, #vdg, dfaure, rkflx, davidedmundson, cfeck

Reviewed By: cfeck

Subscribers: cfeck, #frameworks

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

M  +1    -1    src/kstandardguiitem.cpp

https://commits.kde.org/kwidgetsaddons/6f30aad51452583d5bfc6815acae099b3f0e017c
Comment 4 Christoph Feck 2017-09-28 21:04:56 UTC
Thanks for your bug work, Nate. Hope you don't burn out too soon ;)
Comment 5 Nate Graham 2017-09-28 21:12:41 UTC
Thanks, me too! And thanks for cleaning up some of my messes when triaging bugs. I'll get better over time.

Judging by past experience with previous goal-directed hobbies, you should have my full-time attention for about 2 years. After that I'll burn out if I either feel like I'm not making progress, or lose interest if I've achieved the goal I set out to achieve (Improving KDE software's use cases for regular people such that I can switch my mother from her Mac and not have it be a support nightmare). Assuming I'm still making progress and the goal hasn't been achieved yet, I may just keep working on this stuff for a long time. :)

FWIW, you guys have been amazingly more patient and welcoming than the GNOME folks were when I first tried to jump on board their train. It's a pleasure to work together on this project.