Desktop Notifier v7.0 #200
Conversation
There was a problem hiding this comment.
I've started reviewing this and there is a lot in here that is great, some things where I don't yet understand the benefits, and others that might warrant a larger design discussion.
However, this is really hard to review as a single PR. Could you split this up into separate PRs for each "topic of change"?
Use native `__repr__` of `@dataclass` instead
No functional change, this is just a small quality-of-life change simplifying usage of the constructors.
…ethods We don't need the `Notification` instance in the platform-dependant code, thus drop it. We don't filter notifications on DesktopNotifier-level callbacks. Different notifications servers (and versions) signal differently: Some send any signal of any notification to anyone listening, some send signals of all notifications with the same `app_name`, some just the notifications this particular instance dispatched (and here some filter by PID, some by fd). We don't try to be smarter than the notifications server, simply pass everything through. It's up to the user to filter events as needed - or to use the `Notification` instance callbacks instead.
This truly freezes Notification instances
Linux (DBus) is the only platform that theoretically supports timeouts, but few (if any?) notification servers actually implement timeouts. Windows exposes a `expiration_time` property, but this doesn't close the notification, but just controls after what time the toast is hidden and the notification sent to the notifications center (i.e. the notification isn't actually closed). OS X doesn't support expiration. We therefore implement our own cross-platform notification timeouts.
This event is fired when a notification was closed without user interaction, e.g. because the notification timed out (DBus only, and only if supported by the notifications server; undetectable by capabilities), or because the notification was closed by another process (DBus only).
This event is fired when a notification was sent to the notifications server. This is less useful as class-level callback, but very useful with `Notification`, because it allows for a better separation of concerns (separates creating and sending notifications).
|
Here we go, the next and rather close-to-finished iteration is here... ❤️
That's inevitable for low level changes. Splitting things up into separate PRs makes merging way harder, either due to classic merge conflicts or even worse, if the PRs depend on each other. For example, if I drop the I split things up nevertheless, because I tested PyCharm's Git GUI with this PR and as you've noticed, PyCharm screwed up the commits. Sorry about that. This was the first and last time I used that GUI... So, shell again, way more comfortable. See PRs #202 #203 #204 #205 #206 #207 #208 #209. Some are incomplete and require small changes after other PRs were merged, or at least cause merge conflicts. This PR now consists of the big Note that this PR is out-of-sync nevertheless and missing some commits of the individual PRs. Due to splitting things up it's hard to keep things in sync, so let's merge the other stuff first and I'll update this PR after that. You can check it out, but again ignore linter, unit tests and stuff, that's broken due to stuff being out-of-sync. Open questions:
|
PhrozenByte
force-pushed
the
desktop-notifier-v7.0
branch
from
db278a6 to
ff35694
Compare
The `DispatchedNotification` class is a wrapper for `Notification` instances after they have been dispatched / sent to the notifications server. Instances of `DispatchedNotification` have their own unique identifier whose format varies from platform to platform. For example, on Linux it's a simple uint32 converted to str. On other platforms the identifier might be identical to the randomly generated UUID of the wrapped Notification instance, but users must not rely on this. For example, if the same `Notification` instance is sent twice, the second `DispatchedNotification` will have a different identifier. The `DispatchedNotification` class furthermore exposes the status of the notification, namely whether it was cleared, clicked, dismissed, replied to, or what button was clicked. The `DesktopNotifier.send_notification()` method now returns a `DispatchedNotification` instance. It also accepts `DispatchedNotification` instances to send a previously sent notification again (just create a new `DispatchedNotification` instance with the same `identifier`, but a different `notification`). If a `DispatchedNotification` instance is given and that notification is still active, Desktop Notifier will attempt to replace that notification by the new one (on Linux this works seamlessly, on other platforms the previous notification is closed first and a new notification is sent). Since some platforms (currently just OS X) support returning the "actual" list of active notifications, `DesktopNotifier.get_current_notifications()` doesn't return `DispatchedNotification` instances, but still just a list of the identifiers, matching those of `DispatchedNotification` instances. To access these there's a new `DesktopNotifier.get_cached_notifications()` method.
PhrozenByte
force-pushed
the
desktop-notifier-v7.0
branch
from
ff35694 to
6b7930d
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #200 +/- ##
===========================================
- Coverage 85.13% 72.46% -12.68%
===========================================
Files 10 10
Lines 895 1075 +180
===========================================
+ Hits 762 779 +17
- Misses 133 296 +163
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
PhrozenByte
force-pushed
the
desktop-notifier-v7.0
branch
from
6b7930d to
95ef1ea
Compare
|
I hope you had a great start into the new year @samschott. Did you have time to look into the suggested changes? I'm happy to provide more details about my thoughts and my reasoning for particular changes, so please just ask. If you have alternatives in mind, just let me know, I'm happy to discuss alternative approaches as well. As explained in #200 (comment) I've split things up into separate PRs. Due to GitHub's limited diff capabilities this PR won't show a reasonable diff until all other PRs are merged, so let's get the other things merged first and focus on this PR's major Some PRs now have expected merge conflicts. They don't really matter for a review, so please just go ahead. If you're about to merge the PR just give me a hint, I'll rebase the PR then. |
This is WORK IN PROGRESS.
This primarily attempts to fix #196, but since that requires breaking API changes this ultimately means that this will become Desktop Notifier v7.0 anyway, thus I chose to add some minor quality-of-life changes as well. Any other ideas for a proposed v7.0?
The complete changeset isn't really helpful, but the distinct commits are. I recommend reviewing this PR commit by commit. Please also check the commit messages. Note that the approach slightly differs from what I've outlined in #196.
I will also look into #46 since I need that for my project anyway. Due to the new
on_clearedevent Desktop Notifier should now also be able to properly handle timed out notifications. However, it's still up to the notifications server to actually timeout a notification - and e.g. GNOME simply ignores timeouts.Right now tested on Linux only, will also test on Windows. Since I don't have such device I can't test it on OS X.
Checking linter / CI / unit tests / etc. is currently pending, it's an early draft to allow for comments. You can ignore anything that would be reported by the linter / CI / unit tests for now, I'll look into it.
ToDo: