Differential D14378

Remove custom icon selection for trash
ClosedPublic
Actions

Authored by shubham on Jul 25 2018, 3:59 PM.

Details

Reviewers
ngraham
broulik
pino
elvisangelaccio
cfeck
Group Reviewers
Dolphin
Commits
R318:6e04f50081a9: Remove custom icon selection for trash
Summary

CCBUG: 391200

Test Plan
  1. Open Dolphin
  2. Edit trash in places item

Result: No custom icon option available

Diff Detail

Repository
R318 Dolphin
Lint
Lint Skipped
Unit
Unit Tests Skipped
shubham created this revision.Jul 25 2018, 3:59 PM
Restricted Application added a project: Dolphin. · View Herald TranscriptJul 25 2018, 3:59 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
shubham requested review of this revision.Jul 25 2018, 3:59 PM
ngraham requested changes to this revision.Jul 25 2018, 5:29 PM
ngraham added a reviewer: broulik.
ngraham added inline comments.
src/panels/places/placesitemeditdialog.cpp
149

Let's do this a little differently to conform to KDE style guidelines:

if (m_url.scheme() == QLatin1String("trash")) {
    formLayout->addRow(i18nc("@label", "Choose an icon:"), m_iconButton);
} else {
    m_iconButton->setVisible(false);
}
This revision now requires changes to proceed.Jul 25 2018, 5:29 PM
ngraham added a comment.Jul 25 2018, 5:31 PM

Also, can you add CCBUG: 391200 to the Summary section and edit your Test Plan to indicate that it should be done in Dolphin?

shubham added a comment.Jul 25 2018, 5:35 PM

yeah sure, btw in the inline comment I think you meant .... if (m_url.scheme() != QLatin1String("trash")) instead of ==

ngraham added a comment.Jul 25 2018, 5:37 PM

Er, yeah, oops. Or just reverse the ordering of the statements inside the if and else blocks.

pino added a subscriber: pino.Jul 25 2018, 5:37 PM
pino added inline comments.
src/panels/places/placesitemeditdialog.cpp
149

Or even better, not create the KIconButton at all, in that case -- note you will need to check all the usages of m_iconButton.

shubham updated this revision to Diff 38447.Jul 25 2018, 5:45 PM
shubham edited the summary of this revision. (Show Details)
shubham edited the test plan for this revision. (Show Details)
ngraham added inline comments.Jul 25 2018, 5:47 PM
src/panels/places/placesitemeditdialog.cpp
150

Indent this line

ngraham added inline comments.Jul 25 2018, 5:49 PM
src/panels/places/placesitemeditdialog.cpp
149

+1 for not even creating it at all, if that's feasible.

shubham updated this revision to Diff 38449.Jul 25 2018, 5:56 PM
ngraham added a comment.Jul 25 2018, 6:04 PM

Is this the right diff? It doesn't include the earlier changes.

shubham updated this revision to Diff 38450.Jul 25 2018, 6:10 PM

sorry i uploaded diff relative to my local branch, now uploading correct diff

ngraham accepted this revision.Jul 25 2018, 7:24 PM

This works fine for me, and I think the code is sensible. It's quite a mess to not set m_iconButton at all in this case, so I'm okay with the patch as-is.

Let's wait for a final opinion from another Dolphin person.

This revision is now accepted and ready to land.Jul 25 2018, 7:24 PM
pino added a comment.Jul 26 2018, 5:28 AM
In D14378#298042, @ngraham wrote:

This works fine for me, and I think the code is sensible. It's quite a mess to not set m_iconButton at all in this case, so I'm okay with the patch as-is.

There is not much code associated, I don't see the complications...

shubham updated this revision to Diff 38525.Jul 26 2018, 6:01 PM

Create m_iconButton for all entries except TRASH

pino requested changes to this revision.Jul 26 2018, 6:09 PM

It is a good start, but not enough:

  • PlacesItemEditDialog::icon() will crash now, so its usage of m_iconButton must be guarded
src/panels/places/placesitemeditdialog.cpp
147 ↗(On Diff #38449)

No need to SCREAM in comments :)

This revision now requires changes to proceed.Jul 26 2018, 6:09 PM
elvisangelaccio requested changes to this revision.Jul 28 2018, 3:53 PM
elvisangelaccio added a subscriber: elvisangelaccio.

