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

#14 Implement notification cancellation #15

Merged
merged 3 commits into from
Feb 4, 2024

Conversation

basilevs
Copy link
Contributor

@basilevs basilevs commented Oct 13, 2023

Fixes #14

This introduces a way for user to cancel previously sent notification.
The implementation is naive and dirty - no threading or processes involved, notification is cancelled by its ID.

I've tried a "proper" solution with remote controlled singular side-car process, but found, that this is much simpler, as actual notification object is not needed to cancel it, and cancellation is a non-blocking operation.

Usage example:

from mac_notifications.client import create_notification, Notification 
mac_notification: Notification = None
def set_network_status(message):
    global mac_notification
    if mac_notification:
        mac_notification.cancel()
        mac_notification = None
    if message:
        mac_notification = create_notification(title="Network status", text=message)

set_network_status("Router is unavailable")
set_network_status(None) #Nominal
set_network_status("Domain name server is unavailable")

@Jorricks
Copy link
Owner

Hello @basilevs,
Thanks a lot that you took the time to implement this, that's amazing!
I like your implementation, it's clear and easy. I just have some typing 'complaints', which I added as suggestions to this MR.
Let's get this out in a new version soon ✌️

@basilevs
Copy link
Contributor Author

basilevs commented Nov 1, 2023

some typing 'complaints', which I added as suggestions to this MR.

I don't see the suggestion make sure you've posted them (they are organized in reviews and are only published when the review is published)

Copy link
Owner

@Jorricks Jorricks left a comment

Choose a reason for hiding this comment

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

Woops. I use Gitlab during the day so sometimes it's a bit searching on Github. Thanks for the pointer ✌️

if __name__ == "__main__":
print("Sending meeting notification.")

sent: [Notification] = []
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this is correct Python3.8 syntax.

Suggested change
sent: [Notification] = []
sent: List[Notification] = []

Copy link
Owner

Choose a reason for hiding this comment

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

We also need to add the from typing import List

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed with list builtin

@@ -107,3 +106,9 @@ def userNotificationCenter_didActivateNotification_(

# return notification
return new_notif


def cancel_notification(uid:str):
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
def cancel_notification(uid:str):
def cancel_notification(uid: str) -> None:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -30,6 +31,15 @@
logger = logging.getLogger()


class Notification(object):
def __init__(self, uid):
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
def __init__(self, uid):
def __init__(self, uid: str) -> None:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

def __init__(self, uid):
self.uid = uid

def cancel(self):
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
def cancel(self):
def cancel(self) -> None:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -12,6 +12,7 @@
from mac_notifications.listener_process import NotificationProcess
from mac_notifications.notification_config import NotificationConfig
from mac_notifications.singleton import Singleton
from .notification_sender import cancel_notification
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
from .notification_sender import cancel_notification
from mac_notifications import notification_sender

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced with

from mac_notifications.notification_sender import cancel_notification

self.uid = uid

def cancel(self):
cancel_notification(self.uid)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
cancel_notification(self.uid)
notification_sender.cancel_notification(self.uid)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above

@basilevs
Copy link
Contributor Author

basilevs commented Nov 3, 2023

All suggestions have been acted upon

Copy link
Owner

@Jorricks Jorricks left a comment

Choose a reason for hiding this comment

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

Looks good. Let's merge it :)

@Jorricks Jorricks merged commit 76dbd33 into Jorricks:main Feb 4, 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.

There is no API to delete notifications
2 participants