Rename 'nepomuk' Plasma Theme icons to 'search'
Needs ReviewPublic

Authored by The-Feren-OS-Dev on Jan 9 2020, 11:01 PM.

Details

Reviewers
ndavis
bruns
ngraham
Group Reviewers
Plasma
VDG
Summary

BUG: 416072

Since the Milou/Search applet changed its icon from 'nepomuk' to 'search', the Breeze and Air/Oxygen Plasma Themes no longer theme that Plasmoid icon. This patch fixes that by renaming the themed icons to theme the Milou Plasmoid once more.

Test Plan
  1. Apply one of the Plasma Theme affected by this patch once patched
  1. Use a non-Breeze icon set that would make the 'search' icon pop-out style-wise
  1. Add the Milou/Search Plasmoid to a panel if not already added
  1. It should now use the Plasma Themed icon instead of the icon set's icon for 'search' on that Plasmoid

Diff Detail

Repository
R242 Plasma Framework (Library)
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 20870
Build 20888: arc lint + arc unit
Restricted Application added a project: Frameworks. · View Herald TranscriptJan 9 2020, 11:01 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
The-Feren-OS-Dev requested review of this revision.Jan 9 2020, 11:01 PM
The-Feren-OS-Dev retitled this revision from Rename nepomuk Plasma Theme icons to 'search' to Rename 'nepomuk' Plasma Theme icons to 'search'.Jan 9 2020, 11:03 PM

I still think the proper fix was to investigate why the widget explorer didn't show an icon instead of randomly changing the icon it uses which can have potential impact on other themes

The-Feren-OS-Dev added a comment.EditedJan 9 2020, 11:13 PM

I still think the proper fix was to investigate why the widget explorer didn't show an icon instead of randomly changing the icon it uses which can have potential impact on other themes

This is a fix to it happening in real-time in Plasmashell - me making this patch is not related to a potential issue with Widget Explorer. However, I could easily edit this patch to retain the nepomuk icons alongside the renamed ones if needed.

I still think the proper fix was to investigate why the widget explorer didn't show an icon instead of randomly changing the icon it uses which can have potential impact on other themes

This is a fix to it happening in real-time in Plasmashell me making this patch is not related to a potential issue with Widget Explorer. However, I could easily edit this patch to retain the nepomuk icons alongside the renamed ones if needed.

But if I understood it right this patch is a reaction to an issue caused by changing the icon which was caused by the widget explorer issue?

I still think the proper fix was to investigate why the widget explorer didn't show an icon instead of randomly changing the icon it uses which can have potential impact on other themes

This is a fix to it happening in real-time in Plasmashell me making this patch is not related to a potential issue with Widget Explorer. However, I could easily edit this patch to retain the nepomuk icons alongside the renamed ones if needed.

But if I understood it right this patch is a reaction to an issue caused by changing the icon which was caused by the widget explorer issue?

I honestly haven't heard about the potential Widget Factory issue side of it, but this patch was definitely made because the Milou Plasmoid changed its icon from 'nepomuk' to 'search'.

ndavis accepted this revision.Jan 10 2020, 3:27 PM
ndavis added a subscriber: ndavis.

Icons in the desktop theme that are made to replace icons in the icon theme need to be kept up to date with the icon theme. I'm accepting this as it is, but if @davidre thinks we should continue to support the use of the nepomuk icon name, then include both in the desktop theme. If this seems inelegant, that's because comprehensive icon support is not very pretty, technically. breeze-icons is filled with tons of symlinks for compatibility and there's really nothing we can do about it.

This revision is now accepted and ready to land.Jan 10 2020, 3:27 PM

If anyone has any issues with removing the 'nepomuk' icon theming, please be sure to let me know so I can add them back in again on this patch to be alongside the 'search' theming to sort that issue out.

It appears that no 1st-party Plasmoids I can see in Plasma have the nepomuk icon now, so the only potential issue I can currently see is the loss of theming for 3rd-party Search Plasmoids.

bruns requested changes to this revision.Jan 10 2020, 4:33 PM
bruns added a subscriber: bruns.

The nepomuk icon is completely inappropriate for a generic "search" icon.

This revision now requires changes to proceed.Jan 10 2020, 4:33 PM
ngraham requested changes to this revision.Jan 10 2020, 4:45 PM
ngraham added a subscriber: ngraham.

