SNI/DBus tray support (take 2)#15189
Conversation
|
@slouken What do you think? |
|
Looks promising! |
|
@C0rn3j Once this is merged I will start working on the extra features you requested (scroll callbacks, textual icon descriptions, showing/hiding), do you have any more ideas? |
|
I don't, I think what I highlighted before was all there was for our use case with Tauon |
|
@eafton, can you summarize the benefits and risks of accepting this PR vs what we have currently? |
The current implementation uses a library declared obsolete (see https://github.com/AyatanaIndicators/libayatana-appindicator), and it uses GTK3 which causes all kinds of problems with synbol conflicts, state corruption errors, etc if the client application also wants to use GTK3 (or even anything GLib for that matter from my experience), the current implementation also does not support a few features like the icon click handlers and tooltips. And we can use icon surfaces directly without encoding them to PNG, saving them to disk, and messing with icon theme paths. There are only two risks that come to mind with this implementation that come to mind:
|
|
FWIW: I'm very much in favour of this approach, but there are definitely still a lot of bugs in this implementation. In particular, it has lots of invalid memory accesses if tray icons are destroyed and re-created. A quick addition of some code to reset various variables to NULL upon their destruction at least gets it to not crash here (see patch below), but there are still several others, and I suspect there are some more issues with thread safety I'm not encountering personally. index fcb86331b..993b0a200 100644
--- a/src/tray/unix/SDL_dbustray.c
+++ b/src/tray/unix/SDL_dbustray.c
@@ -545,6 +545,7 @@ void DestroyMenu(SDL_TrayMenu *menu)
if (entry->sub_menu) {
DestroyMenu((SDL_TrayMenu *)entry->sub_menu);
+ entry->sub_menu = NULL;
}
SDL_free(item);
SDL_free(entry);
@@ -582,6 +583,7 @@ void DestroyTray(SDL_Tray *tray)
/* Destroy the menus and entries */
if (tray->menu) {
DestroyMenu(tray->menu);
+ tray->menu = NULL;
}
/* Free the tray */
diff --git a/src/tray/unix/SDL_tray.c b/src/tray/unix/SDL_tray.c
index 6addef54b..c3eed64b9 100644
--- a/src/tray/unix/SDL_tray.c
+++ b/src/tray/unix/SDL_tray.c
@@ -346,5 +346,6 @@ void SDL_DestroyTray(SDL_Tray *tray)
if (!SDL_GetActiveTrayCount()) {
driver->DestroyDriver(driver);
+ driver = NULL;
}
} |
Sorry for not catching these memory errors, I was on a wild goose chase of fixing compatability errors with desktops. I am currenly away from my desk, If you have found more bugs please let me know so I can fix them when I get home. As for thread safety, the tray subsystem is designed to be used on the main thread, I need to add the thread checks present on other tray implementations into CreateTray. |
|
Much, much better. I'm only seeing one valgrind warning now, and there aren't any crashes even under the more extreme use case of repeatedly creating/destroying tray icons. The valgrind warning seems to be an issue with dbus (maybe struct padding or something?) rather than SDL, but I've included it here in case you want to look into it further… Valgrind: Syscall param sendmsg(msg.msg_iov[1]) points to uninitialised byte(s) (allocated dbus_message_iter_append_basic)But this is definitely working well enough for me so far! Thanks very much! |
I looked at the Valgrind log you have attached and it does seem to be a libdbus problem, thank you for reporting these issues though, I am also glad its working well now. |
|
In general this looks pretty good. I added a few comments above. In addition, I see you have a tray driver construct in the unix tray handling. Do you anticipate there being other drivers added in the future? |
Thanks, I shortened the credits and fixed the whitespace issues caused by clang-format. I added the driver system due to 3 main reasons:
|
|
@ssokolow Sorry for "stealing" your idea, I just wanted to have a go at it myself after your efforts seemingly stalled... |
|
Not at all a problem. I just had some spare time and some prior experience poking at the relevant APIs and wanted to share that knowledge. I didn't anticipate having time to write and debug C instead of Python to begin with. |
|
@icculus, I'll go ahead and merge this. I don't have time to look at the other issue right now, but if you want to take a look and provide feedback, please do! |
|
I recommend enabling viewing of tabs vs spaces, some wonky indentation made it in. I do mean in the code editor, but this is the GitHub Refined view: 
|
|
I ran clang-format over the changes, which should fix the whitespace issues. |
6 participants
The previous iteration of this had severe bugs on KDE and MATE...