Add icons for context and sidebar menu
ClosedPublic

Authored by acrouthamel on Feb 21 2018, 7:29 PM.

Details

Summary

As requested by @rkflx, menu icons have been added for the context and sidebar menus, continuing the work completed in D10503.

Operations Sidebar Before:


Operations Sidebar After:

Context Before:

Context After:

Trash Context Before:

Trash Context After:

Test Plan

Execute Gwenview, perform moves/copies/links/redeye actions. Verify icon resolution and appropriateness.

Diff Detail

Repository
R260 Gwenview
Branch
icons-fix (branched from Applications/17.12)
Lint
No Linters Available
Unit
No Unit Test Coverage
acrouthamel requested review of this revision.Feb 21 2018, 7:29 PM
acrouthamel created this revision.
acrouthamel edited the summary of this revision. (Show Details)
acrouthamel edited the summary of this revision. (Show Details)Feb 21 2018, 7:32 PM
rkflx edited the test plan for this revision. (Show Details)Feb 21 2018, 7:34 PM
rkflx added a subscriber: muhlenpfordt.

@acrouthamel Wow, that was fast! Thanks.

@muhlenpfordt Could you help out reviewing? Otherwise I'll never get to your patches…

@acrouthamel Wow, that was fast! Thanks.

I aim to please. :)

ngraham accepted this revision.Feb 22 2018, 12:08 AM

Wonderful. Works great! Those missing icons always bugged me, too.

This revision is now accepted and ready to land.Feb 22 2018, 12:08 AM

@ngraham Did you check whether some of the changes should be done in Frameworks? Are there still some spots missing in different context menus?

Yes, these can't be Frameworks changes via KStandardAction because they're all Gwenview-specific. There is still one missing icon in context menus: Edit Tags. @acrouthamel, if you want to add that one too, that would be nice. tag-new seems like an appropriate icon.

acrouthamel added a comment.EditedFeb 22 2018, 12:57 AM

Yes, these can't be Frameworks changes via KStandardAction because they're all Gwenview-specific. There is still one missing icon in context menus: Edit Tags. @acrouthamel, if you want to add that one too, that would be nice. tag-new seems like an appropriate icon.

I noticed that between the version I have installed on Kubuntu, to the Git version, Edit Tags is now gone (missing in my screenshots which is based on latest Git). I also left off icons on the sub-menus (Open With), as that seemed to be the standard.

rkflx requested changes to this revision.Feb 22 2018, 1:30 PM

There is still one missing icon in context menus: Edit Tags. @acrouthamel, if you want to add that one too, that would be nice. tag-new seems like an appropriate icon.

I noticed that between the version I have installed on Kubuntu, to the Git version, Edit Tags is now gone (missing in my screenshots which is based on latest Git).

That would be horrible if something like that had slipped through in the last months without someone noticing! It's likely just a setup problem on your side.

Look at these lines in the file you linked: Probably GWENVIEW_SEMANTICINFO_BACKEND_NONE is not set for you, see CMakeLists.txt. I guess you are just missing the "devel" package for Baloo.

To get all build dependencies, in general you can use apt-get build-dep <app> on Ubuntu or zypper source-install --build-deps-only <app> (zypper si -d for short) on openSUSE. Sometimes the distro packages are older and miss some newer dependencies of the Git version, so when running cmake you should still watch out for missing dependencies, in particular "optional" ones (for required deps CMake will error out anyway). Rerun CMake (sometimes you have to delete CMakeCache.txt beforehand), then try again.

Let me know if this works for you, so we can have the tag icon too ;)

I also left off icons on the sub-menus (Open With), as that seemed to be the standard.

+1

This revision now requires changes to proceed.Feb 22 2018, 1:30 PM

I guess you are just missing the "devel" package for Baloo.

To get all build dependencies, in general you can use apt-get build-dep <app> on Ubuntu. Rerun CMake (sometimes you have to delete CMakeCache.txt beforehand), then try again.

I ran apt build-dep gwenview which did indeed install baloo-kf5-dev which was missing. I'm using KDevelop, so I ran a clean on the project and re-built. No notifications about missing packages, but Edit Tags was still missing for me. I installed baloo-dev manually, to see if that would help, ran a clean and re-build again. Edit Tags still won't show for me.

