Replace SDL_WINDOWEVENT

[icculus]

It's awkward to have:

switch (myevent->type) {
    case SDL_WINDOWEVENT:
        switch (myevent->window.event) {
            case SDL_WINDOWEVENT_FOCUS_LOST:
                // [...]

Why not just make the window events toplevel events in SDL3?

switch (myevent->type) {
    case SDL_WINDOWEVENT_FOCUS_LOST:
        // [...]

[slouken]

I like it!

[flibitijibibo]

This has definitely tripped up some developers before, +1

[akk0rd87]

First option gives you ability handle all SDL_WINDOWEVENT events in one condition.

[slime73]

Is that a common situation? Each windowevent has a fairly different meaning which usually needs fairly different code to handle properly.

[slouken]

The original motivation was to have the window events isolated to the SDL_video.h header for clean separation, but there hasn't been much value in that, and I think this change makes total sense.

[Akaricchi]

First option gives you ability handle all SDL_WINDOWEVENT events in one condition.

If that's really needed for something, how about:

switch (myevent->type) {
    default:
        if (myevent->type & SDL_WINDOWEVENT) {

Make SDL_WINDOWEVENT a high bit that's set in all SDL_WINDOWEVENT_* constants. Seems like the best of both worlds.

[furious-programming]

There is a lot of confusion about events right now. Window events are grouped under SDL_WINDOWEVENT and have sub-events, e.g. SDL_WINDOWEVENT_FOCUS_LOST. But that's not the case with joysticks, for example - there is no SDL_JOYSTICKEVENT group, just ungrouped direct events like SDL_JOYAXISMOTION or SDL_JOYDEVICEADDED. It's a mess.

If the events API is to be changed for the better, I suggest grouping all events as is currently done for window events. This gives additional possibilities (which I am currently using), because I can, with one condition or switch instruction, distinguish which subsystem a given event applies to and send it further.

So, instead of removing the SDL_WINDOWEVENT group, I suggest adding groups for the remaining events, like this:

• SDL_EVENT_SYSTEM — system specific events group,
• SDL_EVENT_WINDOW — window specific events group, with sub-events:
  • SDL_EVENT_WINDOW_CLOSE,
  • SDL_EVENT_WINDOW_ENTER,
  • SDL_EVENT_WINDOW_MOVED,
  • ...
• SDL_EVENT_JOYSTICK — joystick specific events group, with sub-events:
  • SDL_EVENT_JOYSTICK_HAT_MOTION,
  • SDL_EVENT_JOYSTICK_BUTTON_UP,
  • SDL_EVENT_JOYSTICK_BUTTON_DOWN,
  • SDL_EVENT_JOYSTICK_DEVICE_ADDED,
  • SDL_EVENT_JOYSTICK_DEVICE_REMOVED,
  • ...
• ...

Also pay attention to the nomenclature — it is different from the current one. Constants and function names should be set in such a way as to use common parts (as above SDL_EVENT_*, SDL_EVENT_WINDOW_* etc.), because it makes writing code much easier, especially when using IDE tools. And please don't combine words into one string, use _ to separate words in constants.

The topic of naming conventions should also be addressed, as SDL functions also have different styles. For example, we have the functions SDL_RenderClear, SDL_RenderGeometry and so on, where we have the prefix SDL_Render*, while, for example, in the case of window-related functions, there are the functions SDL_RestoreWindow, SDL_GetWindowTitle and so on, where the prefix SDL_Window* is not used. This produces confusion, there is no way to filter the functions in the code completion window by writing the identifier letter by letter.

[icculus]

For example, we have the functions SDL_RenderClear, SDL_RenderGeometry and so on, where we have the prefix SDL_Render*, while, for example, in the case of window-related functions, there are the functions SDL_RestoreWindow, SDL_GetWindowTitle and so on, where the prefix SDL_Window* is not used

There is a bug open about deciding what to do about this for SDL3, fwiw. I'm personally in favor of more consistency but I don't want a dogmatic requirement that everything get named SDL_SubsystemNounVerb. I don't know how that discussion will shake out yet.

[slime73]

I suggest adding groups for the remaining events

SDL events are already grouped by value, so you can have code which gathers all joystick events by checking whether the event enum's integer value is within a certain range. Maybe that should be a more formalized concept in SDL3? That said, personally it's never been any trouble at all to just have those few joystick events in a switch case fallthrough setup.

[icculus]

I can, with one condition or switch instruction, distinguish which subsystem a given event applies to and send it further.

Honestly I'm not a fan of this personally...but perhaps a separate field in the event can make everyone happy?

// the usual:
switch (event->type) {
    case SDL_EVENT_WINDOW_FOCUS_LOST: do_something(); break;
}

// with categories:
switch (event->category) {
    // do_something_with_window_events will check event->type.
    case SDL_EVENT_CATEGORY_WINDOW: do_something_with_window_events(event); break;
}

I don't know if this is a good idea, I'm just spitballing here. Maybe we just add an SDL_GetEventCategory() function that knows how to split the types up for you, idk.

[furious-programming]

SDL events are already grouped by value, so you can have code which gathers all joystick events by checking whether the event enum's integer value is within a certain range.

I don't like this approach, because it is unclear and it force you to do much longer switch statements.

The category system proposed by @icculus sounds like a decent compromise between grouping events and ranging event values. After removing the current grouping of (window) events and introducing categories, everyone should be happy, because it will be possible to freely process events — with the distinction of subsystems and without. So, I am definitely in favor of the proposed categories.

[furious-programming]

There is a bug open about deciding what to do about this for SDL3, fwiw.

I didn't find such a topic.

I'm personally in favor of more consistency but I don't want a dogmatic requirement that everything get named SDL_SubsystemNounVerb.

I am not talking about a dogmatic requirement, but about a convention that is clear, easy to understand and compatible with IDE tools (code completion, searching text etc.) — it's just a reasonable approach. Eskil Steenberg said something about this in the video entitled. "How I program C" — it is a really good naming that makes working with code much much easier.

The only difference I use from Eskil is a different approach to getters and setters. Eskil uses get and set at the end of the identifiers, while I use them after the subsystem name. This makes it easy to distinguish regular functions from getters and setters. For example, a regular function would be named SDL_WindowMaximize, a getter would be named SDL_WindowGetTitle, and a setter would be named SDL_WindowSetTitle. More examples from the Window API:

Regular functions:

• SDL_WindowRaise
• SDL_WindowMinimize
• SDL_WindowMaximize
• SDL_WindowCreate
• SDL_WindowCreateFrom
• SDL_WindowUpdateSurface
• SDL_WindowUpdateSurfaceRects ...

Getters (same pattern for setters):

• SDL_WindowGetSize
• SDL_WindowGetSizeMinimum
• SDL_WindowGetSizeMaximum
• SDL_WindowGetSizeBorders
• SDL_WindowGetGrab
• SDL_WindowGetGrabMouse
• SDL_WindowGetGrabKeyboard ...

See how the words in the identifiers stack up — the beginning fragments are consistent, which is easy to filter, while getters and setters are easy to distinguish from regular functions.

Three variants of getters should also be taken into account, i.e. the prefixes Get, Has and Is, because the latter two are also used in function names currently (and should stay that way) — e.g. SDL_WindowGetTitle, SDL_JoystickIsVirtual, SDL_GameControllerHasAxis.

[slime73]

I didn't find such a topic.

Here: #6569

[icculus]

I didn't find such a topic.

Sam copied your (very good!) comment into 6569, and feel free to carry on with that topic over there if you like, and we can continue on with event system change conversation here.

[icculus]

Ok, there's a first shot at this API change in the pull request.

Since the discussion was happening here, I'll mention that it doesn't have any sort of event category thing at this point, but that would be a separate thing if we decide to do it.

Feedback on the patch is welcome over in #6863. Thanks!

[slouken]

Ok, there's a first shot at this API change in the pull request.

Since the discussion was happening here, I'll mention that it doesn't have any sort of event category thing at this point, but that would be a separate thing if we decide to do it.

Feedback on the patch is welcome over in #6867. Thanks!

[icculus]

So it doesn't get lost in a closed bug, I moved the event category idea to #6869, for those that would like to follow along.