desktop-notifier icon indicating copy to clipboard operation
desktop-notifier copied to clipboard

Api overhaul

Open samschott opened this issue 1 year ago • 2 comments

  • Remove notification_limit API
  • Always generate notification identifiers ourselves
  • Allow callers to specify a button identifier and internally use it to identify callbacks
  • Make the notification identifier the input to clear existing notifications
  • Make the notification identifier the output to list existing notifications
  • Return the notification identifier from send functions instead of the notification instance
  • Allow setting class-level callbacks for user interactions with a notification

samschott avatar Jul 14 '24 14:07 samschott

Codecov Report

Attention: Patch coverage is 80.58968% with 79 lines in your changes missing coverage. Please review.

Project coverage is 75.61%. Comparing base (89355a1) to head (32e1e45). Report is 67 commits behind head on main.

Files with missing lines Patch % Lines
src/desktop_notifier/backends/base.py 65.75% 25 Missing :warning:
src/desktop_notifier/main.py 72.72% 15 Missing :warning:
src/desktop_notifier/backends/winrt.py 57.57% 14 Missing :warning:
src/desktop_notifier/backends/dbus.py 71.79% 11 Missing :warning:
src/desktop_notifier/backends/macos.py 70.37% 8 Missing :warning:
src/desktop_notifier/common.py 96.27% 6 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #164      +/-   ##
==========================================
+ Coverage   74.74%   75.61%   +0.86%     
==========================================
  Files           9       10       +1     
  Lines         891      898       +7     
==========================================
+ Hits          666      679      +13     
+ Misses        225      219       -6     
Flag Coverage Δ
pytest 75.61% <80.58%> (+0.86%) :arrow_up:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Jul 14 '24 14:07 codecov-commenter

I also seem to have found a bug in this PR on macOS. Reproduction steps:

  1. Send notification w/one reply_field and one Button
  2. Respond to reply_field with some text (my handler is just print(reply_text))
  3. Send another notification with same options (one reply_field, one Button, same text for both) but new instantiation of Button
  4. Click the button

Fails when desktop-notifier can't locate the new instance of Button:

Exception ignored on calling ctypes callback function: <rubicon.objc.api.objc_method object at 0x108feb170>
Traceback (most recent call last):
  File "/Users/uzor/workspace/project/.venv/lib/python3.12/site-packages/rubicon/objc/api.py", line 339, in __call__
    result = self.py_method(py_self, *args)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/uzor/workspace/lib/desktop-notifier/src/desktop_notifier/macos.py", line 127, in userNotificationCenter_didReceiveNotificationResponse_withCompletionHandler_
    if notification and get_button(notification, button_id).on_pressed:
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/uzor/workspace/lib/desktop-notifier/src/desktop_notifier/implementation_base.py", line 151, in get_button
    raise ValueError(ValueError: Notification f'ef2749b5-0960-4694-86dc-88766641b564' does not have button with id '52d83774-ab98-49bc-a76e-45e8a2a221bc')

This set of steps works fine in both current release and current main branch.

UPDATE

After adding some more logging I can see that the issue is that the second interaction is using the button_id from the first notification.

First Notification

Receives reply_text correctly:

07/29/24 16:20:22.008818 DEBUG    [DESKTOP_NOTIFIER] Notification sent: <Notification(identifier='41b7c080-3574-4545-9408-075a59a77e72', button_ids=['34947f52-f0ab-4718-948e-f79ee7ffbfc5'])>
07/29/24 16:20:41.540581 DEBUG    [DESKTOP_NOTIFIER] Response action for <Notification(identifier='41b7c080-3574-4545-9408-075a59a77e72', title='TITLE: message',       macos.py:102
                                  message='Message!                                                                                             
                                                                                                                                                                                       
                                  If you want to use the 'Report' button to explain.',                                                                   
                                  button_ids=['34947f52-f0ab-4718-948e-f79ee7ffbfc5'])>: com.desktop-notifier.ReplyActionIdentifier                                                     
07/29/24 16:20:41.545788 DEBUG    [DESKTOP_NOTIFIER] Received reply_text: 'wdgwdgwdgwgd'    

Second Notification

Receives button press and raises exception about not finding the button_id:

07/29/24 16:21:17.309109 DEBUG    [DESKTOP_NOTIFIER] Notification sent: <Notification(identifier='ff50a1c1-3369-4482-9ca6-1aca5667abcb', title='TITLE: message',       macos.py:102
                                  message='Message!                                                                                             
                                                                                                                                                                                       
                                  If you want to use the 'Report' button to explain.',                                                                 
                                  button_ids=['67d3f31f-6901-4a77-81ac-599d448632c5'])>                                                                                                
07/29/24 16:21:20.776686 DEBUG    [DESKTOP_NOTIFIER] Response action for <Notification(identifier='ff50a1c1-3369-4482-9ca6-1aca5667abcb', title='TITLE: message',      macos.py:102
                                  message='Message!                                                                                             
                                                                                                                                                                                       
                                  If you want to use the 'Report' button to explain.',                                                              
                                  button_ids=['67d3f31f-6901-4a77-81ac-599d448632c5'])>: 34947f52-f0ab-4718-948e-f79ee7ffbfc5                                                          
Exception ignored on calling ctypes callback function: <rubicon.objc.api.objc_method object at 0x106d99910>
Traceback (most recent call last):
  File "/Users/uzor/projekt/.venv/lib/python3.12/site-packages/rubicon/objc/api.py", line 339, in __call__
    result = self.py_method(py_self, *args)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/uzor/projekt/lib/desktop-notifier/src/desktop_notifier/macos.py", line 127, in userNotificationCenter_didReceiveNotificationResponse_withCompletionHandler_
    button = get_button(notification, button_id)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/uzor/projekt/lib/desktop-notifier/src/desktop_notifier/implementation_base.py", line 151, in get_button
    raise ValueError(
ValueError: Notification f'ff50a1c1-3369-4482-9ca6-1aca5667abcb' does not have button with id '34947f52-f0ab-4718-948e-f79ee7ffbfc5' (available buttons: 67d3f31f-6901-4a77-81ac-599d448632c5)
^C

So it is reusing the button UUID 34947f52-f0ab-4718-948e-f79ee7ffbfc5 from the first notification when looking for the button for the second notification.

michelcrypt4d4mus avatar Jul 29 '24 19:07 michelcrypt4d4mus

@michelcrypt4d4mus, could you post the exact code to reproduce this issue?

samschott avatar Aug 10 '24 13:08 samschott