Re: [PATCH] On the nasty "ghost key" problem on NS

[Stefan Monnier <monnier@iro.umontreal.ca>]

Po Lu [2022-11-04 20:17:29] wrote:
> Kai Ma <justksqsf@gmail.com> writes:
>> Either safe_call alone or safe_call+block_input does not fix the bug.
> Okay, I see.
> Would people on NS run this patch for a while and ack?  I don't see any
> adverse affects on GNUstep.

Just a comment from someone who's rather unfamiliar with the way input
methods work in Emacs (well, I use `TeX`, but that's about it), and even
more so for the NS code in general:

    diff --git a/src/nsterm.m b/src/nsterm.m
    index 17f40dc7e3..0788442b98 100644
    --- a/src/nsterm.m
    +++ b/src/nsterm.m
    @@ -7063,16 +7063,23 @@ - (NSRect)firstRectForCharacterRange: (NSRange)theRange
       NSRect rect;
       NSPoint pt;
       struct window *win;
    +  bool was_waiting_for_input;
    +  specpdl_ref count = SPECPDL_INDEX ();
     
       NSTRACE ("[EmacsView firstRectForCharacterRange:]");
     
       if (NS_KEYLOG)
         NSLog (@"firstRectForCharRange request");
     
    -  if (WINDOWP (echo_area_window) && ! NILP (call0 (intern ("ns-in-echo-area"))))
    +  was_waiting_for_input = waiting_for_input;
    +  waiting_for_input = false;
    +  specbind (Qinhibit_quit, Qt);
    +  if (WINDOWP (echo_area_window) && ! NILP (safe_call (true, 0, Qns_in_echo_area)))

I'm glad we found a way to make the code work, apparently, but
Here we need a comment explaining why we do this gymnastic of
`safe_call` + `inhibit_quit` + `waiting_for_input`.

It's very unusual to have to do that.

         win = XWINDOW (echo_area_window);
       else
         win = XWINDOW (FRAME_SELECTED_WINDOW (emacsframe));
    +  unbind_to (count, Qnil);
    +  waiting_for_input = was_waiting_for_input;

The `unbind_to` suggests that non-local exits are still possible, but if
so, we will sometimes not restore `waiting_for_input`.  So a comment
here would be welcome explaining either that non-local exits aren't
possible, or why it's OK not to restore `waiting_for_input`.
Or maybe change the code to use an unwind_protect to reliably restore
`waiting_for_input` so we don't have to worry about it?

    +  DEFSYM (Qns_in_echo_area, "ns-in-echo-area");

Thanks :-)

Now for the other part of my comment:

    (defun ns-in-echo-area ()
      "Whether, for purposes of inserting working composition text, the minibuffer
    is currently being used."
      (or isearch-mode
          (and cursor-in-echo-area (current-message))
          ;; Overlay strings are not shown in some cases.
          (get-char-property (point) 'invisible)
          (and (not (bobp))
    	   (or (and (get-char-property (point) 'display)
    		    (eq (get-char-property (1- (point)) 'display)
    			(get-char-property (point) 'display)))
    	       (and (get-char-property (point) 'composition)
    		    (eq (get-char-property (1- (point)) 'composition)
    			(get-char-property (point) 'composition)))))))

In which sense does (get-char-property (point) 'invisible), or the big
(and ...) below, indicate that "the minibuffer is currently being used"?

Could someone clarify this code?  Maybe all it takes is to change the
docstring, I don't know, but as it stands it looks quite odd.


        Stefan