Differential D10843

Remove symbolic icons in favor of standard icons
ClosedPublic
Actions

Authored by acrouthamel on Feb 25 2018, 8:52 PM.

Details

Reviewers
rkflx
ngraham
Group Reviewers
Gwenview
Commits
R260:a4b8511d65c6: Remove symbolic icons in favor of standard icons
Summary

This removes -symbolic icons in favor of theme-neutral standard icon names to ensure compatibility.

Follow-up to efaebe290f1b as discussed in D10770.

Before:


After:

Test Plan

Test Open Containing Folder and Trash menu options to ensure functionality.

Diff Detail

Repository
R260 Gwenview
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
acrouthamel requested review of this revision.Feb 25 2018, 8:52 PM
acrouthamel created this revision.
acrouthamel edited the summary of this revision. (Show Details)Feb 25 2018, 8:58 PM
acrouthamel added a reviewer: Gwenview.
acrouthamel added a project: Gwenview.
ngraham added a subscriber: ngraham.Feb 25 2018, 10:50 PM
ngraham added inline comments.
app/fileopscontextmanageritem.cpp
161–164

Is there a Bugzilla ticket tracking this TODO? If so, we should add it.

acrouthamel marked an inline comment as done.Feb 26 2018, 2:33 AM

Added bug report for gwenview. https://bugs.kde.org/show_bug.cgi?id=391078

ngraham added inline comments.Feb 26 2018, 4:21 AM
app/fileopscontextmanageritem.cpp
161–164

Thanks for the bug report! Can you mention it in this comment, so anyone who reads it in the future will be able to see what still needs to be done. or can know that it's been done and they can change the icon now?

acrouthamel updated this revision to Diff 28109.Feb 26 2018, 2:09 PM
acrouthamel marked an inline comment as done.
  • Merge branch 'Applications/17.12' into icons-symbolic-removal
  • Added bug link in comment for mTrashAction
acrouthamel added a comment.Feb 26 2018, 2:11 PM

I've added the bug link to the code comment. Let me know if that looks good now.

rkflx requested changes to this revision.Feb 26 2018, 10:33 PM
rkflx added a subscriber: rkflx.

Almost there ;)

app/fileopscontextmanageritem.cpp
161–164

Excuse my nitpicking for this one, only doing it here so I don't have to anymore in the future:

  • Add space after //.
  • Line breaks once in a while.
  • Prefered bug format: See Bug #1243 (I know there are bad examples…)
  • Don't repeat your point again.

Better:

// TODO: Change to only "user-trash" once Breeze changed it to provide the same
// colours for all sizes. Needed for visual consistency on HiDPI displays.
// See Bug #391078
162

Remove superfluous set of () and don't trust every code I paste into a random comment ;)

This revision now requires changes to proceed.Feb 26 2018, 10:33 PM
rkflx edited the summary of this revision. (Show Details)Feb 26 2018, 10:44 PM
acrouthamel updated this revision to Diff 28168.Feb 27 2018, 4:36 AM
  • Fixed comment formatting and verbage
acrouthamel marked 2 inline comments as done.Feb 27 2018, 4:38 AM

Ok should be fixed up now. Thanks for the nitpicking to make me a better coder. :)

ngraham accepted this revision as: ngraham.Feb 27 2018, 4:28 PM

Looks good to me now!

rkflx accepted this revision.Feb 27 2018, 4:39 PM

Thanks, now it can land ;)

@acrouthamel If you like it here, let us know if you need ideas for what to work on next…

This revision is now accepted and ready to land.Feb 27 2018, 4:39 PM
Closed by commit R260:a4b8511d65c6: Remove symbolic icons in favor of standard icons (authored by acrouthamel, committed by rkflx). · Explain WhyFeb 27 2018, 4:41 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mapp/fileopscontextmanageritem.cpp (7 lines)

Diff 28195

View Options

app/fileopscontextmanageritem.cpp

Loading...