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

Can only clear notifications of the same DesktopNotifier instance #196

Open
PhrozenByte opened this issue Nov 11, 2024 · 7 comments · May be fixed by #200
Open

Can only clear notifications of the same DesktopNotifier instance #196

PhrozenByte opened this issue Nov 11, 2024 · 7 comments · May be fixed by #200

Comments

@PhrozenByte
Copy link
Contributor

  • Desktop Notifier version: 6.0.0
  • Python version: 3.11.10
  • Operating System: Arch Linux

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! ❤️

@PhrozenByte
Copy link
Contributor Author

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
Copy link
Owner

samschott commented Nov 12, 2024

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

As far as I understand #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
Copy link
Contributor Author

PhrozenByte commented Nov 12, 2024

Thanks for the fast response 👍

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 👏 👍

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 #196 (comment), 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 #196 (comment) 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?

@PhrozenByte
Copy link
Contributor Author

PhrozenByte commented Nov 17, 2024

It took me quite some time to find the cause of my issue: It seems like that one can't close a notification using another DBus bus if the bus that sent / dispatched that notification is still connected. After disconnecting the original bus I can close the notification with any other DBus bus. However, for any other DBus bus than the original one the action callbacks don't fire. See reproducer snippet below (w/o Desktop Notifier, proving that it's no issue with Desktop Notifier).

I recently upgraded from Gnome 3.28 (yes, quite ancient) to Gnome 47.1 and I found that bkw777's notice.sh I used before stopped working too, so this might be some new Gnome limitation.

Do you know anything about this? Any ideas in general? Please let me know, my next step is to open an upstream issue with Gnome.

My ultimate goal still is that Desktop Notifier gets support for my use case, as described earlier. I hope you're willing to review and merge such PR. Thanks for your help!

from __future__ import annotations

import asyncio
from typing import Callable

from dbus_next.signature import Variant
from dbus_next.aio.message_bus import MessageBus
from dbus_next.aio.proxy_object import ProxyInterface

async def main():
    async def _init(on_action: Callable[[int, str], None] | None = None) -> ProxyInterface:
        bus = await MessageBus().connect()
        introspection = await bus.introspect(
            "org.freedesktop.Notifications", "/org/freedesktop/Notifications"
        )
        proxy_object = bus.get_proxy_object(
            "org.freedesktop.Notifications",
            "/org/freedesktop/Notifications",
            introspection,
        )
        interface = proxy_object.get_interface(
            "org.freedesktop.Notifications"
        )

        if on_action is not None:
            interface.on_action_invoked(on_action)

        return interface

    async def _reset(interface: ProxyInterface) -> None:
        bus: MessageBus = interface.bus
        bus.disconnect()
        await bus.wait_for_disconnect()

    async def _send(interface: ProxyInterface, title: str, message: str, app_name: str = "", icon: str = "") -> int:
        nid: int = await interface.call_notify(
            app_name, 0, icon, title, message,
            ["default", ""], {"urgency": Variant("y", 1)}, -1,
        )
        print("Dispatched notification {}".format(nid))
        return nid

    async def _close(interface: ProxyInterface, nid: int) -> None:
        print("Closing notification {}...".format(nid))
        await interface.call_close_notification(nid)

    def _on_action(nid: int, action_key: str) -> None:
        print("Action {} invoked on notification {}".format(action_key, nid))

    interface = await _init(_on_action)
    nid = await _send(interface, "Hello World", "This is a notification test", app_name="test app")

    # we can: invoke the default action, close the notification using the same DBus bus
    # we can not: close the notification using another DBus bus
    await asyncio.sleep(5)

    print("Resetting DBus bus...")
    await _reset(interface)
    interface = await _init(_on_action)

    # we can: close the notification using the new DBus bus
    # we can not: invoke the default action, do anything with the old DBus bus (obviously...)
    await asyncio.sleep(5)

    await _close(interface, nid)

asyncio.run(main())

@PhrozenByte
Copy link
Contributor Author

It's definitely an upstream issue that was introduced to workaround misbehaving clients... 😒

See https://gitlab.gnome.org/GNOME/gnome-shell/-/issues/8072

@PhrozenByte PhrozenByte linked a pull request Nov 19, 2024 that will close this issue
8 tasks
@samschott
Copy link
Owner

Thanks for the digging and the background!

I understand that the current dbus backend setup in DesktopNotifier is not ideal, but I am not entirely sure whether a breaking change to the API is worth the added benefits here. This is partially because I do not fully understand your use case. Is it to interact with notifications that we sent by a previous run of the app? Or is it rather about having multiple processes running at the same time interacting with the same notification?

In principle, letting the client provide a notification ID is nicer because:

  1. It allows clients to retry on the same ID without creating duplicate notifications even if they do not get a response back, e.g., when the dbus connection closes after the notification was sent but before the response is transmitted to caller.
  2. It allows clients to identify a resource without needing store a returned identifier locally.

Admittedly, either is less of a concern with local desktop notifications vs remote push notifications, but that's where the design idea originated from.

Letting a client provide the ID almost then requires using UUIDs to prevent accidental clashes. The 32 bit space of the DBus API suddenly looks a lot smaller when IDs are picked randomly instead of sequentially (you have a ~1% chance of collisions after ~ 10k notifications).

An option to keep the current API but allow interacting with notifications between multiple instances of the same application would be for DesktopNotifierDbusBackend to use local storage to persist the mapping between generated UUID and returned notification ID.

Any client that meaningfully wants to interact with notifications between different runs of the same app would need to do this anyways. And parallel processes would still need IPC to communicate which notification was scheduled for the other process to interact with it.

@PhrozenByte
Copy link
Contributor Author

Is it to interact with notifications that we sent by a previous run of the app? Or is it rather about having multiple processes running at the same time interacting with the same notification?

Both. Plus, it needs to be able to interact with notifications sent by other libraries as well.

In principle, letting the client provide a notification ID is nicer because

I understand that you prefer client-provided identifiers and UUIDs, but I truly don't want to argue about that. We must separate topics here: One is the FreeDesktop.org API and how this API could be improved, the other is how this API should be used by Desktop Notifier.

The FreeDesktop.org API doesn't support client-provided identifiers and UUIDs, but uses an incrementing index provided by the server. That's a fact Desktop Notifier must work with, no matter the advantages and disadvantages of the approach chosen by the FreeDesktop.org project. One could suggest the FreeDesktop.org project to change their API, but I doubt that they will do that.

In the end this is solely about how Desktop Notifier uses the FreeDesktop.org API. With v5.0 there was no such issue because it used the identifier provided by the server. With v6.0 the native identifier is fully masked and there's no "legal" way to access the native identifier. The native identifier is nowhere exposed in the Desktop Notifier API. I can't do IPC with or store information I don't have.

This isn't the only reason for changing the Desktop Notifier API though. The other reason is this (also see #196 (comment)):

import asyncio
import signal
from desktop_notifier import DesktopNotifier, Notification

async def main() -> None:
    notifier = DesktopNotifier(app_name="Sample App")

    notification = Notification(
        title="Julius Caesar",
        message="Et tu, Brute?",
    )

    await notifier.send_notification(notification)

    await notifier.send_notification(notification)

    # raises bidict.ValueDuplicationError exception:
    #
    # Notification failed
    # Traceback (most recent call last):
    #   File "…/src/desktop_notifier/backends/base.py", line 61, in send
    #     await self._send(notification)
    #   File "…/src/desktop_notifier/backends/dbus.py", line 173, in _send
    #     self._platform_to_interface_notification_identifier[platform_id] = (
    #     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^
    #   File "…/.venv/lib/python3.11/site-packages/bidict/_bidict.py", line 80, in __setitem__
    #     self.put(key, val, on_dup=self.on_dup)
    #   File "…/.venv/lib/python3.11/site-packages/bidict/_bidict.py", line 106, in put
    #     self._update(((key, val),), on_dup=on_dup)
    #   File "…/.venv/lib/python3.11/site-packages/bidict/_base.py", line 454, in _update
    #     dedup_result = self._dedup(key, val, on_dup)
    #                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    #   File "…/.venv/lib/python3.11/site-packages/bidict/_base.py", line 347, in _dedup
    #     raise ValueDuplicationError(val)
    # bidict.ValueDuplicationError: 1022a805-3ec4-4a17-9933-0afecf565342

    event = asyncio.Event()

    loop = asyncio.get_running_loop()
    loop.add_signal_handler(signal.SIGINT, event.set)
    loop.add_signal_handler(signal.SIGTERM, event.set)

    await event.wait()


asyncio.run(main())

I assume (untested) the same happens on both Windows and OS X, just with an error from the notifications server. That's a bug.

Even with client-provided identifiers the identifier just can't be created this early, but must be created right at the moment the notification is dispatched. The Notification class describes the contents of a notification (title, message, buttons, ...), not an notification that was sent to the server. That's exactly the reason why I suggest adding a DispatchedNotification class. It properly distinguishes between a notification's contents, and a notification that was sent to the server. That's not just true for Linux, but also for Windows and OS X. Also see 33682cd.

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 a pull request may close this issue.

2 participants