samschott / desktop-notifier

Python library for cross-platform desktop notifications
https://desktop-notifier.readthedocs.io
MIT License
96 stars 11 forks source link

Can only clear notifications of the same `DesktopNotifier` instance #196

Open PhrozenByte opened 5 days ago

PhrozenByte commented 5 days ago

Description

Since #164 Desktop Notifier always creates custom notification identifiers and disregards/hides the "real" notification identifiers given by the backend. This has a major downside: It is now impossible to clear a notification from another process without some form of IPC, because the UUID to "real" identifier mapping is only known to the DesktopNotifier instance that dispatched/sent that notification. This also means that one can't clear a notification that was sent by a process that exited already.

As far as I understand #164 right the goal was to make Notification instances immutable? IMHO two things got mixed up here: There might indeed be some value (e.g. an UUID) to quickly identify a specific set of notification properties (title, message, icon, buttons, …), but this isn't the same as the identifier given by the backend after dispatching/sending the notification. Just think about dispatching/sending the same notification multiple times: The properties are the same, but the notification identifier changes.

IMHO there should be a DispatchedNotification class instead that stores the backend's notification identifier as property and optionally also the Notification instance, if that's still available (it is when the notification was sent by the same DesktopNotifier, but might not otherwise). This class could then also have a clear() method. DesktopNotifier.send_notification() could then return instances of this class.

I switched to Desktop Notifier 5.0.1 with some (rather sketchy) code to get some features of v6.0 back (notably callbacks at the DesktopNotifier level and to identify buttons) in the meantime.

In any case: Great project, thank you for sharing your work! :heart:

PhrozenByte commented 4 days ago

I did some more testing with Desktop Notifier 5.0.1 and it seems like that for some unknown reason I can't clear notifications created by another DesktopNotifier instance no matter what. Take the following testing snippet:

import asyncio
from desktop_notifier import DesktopNotifier, Notification

async def main():
    notifier = DesktopNotifier()
    notification = await notifier.send(title="Hello world!", message="Sent from Python")

    await asyncio.sleep(3)

    notifier2 = DesktopNotifier()

    # workaround, make sure that the DBus interface is initialized
    await notifier2.get_capabilities()

    notification2 = Notification(title="", message="")
    notification2.identifier = notification.identifier

    await notifier2.clear(notification2)

asyncio.run(main())

Considering that the code just calls self.interface.call_close_notification(nid) and since I don't see anything in the DBus interface init code that could limit access, I don't really see a reason why it shouldn't properly clear the notification. However, it doesn't, it throws a "Invalid notification ID" exception instead:

dbus_next.errors.DBusError: Invalid notification ID

With other software (e.g. libnotify's notify-send and vlevit's notify-send.sh) it's no problem to clear notifications created by other processes, even created by other libraries. The issue definitely is with the notification created by DesktopNotifier: I can't close a notification created by DesktopNotifier with anything other than the DesktopNotifier instance that sent/dispatched that notification (notify-send.sh e.g. also just prints "Invalid notification ID"), but I can close a notification created by e.g. notify-send.sh with DesktopNotifier. That's weird.

Any ideas?

samschott commented 4 days ago

@PhrozenByte, this is indeed a current limitation of the Dbus implementation. A bit of background:

As far as I understand https://github.com/samschott/desktop-notifier/pull/164 right the goal was to make Notification instances immutable?

Partially. This PR bundled lots of stuff and since I am the only maintainer the package doesn't really have wide-spread usage, I did not actually explain most of the decisions.

DesktopNotifier creates its own notification identifiers in a platform independent format, using UUIDs generated by Python. This allows the API to be uniform and also allows making the notification instance immutable because we can generate the ID when the instance is created. On macOS, iOS and Windows, the caller can actually specify the identifier, so we just pass on the ID generated by Python on to the OS. Clearing notifications or reacting to callbacks then works as expected between instances and even processes (though the App is still responsible for persisting notification IDs where needed).

On Dbus however, notification IDs are set by the platform / notification server and returned after the notification was sent. They are also int32 and not strings or 128-bit integers which would be compatible with typical UUIDs. int32 is a lot more likely to lead to collisions and there is no good mapping from UUIDs to int32, so I've just opted for a hacky workaround of maintaining an internal map in the DBUS backend instead of exposing their arguably bad API and trying to find abstraction that works across Dbus and other backends.

This does mean however that for Dbus, the notification IDs are indeed only meaningful to the DesktopNotifier instance that created them.

Edited for typos.

PhrozenByte commented 4 days ago

Thanks for the fast response :+1:

I honestly don't see an issue with DBus' uint32 IDs: Since the ID is provided by the notification server, the ID is guaranteed to be unique (i.e. no collisions, so there isn't really a need for a random UUID) and an uint32 allows for 4,294,967,295 notifications per session, so no issue there either. I agree that using a 128-bit UUID instead "looks" a bit "nicer", but it has major practical disadvantages. My use case mentioned earlier is made impossible by that change, but my use case isn't very common to be fair. However, it also blocks other, way more commonly used features of FreeDesktop.org notifications, like replacing existing notifications in-place (which is one of the primary reasons why so many alternatives to libnotify's notify-send exist btw, like vlevit's notify-send.sh and phuhl's notify-send.py).

I like Desktop Notifier very much, because it's the only modern, fully-featured cross-platform notification library written in Python I found. Did you check your download stats recently? I'd say that with more than 8,500 downloads last month we can assume a pretty decent user basis :clap: :+1:

Anyway, back to topic: If you want to keep the 128-bit UUIDs we can do that, no problem. We could simply also expose the ID provided by the backend. I could open a PR to implement that in the way I've outlined in https://github.com/samschott/desktop-notifier/issues/196#issue-2649653643, i.e. by wrapping the then still immutable Notification class (including its random 128-bit UUID) in a DispatchedNotification class additionally storing the ID provided by the backend. If the backend provides no other ID (following your explanation I assume that to be true on the other platforms) they simply match. Desktop Notifier could then use either-or to identify the notification to work with (i.e. its up to the developer using DesktopNotifier what they want to use). Are you willing to review and merge such PR?

However, before investing time in improving Desktop Notifier I got a more pressing issue that blocks me from using the library at all: As explained in https://github.com/samschott/desktop-notifier/issues/196#issuecomment-2470583339 I'm currently using Desktop Notifier 5.0, i.e. the library doesn't use UUIDs a different DesktopNotifier instance can't know, but the uint32 IDs provided by DBus / the backend. The problem is that I still can't clear/close a notification created by Desktop Notifier with anything other than the DesktopNotifier instance that sent/dispatched that notification. Again, just to highlight that, even though I'm using the uint32 ID! I've no idea what's going on there... Please check the snippet I've provided earlier (with desktop-notifier~=5.0). Any ideas?