bug#73559: [PATCH] fix NS build focus-in event processing

bug#73559: [PATCH] fix NS build focus-in event processing

[Daniel Colascione]

In Emacs NS build, frames don't respond to focus-in events right away.
Instead, they store the focus-in event and process it (and other queued
events) whenever some other event happens to occur on that frame.

This patch kicks the NS event loop immediately when a focus-in event
happens, allowing Emacs to respond to focus-in events without some other
event triggering the processing.

commit c6d98bfc2a098b57fa9631978224ead2668d3a88
Author: Daniel Colascione <dancol@dancol.org>
Date:   Wed Aug 21 19:48:05 2024 -0700

    process events on focus in

diff --git a/src/nsterm.m b/src/nsterm.m
index 8c405738467..f68a22d9fbc 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -7973,6 +7973,7 @@ - (void)windowDidBecomeKey      /* for direct calls */
   event.kind = FOCUS_IN_EVENT;
   XSETFRAME (event.frame_or_window, emacsframe);
   kbd_buffer_store_event (&event);
+  ns_send_appdefined (-1);  // Kick main loop
 }
 
 

bug#73559: [PATCH] fix NS build focus-in event processing

[Stefan Kangas]

Daniel Colascione <dancol@dancol.org> writes:

> In Emacs NS build, frames don't respond to focus-in events right away.
> Instead, they store the focus-in event and process it (and other queued
> events) whenever some other event happens to occur on that frame.
>
> This patch kicks the NS event loop immediately when a focus-in event
> happens, allowing Emacs to respond to focus-in events without some other
> event triggering the processing.

Thanks for the patch.

Please always send patches as attachments if possible, as formatted by

    git format-patch -1

> commit c6d98bfc2a098b57fa9631978224ead2668d3a88
> Author: Daniel Colascione <dancol@dancol.org>
> Date:   Wed Aug 21 19:48:05 2024 -0700
>
>     process events on focus in
>
> diff --git a/src/nsterm.m b/src/nsterm.m
> index 8c405738467..f68a22d9fbc 100644
> --- a/src/nsterm.m
> +++ b/src/nsterm.m
> @@ -7973,6 +7973,7 @@ - (void)windowDidBecomeKey      /* for direct calls */
>    event.kind = FOCUS_IN_EVENT;
>    XSETFRAME (event.frame_or_window, emacsframe);
>    kbd_buffer_store_event (&event);
> +  ns_send_appdefined (-1);  // Kick main loop
>  }
>
>

bug#73559: [PATCH] fix NS build focus-in event processing

[Eli Zaretskii]

> From: Daniel Colascione <dancol@dancol.org>
> Date: Sun, 29 Sep 2024 22:47:46 -0400
> 
> 
> In Emacs NS build, frames don't respond to focus-in events right away.
> Instead, they store the focus-in event and process it (and other queued
> events) whenever some other event happens to occur on that frame.

Hmm... isn't this the same on all other GUI systems?
kbd_buffer_store_event adds the event to the Emacs input queue, and
AFAIU it will be processed as soon as Emacs gets back to its main loop
and calls read_char.  No other event should be needed, AFAIK.  Po Lu,
am I missing something here?

> This patch kicks the NS event loop immediately when a focus-in event
> happens, allowing Emacs to respond to focus-in events without some other
> event triggering the processing.

I know nothing about the "NS event loop", so I'm probably missing
something.

Thanks.

bug#73559: [PATCH] fix NS build focus-in event processing

[Daniel Colascione]

On September 30, 2024 4:41:49 AM PDT, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Daniel Colascione <dancol@dancol.org>
>> Date: Sun, 29 Sep 2024 22:47:46 -0400
>> 
>> 
>> In Emacs NS build, frames don't respond to focus-in events right away.
>> Instead, they store the focus-in event and process it (and other queued
>> events) whenever some other event happens to occur on that frame.
>
>Hmm... isn't this the same on all other GUI systems?
>kbd_buffer_store_event adds the event to the Emacs input queue, and
>AFAIU it will be processed as soon as Emacs gets back to its main loop
>and calls read_char.  No other event should be needed, AFAIK.  Po Lu,
>am I missing something here?
>
>> This patch kicks the NS event loop immediately when a focus-in event
>> happens, allowing Emacs to respond to focus-in events without some other
>> event triggering the processing.

