Additional xterm-mouse cleanup

Additional xterm-mouse cleanup

[jared--- via Emacs development discussions.]

[-- Attachment #1.1: Type: text/plain, Size: 620 bytes --]

I've got four small patches for my continued work to make xterm-mouse
function better. #1 also makes Linux terminal function keys behave
better when running help-for-help and #2 and #3 both improve GPM-based
mouse behavior too. 

I'd like feedback in particular on patch #4, which is small but in a
sensitive location -- it's one additional code line in read_key_sequence
along the input-decode-map path. I considered instead putting the change
as part of the other cleanup right after replay_sequence, but ultimately
thought that would be riskier and serve no additional purpose. 

Let me know your thoughts. 

  -- MJF

[-- Attachment #1.2: Type: text/html, Size: 835 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Improve-behavior-for-make-help-screen.patch --]
[-- Type: text/x-diff; name=0001-Improve-behavior-for-make-help-screen.patch, Size: 3188 bytes --]

From 77a23c82f46b22ff3d7644bb31c51e0e319f0312 Mon Sep 17 00:00:00 2001
From: Jared Finder <jared@finder.org>
Date: Thu, 3 Dec 2020 22:42:05 -0800
Subject: [PATCH 1/4] Improve behavior for `make-help-screen'

* lisp/help-macro.el (make-help-screen): Don't read just the ESC in a
terminal escape sequence.  Allow mouse wheel scroll of the help window.
---
 lisp/help-macro.el | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/lisp/help-macro.el b/lisp/help-macro.el
index 791b10a878..15c17fdbdd 100644
--- a/lisp/help-macro.el
+++ b/lisp/help-macro.el
@@ -119,9 +119,16 @@ make-help-screen
 		 (let ((minor-mode-map-alist nil))
 		   (setcdr local-map ,helped-map)
 		   (define-key local-map [t] 'undefined)
-		   ;; Make the scroll bar keep working normally.
-		   (define-key local-map [vertical-scroll-bar]
-		     (lookup-key global-map [vertical-scroll-bar]))
+                   ;; Make scrolling commands keep working normally.
+                   (dolist (key '(;; Clicks in the scrollbar
+                                  [vertical-scroll-bar]
+                                  ;; Mouse wheel events
+                                  [mouse-4] [mouse-5] [down-mouse-4]
+                                  [down-mouse-5]))
+                     (define-key local-map key (lookup-key global-map key)))
+                   ;; Make terminal escape sequences be fully read as
+                   ;; well.
+                   (define-key local-map "\e" nil)
 		   (if three-step-help
 		       (progn
 			 (setq key (let ((overriding-local-map local-map))
@@ -148,8 +155,12 @@ make-help-screen
 		       (help-mode)
 		       (setq new-minor-mode-map-alist minor-mode-map-alist))
 		     (goto-char (point-min))
-		     (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)))
 				(eq (car-safe char) 'switch-frame)
 				(equal key "\M-v"))
 		       (condition-case nil
@@ -171,9 +182,11 @@ make-help-screen
 						"" ", or SPACE or DEL to scroll")))
 			       char (aref key 0)))
 
-		       ;; If this is a scroll bar command, just run it.
-		       (when (eq char 'vertical-scroll-bar)
-			 (command-execute (lookup-key local-map key) nil key))))
+                       ;; If this is a scroll bar or scroll wheel
+                       ;; command, just run it.
+                       (when (memq (event-basic-type char)
+                                   '(vertical-scroll-bar mouse-4 mouse-5))
+                         (command-execute (lookup-key local-map key) nil key))))
 		   ;; We don't need the prompt any more.
 		   (message "")
 		   ;; Mouse clicks are not part of the help feature,
-- 
2.20.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-lisp-tab-line.el-tab-line-new-tab-Use-tty-menus-when.patch --]
[-- Type: text/x-diff; name=0002-lisp-tab-line.el-tab-line-new-tab-Use-tty-menus-when.patch, Size: 1036 bytes --]

From d2ee448ffa70ddb02645f5397b727ae2b90e20ed Mon Sep 17 00:00:00 2001
From: Jared Finder <jared@finder.org>
Date: Mon, 7 Dec 2020 22:44:32 -0800
Subject: [PATCH 2/4] * lisp/tab-line.el (tab-line-new-tab): Use tty menus when
 supported.

---
 lisp/tab-line.el | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lisp/tab-line.el b/lisp/tab-line.el
index 2726947a4c..9209f2d46e 100644
--- a/lisp/tab-line.el
+++ b/lisp/tab-line.el
@@ -651,7 +651,9 @@ tab-line-new-tab
   (if (functionp tab-line-new-tab-choice)
       (funcall tab-line-new-tab-choice)
     (let ((tab-line-tabs-buffer-groups mouse-buffer-menu-mode-groups))
-      (if (and (listp mouse-event) window-system) ; (display-popup-menus-p)
+      (if (and (listp mouse-event)
+               (display-popup-menus-p)
+               (not tty-menu-open-use-tmm))
           (mouse-buffer-menu mouse-event) ; like (buffer-menu-open)
         ;; tty menu doesn't support mouse clicks, so use tmm
         (tmm-prompt (mouse-buffer-menu-keymap))))))
-- 
2.20.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Support-mouse-autoselect-window-for-GPM-and-xterm-mo.patch --]
[-- Type: text/x-diff; name=0003-Support-mouse-autoselect-window-for-GPM-and-xterm-mo.patch, Size: 2567 bytes --]

From 3f4fe8dc4c1961dfa185a47c5256bf9103c90ff8 Mon Sep 17 00:00:00 2001
From: Jared Finder <jared@finder.org>
Date: Sat, 23 Jan 2021 16:53:43 -0800
Subject: [PATCH 3/4] Support 'mouse-autoselect-window' for GPM and xterm mouse

* src/dispnew.c (update_mouse_position): Generate SELECT_WINDOW_EVENT.
---
 src/dispnew.c | 32 ++++++++++++++++++++++++++++++--
 src/w32term.c |  1 +
 2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/src/dispnew.c b/src/dispnew.c
index e603c67136..5dc4ac24db 100644
--- a/src/dispnew.c
+++ b/src/dispnew.c
@@ -3335,11 +3335,39 @@ update_frame_with_menu (struct frame *f, int row, int col)
 int
 update_mouse_position (struct frame *f, int x, int y)
 {
+  int event_count = 0;
+
   previous_help_echo_string = help_echo_string;
   help_echo_string = Qnil;
 
   note_mouse_highlight (f, x, y);
 
+  /* When the mouse moves over a new window, generate a
+     SELECT_WINDOW_EVENT.  */
+  if (!NILP (Vmouse_autoselect_window))
+    {
+      static Lisp_Object last_mouse_window;
+      Lisp_Object window = window_from_coordinates (f, x, y, 0, 0, 0);
+
+      /* Window will be selected only when it is not selected now and
+	 last mouse movement event was not in it.  Minibuffer window
+	 will be selected only when it is active.  */
+      if (WINDOWP (window)
+	  && !EQ (window, last_mouse_window)
+	  && !EQ (window, selected_window))
+	{
+	  struct input_event event;
+	  EVENT_INIT (event);
+	  event.kind = SELECT_WINDOW_EVENT;
+	  event.frame_or_window = window;
+	  kbd_buffer_store_event (&event);
+	  ++event_count;
+	}
+
+      /* Remember the last window where we saw the mouse.  */
+      last_mouse_window = window;
+    }
+
   /* If the contents of the global variable help_echo_string
      has changed, generate a HELP_EVENT.  */
   if (!NILP (help_echo_string)
@@ -3350,10 +3378,10 @@ update_mouse_position (struct frame *f, int x, int y)
 
       gen_help_event (help_echo_string, frame, help_echo_window,
                       help_echo_object, help_echo_pos);
-      return 1;
+      ++event_count;
     }
 
-  return 0;
+  return event_count;
 }
 
 DEFUN ("display--update-for-mouse-movement", Fdisplay__update_for_mouse_movement,
diff --git a/src/w32term.c b/src/w32term.c
index 109aa58d73..041f2ec31f 100644
--- a/src/w32term.c
+++ b/src/w32term.c
@@ -5068,6 +5068,7 @@ w32_read_socket (struct terminal *terminal,
 	  break;
 
 	case WM_MOUSEMOVE:
+	  /* FIXME: Combine this logic with update_mouse_position.  */
 	  /* Ignore non-movement.  */
 	  {
 	    int x = LOWORD (msg.msg.lParam);
-- 
2.20.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0004-Echo-area-stays-visible-when-moving-xterm-mouse.patch --]
[-- Type: text/x-diff; name=0004-Echo-area-stays-visible-when-moving-xterm-mouse.patch, Size: 1517 bytes --]

From caad79ddc94f4646eb68d7a7bbbbad83cfc46edc Mon Sep 17 00:00:00 2001
From: Jared Finder <jared@finder.org>
Date: Sat, 23 Jan 2021 19:31:02 -0800
Subject: [PATCH 4/4] Echo area stays visible when moving xterm mouse

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.

* src/keyboard.c (read_key_sequence): Restore the last displayed echo
area whenever a transformation is applied by input-decode-map.
---
 src/keyboard.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/keyboard.c b/src/keyboard.c
index 9ee4c4f6d6..f6bdadbed6 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -9955,6 +9955,14 @@ read_key_sequence (Lisp_Object *keybuf, Lisp_Object prompt,
 	  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;
 	    }
 	}
-- 
2.20.1

Re: Additional xterm-mouse cleanup

[Stefan Monnier]

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

> Subject: [PATCH 3/4] Support 'mouse-autoselect-window' for GPM and xterm mouse

Oh, thank you, thank you, thank you.

> --- a/src/dispnew.c
> +++ b/src/dispnew.c
> @@ -3335,11 +3335,39 @@ update_frame_with_menu (struct frame *f, int row, int col)
[...]
> +  /* When the mouse moves over a new window, generate a
> +     SELECT_WINDOW_EVENT.  */
> +  if (!NILP (Vmouse_autoselect_window))
> +    {
> +      static Lisp_Object last_mouse_window;
> +      Lisp_Object window = window_from_coordinates (f, x, y, 0, 0, 0);
> +
> +      /* Window will be selected only when it is not selected now and
> +	 last mouse movement event was not in it.  Minibuffer window
> +	 will be selected only when it is active.  */
> +      if (WINDOWP (window)
> +	  && !EQ (window, last_mouse_window)
> +	  && !EQ (window, selected_window))
> +	{
> +	  struct input_event event;
> +	  EVENT_INIT (event);
> +	  event.kind = SELECT_WINDOW_EVENT;
> +	  event.frame_or_window = window;
> +	  kbd_buffer_store_event (&event);
> +	  ++event_count;
> +	}
> +
> +      /* Remember the last window where we saw the mouse.  */
> +      last_mouse_window = window;
> +    }

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

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

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

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?  ]
- How do we know that this extra line has the effect of restoring it to
  its pristine state?

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?

WDYT?


        Stefan

Re: Additional xterm-mouse cleanup

[jared--- via Emacs development discussions.]

[-- Attachment #1: Type: text/plain, Size: 5188 bytes --]

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Improve-behavior-for-make-help-screen.patch --]
[-- Type: text/x-diff; name=0001-Improve-behavior-for-make-help-screen.patch, Size: 5445 bytes --]

From 12e442317862f32c35a990290e0fddf1db05d4ff Mon Sep 17 00:00:00 2001
From: Jared Finder <jared@finder.org>
Date: Thu, 3 Dec 2020 22:42:05 -0800
Subject: [PATCH 1/3] Improve behavior for `make-help-screen'

* lisp/help-macro.el (make-help-screen): Don't read just the ESC in a
terminal escape sequence.  Add keymap to control scrolling logic for
help screen.  Then add mouse wheel to that keymap.
---
 lisp/help-macro.el | 75 ++++++++++++++++++++++++----------------------
 1 file changed, 39 insertions(+), 36 deletions(-)

diff --git a/lisp/help-macro.el b/lisp/help-macro.el
index 791b10a878..5197ae496f 100644
--- a/lisp/help-macro.el
+++ b/lisp/help-macro.el
@@ -103,10 +103,14 @@ make-help-screen
 	   (when three-step-help
 	     (message "%s" line-prompt))
 	   (let* ((help-screen (documentation (quote ,doc-fn)))
+                  (help-chars (append (list ?? help-char) help-event-list))
+                  ;; Commands in this map are executed with errors
+                  ;; ignored.
+                  (navigation-map (make-sparse-keymap))
 		  ;; We bind overriding-local-map for very small
 		  ;; sections, *excluding* where we switch buffers
 		  ;; and where we execute the chosen help command.
-		  (local-map (make-sparse-keymap))
+		  (local-map (make-composed-keymap navigation-map ,helped-map))
 		  (new-minor-mode-map-alist minor-mode-map-alist)
 		  (prev-frame (selected-frame))
 		  config new-frame key char)
@@ -117,22 +121,32 @@ make-help-screen
 				    t t help-screen)))
 	     (unwind-protect
 		 (let ((minor-mode-map-alist nil))
-		   (setcdr local-map ,helped-map)
 		   (define-key local-map [t] 'undefined)
-		   ;; Make the scroll bar keep working normally.
-		   (define-key local-map [vertical-scroll-bar]
-		     (lookup-key global-map [vertical-scroll-bar]))
-		   (if three-step-help
-		       (progn
-			 (setq key (let ((overriding-local-map local-map))
-				     (read-key-sequence nil)))
-			 ;; Make the HELP key translate to C-h.
-			 (if (lookup-key function-key-map key)
-			     (setq key (lookup-key function-key-map key)))
-			 (setq char (aref key 0)))
-		     (setq char ??))
-		   (when (or (eq char ??) (eq char help-char)
-			     (memq char help-event-list))
+                   ;; Make terminal escape sequences be fully read.
+                   (define-key local-map "\e" nil)
+
+                   ;; Custom navigation commands.
+                   (dolist (key '("\C-v" "\s"))
+                     (define-key navigation-map key 'scroll-up))
+                   (dolist (key '("\M-v" "\d" [delete] [backspace]))
+                     (define-key navigation-map key 'scroll-down))
+                   ;; Navigation commands that keep working normally.
+                   (dolist (key '(;; Clicks in the scrollbar
+                                  [vertical-scroll-bar]
+                                  ;; Mouse wheel events
+                                  [mouse-4] [mouse-5] [down-mouse-4]
+                                  [down-mouse-5]
+                                  ;; Frame switching
+                                  [switch-frame]))
+                     (define-key navigation-map key
+                       (lookup-key global-map key)))
+
+                   (setq key (if three-step-help
+                                 (let ((overriding-local-map local-map))
+                                   (read-key-sequence-vector nil))
+                               [??])
+                         char (aref key 0))
+		   (when (memq char help-chars)
 		     (setq config (current-window-configuration))
 		     (pop-to-buffer " *Metahelp*" nil t)
 		     (and (fboundp 'make-frame)
@@ -148,32 +162,21 @@ make-help-screen
 		       (help-mode)
 		       (setq new-minor-mode-map-alist minor-mode-map-alist))
 		     (goto-char (point-min))
-		     (while (or (memq char (append help-event-list
-						   (cons help-char '(?? ?\C-v ?\s ?\177 delete backspace vertical-scroll-bar ?\M-v))))
-				(eq (car-safe char) 'switch-frame)
-				(equal key "\M-v"))
-		       (condition-case nil
-			   (cond
-			    ((eq (car-safe char) 'switch-frame)
-			     (handle-switch-frame char))
-			    ((memq char '(?\C-v ?\s))
-			     (scroll-up))
-			    ((or (memq char '(?\177 ?\M-v delete backspace))
-				 (equal key "\M-v"))
-			     (scroll-down)))
-			 (error nil))
+                     (while (or (memq (event-basic-type char) help-chars)
+                                (lookup-key navigation-map key))
+                       (let ((binding (lookup-key navigation-map key)))
+                         (when binding
+                           (condition-case nil
+                               (command-execute binding nil key)
+                             (error nil))))
 		       (let ((cursor-in-echo-area t)
 			     (overriding-local-map local-map))
-			 (setq key (read-key-sequence
+			 (setq key (read-key-sequence-vector
 				    (format "Type one of the options listed%s: "
 					    (if (pos-visible-in-window-p
 						 (point-max))
 						"" ", or SPACE or DEL to scroll")))
-			       char (aref key 0)))
-
-		       ;; If this is a scroll bar command, just run it.
-		       (when (eq char 'vertical-scroll-bar)
-			 (command-execute (lookup-key local-map key) nil key))))
+			       char (aref key 0)))))
 		   ;; We don't need the prompt any more.
 		   (message "")
 		   ;; Mouse clicks are not part of the help feature,
-- 
2.20.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Support-mouse-autoselect-window-for-GPM-and-xterm-mo.patch --]
[-- Type: text/x-diff; name=0002-Support-mouse-autoselect-window-for-GPM-and-xterm-mo.patch, Size: 4307 bytes --]

From 0219f8e1af24844189b6f3ac3af7f11379fd3a94 Mon Sep 17 00:00:00 2001
From: Jared Finder <jared@finder.org>
Date: Sat, 23 Jan 2021 16:53:43 -0800
Subject: [PATCH 2/3] Support 'mouse-autoselect-window' for GPM and xterm mouse

* src/dispnew.c (update_mouse_position): Generate SELECT_WINDOW_EVENT.
---
 src/dispnew.c  | 32 ++++++++++++++++++++++++++++++--
 src/msdos.c    |  1 +
 src/nsterm.m   |  1 +
 src/w32inevt.c |  1 +
 src/w32term.c  |  1 +
 src/xterm.c    |  1 +
 6 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/src/dispnew.c b/src/dispnew.c
index e603c67136..5dc4ac24db 100644
--- a/src/dispnew.c
+++ b/src/dispnew.c
@@ -3335,11 +3335,39 @@ update_frame_with_menu (struct frame *f, int row, int col)
 int
 update_mouse_position (struct frame *f, int x, int y)
 {
+  int event_count = 0;
+
   previous_help_echo_string = help_echo_string;
   help_echo_string = Qnil;
 
   note_mouse_highlight (f, x, y);
 
+  /* When the mouse moves over a new window, generate a
+     SELECT_WINDOW_EVENT.  */
+  if (!NILP (Vmouse_autoselect_window))
+    {
+      static Lisp_Object last_mouse_window;
+      Lisp_Object window = window_from_coordinates (f, x, y, 0, 0, 0);
+
+      /* Window will be selected only when it is not selected now and
+	 last mouse movement event was not in it.  Minibuffer window
+	 will be selected only when it is active.  */
+      if (WINDOWP (window)
+	  && !EQ (window, last_mouse_window)
+	  && !EQ (window, selected_window))
+	{
+	  struct input_event event;
+	  EVENT_INIT (event);
+	  event.kind = SELECT_WINDOW_EVENT;
+	  event.frame_or_window = window;
+	  kbd_buffer_store_event (&event);
+	  ++event_count;
+	}
+
+      /* Remember the last window where we saw the mouse.  */
+      last_mouse_window = window;
+    }
+
   /* If the contents of the global variable help_echo_string
      has changed, generate a HELP_EVENT.  */
   if (!NILP (help_echo_string)
@@ -3350,10 +3378,10 @@ update_mouse_position (struct frame *f, int x, int y)
 
       gen_help_event (help_echo_string, frame, help_echo_window,
                       help_echo_object, help_echo_pos);
-      return 1;
+      ++event_count;
     }
 
-  return 0;
+  return event_count;
 }
 
 DEFUN ("display--update-for-mouse-movement", Fdisplay__update_for_mouse_movement,
diff --git a/src/msdos.c b/src/msdos.c
index 5da01c9e7c..0dab89d9df 100644
--- a/src/msdos.c
+++ b/src/msdos.c
@@ -2639,6 +2639,7 @@ dos_rawgetc (void)
       /* Check for mouse movement *before* buttons.  */
       mouse_check_moved ();
 
+      /* FIXME: Combine this logic with update_mouse_position.  */
       /* If the mouse moved from the spot of its last sighting, we
          might need to update mouse highlight.  */
       if (mouse_last_x != mouse_prev_x || mouse_last_y != mouse_prev_y)
diff --git a/src/nsterm.m b/src/nsterm.m
index 1b2328628e..60acda1bcd 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -7037,6 +7037,7 @@ - (void) scrollWheel: (NSEvent *)theEvent
 }
 
 
+/* FIXME: Combine this logic with update_mouse_position.  */
 /* Tell emacs the mouse has moved.  */
 - (void)mouseMoved: (NSEvent *)e
 {
diff --git a/src/w32inevt.c b/src/w32inevt.c
index 1a80a00197..383169c3d1 100644
--- a/src/w32inevt.c
+++ b/src/w32inevt.c
@@ -473,6 +473,7 @@ do_mouse_event (MOUSE_EVENT_RECORD *event,
   switch (flags)
     {
     case MOUSE_MOVED:
+      /* FIXME: Combine this logic with update_mouse_position.  */
       {
 	struct frame *f = get_frame ();
 	Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (f);
diff --git a/src/w32term.c b/src/w32term.c
index 0ee805a852..7e9b641414 100644
--- a/src/w32term.c
+++ b/src/w32term.c
@@ -5083,6 +5083,7 @@ w32_read_socket (struct terminal *terminal,
 	  break;
 
 	case WM_MOUSEMOVE:
+	  /* FIXME: Combine this logic with update_mouse_position.  */
 	  /* Ignore non-movement.  */
 	  {
 	    int x = LOWORD (msg.msg.lParam);
diff --git a/src/xterm.c b/src/xterm.c
index 744b80c68a..d9fb389f89 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -8830,6 +8830,7 @@ handle_one_xevent (struct x_display_info *dpyinfo,
       goto OTHER;
 
     case MotionNotify:
+      /* FIXME: Combine this logic with update_mouse_position.  */
       {
         x_display_set_last_user_time (dpyinfo, event->xmotion.time);
         previous_help_echo_string = help_echo_string;
-- 
2.20.1

Re: Additional xterm-mouse cleanup

[Stefan Monnier]

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

LGTM.

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

I thought the parts in `src/msdos.c` and `src/w32inevt.c` were
sufficiently similar that the change can be done without much risk of
introducing a regression.

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

It does sound right, but I wouldn't be surprised if you encounter some
difficulty because the code behaves differently in batch mode or because
that message somehow gets erased just before your Elisp code gets to run.

We don't yet have much experience testing these "interactive" aspects of
Emacs, so it still frequently requires further changes to the code to
make such testing possible.  That's another reason it'd be good to have
a test case here (it's a good opportunity to improve our testing
facilities).

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

I have the impression that a whole lot of code can run between the
`clear_message` and your code, so I don't immediately see why we can be
sure that `echo_area_buffer[1]` indeed always contains the thing before
the `clear_message`.  And if it doesn't, then maybe we shouldn't try to
revert the echo message.

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

I agree it doesn't seem easy, but in my experience "do and later undo"
sooner or later leads to extra difficulties so I tend to prefer "delay
the do until we're sure we want to perform it".


        Stefan

Re: Additional xterm-mouse cleanup

[jared--- via Emacs development discussions.]

[-- Attachment #1: Type: text/plain, Size: 3213 bytes --]

On 2021-02-02 6:37 am, Stefan Monnier wrote:
>>>> 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.
> 
> I thought the parts in `src/msdos.c` and `src/w32inevt.c` were
> sufficiently similar that the change can be done without much risk of
> introducing a regression.

Ah, got it. I agree, it is mostly straightforward. To do this properly 
required making an assumption that .timestamp=0 for SELECT_WINDOW_EVENT 
is ok. Looking through the C code, I don't see any location that reads 
.timestamp for the SELECT_WINDOW_EVENT, so I make it uniformly 0 
throughout.  Updated patch attached.

>>> [ Oh ... how I hate that echo area code.  ]
> 
> I have the impression that a whole lot of code can run between the
> `clear_message` and your code, so I don't immediately see why we can be
> sure that `echo_area_buffer[1]` indeed always contains the thing before
> the `clear_message`.  And if it doesn't, then maybe we shouldn't try to
> revert the echo message.

Good point. I will update my patch to have a copy of the echo area made 
inside read_key_sequence.

> I agree it doesn't seem easy, but in my experience "do and later undo"
> sooner or later leads to extra difficulties so I tend to prefer "delay
> the do until we're sure we want to perform it".

I completely agree with the sentiment, but I do not think it is the 
right tradeoff. To delay until we're sure, we'd need to have some sort 
of assumption of how terminal escape sequences are received that normal 
humans would never do. Consider that the following key sequence is a 
mouse movement escape sequence but is completely possible for a human to 
type slowly:

ESC [ < 3 5 ; 1 9 ; 3 4 m

What should the echo area display if it has read "ESC ["? At this point, 
input-decode-map still doesn't know if this is a xterm escape sequence 
or not. This could just be part way through the above sequence, in which 
case nothing should be displayed. Alternatively, a user could have "ESC 
[ a" bound to an arbitrary command and they're part of the way through 
triggering that command, in which sase "ESC [-" should be displayed. Any 
change here is going to be *BIG*, which feels like a high risk change 
for a not-yet-demonstrated bug.

Or to use a metaphor: this feels like you're asking for heart bypass 
surgery before putting a bandage over a cut on Mr. Echo Area's elbow. I 
don't disagree that Mr. Echo Area would be able to be much more active 
after the surgery (and all that smoking raised his blood pressure a 
lot), but right now all he really wants is the bandage.

I do completely agree with adding the unit tests to ensure I don't make 
things worse.

   -- MJF

[-- Attachment #2: 0002-Support-mouse-autoselect-window-for-GPM-and-xterm-mo.patch --]
[-- Type: text/x-diff, Size: 10011 bytes --]

From 29093ad3ea2e29050db7000847f91c92b0d4a1b5 Mon Sep 17 00:00:00 2001
From: Jared Finder <jared@finder.org>
Date: Wed, 3 Feb 2021 20:35:33 -0800
Subject: [PATCH 2/3] Support 'mouse-autoselect-window' for GPM and xterm mouse

* src/dispnew.c (update_mouse_position): Generate SELECT_WINDOW_EVENT.
Timestamp will be 0.
* src/msdos.c (dos_rawgetc):
* src/w32inevt.c (do_mouse_event): Refactor to call
update_mouse_position.
* src/nsterm.m (EV_TRAILER):([EmacsView mouseMoved:]): Make
SELECT_WINDOW_EVENT have .timestamp set to 0.
* src/termhooks.h (enum event_kind): Document that SELECT_WINDOW_EVENT
always has .timestamp set to 0.
* src/w32term.c (w32_read_socket):
* src/xterm.c (handle_one_xevent): Add FIXMEs to refactor in the
future.
---
 src/dispnew.c   | 45 +++++++++++++++++++++++++++++++++++++++++----
 src/msdos.c     | 41 +----------------------------------------
 src/nsterm.m    |  9 +++++----
 src/termhooks.h |  3 ++-
 src/w32inevt.c  | 45 ++-------------------------------------------
 src/w32term.c   |  1 +
 src/xterm.c     |  1 +
 7 files changed, 53 insertions(+), 92 deletions(-)

diff --git a/src/dispnew.c b/src/dispnew.c
index e603c67136..e7cb926d0e 100644
--- a/src/dispnew.c
+++ b/src/dispnew.c
@@ -3335,9 +3335,46 @@ update_frame_with_menu (struct frame *f, int row, int col)
 int
 update_mouse_position (struct frame *f, int x, int y)
 {
-  previous_help_echo_string = help_echo_string;
-  help_echo_string = Qnil;
+  Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (f);
+  int event_count = 0;
+
+  if (hlinfo->mouse_face_hidden)
+    {
+      hlinfo->mouse_face_hidden = 0;
+      clear_mouse_face (hlinfo);
+    }
+
+  /* When the mouse moves over a new window, generate a
+     SELECT_WINDOW_EVENT.  */
+  if (!NILP (Vmouse_autoselect_window))
+    {
+      static Lisp_Object last_mouse_window;
+      Lisp_Object window = window_from_coordinates (f, x, y, 0, 0, 0);
 
+      /* Window will be selected only when it is not selected now and
+	 last mouse movement event was not in it.  Minibuffer window
+	 will be selected only when it is active.  */
+      if (WINDOWP (window)
+	  && !EQ (window, last_mouse_window)
+	  && !EQ (window, selected_window))
+	{
+	  struct input_event event;
+	  EVENT_INIT (event);
+	  event.kind = SELECT_WINDOW_EVENT;
+	  event.frame_or_window = window;
+	  event.arg = Qnil;
+	  event.timestamp = 0;
+	  kbd_buffer_store_event (&event);
+	  ++event_count;
+	}
+
+      /* Remember the last window where we saw the mouse.  */
+      last_mouse_window = window;
+    }
+
+  previous_help_echo_string = help_echo_string;
+  help_echo_string = help_echo_object = help_echo_window = Qnil;
+  help_echo_pos = -1;
   note_mouse_highlight (f, x, y);
 
   /* If the contents of the global variable help_echo_string
@@ -3350,10 +3387,10 @@ update_mouse_position (struct frame *f, int x, int y)
 
       gen_help_event (help_echo_string, frame, help_echo_window,
                       help_echo_object, help_echo_pos);
-      return 1;
+      ++event_count;
     }
 
-  return 0;
+  return event_count;
 }
 
 DEFUN ("display--update-for-mouse-movement", Fdisplay__update_for_mouse_movement,
diff --git a/src/msdos.c b/src/msdos.c
index 5da01c9e7c..40b498941b 100644
--- a/src/msdos.c
+++ b/src/msdos.c
@@ -2643,46 +2643,7 @@ dos_rawgetc (void)
          might need to update mouse highlight.  */
       if (mouse_last_x != mouse_prev_x || mouse_last_y != mouse_prev_y)
 	{
-	  if (hlinfo->mouse_face_hidden)
-	    {
-	      hlinfo->mouse_face_hidden = 0;
-	      clear_mouse_face (hlinfo);
-	    }
-
-	  /* Generate SELECT_WINDOW_EVENTs when needed.  */
-	  if (!NILP (Vmouse_autoselect_window))
-	    {
-	      static Lisp_Object last_mouse_window;
-
-	      mouse_window = window_from_coordinates
-		(SELECTED_FRAME (), mouse_last_x, mouse_last_y, 0, 0, 0);
-	      /* A window will be selected only when it is not
-		 selected now, and the last mouse movement event was
-		 not in it.  A minibuffer window will be selected iff
-		 it is active.  */
-	      if (WINDOWP (mouse_window)
-		  && !EQ (mouse_window, last_mouse_window)
-		  && !EQ (mouse_window, selected_window))
-		{
-		  event.kind = SELECT_WINDOW_EVENT;
-		  event.frame_or_window = mouse_window;
-		  event.arg = Qnil;
-		  event.timestamp = event_timestamp ();
-		  kbd_buffer_store_event (&event);
-		}
-	      /* Remember the last window where we saw the mouse.  */
-	      last_mouse_window = mouse_window;
-	    }
-
-	  previous_help_echo_string = help_echo_string;
-	  help_echo_string = help_echo_object = help_echo_window = Qnil;
-	  help_echo_pos = -1;
-	  note_mouse_highlight (SELECTED_FRAME (), mouse_last_x, mouse_last_y);
-	  /* If the contents of the global variable help_echo has
-	     changed, generate a HELP_EVENT.  */
-	  if (!NILP (help_echo_string) || !NILP (previous_help_echo_string))
-	    gen_help_event (help_echo_string, selected_frame, help_echo_window,
-			    help_echo_object, help_echo_pos);
+	  update_mouse_position (SELECTED_FRAME (), mouse_last_x, mouse_last_y);
 	}
 
       for (but = 0; but < NUM_MOUSE_BUTTONS; but++)
diff --git a/src/nsterm.m b/src/nsterm.m
index 1b2328628e..c3c82b5fef 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -418,12 +418,12 @@ - (NSColor *)colorUsingDefaultColorSpace
 #define EV_TRAILER(e)                                                   \
   {                                                                     \
     XSETFRAME (emacs_event->frame_or_window, emacsframe);               \
-    EV_TRAILER2 (e);                                                    \
+    if (e) emacs_event->timestamp = EV_TIMESTAMP (e);                   \
+    EV_TRAILER2 ();                                                     \
   }
 
-#define EV_TRAILER2(e)                                                  \
+#define EV_TRAILER2()                                                   \
   {                                                                     \
-      if (e) emacs_event->timestamp = EV_TIMESTAMP (e);                 \
       if (q_event_ptr)                                                  \
         {                                                               \
           Lisp_Object tem = Vinhibit_quit;                              \
@@ -7037,6 +7037,7 @@ - (void) scrollWheel: (NSEvent *)theEvent
 }
 
 
+/* FIXME: Combine this logic with update_mouse_position.  */
 /* Tell emacs the mouse has moved.  */
 - (void)mouseMoved: (NSEvent *)e
 {
@@ -7081,7 +7082,7 @@ - (void)mouseMoved: (NSEvent *)e
           NSTRACE_MSG ("in_window");
           emacs_event->kind = SELECT_WINDOW_EVENT;
           emacs_event->frame_or_window = window;
-          EV_TRAILER2 (e);
+          EV_TRAILER2 ();
         }
       /* Remember the last window where we saw the mouse.  */
       last_mouse_window = window;
diff --git a/src/termhooks.h b/src/termhooks.h
index 3800679e80..a9a0fb2761 100644
--- a/src/termhooks.h
+++ b/src/termhooks.h
@@ -212,7 +212,8 @@ #define EMACS_TERMHOOKS_H
   /* Generated when a frame is moved.  */
   MOVE_FRAME_EVENT,
 
-  /* Generated when mouse moves over window not currently selected.  */
+  /* Generated when mouse moves over window not currently selected.
+     .timestamp is always 0.  */
   SELECT_WINDOW_EVENT,
 
   /* Queued from XTread_socket when session manager sends
diff --git a/src/w32inevt.c b/src/w32inevt.c
index 1a80a00197..f46494c841 100644
--- a/src/w32inevt.c
+++ b/src/w32inevt.c
@@ -482,50 +482,9 @@ do_mouse_event (MOUSE_EVENT_RECORD *event,
 
 	if (f->mouse_moved)
 	  {
-	    if (hlinfo->mouse_face_hidden)
-	      {
-		hlinfo->mouse_face_hidden = 0;
-		clear_mouse_face (hlinfo);
-	      }
-
-	    /* Generate SELECT_WINDOW_EVENTs when needed.  */
-	    if (!NILP (Vmouse_autoselect_window))
-	      {
-		Lisp_Object mouse_window = window_from_coordinates (f, mx, my,
-								    0, 0, 0);
-		/* A window will be selected only when it is not
-		   selected now, and the last mouse movement event was
-		   not in it.  A minibuffer window will be selected iff
-		   it is active.  */
-		if (WINDOWP (mouse_window)
-		    && !EQ (mouse_window, last_mouse_window)
-		    && !EQ (mouse_window, selected_window))
-		  {
-		    struct input_event event;
-
-		    EVENT_INIT (event);
-		    event.kind = SELECT_WINDOW_EVENT;
-		    event.frame_or_window = mouse_window;
-		    event.arg = Qnil;
-		    event.timestamp = movement_time;
-		    kbd_buffer_store_event (&event);
-		  }
-		last_mouse_window = mouse_window;
-	      }
-	    else
-	      last_mouse_window = Qnil;
-
-	    previous_help_echo_string = help_echo_string;
-	    help_echo_string = help_echo_object = help_echo_window = Qnil;
-	    help_echo_pos = -1;
-	    note_mouse_highlight (f, mx, my);
-	    /* If the contents of the global variable help_echo has
-	       changed (inside note_mouse_highlight), generate a HELP_EVENT.  */
-	    if (!NILP (help_echo_string) || !NILP (previous_help_echo_string))
-	      gen_help_event (help_echo_string, selected_frame,
-			      help_echo_window, help_echo_object,
-			      help_echo_pos);
+	    update_mouse_position (f, mx, my);
 	  }
+
 	/* We already called kbd_buffer_store_event, so indicate to
 	   the caller it shouldn't.  */
 	return 0;
diff --git a/src/w32term.c b/src/w32term.c
index 0ee805a852..7e9b641414 100644
--- a/src/w32term.c
+++ b/src/w32term.c
@@ -5083,6 +5083,7 @@ w32_read_socket (struct terminal *terminal,
 	  break;
 
 	case WM_MOUSEMOVE:
+	  /* FIXME: Combine this logic with update_mouse_position.  */
 	  /* Ignore non-movement.  */
 	  {
 	    int x = LOWORD (msg.msg.lParam);
diff --git a/src/xterm.c b/src/xterm.c
index 744b80c68a..d9fb389f89 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -8830,6 +8830,7 @@ handle_one_xevent (struct x_display_info *dpyinfo,
       goto OTHER;
 
     case MotionNotify:
+      /* FIXME: Combine this logic with update_mouse_position.  */
       {
         x_display_set_last_user_time (dpyinfo, event->xmotion.time);
         previous_help_echo_string = help_echo_string;
-- 
2.20.1

Re: Additional xterm-mouse cleanup

[Jared Finder via Emacs development discussions.]

Looks like this email got missed.  Thoughts on the updated patch?

I'd like to get the two remaining straightforward patches 
(make-help-screen, mouse-auto-select-window) done so that I can focus on 
the echo area improvements, which I think will be significantly more 
complex, as discussed below.

   -- MJF

On 2021-02-03 10:54 pm, Jared Finder wrote:
> On 2021-02-02 6:37 am, Stefan Monnier wrote:
>>>>> 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.
>> 
>> I thought the parts in `src/msdos.c` and `src/w32inevt.c` were
>> sufficiently similar that the change can be done without much risk of
>> introducing a regression.
> 
> Ah, got it. I agree, it is mostly straightforward. To do this properly
> required making an assumption that .timestamp=0 for
> SELECT_WINDOW_EVENT is ok. Looking through the C code, I don't see any
> location that reads .timestamp for the SELECT_WINDOW_EVENT, so I make
> it uniformly 0 throughout.  Updated patch attached.
> 
>>>> [ Oh ... how I hate that echo area code.  ]
>> 
>> I have the impression that a whole lot of code can run between the
>> `clear_message` and your code, so I don't immediately see why we can 
>> be
>> sure that `echo_area_buffer[1]` indeed always contains the thing 
>> before
>> the `clear_message`.  And if it doesn't, then maybe we shouldn't try 
>> to
>> revert the echo message.
> 
> Good point. I will update my patch to have a copy of the echo area
> made inside read_key_sequence.
> 
>> I agree it doesn't seem easy, but in my experience "do and later undo"
>> sooner or later leads to extra difficulties so I tend to prefer "delay
>> the do until we're sure we want to perform it".
> 
> I completely agree with the sentiment, but I do not think it is the
> right tradeoff. To delay until we're sure, we'd need to have some sort
> of assumption of how terminal escape sequences are received that
> normal humans would never do. Consider that the following key sequence
> is a mouse movement escape sequence but is completely possible for a
> human to type slowly:
> 
> ESC [ < 3 5 ; 1 9 ; 3 4 m
> 
> What should the echo area display if it has read "ESC ["? At this
> point, input-decode-map still doesn't know if this is a xterm escape
> sequence or not. This could just be part way through the above
> sequence, in which case nothing should be displayed. Alternatively, a
> user could have "ESC [ a" bound to an arbitrary command and they're
> part of the way through triggering that command, in which sase "ESC
> [-" should be displayed. Any change here is going to be *BIG*, which
> feels like a high risk change for a not-yet-demonstrated bug.
> 
> Or to use a metaphor: this feels like you're asking for heart bypass
> surgery before putting a bandage over a cut on Mr. Echo Area's elbow.
> I don't disagree that Mr. Echo Area would be able to be much more
> active after the surgery (and all that smoking raised his blood
> pressure a lot), but right now all he really wants is the bandage.
> 
> I do completely agree with adding the unit tests to ensure I don't
> make things worse.
> 
>   -- MJF

Re: Additional xterm-mouse cleanup

[Stefan Monnier]

Sorry for not answering earlier.

> Ah, got it. I agree, it is mostly straightforward. To do this properly
>  required making an assumption that .timestamp=0 for SELECT_WINDOW_EVENT is
>  ok. Looking through the C code, I don't see any location that reads
>  .timestamp for the SELECT_WINDOW_EVENT, so I make it uniformly
>  0 throughout.  Updated patch attached.

Looks good to me.

>>>> [ Oh ... how I hate that echo area code.  ]
>> I have the impression that a whole lot of code can run between the
>> `clear_message` and your code, so I don't immediately see why we can be
>> sure that `echo_area_buffer[1]` indeed always contains the thing before
>> the `clear_message`.  And if it doesn't, then maybe we shouldn't try to
>> revert the echo message.
>
> Good point. I will update my patch to have a copy of the echo area made
> inside read_key_sequence.

I don't see this in your patch, so I assume it'll be in a subsequent patch.

>> I agree it doesn't seem easy, but in my experience "do and later undo"
>> sooner or later leads to extra difficulties so I tend to prefer "delay
>> the do until we're sure we want to perform it".
> I completely agree with the sentiment, but I do not think it is the right
> tradeoff. To delay until we're sure, we'd need to have some sort of
> assumption of how terminal escape sequences are received that normal humans
> would never do. Consider that the following key sequence is a mouse movement
> escape sequence but is completely possible for a human to type slowly:
>
> ESC [ < 3 5 ; 1 9 ; 3 4 m
>
> What should the echo area display if it has read "ESC ["? At this point,
> input-decode-map still doesn't know if this is a xterm escape sequence or
> not.

Right, so indeed the best we can do is to record the clear in such a way
that we can undo it as faithfully as possible.  This also begs the question
of what I mean by "*the* clear" since there's presumably going to be one
clear per byte in the above sequence.

> Or to use a metaphor: this feels like you're asking for heart bypass surgery
> before putting a bandage over a cut on Mr. Echo Area's elbow.

Actually, I suspect it's worse than bypass surgery, because it requires
an oracle.


        Stefan

Re: Additional xterm-mouse cleanup

[Jared Finder via Emacs development discussions.]

On 2021-02-24 7:56 pm, Stefan Monnier wrote:
> 
>> Ah, got it. I agree, it is mostly straightforward. To do this properly
>>  required making an assumption that .timestamp=0 for 
>> SELECT_WINDOW_EVENT is
>>  ok. Looking through the C code, I don't see any location that reads
>>  .timestamp for the SELECT_WINDOW_EVENT, so I make it uniformly
>>  0 throughout.  Updated patch attached.
> 
> Looks good to me.

Thank you.  If it's okay, I'd like it if the two, now simple, patches 
could be merged now before the third one, as this third one will take 
some time.

>>>>> [ Oh ... how I hate that echo area code.  ]
>>> I have the impression that a whole lot of code can run between the
>>> `clear_message` and your code, so I don't immediately see why we can 
>>> be
>>> sure that `echo_area_buffer[1]` indeed always contains the thing 
>>> before
>>> the `clear_message`.  And if it doesn't, then maybe we shouldn't try 
>>> to
>>> revert the echo message.
>> 
>> Good point. I will update my patch to have a copy of the echo area 
>> made
>> inside read_key_sequence.
> 
> I don't see this in your patch, so I assume it'll be in a subsequent 
> patch.

Yup!

>> To delay until we're sure, we'd need to have some sort of
>> assumption of how terminal escape sequences are received that normal 
>> humans
>> would never do. Consider that the following key sequence is a mouse 
>> movement
>> escape sequence but is completely possible for a human to type slowly:
>> 
>> ESC [ < 3 5 ; 1 9 ; 3 4 m
>> 
>> What should the echo area display if it has read "ESC ["? At this 
>> point,
>> input-decode-map still doesn't know if this is a xterm escape sequence 
>> or
>> not.
> 
> Right, so indeed the best we can do is to record the clear in such a 
> way
> that we can undo it as faithfully as possible.  This also begs the 
> question
> of what I mean by "*the* clear" since there's presumably going to be 
> one
> clear per byte in the above sequence.

I don't think remembering per input character is needed. Just one value 
is sufficient because the only case that needs to be handled is 
restoring the echo state when mock_input would be 0.

Consider what *should* happens when if, for example, the input is:

C-x 8 ESC [ < 3 5 ; 1 9 ; 3 4 m

The mouse movement escape sequence will be stripped, mock_input will be 
set to 2, and the code will goto replay_sequence.  At this point, one of 
two things should happen:

a) If this was done faster than `echo-keystrokes', the echo area should 
be empty as pressing a key clears the echo area.
b) If this was done slower than `echo-keystrokes', the echo area should 
display "C-x 8-" as waiting after pressing a key displays the keys typed 
in.

Both of the above will happen based on the existing logic of calling 
add_command_key() followed by echo_now() when mock_input is non-zero.  
The only case this does not handle is when mock_input is zero, hence 
that's the case to restore to.

   -- MJF

Re: Additional xterm-mouse cleanup

[Stefan Monnier]

>>> Ah, got it. I agree, it is mostly straightforward. To do this properly
>>>  required making an assumption that .timestamp=0 for SELECT_WINDOW_EVENT
>>> is
>>>  ok. Looking through the C code, I don't see any location that reads
>>>  .timestamp for the SELECT_WINDOW_EVENT, so I make it uniformly
>>>  0 throughout.  Updated patch attached.
>> Looks good to me.
>
> Thank you.  If it's okay, I'd like it if the two, now simple, patches could
> be merged now before the third one, as this third one will take some time.

Fine by me, yes.


        Stefan

Re: Additional xterm-mouse cleanup

[Eli Zaretskii]

> Date: Wed, 27 Jan 2021 22:32:21 -0800
> From: jared--- via "Emacs development discussions." <emacs-devel@gnu.org>
> 
> I've got four small patches for my continued work to make xterm-mouse function better. #1 also makes Linux
> terminal function keys behave better when running help-for-help and #2 and #3 both improve GPM-based
> mouse behavior too.
> 
> I'd like feedback in particular on patch #4, which is small but in a sensitive location -- it's one additional code
> line in read_key_sequence along the input-decode-map path. I considered instead putting the change as
> part of the other cleanup right after replay_sequence, but ultimately thought that would be riskier and serve
> no additional purpose.
> 
> Let me know your thoughts.

Thanks, I installed this part:

  From d2ee448ffa70ddb02645f5397b727ae2b90e20ed Mon Sep 17 00:00:00 2001
  From: Jared Finder <jared@finder.org>
  Date: Mon, 7 Dec 2020 22:44:32 -0800
  Subject: [PATCH 2/4] * lisp/tab-line.el (tab-line-new-tab): Use tty menus when
   supported.

which seems to be a no-brainer.

The rest is still being discussed, so I will wait for the discussions
to come to completion.

P.S. Please in the future post patches via "M-x report-emacs-bug", so
that our issue tracker records all the pertinent discussions.

Re: Additional xterm-mouse cleanup

[jared--- via Emacs development discussions.]

On 2021-01-30 1:38 am, Eli Zaretskii wrote:
>> Date: Wed, 27 Jan 2021 22:32:21 -0800
>> From: jared--- via "Emacs development discussions." 
>> <emacs-devel@gnu.org>
>> 
>> I've got four small patches for my continued work to make xterm-mouse 
>> function better. #1 also makes Linux
>> terminal function keys behave better when running help-for-help and #2 
>> and #3 both improve GPM-based
>> mouse behavior too.
>> 
>> I'd like feedback in particular on patch #4, which is small but in a 
>> sensitive location -- it's one additional code
>> line in read_key_sequence along the input-decode-map path. I 
>> considered instead putting the change as
>> part of the other cleanup right after replay_sequence, but ultimately 
>> thought that would be riskier and serve
>> no additional purpose.
>> 
>> Let me know your thoughts.
> 
> Thanks, I installed this part:
> 
>   From d2ee448ffa70ddb02645f5397b727ae2b90e20ed Mon Sep 17 00:00:00 
> 2001
>   From: Jared Finder <jared@finder.org>
>   Date: Mon, 7 Dec 2020 22:44:32 -0800
>   Subject: [PATCH 2/4] * lisp/tab-line.el (tab-line-new-tab): Use tty 
> menus when
>    supported.
> 
> which seems to be a no-brainer.
> 
> The rest is still being discussed, so I will wait for the discussions
> to come to completion.
> 
> P.S. Please in the future post patches via "M-x report-emacs-bug", so
> that our issue tracker records all the pertinent discussions.

Sorry about that.  I thought report-emacs-bug was for bugreports only, 
not for arbitrary feature development.  I guess I should delete most of 
the template text if is just straightforward feature dev.  Is that 
right?

   -- MJF

Re: Additional xterm-mouse cleanup

[Eli Zaretskii]

> Date: Tue, 02 Feb 2021 00:28:24 -0800
> From: Jared Finder <jared@finder.org>
> Cc: emacs-devel@gnu.org
> 
> > P.S. Please in the future post patches via "M-x report-emacs-bug", so
> > that our issue tracker records all the pertinent discussions.
> 
> Sorry about that.

No need to apologize, no disaster happened.

> I guess I should delete most of the template text if is just
> straightforward feature dev.  Is that right?

Yes, if the change you propose is not for some issue that is specific
to the platform on which you write the message.

Re: Additional xterm-mouse cleanup

[jared--- via Emacs development discussions.]

On 2021-02-02 7:08 am, Eli Zaretskii wrote:
>> Date: Tue, 02 Feb 2021 00:28:24 -0800
>> From: Jared Finder <jared@finder.org>
>> Cc: emacs-devel@gnu.org
>> 
>> > P.S. Please in the future post patches via "M-x report-emacs-bug", so
>> > that our issue tracker records all the pertinent discussions.
>> 
>> Sorry about that.
> 
> No need to apologize, no disaster happened.
> 
>> I guess I should delete most of the template text if is just
>> straightforward feature dev.  Is that right?
> 
> Yes, if the change you propose is not for some issue that is specific
> to the platform on which you write the message.

Okay, I will do that for future changes.  Let me know if I should move 
this thread to report-emacs-bug as well. My current plan is to finish 
off the two changes that are nearly done and not controversial (i.e., 
not the change to read_key_sequence), and then have all future changes 
done via report-emacs-bug.

   -- MJF

Re: Additional xterm-mouse cleanup

[Eli Zaretskii]

> Date: Wed, 03 Feb 2021 22:57:23 -0800
> From: Jared Finder <jared@finder.org>
> Cc: emacs-devel@gnu.org
> 
> > Yes, if the change you propose is not for some issue that is specific
> > to the platform on which you write the message.
> 
> Okay, I will do that for future changes.  Let me know if I should move 
> this thread to report-emacs-bug as well.

No need, we can finish handling this series here.

Thanks.