mity / mctrl

C library providing set of additional user interface controls for Windows, intended to be complementary to standard Win32API controls from USER32.DLL and COMCTL32.DLL.
http://mctrl.org
235 stars 52 forks source link

Rework/cleanup of `<ALT>` handling in `mcIsMenubarMessage()` #52

Closed mity closed 7 years ago

mity commented 7 years ago

This is a counter-proposal to patch #51.

vslavik commented 7 years ago

My testing of this so far on Windows 10:

  1. Alt+numpad typing works as expected.
  2. This partially re-breaks AltGr handling (see #50). It is possible to type with it, but it also produces the invalid shortcut beep when I press e.g. AltGr+E.
  3. Sometimes the beep for invalid shortcut (not as the side-effect of 2, the "normal" case) is delayed about 1.5s: hold Alt down, press a key, keep holding Alt. In Notepad, this beeps the moment I press (not even release yet) the non-shortcut key. In Poedit+mCtrl the beep is delayed the first time, but immediate subsequently… until I change the keyboard (sometimes changing focus is enough).
mity commented 7 years ago

Thanks for testing it.

  1. Alt+numpad typing works as expected.

Great.

  1. This partially re-breaks AltGr handling (see #50). It is possible to type with it, but it also produces the invalid shortcut beep when I press e.g. AltGr+E.

Ouch. I more or less never use ALTGR+ so I tend to forget to test that. The previous commit fixes that.

  1. Sometimes the beep for invalid shortcut (not as the side-effect of 2, the "normal" case) is delayed about 1.5s: hold Alt down, press a key, keep holding Alt. In Notepad, this beeps the moment I press (not even release yet) the non-shortcut key. In Poedit+mCtrl the beep is delayed the first time, but immediate subsequently… until I change the keyboard (sometimes changing focus is enough).

Unfortunately I cannot reproduce. I'll try later with the build of poedit you mentioned in #51

vslavik commented 7 years ago

The previous commit fixes that.

Thanks!

Unfortunately I cannot reproduce. I'll try later with the build of poedit you mentioned in #51

Here it is: https://jumpshare.com/v/m53rqrgGNeAIIliUd14B FWIW, I'm having trouble reliably reproducing it myself.

mity commented 7 years ago

Thank you.

Well, after some more testing, I reproduced the point 3, I think. The easiest way how to reproduce, at least on my machine, is as follows:

  1. Press ALT (and keep holding it).
  2. Repeatedly press a key not matching any hotkey in the menubar.
  3. Release ALT.

If the step 2 is done really fast and many times, there are some strange effects. Most often, the beeps sometimes interrupt each other and when that happens, then (when you stop pressing it) there is usually the delay before another final beep comes although you are not using the keyboard anymore.

But there is one funny thing: This way I reproduce the issue even for standard native menu (tested with notepad.exe). So I am not sure we are worse then the native in this particular case...

Anyway, I have actually realized some other reasons, why mcIsMenubarMessage() should not do the beeping at all, at least for now until I find some more answers how Windows does that.

  1. mcIsMenubarMessage() does not have complete information to decide when it should beep:

    while(GetMessage(&msg, NULL, 0, 0)) {
    // With ALT+something, this might beep...
    if(mcIsMenubarMessage(hwndMenubar, &msg))
        continue;
    
    // ... although potentially there might be an something else (e.g. accelerator table
    // or another menubar) happy to understand that sequence.
    if(TranslateAccelerator(hwnd, hAccel, &msg))
        continue;
    
    TranslateMessage(&msg);
    DispatchMessage(&msg);
    }
  2. Native menubar beeps also in other situations, e.g. when ALT alone is used to activate it, and then (after it is released) an invalid key is used. So, it would require more work and exploration what exactly the native beast does.

  3. Many apps with their own menubar implementations, even from Microsoft, do not beep.

mity commented 7 years ago

Closing. I pushed the patches (with some removed dead-ends to simplify the history) into master manually.

vslavik commented 7 years ago

If the step 2 is done really fast and many times

FWIW, that wasn't the case for — a single keystroke (well, not counting Alt) was all it took here, if I was able to reproduce, and I never could in native menubar.

Anyway, I have actually realized some other reasons, why mcIsMenubarMessage() should not do the beeping at all,

No argument here. This removal of beeping fixed everything for my use (beeping is done by wxWidgets then, apparently, so it's entirely possible that this was some interaction with wx).

Thanks again for the much better fix!