This will crash as soon as you try to edit the Trash place (because m_iconButton in PlacesItemEditDialog::icon() won't be initialized).

shubham updated this revision to Diff 38666.Jul 28 2018, 4:38 PM

dedicated function to check if entry is editable or not.

pino requested changes to this revision.Jul 28 2018, 4:41 PM
pino added inline comments.
src/panels/places/placesitemeditdialog.cpp
67–71 ↗(On Diff #38449)

This is slightly overkill:

return m_iconButton ? m_iconButton->icon() : QString();

is more than enough.

This revision now requires changes to proceed.Jul 28 2018, 4:41 PM
shubham updated this revision to Diff 38667.Jul 28 2018, 4:44 PM
This comment was removed by shubham.
shubham updated this revision to Diff 38668.Jul 28 2018, 4:47 PM
pino requested changes to this revision.Jul 28 2018, 4:54 PM

No need to rush with the updates, please take your time to test your changes.

src/panels/places/placesitemeditdialog.cpp
60 ↗(On Diff #38449)

This is unused, and it will even fail to build (since there is no more class declaration for it).

This revision now requires changes to proceed.Jul 28 2018, 4:54 PM
shubham updated this revision to Diff 38669.Jul 28 2018, 4:58 PM
This comment was removed by shubham.
pino requested changes to this revision.Jul 28 2018, 5:02 PM

Please, please, please:

  • again, no rush with changes
  • read again your own changes, because it looks like there is even no review by the own patch author
  • TEST your changes
src/panels/places/placesitemeditdialog.cpp
62 ↗(On Diff #38449)

And actually this must return m_icon in case m_iconButton is null, to look like there was no icon change.

This revision now requires changes to proceed.Jul 28 2018, 5:02 PM
shubham updated this revision to Diff 38672.Jul 28 2018, 5:34 PM

return m_icon in case m_iconButton is nullptr

ngraham accepted this revision.Jul 30 2018, 11:03 PM

Thanks, this now works, doesn't crash, and setting the icon still works for other locations. Code looks sane to me, but is this also good to you guys now, @pino, @broulik, and @elvisangelaccio?

ngraham added a subscriber: cfeck.Aug 2 2018, 12:39 PM

D14360 was just committed; can we get a final review on this?

@cfeck?

cfeck accepted this revision.Aug 2 2018, 12:56 PM

This looks fine to me, and for consistency with KIO commit, we should ship 18.08 with this commit. Unless there are objections, or someone beats me to it, I will commit it to 18.08 branch in the coming days.

ngraham accepted this revision.Aug 2 2018, 12:57 PM

+1, please do. Thanks Christoph!

shubham added a comment.Aug 2 2018, 2:27 PM
This comment was removed by shubham.
elvisangelaccio accepted this revision.Aug 4 2018, 2:15 PM

@pino does it look good to you now?

pino added a comment.Aug 4 2018, 4:56 PM
In D14378#303280, @elvisangelaccio wrote:

@pino does it look good to you now?

I cannot find anything wrong (finally, eh).

ngraham added a comment.Aug 4 2018, 4:57 PM

Can you change your status to Accepted or else resign from the revision, then?

pino added a comment.Aug 4 2018, 5:27 PM
In D14378#303371, @ngraham wrote:

Can you change your status to Accepted or else resign from the revision, then?

This does not make any sense to me. This looks already with the acks needed, doesn't it?

ngraham added a comment.Aug 4 2018, 5:30 PM
In D14378#303383, @pino wrote:
In D14378#303371, @ngraham wrote:

Can you change your status to Accepted or else resign from the revision, then?

This does not make any sense to me. This looks already with the acks needed, doesn't it?

The current status is "Needs Review" up at the top of the page because you're still marked as a "Request Changes". Phabricator doesn't mark it as accepted as long as there's anyone who's still requesting changes.

pino added a comment.Aug 4 2018, 5:41 PM
In D14378#303384, @ngraham wrote:
In D14378#303383, @pino wrote:
In D14378#303371, @ngraham wrote:

Can you change your status to Accepted or else resign from the revision, then?

This does not make any sense to me. This looks already with the acks needed, doesn't it?

The current status is "Needs Review" up at the top of the page because you're still marked as a "Request Changes". Phabricator doesn't mark it as accepted as long as there's anyone who's still requesting changes.

Then fix Phabricator, since my "Request changes" is not for the latest version (which has only acks), but for previous ones.

ngraham added a comment.Aug 4 2018, 6:41 PM
In D14378#303387, @pino wrote:
In D14378#303384, @ngraham wrote:
In D14378#303383, @pino wrote:
In D14378#303371, @ngraham wrote:

Can you change your status to Accepted or else resign from the revision, then?

This does not make any sense to me. This looks already with the acks needed, doesn't it?

The current status is "Needs Review" up at the top of the page because you're still marked as a "Request Changes". Phabricator doesn't mark it as accepted as long as there's anyone who's still requesting changes.

Then fix Phabricator, since my "Request changes" is not for the latest version (which has only acks), but for previous ones.

I can't change Phabricator, but you can adapt to its quirks as we all have. It's just two clicks.

pino added a comment.Aug 4 2018, 7:48 PM
In D14378#303429, @ngraham wrote:

It's just two clicks.

... that change the role I have in this review. Sorry, I will not do that. You have already all the data needed, including the fact that the patch is acked already. Unless you really care about the green tick, just to see that "everybody accepted it".

rkflx added a subscriber: rkflx.Aug 4 2018, 10:52 PM
In D14378#303463, @pino wrote:

... that change the role I have in this review. Sorry, I will not do that. You have already all the data needed, including the fact that the patch is acked already. Unless you really care about the green tick, just to see that "everybody accepted it".

There are two more options:

  • You "resign" as a reviewer from the revision.
  • Someone else "removes" you as a reviewer.
ngraham added a comment.Aug 4 2018, 11:08 PM

Regardless, I'll land this on the stable branch now (per @cfeck) since there don't appear to be any remaining change requests.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 4 2018, 11:09 PM
Closed by commit R318:6e04f50081a9: Remove custom icon selection for trash (authored by shubham, committed by ngraham). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 38450

View Options

src/panels/places/placesitemeditdialog.cpp

Loading...