Simplify Raven notification group #452
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Testing with Discord and seems a bit busted. Incorrect title and notification group icons aren't that of the application, just of the first icon of the first notification you get. Provided details in Matrix.
|
serebit
requested changes
Sep 27, 2023
|
@serebit Could you clarify what styling changes you are referring to?
It looks identical to me, but I do need to get my eyes checked sooooo. |
This does a couple things. First, when constructing a notification group, just get the image directly from our Notification object. We did all the work to get the correct image, let's actually use it. Secondly, remove our HashTable mapping application name to notification group. We don't need it; anything it was used for we can get from the ListBox. This avoids having to keep track of groups in multiple data structures (the table and the list box). This makes the code easier to maintain, and slightly more memory effecient. Lastly, switch to `ngettext` to build the text for the notification view header instead of an if/elseif chain. Fixes #450 Signed-off-by: Evan Maddock <[email protected]>
…and fix notification counting Signed-off-by: Evan Maddock <[email protected]>
- rename app_icon to notification_icon since it is not necessarily the one for the app - implemented an app_image that will be prefered over image (from notification not app) for Raven. notification popups will use one or the other depending on what is provided. - broken notification grouping as it was assuming an app_name which can technically be empty when the application only supplies a desktop-entry for app info. use notification's app name otherwise try with the app id. - missing application name when application only supplies desktop-entry - incorrect image for some applications such as discord. it would use the Discord message sender's avatar rather than discord icon - spotify reports as com.spotify.Client and not Spotify nowadays, so added that to list of spam-apps to prevent Raven noise - missing printf on ngettext for header resulting in number not being added - missing use_markup on notification title resulting in ampersands not being parsed. noticable in song titles on spotify where many use & instead of "feat."
JoshStrobl
force-pushed
the
simplify-raven-notification-group
branch
from
850dbac
to
bad102c
Compare
serebit
approved these changes
Sep 30, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This does a couple of things. First, when constructing a notification group, just get the image directly from our Notification object. We did all the work to get the correct image, let's actually use it.
Secondly, remove our HashTable mapping application name to notification group. We don't need it; anything it was used for we can get from the ListBox. This avoids having to keep track of groups in multiple data structures (the table and the list box). This makes the code easier to maintain, and slightly more memory efficient.
The NotificationGroup widget has been changed to a
Gtk.ListBoxRowto make it easier to find and remove groups from the view.Lastly, switch to
ngettextto build the text for the notification view header instead of an if/elseif chain.Fixes #450
Submitter Checklist
git rebase -i(if needed)