Move firewall-applet icons to status category
ClosedPublic

Authored by ndavis on Nov 18 2018, 6:43 PM.

Details

Summary

breeze-icons was failing the TestSuite.scalable test because firewall-applet didn't have icons in a directory marked as scalable by the index.theme files. status/22 is a scalable directory.

Diff Detail

Repository
R266 Breeze Icons
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ndavis created this revision.Nov 18 2018, 6:43 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptNov 18 2018, 6:43 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
ndavis requested review of this revision.Nov 18 2018, 6:43 PM
ngraham requested changes to this revision.Nov 18 2018, 7:08 PM
ngraham added a subscriber: ngraham.

The scalable test still fails; you need to add more symlinks:

FAIL!  : ScalableTest::test_scalable(icons:Applications) The following icons are not available in a scalable directory:
  firewall-applet-error
  firewall-applet-shields_up
  firewall-applet-panic
   Loc: [/home/dev/repos/breeze-icons/autotests/scalabletest.cpp(262)]
FAIL!  : ScalableTest::test_scalable(icons-dark:Applications) The following icons are not available in a scalable directory:
  firewall-applet-error
  firewall-applet-shields_up
  firewall-applet-panic
   Loc: [/home/dev/repos/breeze-icons/autotests/scalabletest.cpp(262)]
This revision now requires changes to proceed.Nov 18 2018, 7:08 PM

@ngraham Realistically, users will never need the 48px versions of each icon. Can I just make more symlinks of firewall-config or should I make icons that actually represent the other icons?

ndavis updated this revision to Diff 45746.Nov 18 2018, 7:53 PM

Add more symlinks

ndavis retitled this revision from Add 48px firewall-applet icon to Add 48px firewall-applet* icons.Nov 18 2018, 7:55 PM
ndavis edited the summary of this revision. (Show Details)
aacid added a subscriber: aacid.Nov 18 2018, 10:03 PM

I'm sorry but this makes no sense at all, why is firewall-applet-error now suddenly firewall-config?

And why do you say users will never need a 48px verison of it?

ndavis added a comment.EditedNov 18 2018, 10:20 PM

I'm sorry but this makes no sense at all, why is firewall-applet-error now suddenly firewall-config?

And why do you say users will never need a 48px verison of it?

firewall-applet and firewall-config are closely accociated: https://github.com/firewalld/firewalld/tree/master/src

firewall-applet is just an applet for the system tray showing the state of firewalld with an option to open firewall-config, so it makes sense to re-use the firewall-config icon. firewall-applet doesn't show up in the application menu or application menu searches either. Even if it did, only firewall-applet.svg would be used. The other firewall-applet-* icons will only be seen in the system tray applet itself and never at 48px.

firewall-applet does have a .desktop file, but it is placed into /etc/xdg/autostart, not /usr/share/applications

ngraham accepted this revision.Nov 19 2018, 3:30 AM

Thanks, much better now.

@aacid all of this confused me too at first, but these icons are targeting a particular piece of software and use case where they make sense.

This revision is now accepted and ready to land.Nov 19 2018, 3:30 AM
ndavis added a subscriber: sitter.Nov 19 2018, 8:23 PM

@sitter sent me an email saying that it would be better to put the firewall-applet icons into status/. He's probably right and it would mean I don't need to make a bunch of pointless symlinks.

aacid added a comment.Nov 19 2018, 9:15 PM

These symlinks as they stand are not poinless, they are wrong.

Icon sets are not "targeting a particular piece of software", icon sets are system wide and any software can ask for a given icon, so firewall-applet-shields_up symlinking to firewall-config in size 48 when in size 32 are clearly different is wrong.

These symlinks as they stand are not poinless, they are wrong.

Icon sets are not "targeting a particular piece of software", icon sets are system wide and any software can ask for a given icon, so firewall-applet-shields_up symlinking to firewall-config in size 48 when in size 32 are clearly different is wrong.

Do you think the firewall-applet icons should be moved to status/ as well?

ndavis updated this revision to Diff 45929.Nov 21 2018, 7:13 AM

Remove 48px symlinks and move firewall-applet icons to status category

Anyone opposed to moving the firewall-applet icons into the status category? status/22 is a scalable directory. If no-one speaks by tomorrow, I'm going to land the patch.

ndavis retitled this revision from Add 48px firewall-applet* icons to Move firewall-applet icons to status category.Nov 21 2018, 9:33 PM
ndavis edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.