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

notifications actions #1735

Closed
totaam opened this issue Jan 7, 2018 · 10 comments
Closed

notifications actions #1735

totaam opened this issue Jan 7, 2018 · 10 comments

Comments

@totaam
Copy link
Collaborator

totaam commented Jan 7, 2018

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)
@totaam
Copy link
Collaborator Author

totaam commented Jan 13, 2018

2018-01-13 06:16:08: antoine uploaded file notifyicon-merge.patch (1.6 KiB)

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

@totaam
Copy link
Collaborator Author

totaam commented Jan 13, 2018

  • 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"

@totaam
Copy link
Collaborator Author

totaam commented Jan 18, 2018

2018-01-18 06:41:33: antoine uploaded file notifications-actions.patch (15.4 KiB)

work in progress

@totaam
Copy link
Collaborator Author

totaam commented Jan 18, 2018

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?)

@totaam
Copy link
Collaborator Author

totaam commented Jan 19, 2018

  • 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 html5 2.3 updates #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.

@totaam
Copy link
Collaborator Author

totaam commented Jan 19, 2018

2018-01-19 19:02:13: maxmylyn commented


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/]

@totaam
Copy link
Collaborator Author

totaam commented Jan 19, 2018

2018-01-19 19:03:51: maxmylyn uploaded file 1735discordlogddbus.txt (2249.7 KiB)

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

@totaam
Copy link
Collaborator Author

totaam commented Jan 20, 2018

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").

@totaam
Copy link
Collaborator Author

totaam commented Jan 23, 2018

2018-01-23 17:40:24: maxmylyn commented


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 18136 and the notifications no longer crash my client.

Closing.

@totaam totaam closed this as completed Jan 23, 2018
@totaam
Copy link
Collaborator Author

totaam commented Feb 18, 2018

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

@totaam totaam added the v2.2.x label Jan 22, 2021
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

No branches or pull requests

1 participant