Are we sure this is the right approach? In the Breeze theme, the icon is the same as the generic magnifying glass search icon, but in the Oxygen and Breeze theme, it's some kind of paint splatter, which as @bruns suggests, is not appropriate to use as a generic search icon.

How is anything still using this icon anyway? I thought milou switched to using the search icon? This should fall back to the icon in the Breeze theme.

If we need a search icon in the plasma theme, then we need to make sure it's semantically appropriate. For Breeze, it should be a line-art magnifying glass, for Oxygen, it should be a skeumorphic magnifying glass, etc.

Alright, I'll look into having a replacement icon for Oxygen/Air.

Rename 'nepomuk' Plasma Theme icons to 'search'

Hmph... Oxygen has a low-quality 'search' icon. I'll update the diff again in a bit to add a higher quality version of the icon in SVG form if I can find a higher quality SVG of the icon from Oxygen.

Rename 'nepomuk' Plasma Theme icons to 'search' and restyle Oxygen/Air's icon

Rename 'nepomuk' Plasma Theme icons to 'search' and restyle Oxygen/Air Search Icon

^ that was just me scour-icon'ing the SVG file as I forgot to initially.

*sigh* It seems scour-icon decided to nuke the layer names that made the icons identifiable, working on fixing that now.

Rename 'nepomuk' Plasma Theme icons to 'search'

Rename 'nepomuk' Plasma Theme icons to 'search' and redesign Oxygen/Air 'search' icon

There we go. Noticed there was a slight imperfection with the icon alignment in the previous edit so fixed that.

The-Feren-OS-Dev added a comment.EditedJan 10 2020, 7:53 PM

@bruns @ngraham What do you two think about using the find icon as the search icon, given we use the find icon for search in Dolphin in the Oxygen icon set?

Looks like this needs a bit of CMake work to stop installing the old icons:

# from directory: /home/nate/kde/build/plasma-framework
gmake[2]: *** No rule to make target '/home/nate/kde/src/plasma-framework/src/desktoptheme/air/icons/nepomuk.svg', needed by 'src/desktoptheme/oxygen/oxygen.gzipped/icons/nepomuk.svgz'.  Stop.
gmake[1]: *** [CMakeFiles/Makefile2:880: src/desktoptheme/oxygen/CMakeFiles/oxygen_desktoptheme_graphics_icons.dir/all] Error 2

Probably need to add the new ones too.

The-Feren-OS-Dev added a comment.EditedJan 10 2020, 8:34 PM

Looks like this needs a bit of CMake work to stop installing the old icons:

# from directory: /home/nate/kde/build/plasma-framework
gmake[2]: *** No rule to make target '/home/nate/kde/src/plasma-framework/src/desktoptheme/air/icons/nepomuk.svg', needed by 'src/desktoptheme/oxygen/oxygen.gzipped/icons/nepomuk.svgz'.  Stop.
gmake[1]: *** [CMakeFiles/Makefile2:880: src/desktoptheme/oxygen/CMakeFiles/oxygen_desktoptheme_graphics_icons.dir/all] Error 2

Probably need to add the new ones too.

Hmm, can't seem to find any mentions of 'nepomuk' anywhere in the repos nor in the files...

bruns added a comment.Mar 9 2020, 10:45 AM

Looks like this needs a bit of CMake work to stop installing the old icons:

# from directory: /home/nate/kde/build/plasma-framework
gmake[2]: *** No rule to make target '/home/nate/kde/src/plasma-framework/src/desktoptheme/air/icons/nepomuk.svg', needed by 'src/desktoptheme/oxygen/oxygen.gzipped/icons/nepomuk.svgz'.  Stop.
gmake[1]: *** [CMakeFiles/Makefile2:880: src/desktoptheme/oxygen/CMakeFiles/oxygen_desktoptheme_graphics_icons.dir/all] Error 2

Probably need to add the new ones too.

@ngraham - the icons are picked up by a glob, https://phabricator.kde.org/source/plasma-framework/browse/master/src/desktoptheme/air/CMakeLists.txt$30, probably just some stale CMake cache on your side? Can you recheck?

ngraham accepted this revision.Mar 9 2020, 6:20 PM

You're right, it was a local cache.