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

[Po Lu]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> 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`.

That is done all over the place in the NS code.  I don't really know
why, you will have to ask its original authors for that, but suffice it
to say calling Lisp from firstRectForCharacterRange (and also the menu
bar update callbacks) will otherwise crash upon Fsignal being called.

> 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`.

I think the unbind_to is just a general pattern and could easily be
replaced with "Vinhibit_quit = old_inhibit_quit" (and so on and so
forth.)

> 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"?

I really have no idea...

> 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.

+1