Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

notifications: Try to guess app id if getting from hints fails #479

Merged
merged 2 commits into from
Oct 18, 2023

Conversation

EbonJaeger
Copy link
Member

@EbonJaeger EbonJaeger commented Oct 15, 2023

Description

The notification spec states that the desktop-entry hint should be the prefix of the Desktop Application Info file. However, because of the push to have fully-qualified app ID's, some applications now have a mismatch between the ID and the desktop file, meaning the hint cannot be used for that application.

In cases where the hint fails, try to guess the ID using the application's name given to us from DBus.

It seems to fix #477.

Original description:

The spec states that the image data from a notification should be used before the application icon or any other icon. This change should reflect that.

It seems to fix #477. Nemo and Twitch Streamlink notifications now display properly in Raven on my system.

Signed-off-by: Evan Maddock maddock.evan@vivaldi.net

Submitter Checklist

  • Squashed commits with git rebase -i (if needed)
  • Built budgie-desktop and verified that the patch worked (if needed)

@EbonJaeger EbonJaeger linked an issue Oct 15, 2023 that may be closed by this pull request
@EbonJaeger EbonJaeger requested review from JoshStrobl and a team October 15, 2023 20:09
@JoshStrobl
Copy link
Member

JoshStrobl commented Oct 15, 2023

This change will result in Discord iconography being that of the avatar of the first avatar of the first person / channel / server you get a notification from, regardless of whether or not any other notifications come from the same source, That's why I have it set to prioritize the app info image.

@JoshStrobl
Copy link
Member

See 6c0608b

The specification is for the Notification itself, it shouldn't dictate how it should be handled in Raven. I would be curious what sort of AppInfo we are getting from those applications sending the notifications.

@EbonJaeger EbonJaeger marked this pull request as draft October 15, 2023 23:22
@EbonJaeger
Copy link
Member Author

Gonna give this more thought...

@EbonJaeger EbonJaeger force-pushed the 477-notification-icons-broken-bug branch from a438856 to 627710d Compare October 16, 2023 16:13
@EbonJaeger EbonJaeger changed the title notifications_view: Check for notification image before app image notifications: Try to guess app id if getting from hints fails Oct 16, 2023
The notification spec states that the `desktop-entry` hint should be the prefix of the Desktop Application Info file. However, because of the push to have fully-qualified app ID's, some applications now have a mismatch between the ID and the desktop file, meaning the hint cannot be used for that application.

In cases where the hint fails, try to guess the ID using the application's name given to us from DBus.

It seems to fix #477.

Signed-off-by: Evan Maddock <maddock.evan@vivaldi.net>
@EbonJaeger EbonJaeger force-pushed the 477-notification-icons-broken-bug branch from 627710d to b4fb60d Compare October 16, 2023 16:15
@EbonJaeger EbonJaeger marked this pull request as ready for review October 16, 2023 16:15
@EbonJaeger
Copy link
Member Author

Thought given.

JoshStrobl
JoshStrobl previously approved these changes Oct 16, 2023
Copy link
Member

@JoshStrobl JoshStrobl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Things no bork so LGTM 👍

@EbonJaeger
Copy link
Member Author

Found two more things bork: Solus Update Service and budgie-daemon when we get the notification that there are notifications.

Signed-off-by: Evan Maddock <maddock.evan@vivaldi.net>
@EbonJaeger EbonJaeger dismissed JoshStrobl’s stale review October 16, 2023 18:56

Needs re-review due to new commit

@JoshStrobl JoshStrobl merged commit bfee2a0 into main Oct 18, 2023
1 check passed
@JoshStrobl JoshStrobl deleted the 477-notification-icons-broken-bug branch October 18, 2023 09:06
fossfreedom added a commit that referenced this pull request Oct 18, 2023
fossfreedom added a commit that referenced this pull request Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Notification icons broken [Bug]
2 participants