Any ideas? I'm running Kubuntu 17.10.

rkflx added a comment.EditedFeb 22 2018, 4:34 PM

A single make clean is not enough, because that only removes the binaries, but not the "configuration" of the source. Did you remove CMakeCache.txt from your build dir? In such cases I always fall back to using the shell, so I see exactly what's going on:

cd <build dir>
rm CMakeCache.txt
cmake <source dir>

What's important here is the output of the last command. No idea about KDevelop, but it should allow something akin to that and also show the CMake output. If that does not help, remove the build dir and start over.

A single make clean is not enough, because that only rebuilds the binaries, but not the "configuration" of the source. Did you remove CMakeCache.txt from your build dir?

Hey, look at that. If I actually listen, it works. I'll have a new diff in a few minutes.

rkflx added a comment.Feb 22 2018, 4:42 PM

Ah, that's great.

Just looked at KDevelop, if you click on ProjectPrune Selection, it issues rm -rf <build dir>, which corresponds to what I wrote above.

Ah, that's great.

Just looked at KDevelop, if you click on ProjectPrune Selection, it issues rm -rf <build dir>, which corresponds to what I wrote above.

Cool, thanks. I'm just starting with it, so I'm stumbling around still.

Added icons for Edit Tags and Restore in context menus.

acrouthamel edited the summary of this revision. (Show Details)Feb 22 2018, 5:02 PM

Ok updated. I used the "tag" icon since its generic for either adding/removing/editing tag names.

Bonus round: I added "edit-undo" for the Restore action in the context menu while in the Trash folder.

rkflx added a comment.Feb 22 2018, 5:07 PM

Ok updated. I used the "tag" icon since its generic for either adding/removing/editing tag names.

Yeah, I like that one better, too. What do you think about link instead of insert-link?

Formally you should also test whether it still looks good when changing the icon theme to "Oxygen".

Ok updated. I used the "tag" icon since its generic for either adding/removing/editing tag names.

Yeah, I like that one better, too. What do you think about link instead of insert-link?

It is a bit small due to the icon design on Breeze. What about emblem-symbolic-link?

Formally you should also test whether it still looks good when changing the icon theme to "Oxygen".

Is Oxygen still official as well? I didn't know.

Oh wait, did I just link to the same file? Lol. link links to emblem-symbolic-link.

acrouthamel edited the summary of this revision. (Show Details)

Changed insert-link to link.

acrouthamel edited the summary of this revision. (Show Details)Feb 22 2018, 5:28 PM
rkflx added a comment.Feb 22 2018, 5:28 PM

Ok updated. I used the "tag" icon since its generic for either adding/removing/editing tag names.

Yeah, I like that one better, too. What do you think about link instead of insert-link?

Turns out link does not work on Oxygen, but still I find link better for Breeze where most users are. I think we could make a little compromise here, i.e. if Oxygen users want the icon to show up, they should file a bug report against Oxygen…

Not sure what is officially supported, I guess only Breeze. Still thinking about the other cases is sometimes useful.

Bonus round: I added "edit-undo" for the Restore action in the context menu while in the Trash folder.

Cool, I think we've covered all icons now.

Oh wait, did I just link to the same file? Lol. link links to emblem-symbolic-link.

For me both look just the same, no difference in size. Perhaps share a screenshot? For me it looks like this:


Anyway, accepting for now.

@ngraham Still happy with the changed icons? If you are, we can land.

rkflx accepted this revision.Feb 22 2018, 5:28 PM
This revision is now accepted and ready to land.Feb 22 2018, 5:28 PM
acrouthamel edited the summary of this revision. (Show Details)Feb 22 2018, 5:31 PM

Yeah let's go with

Ok updated. I used the "tag" icon since its generic for either adding/removing/editing tag names.

Yeah, I like that one better, too. What do you think about link instead of insert-link?

Turns out link does not work on Oxygen, but still I find link better for Breeze where most users are. I think we could make a little compromise here, i.e. if Oxygen users want the icon to show up, they should file a bug report against Oxygen…

I agree. Oxygen seems to be missing a number of icons compared to Breeze. Missing icons should go to them.

Oh wait, did I just link to the same file? Lol. link links to emblem-symbolic-link.

For me both look just the same, no difference in size. Perhaps share a screenshot? For me it looks like this:

