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

Add cross-platform notification timeouts #206

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

PhrozenByte
Copy link
Contributor

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 IS WORK IN PROGRESS

I want to make sure that the on_cleared event is called in case of an timeout on all platforms, not just on Linux. I'm not entirely sure how to implement this yet... Just calling handle_cleared doesn't work very well because backends will still issue an on_dismissed event. This won't trigger Notification.on_dismissed because the notification was removed from the cache by handle_cleared already, but the class-level callback will still fire... I have to think about that...

Closes #46

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.
@PhrozenByte PhrozenByte marked this pull request as draft November 26, 2024 13:24
@codecov-commenter
Copy link

codecov-commenter commented Nov 26, 2024

Codecov Report

Attention: Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.

Project coverage is 85.13%. Comparing base (26c6af2) to head (48678af).
Report is 14 commits behind head on main.

Files with missing lines Patch % Lines
src/desktop_notifier/backends/base.py 90.90% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #206      +/-   ##
==========================================
- Coverage   85.13%   85.13%   -0.01%     
==========================================
  Files          10       10              
  Lines         895      915      +20     
==========================================
+ Hits          762      779      +17     
- Misses        133      136       +3     
Flag Coverage Δ
pytest 85.13% <92.30%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@PhrozenByte
Copy link
Contributor Author

Supporting timeouts is a pretty complex topic. First it depends on what one means by "timeout":

"Timeout" could mean "auto-close the notification after n seconds". DesktopNotifier currently interprets it that way and I agree with that interpretation. Windows disagrees and interprets it differently, see below. OS X seems to know neither nor (all I found was a delivery date property). But Linux resp. the FreeDesktop.org specs agree. Other people (like vlevit's notify-send.sh) agree, too. However, it seems like that not all (actually rather looks like few, if any) notifications servers on Linux implement this. GNOME for example simply ignores the property and there's no way to detect whether the notifications server might ignore the provided timeout (i.e. capabilities are useless here).

"Timeout" could also mean "hide the toast after n seconds and send the notification to the notifications center". Windows has a expiration_time property for this. Adding this property was easy and it works as expected, but I didn't commit this change because it differs from what timeout meant previously. There's no way to send a notification to the notifications center on Linux, so it's impossible to implement it this way on Linux. The same seems to be true for OS X.

However, for a notifications server without persistence (i.e. without a notifications center) there's just no such difference: If a notification times out on such notifications server, it's gone. This difference is also acknowledged by the FreeDesktop.org specs on Linux by adding a persistence capability: If the server doesn't have that capability the notification will just disappear after timeout.

So in the end I'd say that interpreting "timeout" like Windows doesn't make much sense for DesktopNotifier, because we can't implement it for any other platform. However, no platform reliably supports the interpretation as "auto-close notification" either. So, in the end I recommend adding an universal solution just like #46. To not interfere with an auto-close feature of the notifications server on Linux I recommend sending notifications with timeout=-1 no matter what. I've just implemented it like this.

WDYT?

This was referenced Nov 26, 2024
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