Handle mod key edge-cases in tiling WMs

[rendello]

This commit ignores keypresses when the mod key is held.

The reasoning is that an odd interaction happens between SDL applications and
tiling window managers. Tiling window managers like Xmonad and i3 usually use
the mod ("windows") key and a number to change workspaces. When changing
workspaces, however, the WMs still send the number key through instead of
"eating" it. It's not clear why, exactly, but it seems universal.

Mod+1 -> Goes to workspace #1
Mod+2 -> Goes to workspace #2
...
Mod+9 -> Goes to workspace #9

Most applications don't even see the number key being sent, so if you move to
workspace 1, Firefox won't type "1" into the browser bar, Vim won't type "1"
into your file, etc. But SDL applications, for whatever reason, DO see this
keydown. Of course, they'll handle it like a regular key press. So if you move
to workspace 1, which contains OpenRCT, it inadvertently toggles x-ray mode.

I first found this bug in another SDL game, The Powder Toy. After some
discussion with the devs, they fixed it like this, by ignoring keydown events
when the mod key is pressed, since the mod key is reserved for the window
manager anyway. It works well and should be in the next release.

The-Powder-Toy/The-Powder-Toy@c761938...93b920a

I did the same thing here.

[IntelOrca]

I think this should be excluded from macOS.

[rendello]

I think this should be excluded from macOS.

That's a good call, this [1] suggests it may overlap with control, although I can't test that out. It wouldn't be useful to leave in in any case. I suppose Windows could also probably be excluded, but I can't think of any real argument either way for that.

I wouldn't want to do an ifndef for only Linux, since the BSDs and what have you can also run custom WMs.

1. https://support.apple.com/en-ca/guide/mac-help/cpmh0152/mac

[IntelOrca]

It is probably safe to exclude for Windows too. OpenRCT2 doesn't receive events that invoke Window OS shortcuts, e.g. WIN+D will not be received by the game from what I can remember.

[rendello]

I don't have a Window / Mac box to test this on but this #if !(defined(__MACOSX__) || defined(__WINDOWS__)) should do the trick. I could get more granular with the #if but I don't think it would do the operating systems (or fellow programmers) any favors

[tupaschoal]

I don't have a Window / Mac box to test this on but this #if !(defined(__MACOSX__) || defined(__WINDOWS__)) should do the trick. I could get more granular with the #if but I don't think it would do the operating systems (or fellow programmers) any favors

You can implement the function into the platform files and do an empty implementation for those platforms, then you won't need the ifdef because they are already there :)

[tupaschoal]

Like I commented, I would move this to the Platform file and call this function shouldIgnoreModKey(). Then you can clean up the documentation from here and also the ifdefs.

This would then become:

Suggested change

	#if !(defined(__MACOSX__) || defined(__WINDOWS__))
	// Ignore winkey keydowns. Handles edge case where *NIX
	// tiling window managers don't eat the keypresses when
	// changing workspaces.
	if (SDL_GetModState() & KMOD_GUI)
	{
	break;
	}
	#endif
	if (shouldIgnoreModKey(HSDL_GetModState() & KMOD_GUI))
	{
	break;
	}

On the Mac and Windows platform files you would have:

bool shouldIgnoreModKey(bool hasMod)
{
    return hasMod;
}

And on the Linux platform file:

bool shouldIgnoreModKey(bool)
{
    return false;
}

This will prevent ifdefs from spreading into the code.

[rendello]

@tupaschoal

I believe @IntelOrca's concern was about behaviour. I can't test it myself, but some documentation seems to suggest that the command key is used as the KMOD_GUI key on macOS, so disabling it might disable command+c, command+v, etc.

I think I made a mistake in bringing up other OSs, since this change should not change the behavior in those OSs at all. KMOD_GUI is generally the Windows key, if it exists (left and right). I think the macOS check is warranted, but the other OS checks would only serve to confuse other programmers and people porting the game. ifdefs would be (infinitesimally) speedier but annoying, and runtime checks would only make the initial check run more slowly.

I'll make a commit that has the macOS check but removes the Windows check for this reason. If you still wish to move in another direction, let me know.
-- Gaven

[tupaschoal]

Thanks @rendello. I still prefer my suggested approach, because it keeps the OS-specific functions on a single place and does not sprinkle the code with ifdefs. Let me know if you need help implementing it, but it should be very straight forward.

[rendello]

@tupaschoal That's fair, I'll do another patch tomorrow. Thanks for going through this with me, open source can be a thankless job and it's awesome that people are still working with people like me submitting micro-patches!

[duncanspumpkin]

Looking at the surrounding code i can see why you have done it this way. Perhaps this whole function needs a little bit of a rethink to make it less ifdef heavy.

[IntelOrca]

@duncanspumpkin are you okay with this change. It isn't particularly invasive.

[duncanspumpkin]

Fine by me.