I don't recall what we do for other systems, but for NS, we don't wake up the event loop as a side effect of kbd_buffer_store_event by itself. We rely on something else waking up the loop draining events from the queue. Changing the kbd_buffer_store_to_event to wake the main thread unconditionally would be another option, but seemed like a bigger change.



>
>I know nothing about the "NS event loop", so I'm probably missing
>something.
>



>Thanks.

bug#73559: [PATCH] fix NS build focus-in event processing

[Eli Zaretskii]

> Date: Mon, 30 Sep 2024 07:20:26 -0700
> From: Daniel Colascione <dancol@dancol.org>
> CC: 73559@debbugs.gnu.org
> 
> 
> 
> On September 30, 2024 4:41:49 AM PDT, Eli Zaretskii <eliz@gnu.org> wrote:
> >> From: Daniel Colascione <dancol@dancol.org>
> >> Date: Sun, 29 Sep 2024 22:47:46 -0400
> >> 
> >> 
> >> In Emacs NS build, frames don't respond to focus-in events right away.
> >> Instead, they store the focus-in event and process it (and other queued
> >> events) whenever some other event happens to occur on that frame.
> >
> >Hmm... isn't this the same on all other GUI systems?
> >kbd_buffer_store_event adds the event to the Emacs input queue, and
> >AFAIU it will be processed as soon as Emacs gets back to its main loop
> >and calls read_char.  No other event should be needed, AFAIK.  Po Lu,
> >am I missing something here?
> >
> >> This patch kicks the NS event loop immediately when a focus-in event
> >> happens, allowing Emacs to respond to focus-in events without some other
> >> event triggering the processing.
> 
> I don't recall what we do for other systems, but for NS, we don't wake up the event loop as a side effect of kbd_buffer_store_event by itself. We rely on something else waking up the loop draining events from the queue. Changing the kbd_buffer_store_to_event to wake the main thread unconditionally would be another option, but seemed like a bigger change.

That's not what I meant.  kbd_buffer_store_event doesn't trigger
reading from the queue, AFAIU.  Emacs does that itself, when it
becomes idle: it calls read_char as part of its main loop, and that
reads from the input queue.

bug#73559: [PATCH] fix NS build focus-in event processing

[Daniel Colascione]

On September 30, 2024 8:38:18 AM PDT, Eli Zaretskii <eliz@gnu.org> wrote:
>> Date: Mon, 30 Sep 2024 07:20:26 -0700
>> From: Daniel Colascione <dancol@dancol.org>
>> CC: 73559@debbugs.gnu.org
>> 
>> 
>> 
>> On September 30, 2024 4:41:49 AM PDT, Eli Zaretskii <eliz@gnu.org> wrote:
>> >> From: Daniel Colascione <dancol@dancol.org>
>> >> Date: Sun, 29 Sep 2024 22:47:46 -0400
>> >> 
>> >> 
>> >> In Emacs NS build, frames don't respond to focus-in events right away.
>> >> Instead, they store the focus-in event and process it (and other queued
>> >> events) whenever some other event happens to occur on that frame.
>> >
>> >Hmm... isn't this the same on all other GUI systems?
>> >kbd_buffer_store_event adds the event to the Emacs input queue, and
>> >AFAIU it will be processed as soon as Emacs gets back to its main loop
>> >and calls read_char.  No other event should be needed, AFAIK.  Po Lu,
>> >am I missing something here?
>> >
>> >> This patch kicks the NS event loop immediately when a focus-in event
>> >> happens, allowing Emacs to respond to focus-in events without some other
>> >> event triggering the processing.
>> 
>> I don't recall what we do for other systems, but for NS, we don't wake up the event loop as a side effect of kbd_buffer_store_event by itself. We rely on something else waking up the loop draining events from the queue. Changing the kbd_buffer_store_to_event to wake the main thread unconditionally would be another option, but seemed like a bigger change.
>
>That's not what I meant.  kbd_buffer_store_event doesn't trigger
>reading from the queue, AFAIU.  Emacs does that itself, when it
>becomes idle: it calls read_char as part of its main loop, and that
>reads from the input queue.

