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

Fix incorrect notification ID reuse on XDG #1150

Merged
merged 2 commits into from
May 22, 2023

Conversation

cyqsimon
Copy link
Contributor

@cyqsimon cyqsimon commented May 6, 2023

Previously only the first notification (for each time the application launches) is actually sent on XDG (Linux) because the notification ID was reused incorrectly. This is a simple fix that removes the reuse.

@hrkfdn
Copy link
Owner

hrkfdn commented May 9, 2023

Hey, thanks for the PR.

I currently only have access to a macOS system so can't investigate this, but reading from the code this removes the ID reusage completely, right? So whenever a user skips through a bunch of tracks it will always generate a new notification. Was the old behavior causing problems? It worked quite nicely on my GNOME setup.

@cyqsimon
Copy link
Contributor Author

cyqsimon commented May 9, 2023

It worked quite nicely on my GNOME setup.

It's not working too well on my Arch KDE box. The first time a new song starts playing after ncspot startup, a new notification is shown as expected and goes away after a few seconds (the default timeout configured by the user). This is all good. But since the notification is set to transient, it does not stay in the notification widget, so basically it's just gone. Then afterwards, an attempt to reuse the same notification id simply does nothing.

I think we may have different interpretations and expectations of what this feature is supposed to do. Personally, I do expect a new notification for every new song and I'm happy with it (which is what this patch does). I'm completely guessing, but maybe what you expect is to have a persistent notification that stays in the notification widget/area that is updated when a new song is played? (And it's working as expected for you because maybe Gnome doesn't respect the "transient" hint?) Please clarify.

@hrkfdn
Copy link
Owner

hrkfdn commented May 9, 2023

I see how that's annoying. In GNOME the notification will stay in the "notification panel" until it is explicitly dismissed. So for every track change the notification will persist if the ID is not reused. But maybe notifications aren't really used on GNOME as it supports MPRIS out of the box, anyway.

Maybe it would be interesting to see how other players solve this. Unfortunately I'm on vacation for this and the next week, so I don't have access to my Linux desktop.

@hrkfdn
Copy link
Owner

hrkfdn commented May 22, 2023

So it seems GNOME is different with this, but I think your behavior is correct. I have added a small change that removes the notification ID storage as it's unused.

@hrkfdn hrkfdn enabled auto-merge (squash) May 22, 2023 17:48
@hrkfdn hrkfdn merged commit d8faa87 into hrkfdn:main May 22, 2023
@cyqsimon cyqsimon deleted the notification-fix branch May 31, 2023 09:56
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.

2 participants