libsdl-org
Add system tray support
Description
This adds support for system tray (or equivalent) on Windows, macOS and Unix using AppIndicator.
Currently supported:
- Custom tray icon (I used test/trashcan.bmp as an example)
- Tooltip on the tray icon
- Menus including:
- Items
- Disabled items
- Checkboxes
- Separators
- Submenus
- Custom callbacks on any item
Notes
The Unix implementation uses AppIndicator, which in turn uses GTK. I've done my best to keep SDL and the other libraries at arm length; nothing is required at compilation, and runtime will dynamically check for the libraries with dlopen and fail gracefully if they're not found.
Due to the lack of headers, I had to copy over some function declarations and macros into SDL (only the bare minimum). I added the macro APPINDICATOR_HEADER that should expand to <path/to/appindicator.h> if it is set. I currently did not implement looking for the header and setting the macro, so the macro is always unset for now.
On macOS, SDL_INIT_VIDEO is needed.
On all platforms, there is an "inner thread" that runs the tray that feels like it should be part of the main thread. It currently does not need to change anything outside src/tray, but it would be good to review the best practices there, and update anything that needs updates.
TODO & FIXME
- [x] Add various SDL functions to read, move and delete menu items as necessary
- [x] Document the functions
- [x] Review the need for a new subsystem? (Possibly integrate it to the Video subsystem)
- [x] Change OR'able enums to ints
- [x] Review the less-than-pretty AppIndicator implementation
- [x] If the
APPINDICATOR_HEADERfeature is retained, adapt the code so that the function names don't clash withdlsym - [x] Move the function that converts an SDL_Surface to a Win32 HICON somewhere where it can be re-used?
- [x] ~~Use a better
mktemp-like mechanism with AppIndicator (SDL_CreateTempFolder?)~~ (Not possible/worth it) - [x] Properly set the ID of the tray on Unix
- [x] Properly integrate
tests/testtray.c - [x] Document the need for SDL_INIT_VIDEO on macOS
- [x] Review memory management (I wasn't particularly careful)
- [x] Remove the icon file generated for AppIndicator?
- [x] Convert windows functions ending in A (or without letter) to functions ending in W?
- [x] Decide how to handle the behavior of creating a system tray but failing to set an icon
- [ ] Support DBus on Unix?
- [x] Properly support removing/inserting at position taking dividers into account
- [x] Some platforms automatically check/uncheck a checkable item when it is clicked, others don't; make the behavior consistent
- [x] Clean up testtray.c: add more checks for
NULL, add more test cases, integrate with the testing environment. - [x] Extensively test Unix support across distributions and systems, including BSD's
- [x] Create getters and setters for the label
- [x] Finish getters and setters on Unix
Existing Issue(s)
Closes #7822
I'm still working on this on my free time, but I would need counsel for the following points from the to-do list:
- Review the need for a new subsystem? (Possibly integrate it to the Video subsystem)
- Move the function that converts an SDL_Surface to a Win32 HICON somewhere where it can be re-used?
- [I would need help to] Properly integrate
tests/testtray.c- Convert windows functions ending in A (or without letter) to functions ending in W?
- Decide how to handle the behavior of creating a system tray but failing to set an icon [(Currently it silently replaces the icon with the default icon)]
- Review the need for a new subsystem? (Possibly integrate it to the Video subsystem)
We don't need a new subsystem for this.
- Move the function that converts an SDL_Surface to a Win32 HICON somewhere where it can be re-used?
Yes, that sounds good. It can go in core/windows/SDL_windows.c, or if you put this in the video subsystem, you can just put it in there.
- [I would need help to] Properly integrate
tests/testtray.c
Just add it to tests/CMakeLists.txt, use testdialog.c as an example.
- Convert windows functions ending in A (or without letter) to functions ending in W?
You can leave them without a letter. SDL builds with UNICODE defined. You'll need to convert from UTF-8 to WCHAR, and there are plenty of examples of this in the windows video driver.
- Decide how to handle the behavior of creating a system tray but failing to set an icon [(Currently it silently replaces the icon with the default icon)]
That is reasonable behavior, I think. I don't expect failing to create an icon to be common.
@slouken Is there an SDL equivalent to
snwprintffor wide-char strings?
Yep: SDL_swprintf()
Note for reviewers: I replaced the SDL_TrayEntryFlags enum with an Uint32 and #defines to make them OR'able without warnings. I basically copied what I saw for SDL_InitFlags, but I'm not sure if the implementation and/or the documentation is done properly; if someone could check it, it would help me a lot.
Note for reviewers: I replaced the SDL_TrayEntryFlags enum with an Uint32 and #defines to make them OR'able without warnings. I basically copied what I saw for SDL_InitFlags, but I'm not sure if the implementation and/or the documentation is done properly; if someone could check it, it would help me a lot.
Yes, this looks good.
Some general feedback on the API, there are a number of functions that seem redundant. You want the minimal functional set, so you can do things like remove the createorget, since you have both create and get. It might be worth taking a break and coming back to it with fresh eyes with that approach in mind.
Did you consider doing your own StatusNotifierItem implementation instead of using libappindicator? Then you can avoid the gtk stuff too.
Did you consider doing your own StatusNotifierItem implementation instead of using libappindicator? Then you can avoid the gtk stuff too.
+1 from me. SDL is already using DBus, so this would not be an entirely new concept to introduce.
The last point in my to-do list is whether or not to support DBus. I did think about it (I also thought about org.ayatana.indicator), but I'm not even remotely familiar with DBus enough to write an implementation by myself. I can try to look into it after I've revised the API.
Revised API proposal:
SDL_CreateTray; SDL_SetTrayIcon; SDL_SetTrayTooltip; SDL_CreateTrayMenu; SDL_CreateTraySubmenu; SDL_GetTrayMenu; ~~SDL_CreateOrGetTrayMenu~~ -> Let the developers handle that on their own; SDL_GetTraySubmenu; ~~SDL_CreateOrGetTraySubmenu~~ -> Let the developers handle that on their own; SDL_GetTrayEntries; ~~SDL_GetTrayEntryAt~~ -> SDL_GetTrayEntries(...)[i]; ~~SDL_RemoveTrayEntry~~ -> SDL_RemoveTrayEntryAt(..., -1); (It seems like every platform supports -1 == append) SDL_RemoveTrayEntryAt; SDL_InsertTrayEntryAt; ~~SDL_AppendTrayEntry~~ -> SDL_InsertTrayEntryAt(..., -1); ~~SDL_AppendTraySeparator~~ -> SDL_InsertTraySeparatorAt(...); SDL_SetTrayEntryChecked; SDL_GetTrayEntryChecked; SDL_SetTrayEntryEnabled; SDL_GetTrayEntryEnabled; SDL_SetTrayEntryCallback; SDL_DestroyTray;
Does this look good? I've retained the get functions so that someone doesn't have to be extra careful keeping all the pointers in memory.
Small modification to what I wrote above: I realized after posting that SDL_RemoveTrayEntry does not, in fact, remove the last element as SDL_Append* does, but rather, takes a SDL_TrayEntry pointer as argument.
Instead of removing SDL_RemoveTrayEntryAt and suggesting a meaningless SDL_RemoveTrayEntry((SDL_TrayEntry *) -1) as a replacement, I'll keep SDL_RemoveTrayEntry and suggest replacing SDL_RemoveTrayEntryAt(menu, pos) with SDL_RemoveTrayEntry(SDL_GetTrayEntries(menu)[pos]);
API-wise, would it be good to replace SDL_InsertTraySeparatorAt with SDL_InsertTrayEntryAt + a flag to indicate that a separator is wanted?
What prompts the question is the current second-from-last point in my to-do list (properly support getting/removing separators) needing separators to be stocked in an array of SDL_TrayEntry, which means a separator would be a SDL_TrayEntry to the API; also the lower-level APIs all treat separators as normal items. However, that would leave a meaningless "name" argument, that is required for all other types of entries but not for separators.
Additionally, if I do that, should I enforce that the name be null for separators to avoid potential programming mistakes, or should I just ignore it?
Running testtray segfaults immediately on my system (Fedora 39).
It segfaults because SDL_CreateTray returns a NULL.
The following patch detects this and exits the test app.
Additionally, all SDL tray functions should validate their inputs before forwarding to the backend.
--- a/test/testtray.c
+++ b/test/testtray.c
@@ -25,6 +25,10 @@ int main(int argc, char **argv)
SDL_Surface *icon = SDL_LoadBMP("../test/trashcan.bmp");
SDL_Tray *tray = SDL_CreateTray(icon, "Hello!");
+ if (!tray) {
+ SDL_LogError(SDL_LOG_CATEGORY_APPLICATION, "SDL_CreateTray failed (%s)", SDL_GetError());
+ return 1;
+ }
SDL_TrayMenu *menu = SDL_CreateTrayMenu(tray);
SDL_TrayEntry *entry = SDL_AppendTrayEntry(menu, "Hello!", SDL_TRAYENTRY_CHECKBOX | SDL_TRAYENTRY_CHECKED);
SDL_TrayEntry *entrysm = SDL_AppendTrayEntry(menu, "Submenu", SDL_TRAYENTRY_SUBMENU);
After this change, the following error is printed:
ERROR: SDL_CreateTray failed (Could not load GTK/AppIndicator libraries)
After installing libayatana-appindicator-gtk3-devel.x86_64, the error changes to:
(testtray:99729): Gtk-CRITICAL **: 23:23:22.625: gtk_widget_get_scale_factor: assertion 'GTK_IS_WIDGET (widget)' failed
This assertion is thrown in the SDL_AppendTraySeparator(menu); call.
(Using libappindicator3.so instead of libayatana-appindicator3.so does not change anything)
No tray is appearing in the top menu bar of my gnome desktop.
API-wise, would it be good to replace SDL_InsertTraySeparatorAt with SDL_InsertTrayEntryAt + a flag to indicate that a separator is wanted?
What prompts the question is the current second-from-last point in my to-do list (properly support getting/removing separators) needing separators to be stocked in an array of SDL_TrayEntry, which means a separator would be a SDL_TrayEntry to the API; also the lower-level APIs all treat separators as normal items. However, that would leave a meaningless "name" argument, that is required for all other types of entries but not for separators.
Additionally, if I do that, should I enforce that the name be null for separators to avoid potential programming mistakes, or should I just ignore it?
Is it reasonable for a tray entry to be defined as a separator if it's name is NULL?
Running
testtraysegfaults immediately on my system (Fedora 39). It segfaults becauseSDL_CreateTrayreturns aNULL.
The test is barely a stub that helps me with debugging; it is neither error-safe nor exhaustive. Everything that returns a pointer should check for NULL, and many more combinations and scenarios would need to be tested. I haven't done it now because I planned to work on it more seriously when both the API and the code will be pretty much done. I'll add a point on my to-do list to not forget about it.
Is it reasonable for a tray entry to be defined as a separator if it's name is NULL?
Reminds me I forgot to add a getter and a setter for the entry label, I'll add those.
I can define it that way. I've first defined it as a separator if the internal OS-specific pointer was NULL and added a function SDL_TrayEntryIsSeparator, but I think I can keep it consistent by making it the label instead.
Is it reasonable for a tray entry to be defined as a separator if it's name is NULL?
Reminds me I forgot to add a getter and a setter for the entry label, I'll add those.
I can define it that way. I've first defined it as a separator if the internal OS-specific pointer was
NULLand added a function SDL_TrayEntryIsSeparator, but I think I can keep it consistent by making it the label instead.
Visually that's what I would expect. I wasn't sure if that mapped cleanly onto the implementation though.
About entries with a checkbox, I noticed that platforms by default won't change the value of the checkbox when the user presses on it, and it's the job of the callback to implement that.
Should I automatically toggle the checkbox on click, or should I do as platforms do and let the developers do it themselves?
About entries with a checkbox, I noticed that platforms by default won't change the value of the checkbox when the user presses on it, and it's the job of the callback to implement that.
Should I automatically toggle the checkbox on click, or should I do as platforms do and let the developers do it themselves?
It seems reasonable to take care of this on behalf of the application.
About entries with a checkbox, I noticed that platforms by default won't change the value of the checkbox when the user presses on it, and it's the job of the callback to implement that. Should I automatically toggle the checkbox on click, or should I do as platforms do and let the developers do it themselves?
It seems reasonable to take care of this on behalf of the application.
Would it be more intuitive to have the callback invoked before updating the checkbox or after?
After digging in a bit more, I've found that on both macOS and Windows, all tray entries are checkboxes; there's no low-level distinction between a "checkbox" and a non-checkbox. If I were to implement automatic toggling, I may end up accidentally toggling something that shouldn't be checkable.
I can work around this by keeping track internally of the creation flags, and only toggle what has been created as a checkbox. However, it won't prevent a developer from checking an entry created as a non-checkbox through SDL functions.
Should I keep track of creating flags, and should I forbid manual checking/unchecking of entries not created as checkboxes?
Should I keep track of creating flags, and should I forbid manual checking/unchecking of entries not created as checkboxes?
Sure, we can remove that limitation later if it ends up being a problem, but will probably save someone some headache.
@slouken Is it okay for test programs to depend on systems other than the one they're testing? I'm remaking testtray.c and I thought about doing a dynamically editable tray menu tree, and I'd like to use dialogs to have the user enter various inputs, like the file dialogs for the icon.
Also, does SDL support a MessageBox-style dialog that prompts the user to type some text, and if not, would it be worth adding it?
@slouken Is it okay for test programs to depend on systems other than the one they're testing? I'm remaking testtray.c and I thought about doing a dynamically editable tray menu tree, and I'd like to use dialogs to have the user enter various inputs, like the file dialogs for the icon.
Yes, that's fine.
Also, does SDL support a MessageBox-style dialog that prompts the user to type some text, and if not, would it be worth adding it?
We don't currently have that. We could add it, but suddenly we're in the realm of dealing with on-screen keyboards, IMEs, etc. I would lean towards not adding that.
Also, does SDL support a MessageBox-style dialog that prompts the user to type some text, and if not, would it be worth adding it?
We should use SDL's normal text input capabilities for this.
We should use SDL's normal text input capabilities for this.
I'm aware of SDL_StartTextInput(), but is there a standard graphical way to prompt the user to enter a string?
I believe this should be all for the initial implementation.
Unix currently does not support any of the various DBus mechanisms I could find, and I don't think I can understand and implement any myself. The current AppIndicator implementation works on Gnome systems with the AppIndicator shell extension, which is installed by default on Ubuntu and available on Fedora and other systems. Other systems are not supported, and calling SDL_CreateTray will error normally. I would prefer to let someone more knowledgeable than me work on the DBus implementation.
I've applied the proposed Windows changes; I've noticed that the tray doesn't work correctly in Wine 9.0. The tray icons are replacing each other, and the menu doesn't show up. Not too sure on how to proceed to fix this, as it may be a bug either in my code or in Wine. I only have Windows 10 to test; can someone test on Windows 11 if possible?