Yes. So now imagine that Emacs is idle and not the focused window. I command-tab to it. Now Emacs is the focused window. I would expect Emacs to have run the functions in focus-in-hook by now, but it didn't, because when we got focus, we queued the focus event but didn't wake up the main thread to process it. Now I hit a key. Emacs wakes up, drains its event loop (firing off focus-in-hook functions), and processes my keystroke. Isn't it correct for Emacs to run that hook immediately when it gets focus, not some time after?

In general, I don't see why we'd wire it up such that the event queue can be non-empty and the main thread asleep indefinitely. If we have an event to process, then in all circumstances, we should process it, yes?

bug#73559: [PATCH] fix NS build focus-in event processing

[Eli Zaretskii]

> Date: Mon, 30 Sep 2024 08:51:27 -0700
> From: Daniel Colascione <dancol@dancol.org>
> CC: luangruo@yahoo.com, 73559@debbugs.gnu.org
> 
> >That's not what I meant.  kbd_buffer_store_event doesn't trigger
> >reading from the queue, AFAIU.  Emacs does that itself, when it
> >becomes idle: it calls read_char as part of its main loop, and that
> >reads from the input queue.
> 
> Yes. So now imagine that Emacs is idle and not the focused window. I command-tab to it. Now Emacs is the focused window. I would expect Emacs to have run the functions in focus-in-hook by now, but it didn't, because when we got focus, we queued the focus event but didn't wake up the main thread to process it. Now I hit a key. Emacs wakes up, drains its event loop (firing off focus-in-hook functions), and processes my keystroke. Isn't it correct for Emacs to run that hook immediately when it gets focus, not some time after?

Yes, of course it is.

What happens on MS-Windows in this case is that when the Emacs frame
gets focus, we are sent the WM_SETFOCUS message.  That causes us to
add to the input queue (by using kbd_buffer_store_event) an input
event whose event->kind is FOCUS_IN_EVENT.  Immediately after that, I
see read_char call kbd_buffer_get_event (via several intermediate
calls), and the FOCUS_IN_EVENT is read and processed

I wonder why this doesn't happen for NS.  (But I know that the NS port
uses some tricky code to process events, so I'm hardly surprised.)

> In general, I don't see why we'd wire it up such that the event queue can be non-empty and the main thread asleep indefinitely. If we have an event to process, then in all circumstances, we should process it, yes?

Yes, definitely.  And AFAIU that should happen because we call
read_char whenever we are idle.  Maybe the NS version of pselect
sleeps indefinitely or for too long if no keyboard keys are pressed?
On other systems, any message from the window-system exits pselect,
but maybe that doesn't happen on NS?

Does having a frequently-expiring timer cause the focus-in events to
be processed more timely, without your patch?

bug#73559: [PATCH] fix NS build focus-in event processing

[Daniel Colascione]

On September 30, 2024 9:29:09 AM PDT, Eli Zaretskii <eliz@gnu.org> wrote:
>> Date: Mon, 30 Sep 2024 08:51:27 -0700
>> From: Daniel Colascione <dancol@dancol.org>
>> CC: luangruo@yahoo.com, 73559@debbugs.gnu.org
>> 
>> >That's not what I meant.  kbd_buffer_store_event doesn't trigger
>> >reading from the queue, AFAIU.  Emacs does that itself, when it
>> >becomes idle: it calls read_char as part of its main loop, and that
>> >reads from the input queue.
>> 
>> Yes. So now imagine that Emacs is idle and not the focused window. I command-tab to it. Now Emacs is the focused window. I would expect Emacs to have run the functions in focus-in-hook by now, but it didn't, because when we got focus, we queued the focus event but didn't wake up the main thread to process it. Now I hit a key. Emacs wakes up, drains its event loop (firing off focus-in-hook functions), and processes my keystroke. Isn't it correct for Emacs to run that hook immediately when it gets focus, not some time after?
>
>Yes, of course it is.
>
>What happens on MS-Windows in this case is that when the Emacs frame
>gets focus, we are sent the WM_SETFOCUS message.  That causes us to
>add to the input queue (by using kbd_buffer_store_event) an input
>event whose event->kind is FOCUS_IN_EVENT.  Immediately after that, I
>see read_char call kbd_buffer_get_event (via several intermediate
>calls), and the FOCUS_IN_EVENT is read and processed
>
>I wonder why this doesn't happen for NS.  (But I know that the NS port
>uses some tricky code to process events, so I'm hardly surprised.)
>
>> In general, I don't see why we'd wire it up such that the event queue can be non-empty and the main thread asleep indefinitely. If we have an event to process, then in all circumstances, we should process it, yes?
>
>Yes, definitely.  And AFAIU that should happen because we call
>read_char whenever we are idle.  Maybe the NS version of pselect
>sleeps indefinitely or for too long if no keyboard keys are pressed?
>On other systems, any message from the window-system exits pselect,
>but maybe that doesn't happen on NS?

