xpra icon
Bug tracker and wiki

Opened 12 months ago

Closed 11 months ago

Last modified 10 months ago

#1735 closed enhancement (fixed)

notifications actions

Reported by: Antoine Martin Owned by: J. Max Mena
Priority: major Milestone: 2.3
Component: core Version: 2.2.x
Keywords: Cc:

Description (last modified by Antoine Martin)

Follow up from #1492, adding support for "actions" and "action-icons" optional capabilities.

Would also be nice to manage to:

  • provide our guid with the notifications (the tray does use it)
  • merge the two slightly different implementations of notifyicon ctypes struct
  • tie the notifications to the tray icon belonging to the application which is emitting the notification on win32 (using the appid attribute to identify the tray icon)

Attachments (3)

notifyicon-merge.patch (1.6 KB) - added by Antoine Martin 11 months ago.
switching to the other ctypes implementation does not work: the structures are not equivallent
notifications-actions.patch (15.4 KB) - added by Antoine Martin 11 months ago.
work in progress
1735discordlogddbus.txt (2.2 MB) - added by J. Max Mena 11 months ago.
Server started with -d dbus - sample Discord notification piped into the server logs.

Change History (11)

comment:1 Changed 11 months ago by Antoine Martin

Description: modified (diff)
Status: newassigned

Changed 11 months ago by Antoine Martin

Attachment: notifyicon-merge.patch added

switching to the other ctypes implementation does not work: the structures are not equivallent

comment:2 Changed 11 months ago by Antoine Martin

  • r17997: try to match notifications to the application tray icon - this is unreliable because the notification specification does not have anything in it we can use. Maybe we should submit an optional enhancement?
  • the two different implementations are not equivalent (the size of the struct is different), and things work as they are - so they'll remain separate

Still todo: "actions"

Changed 11 months ago by Antoine Martin

Attachment: notifications-actions.patch added

work in progress

comment:3 Changed 11 months ago by Antoine Martin

r18044 adds most of the code required.

Still TODO:

  • hookup the callback events so we can forward them back to the server
  • add "actions" support to the non-dbus implementations (in particular win32 and macos)
  • enable it by default (the actions are not always show on Fedora with gnome shell?)

comment:4 Changed 11 months ago by Antoine Martin

Owner: changed from Antoine Martin to J. Max Mena
Status: assignednew
  • enabled by default for posix platforms with dbus in r18054
  • added "actions" support to the gtk fallback implementation used on macos in r18055
  • will deal with the HTML5 client in #1670

(also some py3k / gtk3 fixes in r18056)

Turns out that it does work with gnome-shell but the usability is terrible (maybe they're trying to make so bad that they can claim the feature is broken then remove it, like they did for the systray?): you have to move away from the notification if the pointer is already there, then back over it to get the action buttons to slide down and reveal themselves... (why, oh why)

We can't implement "actions" on win32, as the NOTIFYICONDATA API is just too limited for that.
We could switch to the GTK variant (as used on macos) for the notifications that require action buttons, but we would need to:

  • make those custom gtk notification windows less ugly, ideally more similar to the native ones
  • find the systray location and try to point to it in the same way that the system ones do (I don't even think this is possible to do..)

To test, run browser/xpra/trunk/src/tests/xpra/test_apps/test_system_tray.py:

./tests/xpra/test_apps/test_system_tray.py

From the systray that shows up, click "Send Notification" from the context menu.
The notification should show up on the client with 2 options, each option should trigger a different ACTION_ID message on the server, ie:

notification_action(NOTIFICATION_ID, ACTION_ID)

Alternatively, this could be tested with any applications that uses notification actions.
Note: this test app isn't useful on macos, because it seems that the systray forwarding doesn't work with right-clicks... (and with the python3 / GTK3 builds, not even left clicks..)

@maxmylyn: mostly a FYI.

Last edited 11 months ago by Antoine Martin (previous) (diff)

comment:5 Changed 11 months ago by J. Max Mena

Owner: changed from J. Max Mena to Antoine Martin

For reference my client and server are both Fedora 26 machines running trunk r18058.

So I decided to test this using Discord which is a popular internet messaging application (sigh, Electron based) that's focused on internet gaming. It's similar to IRC in that there's servers and channels and what-not. Anyways, Discord notifications totally break the client printing:

dbus[5087]: arguments to dbus_message_iter_open_container() were incorrect, assertion "(type == DBUS_TYPE_ARRAY && contained_signature && *contained_signature == DBUS_DICT_ENTRY_BEGIN_CHAR) || contained_signature == NULL || contained_signature_validity == DBUS_VALID" failed in file ../../dbus/dbus-message.c line 2998.
This is normally a bug in some application using the D-Bus library.

  D-Bus not built with -rdynamic so unable to print a backtrace
Aborted (core dumped)

Thanks to some help with Jake I got a sample notification for you and piped it into a server log with -d dbus. If you need anything more specific let me know.

You can get the Discord application for Fedora here: https://copr.fedorainfracloud.org/coprs/tcg/discord/

Changed 11 months ago by J. Max Mena

Attachment: 1735discordlogddbus.txt added

Server started with -d dbus - sample Discord notification piped into the server logs.

comment:6 Changed 11 months ago by Antoine Martin

Owner: changed from Antoine Martin to J. Max Mena

Updates:

  • r18062: make it possible to disable native notification backends on the client using XPRA_NATIVE_NOTIFIER=0
  • r18064: minor tweaks and fixes, maybe fixes the issue from comment:5
  • r18066: add info to "xpra info", clients report if they support "actions" or not
  • r18067: support added to html5 client, including a standalone test app since the html5 client does not support systray: browser/xpra/trunk/src/tests/xpra/test_apps/test_notification.py
  • r18068: hold "Shift" to trigger the other signal from the system tray: left click does a right click and vice versa, this is a workaround for platforms like macos which only support left click events on the systray (AFAICT)
  • related: r18080 adds a docking area for system trays to the html5 client

Sorry, but I'm not going through the pain of installing some proprietary crap to figure out what it does with its dbus messages. Then registering and figuring out how this new proprietary protocol is supposed to be used. Just no.
If the bug cannot be reproduced with an open source application I can actually use locally and dissect, then it does not exist.

Discord notifications totally break the client printing:

It would be clearer to say: "client, printing:"

BTW, your server log shows just:

org.xpra.Server.Event(connection-lost, ['22c21c9cb6a8b4831179b11e3cd31f43562530e4'])

Since it is the client that crashed, it is the log output from that side that we would need. (with as much detail as possible, maybe even "-d all").

Last edited 11 months ago by Antoine Martin (previous) (diff)

comment:7 Changed 11 months ago by J. Max Mena

Resolution: fixed
Status: newclosed

Sorry, but I'm not going through the pain of installing some proprietary crap to figure out what it does with its dbus messages.

I understand that, but Electron "apps" aren't going away anytime soon, even though I wish they would. But, I understand your trepidation at trying to debug something blindly.

However, it's not an issue anymore - it looks like r18064 did the trick. My client and server are currently at r18136 and the notifications no longer crash my client.

Closing.

comment:8 Changed 10 months ago by Antoine Martin

Related improvements (loop detection, notifications, etc): r18468

Note: See TracTickets for help on using tickets.