Re: Additional xterm-mouse cleanup

[jared--- via "Emacs development discussions." <emacs-devel@gnu.org>]

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