Re: Additional xterm-mouse cleanup

[Jared Finder]

On 2021-01-28 6:25 am, Stefan Monnier wrote:
> > -                    (while (or (memq char (append help-event-list
> > -						   (cons help-char '(?? ?\C-v ?\s ?\177 delete backspace 
> > vertical-scroll-bar ?\M-v))))
> > +                     (while (or (memq (event-basic-type char)
> > +                                      (append help-event-list
> > +                                              (list help-char ?? 
> > ?\C-v ?\s ?\177
> > +                                                    'delete 
> > 'backspace
> > +                                                    
> > 'vertical-scroll-bar
> > +                                                    'mouse-4 'mouse-5 
> > ?\M-v)))
>
> LGTM, but it would be good to improve the code so it determine this set
> of events automatically, e.g. by looking up local-map or something.

I just went ahead and did a cleanup of the logic around here.  Now 
there's a keymap for all the navigation commands.  Also a simple cache 
of all help characters.  Updated patch attached.

> > Subject: [PATCH 3/4] Support 'mouse-autoselect-window' for GPM and 
> > xterm mouse
>
> I think this should be consolidated with the *very* similar code in
> `src/msdos.c` and `src/w32inevt.c`.
>
> [ Ideally it should also be consolidated with the 
> `Vmouse_autoselect_window`
>   support in `src/xterm.c`, `src/nsterm.m`, and `src/w32term.c`.  ]

I completely agree.  However, I don't have access to any of those 
platforms to test against, so I do not feel comfortable making any 
changes.  Instead I added a fixme to all the different locations 
handling mouse movement.  Updated patch attached.

> > The function read_char normally clears the active echo area after
> > reading any event.  When xterm-mouse-mode is active the event can get
> > discarded when input-decode-map is applied (example: a mouse movement
> > escape sequence with 'track-mouse' set to nil) in which case the echo
> > area should stay unchanged.  Other translation keymaps are not
> > intended to ever discard events so the restoring is only done for
> > input-decode-map.
>
> Actually, I have had translations in `function-key-map` which end up
> discarding the input (and I can't think of any reason why
> `key-translation-map` wouldn't ever want to do that either, other than
> the fact that it's virtually never used).

Okay, this is easy enough to do.  I'll just move the restoration to the 
replay_sequence tag in read_key_sequence.

> >           if (done)
> >             {
> >               mock_input = diff + max (t, mock_input);
> > +
> > +             /* By this point the echo area was cleared by calls to
> > +                read_char.  However, we may have completely thrown
> > +                out the input (for example if decoding a mouse move
> > +                event but `track-mouse' is nil) in which case the
> > +                echo area should be restored to its pristene
> > +                state.  */
> > +             echo_area_buffer[0] = echo_area_buffer[1];
> >               goto replay_sequence;
> >             }
>
> [ Oh ... how I hate that echo area code.  ]
>
> I think this needs a reproducible test case, ideally one we can run in
> batch as part of our test suite.

I can do this, but I'm not sure what is the right way to test here.  
Best I can see is to use current-message to read the current message 
string, set unread-command-events to simulate key presses, and then call 
read-key-sequence with a timeotu (like how read-key works).  Does this 
sound right?

> Also, I'm curious about a few things:
> - How do we know that the echo area was cleared by calls to 
> `read_char`?
>   [ Do we even know for sure that `read_char` always clears it?  ]

read_char clears the echo area by calling "clear_message (1, 0)" if it 
encounters any event other than help-echo, switch-frame, or 
select-window in normal flow.  See the comment "Now wipe the echo area, 
except for" in read_char.

> - How do we know that this extra line has the effect of restoring it to
>   its pristine state?

Isn't that what echo_area_buffer[1] is for?  See echo_area_display.  I 
could be missing something though, let me know if I have this wrong.  It 
would be easy enough to add a local variable instead for the echo 
message when read_key_sequence is called.

> I think I'd like it better if we could make those things clear in
> the code.  E.g. by having the "code that clears" set some variable
> indicating that it's indeed been cleared and maybe also how it's been
> cleared, and which we can then use here.
>
> Or alternatively, change the "code that clears" so that it doesn't
> actually clear when called from `read_key_sequence` and let
> `read_key_sequence` take care of clearing the echo area once it has
> *really* read some input?

I don't think these are feasible as read_char's modifications of the 
echo area can be dependent on waiting for user input.  Specifically, it 
echos the dash (via echo_dash) while waiting for input after an idle 
timer activates.  Pulling out read_char's existing code around idle 
timers to control all this will be a beast to pull out.  Additionally, I 
don't see how read_key_sequence could know any better what to do as the 
translation maps are just translating individual character events that 
if typed slowly *should* be echoed.

  -- MJF

0001-Improve-behavior-for-make-help-screen.patch
Description: Text Data

0002-Support-mouse-autoselect-window-for-GPM-and-xterm-mo.patch
Description: Text Data