Yeah I checked the Breeze code. It's just a symlink between the two.

ngraham added a comment.EditedFeb 22 2018, 5:34 PM

I think there's one final bit of polish we can do here. In the "after" screenshots, most icons use the "symbolic" line-art style, but a few show colored folders when run in HiDPI mode, which your screenshots depict. Perhaps we should take the opportunity to fix this, too. It's especially odd for the Trash icon. Looks like it's using user-trash when it should be using user-trash-symbolic.

acrouthamel added a comment.EditedFeb 22 2018, 5:37 PM

I think there's one final bit of polish we can do here. In the "after" screenshots, most icons use the "symbolic" line-art stylr with Breeze, but a few show colored folders when run in HiDPI mode, which your screenshots depict. Perhaps we should take the opportunity to fix this, too. It's especially odd for the Trash icon.

This happened as well in KTorrent D10688. I believe it is an issue in the icon theme itself. Since Qt grabs a "hi-res" icon on HiDPI resolutions, it checks the theme for one, and the theme serves up a standard colored icon, rather than the purpose-built monochrome icons. I guess Breeze would need to make some hires monochrome icons and somehow designate that in the config so Qt would grab the right ones?

rkflx added a comment.Feb 22 2018, 5:41 PM

You are both right. The generic issue is tracked in D6313, but it is a bit stuck.

When using user-trash-symbolic, things get better. Let's do this as a workaround.

Any ideas for folder-open?

ngraham added a comment.EditedFeb 22 2018, 5:44 PM

Hmm, not sure I agree. The -symbolic icons exist specifically for this purpose: to always show a "symbolic" version. The non-symbolic version happens to look identical at a small size, but when scaled up, takes on its regular full-color appearance.

You are both right. The generic issue is tracked in D6313, but it is a bit stuck.

When using user-trash-symbolic, things get better. Let's do this as a workaround.

Any ideas for folder-open?

How about document-open-folder?

I think there's one final bit of polish we can do here. In the "after" screenshots, most icons use the "symbolic" line-art style, but a few show colored folders when run in HiDPI mode, which your screenshots depict. Perhaps we should take the opportunity to fix this, too. It's especially odd for the Trash icon. Looks like it's using user-trash when it should be using user-trash-symbolic.

You are both right. The generic issue is tracked in D6313, but it is a bit stuck.

When using user-trash-symbolic, things get better. Let's do this as a workaround.

Ohhh, I see how it works now. Yeah, if you use the symbolic icons, it will symlink to the /16 SVGs. Good, good.

Any ideas for folder-open?

How about we ask Breeze to add a symbolic icon for /16/folder-open?

How about document-open-folder?

That works!

rkflx added a comment.Feb 22 2018, 5:49 PM

Any ideas for folder-open?

folder-open-symbolic works too. Would that be even better?

Hmm, not sure I agree. The -symbolic icons exist specifically for this purpose: to always show a "symbolic" version. The non-symbolic version happens to look identical at a small size, but when scaled up,

TIL, is that documented somewhere?

How about we ask Breeze to add a symbolic icon for /16/folder-open?

Do we have to? Using that works for me when trying in Gwenview.

Any ideas for folder-open?

folder-open-symbolic works too. Would that be even better?

Yes, that is better. Didn't see that the first time around. Let's use folder-open-symbolic.

Hmm, not sure I agree. The -symbolic icons exist specifically for this purpose: to always show a "symbolic" version. The non-symbolic version happens to look identical at a small size, but when scaled up,

TIL, is that documented somewhere?

This is just based on VDG conversations; perhaps it should be. The HIG is currently being migrated to Sphinx and revamped...

How about we ask Breeze to add a symbolic icon for /16/folder-open?

Do we have to? Using that works for me when trying in Gwenview.

Yeah, no need to bother IMHO. We have a good option already.

acrouthamel edited the summary of this revision. (Show Details)

Fixed icons with -symbolic versions for HiDPI screens.

acrouthamel edited the summary of this revision. (Show Details)Feb 22 2018, 5:55 PM

Phew, OK. I think it's all fixed now. New screenshots as well. Man, now I have to go fix poor KTorrent...

ngraham accepted this revision.Feb 22 2018, 5:56 PM

