Skip to content

Add on_cleared event #208

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

PhrozenByte
Copy link
Contributor

@PhrozenByte PhrozenByte commented Nov 26, 2024

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).

We'll later also utilize this for the cross-platform implementation of timeout.

Without this new event we'd have to remove the if reason == NOTIFICATION_CLOSED_DISMISSED: check in DBusDesktopNotifier.

ToDo:

@codecov-commenter
Copy link

codecov-commenter commented Nov 26, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 85.90%. Comparing base (5dab588) to head (e87b3ca).
Report is 29 commits behind head on main.

Files with missing lines Patch % Lines
src/desktop_notifier/backends/winrt.py 0.00% 3 Missing ⚠️
src/desktop_notifier/main.py 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #208      +/-   ##
==========================================
+ Coverage   85.06%   85.90%   +0.83%     
==========================================
  Files          10       10              
  Lines         951      958       +7     
==========================================
+ Hits          809      823      +14     
+ Misses        142      135       -7     
Flag Coverage Δ
pytest 85.90% <80.00%> (+0.83%) ⬆️

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@PhrozenByte PhrozenByte force-pushed the enhancement/event-on_cleared branch from 9a16756 to 4968441 Compare November 26, 2024 13:32
@PhrozenByte PhrozenByte mentioned this pull request Nov 26, 2024
8 tasks
@samschott
Copy link
Owner

As for #209, could you check if similar events are available on macOS and Windows?

@PhrozenByte
Copy link
Contributor Author

Windows doesn't have a similar event (I checked).

Can't say anything about OS X because I neither understand their overly complex API, nor do I have an Apple device to do manual testing. I'm afraid anything OS-X-related is up to you.

As said earlier, my ultimate goal is to use this for #206 too, i.e. independent of the backend used. I'm just not sure yet how to prevent calling the dismissal class-level handler. However, this isn't really relevant here, see #206.

Copy link
Owner

@samschott samschott left a comment

Choose a reason for hiding this comment

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

Thanks and apologies for the delay in reviewing!

Do you think there is any value in distinguishing between expired and programatically closed notifications?

ON_CLEARED = auto()
"""Supports distinguishing between an user closing a notification, and clearing a
notification programmatically, and consequently supports on-cleared callbacks"""

Copy link
Owner

Choose a reason for hiding this comment

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

How about "Supports callbacks on programmatic closing and notification timeouts"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm… Reading through both my original suggestion five months later and yours now, I'd interpret both like "if this cap isn't set, the notification can't be closed programmatically". I made another attempt, WDYT?

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).
@PhrozenByte
Copy link
Contributor Author

Do you think there is any value in distinguishing between expired and programatically closed notifications?

We could.

It's been five months, so I've forgotten a lot of my original reasoning 😆 Sometimes that's a good thing: I now spent quite some time to think about this, especially in regards to #206, and distinguishing between those as well makes things easier for #206, too: We could then have a separate handler for #206 and the issues with #206 about duplicate events magically disappear thanks to DispatchedNotification. So, I definitely like your idea 👍

I've rebased and updated this PRs accordingly. I'll update the other PRs as well.

@samschott
Copy link
Owner

@PhrozenByte, did you actually update this PR to separate timeout and programmatic clearing calbacks?

Improve docs of the `ON_CLEARED` capability and update other docs to allow distinguishing between expired and programatically closed notifications in the future. Actually adding support for expiring notifications is out-of-scope here.
@PhrozenByte PhrozenByte force-pushed the enhancement/event-on_cleared branch from 7ca8d3f to e87b3ca Compare April 21, 2025 16:38
@PhrozenByte
Copy link
Contributor Author

Yes. I assume you ask because I forgot to remove timeout from the examples? I just fixed that, sorry. Since I was over it I also now fully unified the examples. Should be ready-to-merge now 👍

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.

3 participants