Fix Win32 System Command Input

Reporter: RobertBColton  |  Status: open  |  Last Modified: June 29, 2020, 12:25:31 AM

So after other conversations with Josh I noticed ALT+F4 wasn't actually closing the game in ENIGMA.

Apparently this is handled by DefWindowProc in WM_SYSKEYDOWN which then generates a WM_SYSCOMMAND for SC_CLOSE and sends a WM_CLOSE message to the Window.

ENIGMA System Command Menu

I don't know if this is a problem on other platforms, but I do know it's not an issue with SDL. Anyway the simple solution here is to just break and allow DefWindowProc to handle WM_SYSKEYDOWN for us, which we already do for WM_SYSCOMMAND after we intercept some messages tkg needed.

Microsoft says this is correct too.

As the name implies, system key strokes are primarily intended for use by the operating system. If you intercept the WM_SYSKEYDOWN message, call DefWindowProc afterward. Otherwise, you will block the operating system from handling the command.
https://docs.microsoft.com/en-us/windows/win32/learnwin32/keyboard-input

JoshDreamland  
I can't think of a good reason to call default proc for syskeydown but not up, or any of the many other events there.
RobertBColton  

Ok, well we do call it for the other WM_SYSCOMMAND. But if you are siding with Rusky & I. I can say the SDL developers seem to disagree with us.

https://github.com/emscripten-ports/SDL2/blob/865eaddffed50dbd13e6564c3f73902472cf74e8/src/video/windows/SDL_windowsevents.c#L674
https://github.com/emscripten-ports/SDL2/blob/865eaddffed50dbd13e6564c3f73902472cf74e8/src/video/windows/SDL_windowsevents.c#L1080

codecov[bot]  

Codecov Report

Merging #2056 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2056      +/-   ##
==========================================
+ Coverage   30.94%   30.95%   +0.01%     
==========================================
  Files         197      197              
  Lines       19100    19100              
==========================================
+ Hits         5910     5913       +3     
+ Misses      13190    13187       -3     
Impacted Files Coverage Δ
ENIGMAsystem/SHELL/Platforms/xlib/XLIBmain.cpp 35.00% <0.00%> (+3.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7eac75d...b1236e7. Read the comment docs.

RobertBColton  

Ok, it seems I misinterpreted what Rusky told me to do (930ee6c), he thinks we should let it fall through to the DefWindowProc since we were not handling the message. I also agree with you and him both (31b982d) that the SYSKEYUP should fall through to DefWindowProc because the documentation also states that it too could generate system commands.
https://docs.microsoft.com/en-us/windows/win32/inputdev/wm-syskeyup

The reason I do NOT believe we need to do so for "any of the many other events there" is because those are specifically designated as nonsystem keys. In other words, the system is not going to act upon them and there is no need to delegate those to the default window procedure/aka the system.
https://docs.microsoft.com/en-us/windows/win32/inputdev/wm-keyup

RobertBColton  

So, believe it or not, there's actually another side effect of this that I should document. Now, and in GM8.1, when you press and release the ALT key, its state is toggled, like for a menu.
draw_text(0,0,string(keyboard_check(vk_alt)));

Although this is the same now in Win32, our SDL does not behave this way, and the reason is in the sources I linked above. SDL handles ALT+F4 itself and doesn't delegate to the DefWindowProc like we do here. Now by extension of it not doing this in our SDL, it also doesn't seem to be the case with GMSv1.4 either. In GMSv1.4 and SDL, alt does not toggle and behaves like all other keys.

This menu loop could be avoided by blocking the menu loop system commands.
https://stackoverflow.com/a/11625411

RobertBColton  

You know what all that ALT code looked a little fishy to Josh too, so I devised a little test. For good reason too, the code has been crufting since the dawn of ENIGMA's first commit.

if (keyboard_check(vk_alt) && keyboard_check(ord('F')))
	show_message("ALT and F both down.");
if (keyboard_check_pressed(vk_alt) && keyboard_check_pressed(ord('G')))
	show_message("ALT and G both pressed.");
if (keyboard_check_released(vk_alt) && keyboard_check_released(ord('H')))
	show_message("ALT and H both released.");

It behaved identically before and after deleting the crufted code and identically on our SDL platform. It also behaved the same as GM8.1 and GMSv1.4 did. Therefore, I believe it is now safe to remove it, and combine the SYSKEY messages with the KEY messages as SDL did.

Actually, the definition of a nonsystem key as I said above, is a key that is pressed when ALT is not pressed. Therefore, just handling the system key messages alone suffices for handling the ALT key.
https://stackoverflow.com/a/42757904

I had also devised a test to double check that this code wasn't related to the distinction between left and right ALT keys, regardless of the fact I should have known that by vk_alt or 18 being hardcoded. However, I did find that's not the case because the GM8.1 manual states those constants are only for keyboard_check_direct, but when I tested GMSv1.4 it did in fact work. Anyway, neither GM8.1 or ENIGMA report anything for those through the higher level keyboard functions, but ENIGMA could be fixed now to behave as one would expect.
https://stackoverflow.com/questions/5681284/how-do-i-distinguish-between-left-and-right-keys-ctrl-and-alt/5681468#5681468

var text;
text = "";
text += string(keyboard_check(vk_alt)) + " ";
text += string(keyboard_check_pressed(vk_alt)) + " ";
text += string(keyboard_check_released(vk_alt)) + "#";
text += string(keyboard_check(vk_lalt)) + " ";
text += string(keyboard_check_pressed(vk_lalt)) + " ";
text += string(keyboard_check_released(vk_lalt)) + "#";
text += string(keyboard_check(vk_ralt)) + " ";
text += string(keyboard_check_pressed(vk_ralt)) + " ";
text += string(keyboard_check_released(vk_ralt));
draw_text(0,0,text);

RobertBColton  

This is absolutely hilarious, if we print the wParam apparently system key messages are only delivered for F10 and ALT, which I guess isn't too far off from what the documentation tries to communicate. The regular WM_KEYDOWN messages report shift and control with the other keys, but they also don't differentiate left from right.
Actual System Key Messages

RobertBColton  

Also, I designed another small test to see what keyboard_lastkey should be doing. In GM8.1 there is no difference between the left and right modifiers, but in GMSv1.4 there is. This same test also showed me that only the current key is effected by releases, while both are effected by presses.
draw_text(0,0,string(keyboard_key) + "#" + string(keyboard_lastkey));
Please sign in to post comments, or you can view this issue on GitHub.