It's not that the NS pselect waits too long. It's that it doesn't know to wake up. The focus event is delivered to Emacs by NS as a callback. Unless that callback, one way or another, takes some action to wake up the event loop, nothing gets processed. On Windows, we drain the event queue as a side effect of the message pump, whereas on NS there doesn't seem to be a separate pump that works this way -- just a callback.

>Does having a frequently-expiring timer cause the focus-in events to
>be processed more timely, without your patch?

I'd imagine so, but haven't been able to test yet.

bug#73559: [PATCH] fix NS build focus-in event processing

[Eli Zaretskii]

> Date: Mon, 30 Sep 2024 10:05:09 -0700
> From: Daniel Colascione <dancol@dancol.org>
> CC: luangruo@yahoo.com, 73559@debbugs.gnu.org
> 
> It's not that the NS pselect waits too long. It's that it doesn't know to wake up. The focus event is delivered to Emacs by NS as a callback. Unless that callback, one way or another, takes some action to wake up the event loop, nothing gets processed. On Windows, we drain the event queue as a side effect of the message pump, whereas on NS there doesn't seem to be a separate pump that works this way -- just a callback.

What about making pselect wait on one more descriptor, to which the
callback could then write?

bug#73559: [PATCH] fix NS build focus-in event processing

[Daniel Colascione]

On September 30, 2024 10:40:57 AM PDT, Eli Zaretskii <eliz@gnu.org> wrote:
>> Date: Mon, 30 Sep 2024 10:05:09 -0700
>> From: Daniel Colascione <dancol@dancol.org>
>> CC: luangruo@yahoo.com, 73559@debbugs.gnu.org
>> 
>> It's not that the NS pselect waits too long. It's that it doesn't know to wake up. The focus event is delivered to Emacs by NS as a callback. Unless that callback, one way or another, takes some action to wake up the event loop, nothing gets processed. On Windows, we drain the event queue as a side effect of the message pump, whereas on NS there doesn't seem to be a separate pump that works this way -- just a callback.
>
>What about making pselect wait on one more descriptor, to which the
>callback could then write?

That's the moral equivalent of what my patch does. My patch just uses the existing machinery for waking the main thread instead of adding a new FD.

bug#73559: [PATCH] fix NS build focus-in event processing

[Eli Zaretskii]

> Date: Mon, 30 Sep 2024 10:50:31 -0700
> From: Daniel Colascione <dancol@dancol.org>
> CC: luangruo@yahoo.com, 73559@debbugs.gnu.org
> 
> 
> 
> On September 30, 2024 10:40:57 AM PDT, Eli Zaretskii <eliz@gnu.org> wrote:
> >> Date: Mon, 30 Sep 2024 10:05:09 -0700
> >> From: Daniel Colascione <dancol@dancol.org>
> >> CC: luangruo@yahoo.com, 73559@debbugs.gnu.org
> >> 
> >> It's not that the NS pselect waits too long. It's that it doesn't know to wake up. The focus event is delivered to Emacs by NS as a callback. Unless that callback, one way or another, takes some action to wake up the event loop, nothing gets processed. On Windows, we drain the event queue as a side effect of the message pump, whereas on NS there doesn't seem to be a separate pump that works this way -- just a callback.
> >
> >What about making pselect wait on one more descriptor, to which the
> >callback could then write?
> 
> That's the moral equivalent of what my patch does. My patch just uses the existing machinery for waking the main thread instead of adding a new FD.

The patch was installed on the master branch, so I'm closing this bug.

Thanks.