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

Tray: SDL_GetTrayEntries caller-owned pointer, dark mode support on Windows #11809

Merged
merged 1 commit into from
Jan 4, 2025

Conversation

Semphriss
Copy link
Contributor

@Semphriss Semphriss commented Jan 1, 2025

Description

This fixes two issues with the system tray:

  • Windows now properly supports dark mode for the context menu.
  • SDL_GetTrayEntries returns a caller-owned pointer (it simply duplicates the internal list).

Existing Issue(s)

Fixes #11787

include/SDL3/SDL_tray.h Outdated Show resolved Hide resolved
src/tray/cocoa/SDL_tray.m Outdated Show resolved Hide resolved
if (size) {
*size = menu->nEntries;
*size = (int) menu->nEntries;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't nEntries int?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally, it was because nEntries couldn't be negative and it avoided casts when mixing it with memory/addressing-related functions. I did expose the size parameters as int in the interface though, so I can change it to int if necessary. (Alternatively, I can change the parameters to be size_t, but I'm not sure if size_t is an acceptable type to have in the API)

src/tray/cocoa/SDL_tray.m Outdated Show resolved Hide resolved
src/tray/unix/SDL_tray.c Outdated Show resolved Hide resolved
src/tray/windows/SDL_tray.c Outdated Show resolved Hide resolved
src/tray/cocoa/SDL_tray.m Outdated Show resolved Hide resolved
src/tray/unix/SDL_tray.c Outdated Show resolved Hide resolved
src/tray/windows/SDL_tray.c Outdated Show resolved Hide resolved
@slouken
Copy link
Collaborator

slouken commented Jan 3, 2025

Can you separate out the dark mode fix into a separate PR?

Thinking more about the memory management of the tray API, we usually return a copy of an array of IDs when the underlying objects can change out from under the user. Displays, joysticks, etc. all fall under this category. Returning a copy of an array of pointers doesn't actually make it any safer, because if the underlying data changes, you're left with a dangling pointer that can crash if you dereference it.

I think it's actually safer to just return a const, NULL terminated, array of pointers, like you did before (with the addition of NULL termination).

@icculus, am I missing anything here?

@slouken slouken added this to the 3.2.0 milestone Jan 3, 2025
@icculus
Copy link
Collaborator

icculus commented Jan 3, 2025

Agreed. Unlike unplugging a USB joystick, these shouldn't change outside of the app doing it itself.

@Semphriss
Copy link
Contributor Author

I've reverted the changes and made a new commit that just adds the NULL-termination of the lists; I'll open another PR for the dark theme now.

Copy link
Collaborator

@slouken slouken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

@slouken slouken merged commit 7673b84 into libsdl-org:main Jan 4, 2025
40 checks passed
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 this pull request may close these issues.

SDL_GetTrayEntries() doesn't follow usual array ownership rules
4 participants