Created attachment 4698 [details]
Patch implementing the basic refactoring

This seems reasonable.

Can you change the name of the other backend function to SetWindowMouseGrab() to be explicit about the separation of functionality?

I've committed your patch:
https://hg.libsdl.org/SDL/rev/02a2d609369b

Created attachment 4705 [details]
Rename SetWindowGrab to SetWindowMouseGrab

Yep, that makes sense. Here's a patch doing that rename.

I don't have the setup to build all the modified backends, but I just did it with sed so it should be fine (famous last words).

Reopening to land that final refactoring before implementing the new APIs.

This has been added:
https://hg.libsdl.org/SDL/rev/0258417e281f

It looks like the DirectFB backend needs a bit more work. It has a single grabbed window and does both keyboard and mouse grab at once.

I removed unimplemented grab functions, to simplify your work:
https://hg.libsdl.org/SDL/rev/2d7808ca0ca1

Created attachment 4706 [details]
Patch to split grab handling in DirectFB

Thanks for the cleanup, and nice catch. Looks like I didn't see that DirectFB supported keyboard grab because I was just grepping for SDL_HINT_GRAB_KEYBOARD.

Here is a compile-tested patch to split up that grab code and clean things up a bit.

Patch added:
https://hg.libsdl.org/SDL/rev/3b62773a46ad

Created attachment 4709 [details]
Add keyboard grab functions

Here is the patch implementing the keyboard grab API and tests.

I renamed SDL_WINDOW_INPUT_GRABBED to SDL_WINDOW_MOUSE_GRABBED and left a compatibility version of SDL_WINDOW_INPUT_GRABBED that's just equal to SDL_WINDOW_MOUSE_GRABBED for existing code. Hopefully we can gently nudge users toward using the SDL_WINDOW_MOUSE_GRABBED name which better reflects what SDL_SetWindowGrab() does now. If you prefer me not to make that change, I can spin a v2 without that change.

The existing grab function should grab both mouse and keyboard, since that was the previous behavior. We should have two new functions which separate the functionality, and the old one can just call both of those.

Other than DirectFB (which probably predates SDL_HINT_GRAB_KEYBOARD), the other backends just grabbed mouse only with SetWindowGrab() and SDL_HINT_GRAB_KEYBOARD unset (only X11 and DirectFB even supported keyboard grab at all in SDL 2.0.14). This is an area where the existing documentation which says "input grab" differs from the actual behavior of "mouse grab". I erred on the side of changing the documentation to match the actual behavior, rather than changing the behavior to match the documentation.

If we made SDL_SetWindowGrab() always grab both keyboard and mouse, that would be a behavior change for callers that weren't using SDL_HINT_GRAB_KEYBOARD before. My patch preserves the existing behavior where SDL_SetWindowGrab() only grabs mouse unless SDL_HINT_GRAB_KEYBOARD=1 is set.

Keyboard grab is quite invasive in terms of user experience, so I don't think it should be the default grab behavior without it being explicitly requested. Devs and users would be quite surprised if they updated SDL and couldn't Alt+Tab out of their game anymore.

I can introduce new functions like SDL_SetWindowMouseGrab() and friends too (I believe this was what you were alluding to). It would be simple to implement SDL_SetWindowGrab() by calling both SDL_SetWindowMouseGrab() and SDL_SetWindowKeyboard() grab.

I'm not sure how we'd implement SDL_GetWindowGrab() and SDL_GetGrabbedWindow() that way though. Do we return SDL_TRUE for SDL_GetWindowGrab() if a window has just keyboard or just mouse grab, or only with both grabs? There are similar edge cases with SDL_GetGrabbedWindow() too.

Thinking about it some more, I think part of the problem is that I've overcomplicated it by supporting mouse and keyboard grabs on different windows. This doesn't even make sense because you can't start a grab unless you have focus, and you can't have focus on two windows at once.

Once that's gone, there are no longer weird problems like what to return from SDL_GetGrabbedWindow() when 2 different windows are grabbed. In fact, I can drop the SDL_GetKeyboardGrabbedWindow() function entirely. That also makes it simple to support separate SDL_SetMouseGrabbedWindow() and SDL_GetMouseGrabbedWindow() functions, which I think is what you were getting at in your feedback.

I'll respin this patch and see if you like it better with these changes.

Created attachment 4710 [details]
Add keyboard and mouse grab functions v2

Okay, I like this version much better. Let me know if you still have concerns.

Yup, I like this much better too.
https://hg.libsdl.org/SDL/rev/72ee421785b5

Is there anything more that you want to do before we close this?

Actually, I'm thinking maybe we don't want to expose the keyboard grab functions in the API. As you (and others) have pointed out, generally this is not what users expect from most applications. Leaving it a hint makes sure that the default behavior is sane and most applications stay that way.

Do you have a use case where an API function is significantly better than a hint?

My desired use case is to implement remote desktop-like behavior where Alt+Tab and the Super key can be sent to the host machine while the window is in focus.

The big issue with SDL_HINT_GRAB_KEYBOARD for me is that it unnecessarily ties keyboard grab together with mouse grab. The goal of offering this separate API is that I can actually grab _less_ by only using keyboard grab and not mouse grab. Right now I'm forced to also grab the mouse when I want to grab the keyboard, therefore I'm unable to allow the user to click on other windows using their mouse while I have my input grab in effect.

In terms of avoiding (accidental) abuse, I don't see a huge difference between an API and a hint, since both are clearly documented for developers. If someone wanted to abuse this feature, they could already do that with SDL_HINT_GRAB_KEYBOARD today.

The OS itself helps avoid trouble too. Ctrl+Alt+Del on Windows can't be grabbed, so that will always work. On Wayland, compositors are allowed to pop a dialog to ask the user to approve the keyboard grab and provide a hotkey to break out of it. GNOME does this today.

In terms of what we can do to further minimize any possible misuse, here are some ideas:

Let's clearly document what grabbing the keyboard does in both the API and the hint documentation. Developers should know exactly what they're doing when they grab the keyboard, and not do it unnecessarily.

We can add a SDL_HINT_NEVER_GRAB_KEYBOARD hint which overrides both the API and SDL_HINT_GRAB_KEYBOARD in case developers do misuse it.

We can also provide default Alt+Tab handling when the keyboard is grabbed, similar to how we do with Alt+F4 on the Win32 backend. We could implement a minimize on Alt+Tab behavior, then provide a hint like SDL_HINT_NO_MINIMIZE_ON_ALT_TAB to opt out of that default if you really want to.

I'm happy to provide patches to implement some or all of those ideas if you find any of them worthwhile.

Okay, let's leave the API and the hint as-is for now.

I like the idea of leaving alt-tab enabled even when keyboard is grabbed, although I can see that wouldn't be desired for RDP type programs. Let's maybe implement that with a hint override, as you mentioned?

Sure, will do.