Differential D17281

Add icon to KBuildSycocaProgressDialog cancel button
ClosedPublic
Actions

Authored by GB_2 on Dec 1 2018, 2:31 PM.

Details

Reviewers
ngraham
cfeck
elvisangelaccio
Group Reviewers
Frameworks
Commits
R241:3b320f7a32f3: Add icon to KBuildSycocaProgressDialog cancel button
Summary

The cancel button in the KBuildSycocaProgressDialog (showing that the system configuration is updated) has no icon, so this fixes it.

Test Plan

Open KMenuEdit and click save.

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
GB_2 created this revision.Dec 1 2018, 2:31 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptDec 1 2018, 2:31 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
GB_2 requested review of this revision.Dec 1 2018, 2:31 PM
ngraham accepted this revision.Dec 1 2018, 2:58 PM
ngraham added reviewers: Frameworks, cfeck.
ngraham added a subscriber: ngraham.

Works and seems reasonable enough. Anyone else have any opinions on the matter?

Also @GB_2, please provide your real name and email address so we can land this patch with correct authorship information. If you use arc for your next patch (https://community.kde.org/Infrastructure/Phabricator#Using_Arcanist), we won't have to nag you like this. :)

Also in the future, populating the Test Plan section can make life easier for your reviewer. In this case, something as simple as "open KMenuEdit, change something, and click save" would be nice.

This revision is now accepted and ready to land.Dec 1 2018, 2:58 PM
GB_2 added a comment.Dec 1 2018, 3:23 PM

Ok, thanks for the info :-)

elvisangelaccio added a subscriber: elvisangelaccio.Dec 1 2018, 3:28 PM
elvisangelaccio added inline comments.
src/widgets/kbuildsycocaprogressdialog.cpp
71–73

I'm surprised this works. setCancelButton() take ownership of the button, but the button's parent is created on the stack.

I'd just create it on the heap to be sure. Better safe then sorry.

GB_2 updated this revision to Diff 46637.Dec 1 2018, 7:19 PM
GB_2 edited the test plan for this revision. (Show Details)

Improved code.

GB_2 marked an inline comment as done.Dec 1 2018, 7:19 PM
elvisangelaccio added inline comments.Dec 2 2018, 9:48 AM
src/widgets/kbuildsycocaprogressdialog.cpp
71

Please also pass this as parent, otherwise the button box will leak.

GB_2 updated this revision to Diff 46677.Dec 2 2018, 10:12 AM

Improve code (pass this as parent).

GB_2 marked an inline comment as done.Dec 2 2018, 10:12 AM
elvisangelaccio accepted this revision.Dec 2 2018, 10:12 AM
ngraham added a comment.Dec 2 2018, 6:24 PM

@GB_2, can you provide an email address so we can land this for you? Thanks!

GB_2 added a comment.EditedDec 2 2018, 10:12 PM
In D17281#370253, @ngraham wrote:

@GB_2, can you provide an email address so we can land this for you? Thanks!

How can I provide an email address in the Phabricator webinterface?
I already provided an email in my email settings on here...

bcooksley added a subscriber: bcooksley.Dec 3 2018, 8:29 AM

Phabricator has a strong privacy model, meaning your email address is not accessible to anyone except you, so you'll need to post it here in this review so it can be committed to our repositories.
Sorry for any confusion around this.

GB_2 added a comment.Dec 3 2018, 9:30 AM

Ok.
Email: feber70@gmail.com

Closed by commit R241:3b320f7a32f3: Add icon to KBuildSycocaProgressDialog cancel button (authored by GB_2, committed by ngraham). · Explain WhyDec 6 2018, 8:04 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 46998

View Options

src/widgets/kbuildsycocaprogressdialog.cpp

Loading...