GWL: add notification badges to panel icons #12569
Conversation
|
Yes, new features don't go in past BETA. |
clefebvre
changed the title
|
I don't know if I like the two numbers side by side (it excludes a big portion of the top of the icon and presents two conflicting pieces of information side by side, requiring more effort than necessary to interpret the values since they are very close, despite having different colors). We could try to think of a better way to present that information until the next merge period for the next release. |
Why? I'm not using a phone OS |
clefebvre
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR adds new notification badges for panel icons by introducing a separate badge for notifications along with the existing window count badge and removes the "Show window count numbers" configuration. Key changes include:
- Passing a new desktopEntry hint to notifications in both notificationDaemon.js and messageTray.js.
- Adding event handling for notification updates in workspace.js.
- Overhauling the appGroup.js code to support separate badges for windows and notifications.
Reviewed Changes
Copilot reviewed 5 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| js/ui/notificationDaemon.js | Updated notification creation to include the desktopEntry hint. |
| js/ui/messageTray.js | Extended Notification class and documentation to support desktopEntry. |
| files/usr/share/cinnamon/applets/[email protected]/workspace.js | Added connection to notify-applet-update and a new notificationReceived handler. |
| files/usr/share/cinnamon/applets/[email protected]/applet.js | Removed the obsolete updateWindowNumberState function. |
| files/usr/share/cinnamon/applets/[email protected]/appGroup.js | Modified badge layout and introduced methods for updating notification and window count badges. |
Files not reviewed (3)
- data/theme/cinnamon-sass/_colors.scss: Language not supported
- data/theme/cinnamon-sass/widgets/_windowlist.scss: Language not supported
- files/usr/share/cinnamon/applets/[email protected]/settings-schema.json: Language not supported
|
@anaximeno How about in opposite corners.
I can't think of anything else that will work with all themes except badges. |
It looks better this way you did it now, in my opinion. Good choice.
Yeah, the options are limited thanks to that. |
fredcw
force-pushed
the
gwlnotifications
branch
from
1235bc4 to
b33185a
Compare
fredcw
force-pushed
the
gwlnotifications
branch
2 times, most recently
from
5a21309 to
55d292a
Compare
fredcw
force-pushed
the
gwlnotifications
branch
2 times, most recently
from
543b7bf to
74f74ea
Compare
|
Is there an option to disable it completely? |
|
@leigh123linux Not atm. Perhaps I should add that? |
It would match the option to disable window count. 
|
|
@leigh123linux Ah, I removed that as well. :P |
fredcw
force-pushed
the
gwlnotifications
branch
from
850faf7 to
4907aa0
Compare
|
@leigh123linux I've added the option to hide notification badges. Do you think the option to show window number should be added back? I don't have a strong opinion on this myself. |
|
@clefebvre Why is there a thumbsdown on this PR by a member of your github team? Are my contributions no longer welcome? |
I'd say so...! The more options for customization, the better ✨ Initially, you had both at the top, then you decided to move the window count to the bottom left and the notification count to the top right - I personally might prefer the window count in the top left and the notification count in the bottom right, while yet another person might prefer to have only the window count, only the badge number, or neither number showing up at all. For my two cents, ideally both numbers should be optional, and the user should have a choice of which corner belongs to which number. How would you feel about having a dropdown for each corner in the settings, with the choices of "Window Count", "Notification Count", and "None"? That'd leave room for more corner options in the future, too. |
fredcw
force-pushed
the
gwlnotifications
branch
3 times, most recently
from
4c43b1a to
c173f8c
Compare
|
Added back show window count config option. |
Of course they are. I don't know why @leigh123linux thumbed it down, but even if someone in the team or even if most of the team didn't like a particular PR, it wouldn't mean your contributions are not appreciated. |
|
This is wonderful! Thank you for your work on this feature, I've been excited to see it implemented for a long time now (and so have others, judging by issue #10124)
Is this the only thing this PR is waiting on, now? I'll confess I'm not sure how all the release timing stuff works, exactly... is it possible to merge now, or if not, around when could it be merged? |
Yes, we're hoping to merge this. |
|
@fredcw I think this is great and it's working well. Here's my feedback on what still requires change. I don't like the use of In the default theme, I'd like to keep the badges small, with their original color and original position (top-left). These numbers are only there as hints, they're not supposed to catch your attention. 
There's plenty of space top-right for the notification badge if it's the same size. It has a noticeable color, and it complements the notification applet and the notification itself, so same thing here, there's no need for this number to be big. It's a pity we store the notifications in an array and not just the number since that's all we need, but I understand it's more reliable that way. If we were to concurrently increase/decrease, we could run into bugs. |
|
Small detail, I noticed we're doing maths on the same childBox actor to allocate multiple things (icon and the 2 badges). We're modifying it as we go along so the math is relative to the changes made before.. it's a bit messy :) It might cleaner to declare child boxes for the two badges and let them make their own maths from scratch, they'd look more similar that way in the code. It doesn't really matter but I saw it so I thought I'd mention it. |
|
@clefebvre In notificationDaemon.js, I've found that the only reliable way to determine the source app of a notification from a flatpak app is the desktop-entry hint. While this is set correctly for most flatpak apps, some set it incorrectly, hence the list of "exceptions". While I tested quite a few, I expect there to be some that are not listed. My thinking is that some of these will be found during the beta and can be added to the list. An alternative is to try to guess the app from the appName using a subString search of installed apps' names and ids. I can implement that if you think it's better. |
Let's go with that imo. As you said we can always improve it when we get feedback. |
|
Almost there but there's something funky with the size now.. 
|
|
@clefebvre I fixed the badge size issue. Fixed in #13209 as well.
My main concern there was that the applet should be able to destroy any remaining notifications should it be removed from the panel whilst also being the last remaining applet handing notifications. |
|
It's more visible if you switch to Mint-X. Since we're using new CSS class names, the Mint-X theme doesn't interfere with testing, its light color just makes it easier to see the badge. 
It should be round and placed in the corner of the icon. I'll try to fix it this weekend. This is going in no matter what, I just want to nail the look and feel. |
|
This is master in comparison: 
Not perfectly round when the number has 2 digits but pretty good otherwise. Well positioned, round with 1 digit. The positioning and align code is pretty messy on master already. This should be much easier to do than it is :) If I can't get this done this week-end I'll merge as is and we'll consider this a bug to be fixed during the ALPHA/BETA phases. |
|
@clefebvre I think this is a bit better. Here's cinnamon theme with ubuntu regular 10 Mint-X with ubuntu regular 13 I think some themes will need adjusting though here's Mint-Y with ubuntu regular 13 font |
|
Quick comment. Don't use that terrible color for the notification badge. You already have warning and error colors. Use those instead of just making up some color that doesn't fit at all. Feel like this could have been simpler. Putting 2 badges with numbers is a bit much. Especially if you use a smaller panel height. There should only be one. You could show multiple windows another way. |
|
The font size problem is also a pretty easy fix. In most cases you want to use relative font sizes to respect user settings. But I don't think this is one of them. If a font size of 7 or 8 just work, then just use that in the theme. Using em here is just asking for a problem. |
Thanks, I didn't think of that xD.
I'm not bothered what the colour is, I only picked that colour because I thought it needed to stand out and that red looked too serious. But any colour is fine. I guess most themes will probably change it anyway.
This is the problem though. We would need something that would work with all existing themes. Lines or boxes would conflict with themes that already use lines (Mint-Y) or boxes (Mint-X, Cinnamox) to show which apps have open windows, and I guess you'd have to shrink the icon to make room for it as well. Bear in mind that most apps won't have both badges at the same time, except maybe a browser, but notifications are normally fleeting anyway since the point of them is to get rid of them xD. |
|
With fixed font size 10px, it now looks like this: Mint-Y, panel size 31: panel size 41: Here's 8px font for comparison: Here's 8px and 13 px notification, with window count badge moved closer to icon and using same red as iOS (#e74b37) for notification: |
|
here's what I have now: 9px and 13px and iOS (#e74b37) for notification colour 
Edit: Made sure buttons are always round: |
|
@clefebvre Some themes still override the cinnamon theme font size: Mint-Y theme, panel size 41, font: ubuntu regular 13:
Added PRs for window count badge fixed font size: linuxmint/mint-themes#524, linuxmint/mint-themes#525, linuxmint/mint-l-theme#20 |
I like the sound of that. It'd be nice to not depend on the theme itself for the size of the badge. I think this is something that warrants the more granular control you'd get from an applet-specific setting. |
GWL: add notification badges to top right of panel icons and add config option to disable notification badges Remove 'notifications' extension role as it only allows for one extension. Use extensionsHandlingNotifications variable in messageTray.js for applets to increment/decrement when added/removed from panel instead. Ensure notificationDaemon can identify source of notifications from flatpak apps. Increase max notifications per source from 10 to 20.
fredcw
force-pushed
the
gwlnotifications
branch
from
9c1ab12 to
f03adeb
Compare
7 participants
Continuing on from the great work by @romario-07 in #12561 this adds a different color notification badge on the right of the icon and both badge sizes are adjusted automatically to the size of the icon. The "Show window count numbers" config option is removed as it seems fairly pointless.
@clefebvre @mtwebster Is this too late for the beta?
28px panel:40px panel (Mint default):48px panel:The issue mentioned in this comment is fixed.
In Mint-Y, 40 px:closes #10124
Edit: Changed window count badge to bottom left and adjusted notification color:
Known issue: If config option "Group windows by application" is changed, current notification counts in applet are reset to zero.