Now that looks professional! Let's ship it!

What do you think @rkflx? Master or Stable?

One more thing I just noticed, if you want bonus points:

use edit-delete-symbolic for the red Trash icons, so their lines have the same weight as the black/white Trash icons.

rkflx added a comment.Feb 22 2018, 6:04 PM

What do you think @rkflx? Master or Stable?

Somehow it's both, but as this one contains a HiDPI fix, I won't complain too loudly if you land to stable ;)

@acrouthamel, if you'd like to land this on stable, then you'll need to re-base it on the stable branch. See https://community.kde.org/Infrastructure/Phabricator#Landing_on_the_.22Stable_branch.22

One more thing I just noticed, if you want bonus points:

use edit-delete-symbolic for the red Trash icons, so their lines have the same weight as the black/white Trash icons.

Ok 1 sec. It looks like that icon is built-in, so I believe I can override it.

One more thing I just noticed, if you want bonus points:

use edit-delete-symbolic for the red Trash icons, so their lines have the same weight as the black/white Trash icons.

Ok 1 sec. It looks like that icon is built-in, so I believe I can override it.

In that case, we might instead need to change a KStandardAction somewhere...

One more thing I just noticed, if you want bonus points:

use edit-delete-symbolic for the red Trash icons, so their lines have the same weight as the black/white Trash icons.

Ok 1 sec. It looks like that icon is built-in, so I believe I can override it.

In that case, we might instead need to change a KStandardAction somewhere...

Would you prefer me to not override here then and we'll just fix KStandardAction?

One more thing I just noticed, if you want bonus points:

use edit-delete-symbolic for the red Trash icons, so their lines have the same weight as the black/white Trash icons.

Ok 1 sec. It looks like that icon is built-in, so I believe I can override it.

In that case, we might instead need to change a KStandardAction somewhere...

Would you prefer me to not override here then and we'll just fix KStandardAction?

Yep!

rkflx added a comment.Feb 22 2018, 6:12 PM

One more thing I just noticed, if you want bonus points:

use edit-delete-symbolic for the red Trash icons, so their lines have the same weight as the black/white Trash icons.

Interesting. Looking at it that way, we'd also have to use document-open-folder instead of folder-open-symbolic, or perhaps I'm just confused now!?

One more thing I just noticed, if you want bonus points:

use edit-delete-symbolic for the red Trash icons, so their lines have the same weight as the black/white Trash icons.

Interesting. Looking at it that way, we'd also have to use document-open-folder instead of folder-open-symbolic, or perhaps I'm just confused now!?

I think so. :) The -symbolic icons are supposed to have heavy line weights. So folder-open-symbolic is fine. But if you look at the screenshot that shown both trash icons, it's clear that one has heavy line weights, and the other one doesn't. This may be relevant to all the other icons, too. But perhaps we're overloading this poor patch at this point, and should use another one to track the move to using -symbolic icons in menus etc.

All of this is only relevant to HiDPI; there's no difference with a 1.0 scale factor. So we are on the tip of the spear here, making improvements that only a select few will see!

@acrouthamel, if you'd like to land this on stable, then you'll need to re-base it on the stable branch. See https://community.kde.org/Infrastructure/Phabricator#Landing_on_the_.22Stable_branch.22

Once I figure out how to do that on KDevelop... I will. Lol. Uh, gimme a few minutes.

acrouthamel edited the test plan for this revision. (Show Details)

Rebase as requested.

ngraham closed this revision.Feb 22 2018, 7:20 PM

For me both look just the same, no difference in size. Perhaps share a screenshot? For me it looks like this:

Which app is this? This one?

rkflx added a comment.Feb 22 2018, 8:15 PM

For me both look just the same, no difference in size. Perhaps share a screenshot? For me it looks like this:

Which app is this? This one?

https://phabricator.kde.org/source/plasma-sdk/browse/master/cuttlefish/

Not sure about Ubuntu, on openSUSE it's in the package plasma5-sdk.

For me both look just the same, no difference in size. Perhaps share a screenshot? For me it looks like this:

Which app is this? This one?

https://phabricator.kde.org/source/plasma-sdk/browse/master/cuttlefish/

Not sure about Ubuntu, on openSUSE it's in the package plasma5-sdk.

Thanks, got it. plasma-sdk.