bug#69083: Emacs's keyboard hook state is not reset on session lock (Windows)

[Eli Zaretskii]

> From: Raffael Stocker <r.stocker@mnet-mail.de>
> Cc: 69083@debbugs.gnu.org
> Date: Mon, 26 Feb 2024 21:50:43 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Is there a better place where the remove_w32_kbdhook call could go such
> >> that cleanup can always happen?
> >
> > I think that place is term_ntproc.
> 
> I have added a call to ‘remove_w32_kbdhook’ there, but also left it with
> the WM_DESTROY message.  The cleanup now always happens and the
> setup/remove calls are correctly nested when using multiple frames.
> 
> >> >> Unfortunately, this didn't work for me.  I tried calling
> >> >> ‘EnumWindows(find_child_console, ...)’ with a ‘child_process’ instance
> >> >> containing the current process id as returned by ‘GetCurrentProcessId’,
> >> >> but I don't seem to get a useful window handle.
> >> >
> >> > What do you mean?  What is the result of using find_child_console?
> >> > does the condition in find_child_console, which looks at the
> >> > process_id of all windows, never match the process ID of the Emacs
> >> > session running with -nw?  Or what else goes wrong?
> 
> I figured this out now.  The ‘find_child_console’ function looks for a
> ‘ConsoleWindowClass’ window, but the Emacs process itself only has
> ‘Emacs Clipboard’ and ‘IME’ windows.  The latter seems to be the one I
> need.  I added a function ‘find_ime_window’ in ‘w32console.c’ that
> checks for this window when called through ‘EnumThreadWindows’.  I can
> now register for session notifications using this window.
> 
> To handle the notifications, I added some code to ‘drain_message_queue’,
> calling ‘reset_w32_kbdhook_state’ from there.  This now correctly resets
> the hook state in console Emacs.
> 
> I added ‘WINDOWSNT’ #ifdefs around calls to and the definition of
> ‘reset_w32_kbdhook_state’, as with the setup/remove functions.  I also
> cleaned up the ‘remove_w32_kbdhook’ function to use the previously
> obtained function pointer.
> 
> I believe the attached version should now tick all the important boxes.

Thanks, I have a few minor comments to the patch below.  After fixing
those nits, would you please submit the result in "git format-patch"
format, and include commit log message according to our conventions
(see CONTRIBUTE for the details and "git log" to see examples of how
we do that in the repository).  This will allow me to install the
changes quickly and easily.

> +/* from w32fns.c */
> +extern void
> +remove_w32_kbdhook (void);

Please make the prototype a single line, don't break it in two as we
do with function definitions.

> +/* The IME window is needed to receive the session notifications
> +   required to reset the low level keyboard hook state. */
                                                         ^^
Our style is to leave two SPACEs at the end of a comment.

> +      /* Register session notifications so we get notified about the
> +      computer being locked. */
> +      if (hwnd != NULL)
> +     {
> +       kbdhook.window = hwnd;
> +       HMODULE wtsapi32_lib = LoadLibrary ("wtsapi32.dll");
> +       WTSRegisterSessionNotification_Proc WTSRegisterSessionNotification_fn
> +         = (WTSRegisterSessionNotification_Proc)
> +         get_proc_addr (wtsapi32_lib, "WTSRegisterSessionNotification");
> +       WTSUnRegisterSessionNotification_fn = 
> (WTSUnRegisterSessionNotification_Proc)
> +         get_proc_addr (wtsapi32_lib, "WTSUnRegisterSessionNotification");
> +       if (WTSRegisterSessionNotification_fn != NULL)
> +         WTSRegisterSessionNotification_fn (hwnd, NOTIFY_FOR_THIS_SESSION);
> +     }

This code is run every time Emacs creates a new frame, doesn't it?  If
so, calling LoadLibrary and get_proc_addr each time is a waste of
cycles.  It is better to make both WTSRegisterSessionNotification_fn
and WTSUnRegisterSessionNotification_fn global, and introduce a
boolean flag to indicate that this was already done.  Then the above
code should be run only once per session, and all the other calls
should use the function pointers already set up (if non-NULL).

Note that the boolean flag and the function pointers need to be reset
at startup in globals_of_w32, because we still support the unexec
build of Emacs, where global and static variables set during the
temacs run as part of the build are recorded in the emacs.exe binary,
and will therefore record the values from the build time, which are
not necessarily true for the run time of production sessions (which
could well be on another system).

OTOH, there's something I don't understand here.  If this code is run
for every frame we create/delete, then what window exactly does the
kbdhook.window member record?  It sounds like we overwrite that member
with another window's handle on each call to setup_w32_kbdhook?  And
if so, what is the window handle we will pass to
WTSUnRegisterSessionNotification in remove_w32_kbdhook?  Or is the
hwnd argument to setup_w32_kbdhook somehow non-NULL only once, for the
main session window or something?  I feel that I'm missing something
here.

Thanks.