Make notification icons look like bells
ClosedPublic

Authored by ndavis on Aug 8 2019, 5:56 AM.

Details

Summary

BUG: 384015

Test Plan

See D23018

Diff Detail

Repository
R242 Plasma Framework (Library)
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.Aug 8 2019, 5:56 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptAug 8 2019, 5:56 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
ndavis requested review of this revision.Aug 8 2019, 5:56 AM
ndavis edited the test plan for this revision. (Show Details)Aug 8 2019, 6:10 AM
broulik added a subscriber: broulik.Aug 8 2019, 6:54 AM

Screenshots, please.
The question is what about the transition when this is released in Frameworks but the Plasma version with the change isn't yet.

ndavis edited the test plan for this revision. (Show Details)EditedAug 8 2019, 7:01 AM

Screenshots, please.
The question is what about the transition when this is released in Frameworks but the Plasma version with the change isn't yet.

The screenshots would be the same as in D23018, but I'll post an extra screenshot. As for the transition, D23018 should work fine without this patch.

broulik requested changes to this revision.Aug 8 2019, 7:45 AM

You can't repurpose icon ids for different meanings:


This is what the "no notifications" case looks like now with the old applet.

This revision now requires changes to proceed.Aug 8 2019, 7:45 AM
ndavis added a comment.Aug 8 2019, 7:51 AM

You can't repurpose icon ids for different meanings:


This is what the "no notifications" case looks like now with the old applet.

I didn't repurpose the icon, the old applet just uses a weird default icon (notification-disabled for a non-disabled state). I can wait until Plasma 5.17 to land this patch.

ngraham added a subscriber: ngraham.EditedAug 8 2019, 6:09 PM

I think I approve of the end goal, but it seems like this needs a change in the notifications widget rather than just changing the icon itself.

Edit: Never mind, I'm a dope

ngraham edited the summary of this revision. (Show Details)Aug 8 2019, 6:19 PM

You can't repurpose icon ids for different meanings:


This is what the "no notifications" case looks like now with the old applet.

I didn't repurpose the icon, the old applet just uses a weird default icon (notification-disabled for a non-disabled state). I can wait until Plasma 5.17 to land this patch.

If you change the icon that it uses in the stable branch, then this would probably be just fine IMO.

ngraham accepted this revision.Aug 8 2019, 7:02 PM

Never mind, you did change it to notification-inactive in the other patch. So this is good to go IMO.

As for landing it, we have two options to make sure that rolling release distro users don't see their notification icon change to the one with the red line through it:

  1. Land this patch for 5.63 (the Frameworks release aligned with Plasma 5.17)
  2. Change the inactive applet to use the notification-inactive icon in another patch and land that on the stable branch, then land this
ndavis added a comment.Aug 9 2019, 2:00 AM

@broulik With D23033, the notification-disabled element will be unused. Do you still require changes to this patch?

broulik accepted this revision.Aug 9 2019, 1:31 PM
This revision is now accepted and ready to land.Aug 9 2019, 1:31 PM
This revision was automatically updated to reflect the committed changes.