bug#29104: [PATCH]: xterm-mouse-mode clicks in the modeline dont use the keymap properties of the modeline strings

bug#29104: [PATCH]: xterm-mouse-mode clicks in the modeline dont use the keymap properties of the modeline strings

[Olaf Rogalsky]

1. emacs -Q -nw
2. M-x xterm-mouse-mode RET
3. mouse-1 click on the `*scratch*' string of the modeline.
4. Nothing happens. Expected behaviour would be, to change to the next buffer `*Messages*'.

This seems to be a bug in `keyboard.c:read_key_sequence'. After reading
the first characters of the xterm click event, they are remapped with
`keyremap_step' at line 9605 using the `input-decode-map'. The result of
this is a mouse-1 event structure, which carries the correct click
position. Unfortunately, this position isn't used, but instead, at line
9035, the position of the variable `first_event' is used.

The following patch sets `first_event' (after a key translation /
remapping) to the remapped event.

diff --git a/src/keyboard.c b/src/keyboard.c
index ee353d2b07..acb9c6be95 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -9607,6 +9607,8 @@ read_key_sequence (Lisp_Object *keybuf, int bufsize, Lisp_Object prompt,
 	  if (done)
 	    {
 	      mock_input = diff + max (t, mock_input);
+              first_event = mock_input > 0 ? keybuf[0] : Qnil;
+
 	      goto replay_sequence;
 	    }
 	}
@@ -9647,6 +9649,7 @@ read_key_sequence (Lisp_Object *keybuf, int bufsize, Lisp_Object prompt,
 		/* Adjust the input-decode-map counters.  */
 		indec.end += diff;
 		indec.start += diff;
+                first_event = mock_input > 0 ? keybuf[0] : Qnil;
 
 		goto replay_sequence;
 	      }
@@ -9669,6 +9672,7 @@ read_key_sequence (Lisp_Object *keybuf, int bufsize, Lisp_Object prompt,
 	      indec.start += diff;
 	      fkey.end += diff;
 	      fkey.start += diff;
+              first_event = mock_input > 0 ? keybuf[0] : Qnil;
 
 	      goto replay_sequence;
 	    }


-- 
Olaf Rogalsky
Schwörhausgasse 5
89073 Ulm
Germany

bug#29104: [PATCH]: xterm-mouse-mode clicks in the modeline dont use the keymap properties of the modeline strings

[Olaf Rogalsky]

Hi,

I would like to give some more details on the patch and which problem it
tries to solve.

Ultimately I try to get better support of the mouse under xterm. The
following features currently do not work with the mouse under xterm:

1. Mouse clicks on menu-bar always brings up the left-most menu, but not
   the menu under the spot.
   
2. A mouse click on the mode-line activates the window, but does not
   trigger the local-map property on the spot.

3. When moving the mouse over some text with a mouse-face property or
   help-echo property nothing happens, because xterm-mouse-mode does not
   receive any mouse motion events from the controlling xterm (except
   during dragging operations).

I haven't looked into (1), but I am testing a solution for (2) and (3).

This patch (#29104) addresses (2). It does not implement a new feature,
but tries to make a bugfix to keyboard.c:read_key_sequence. This
function is a 900 lines nightmare of spaggheti code, full of global
state and goto's. At least it is somehow documented.

But here we go, this is what I understood so far:

The function reads input characters until some keybinding is found or
until the sequence does not form a prefix key sequence anymore. If the
trailing characters, which have been read so far, form a sequence in a
translation keymap (input-decode-map, key-translation-map,
local-function-key-map), those trailing characters are replaced by the
binding of the translation map.  Thereafter a jump back close to the
beginning of the function is made and all events up to the replacement
event are reinterpreted again.

Such jumps ("goto replay_sequence;") take place immediately after the
lines, which I introduced in the patch.  When replaying the sequence,
the variable "first_event" will be used to select the currently active
keymaps (line 9035).  In case, that the first characters are bound in a
key translation map, first_event still holds (incrorrectly, as I
presume) the first character of the translation sequence and not its
binding.

My patch takes place after a key translation has been performed.  It
checks, that the key translation is not empty, (or more precisely, that
the current event sequence is not empty),

           first_event = mock_input > 0 ? keybuf[0] : Qnil;  
                         ^^^^^^^^^^^^^^

and if so, sets the variable first_event to the first event in the
sequence.

           first_event = mock_input > 0 ? keybuf[0] : Qnil;  
                                          ^^^^^^^^^

Here, all key translations where allready replaced by their
corresponding binding.  If the current event sequence is empty,
first_event is set to Qnil, which means that the default active keymaps
should be used. This would be the case, if the raw key sequence was
bound to an empty vector in a translation map.

           first_event = mock_input > 0 ? keybuf[0] : Qnil;  
                                                      ^^^^

Now back to my original problem: mouse clicks in the mode line do not
work correctly. xterm-mouse-mode uses the input-decode-map to translate
special character sequences coming from xterm (starting with "\e[") into
proper mouse events.  Without the patch, first_event is set to ?\e.
Therefore the esc-map is used to find a binding for the mouse event,
which of cause does not work as intended. With the patch, first_event is
set to the mouse event and the binding in the local-map property will be
found.

Et vola, mouse clicks in the mode line will work :-)


Olaf


PS: You might ask, why I check for empty key translations.  I need those
    for (3).



-- 
Olaf Rogalsky
Schwörhausgasse 5
89073 Ulm
Germany

bug#29104: [PATCH]: xterm-mouse-mode clicks in the modeline dont use the keymap properties of the modeline strings

[Eli Zaretskii]

> From: Olaf Rogalsky <olaf.rogalsky@t-online.de>
> Date: Tue, 07 Nov 2017 00:47:28 +0100
> 
> My patch takes place after a key translation has been performed.  It
> checks, that the key translation is not empty, (or more precisely, that
> the current event sequence is not empty),
> 
>            first_event = mock_input > 0 ? keybuf[0] : Qnil;  
>                          ^^^^^^^^^^^^^^
> 
> and if so, sets the variable first_event to the first event in the
> sequence.
> 
>            first_event = mock_input > 0 ? keybuf[0] : Qnil;  
>                                           ^^^^^^^^^
> 
> Here, all key translations where allready replaced by their
> corresponding binding.  If the current event sequence is empty,
> first_event is set to Qnil, which means that the default active keymaps
> should be used. This would be the case, if the raw key sequence was
> bound to an empty vector in a translation map.
> 
>            first_event = mock_input > 0 ? keybuf[0] : Qnil;  
>                                                       ^^^^
> 
> Now back to my original problem: mouse clicks in the mode line do not
> work correctly. xterm-mouse-mode uses the input-decode-map to translate
> special character sequences coming from xterm (starting with "\e[") into
> proper mouse events.  Without the patch, first_event is set to ?\e.
> Therefore the esc-map is used to find a binding for the mouse event,
> which of cause does not work as intended. With the patch, first_event is
> set to the mouse event and the binding in the local-map property will be
> found.
> 
> Et vola, mouse clicks in the mode line will work :-)

Thanks for the detailed explanations, they help a lot.

I have a few questions.  First, why did you need this hunk:

> @@ -9669,6 +9672,7 @@ read_key_sequence (Lisp_Object *keybuf, int bufsize, Lisp_Object prompt,
>  	      indec.start += diff;
>  	      fkey.end += diff;
>  	      fkey.start += diff;
> +              first_event = mock_input > 0 ? keybuf[0] : Qnil;
>  
>  	      goto replay_sequence;
>  	    }

This is no longer about input-decode-map, it's about
key-translation-map.  And xterm-mouse-mode doesn't use
key-translation-map, so why change this part?

The next question is whether this patch caters correctly to features
other than xterm-mouse-mode.  The input-decode-map is used by
terminal-specific support in xterm.el and rxvt.el -- does it still
make sense to use keybuf[0] as first_event for replaying their
sequences?

bug#29104: AW: bug#29104: [PATCH]: xterm-mouse-mode clicks in the modeline dont use the keymap properties of the modeline strings

[olaf.rogalsky]

> This is no longer about input-decode-map, it's about
> key-translation-map.  And xterm-mouse-mode doesn't use
> key-translation-map, so why change this part?

This hunk, and the hunk

@@ -9647,6 +9649,7 @@ read_key_sequence (Lisp_Object *keybuf, int bufsize,
Lisp_Object prompt,
                /* Adjust the input-decode-map counters.  */
                indec.end += diff;
                indec.start += diff;
+                first_event = mock_input > 0 ? keybuf[0] : Qnil;

                goto replay_sequence;
              }

are actually unrelated to xterm-mouse and where included for symmetry,
only. But I still think, its correct to do so:

After initialization, the only place where first_event is set, is at
line 9282:

          if (NILP (first_event))
            {
              first_event = key;

Unfortunately, this line is not reached during the replay of a key
translation (either input-decode-map or key-translation-map). So after
a key translation, ideally one should check, if the first key of the
key sequence was translated, and set first_event accordingly. But
since keybuf[0] holds the first event in any case, I set first_event
unconditionally to keybuf[0].

The reason, why this bug has not been recognized so far is, that the
only use of first_event is as an argument to the function
active_maps. In that function, first_event is ignored, if it is not a
mouse event. Afaik,translations, which result in mouse events, are
only generated xterm-mouse.

Summarizing, I believe, it does no harm to also set first_event after a
key-translation-map, but I included these to hunks for consistency
only. I would understand you, if you fear, that we might not
fully understand its consequences.


> The next question is whether this patch caters correctly to features
> other than xterm-mouse-mode.  The input-decode-map is used by
> terminal-specific support in xterm.el and rxvt.el -- does it still
> make sense to use keybuf[0] as first_event for replaying their
> sequences?

Yes I think so. As mentioned before, the only use of first_event is,
in active_maps(first_event), which prepends the active maps with the
local-map property after a mouse click. If first_event is not a mouse
event, its particular value is ignored. xterm.el and rxvt.el do not
generate mouse events and are therefore not affected (but even if they
did, setting first_event to keybuf[0] would still be the right thing).

Just for reference, this is the definition of active_maps():

static Lisp_Object
active_maps (Lisp_Object first_event)
{
  Lisp_Object position
    = CONSP (first_event) ? CAR_SAFE (XCDR (first_event)) : Qnil;
      |                     |                               |
      check for compound    get mouse position              otherwise ignore
      event (mouse click)                                   first_event

  return Fcons (Qkeymap, Fcurrent_active_maps (Qt, position));
}

Olaf


----------------------------------------------------------------
Gesendet mit Telekom Mail <https://t-online.de/email-kostenlos> - kostenlos und sicher für alle!

bug#29104: AW: bug#29104: [PATCH]: xterm-mouse-mode clicks in the modeline dont use the keymap properties of the modeline strings

[olaf.rogalsky]

In a more radical approach and instead of my original patch, one can get rid off
with the variable first_event all together, see patch below. It simplifies the code
a little bit, but of course is more risky.

Olaf


diff --git a/src/keyboard.c b/src/keyboard.c
index ee353d2..8df43b8 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -8916,7 +8916,6 @@ static void process_tool_bar_item (Lisp_Object, Lisp_Object, Lisp_Object,
   ptrdiff_t keys_start;
 
   Lisp_Object current_binding = Qnil;
-  Lisp_Object first_event = Qnil;
 
   /* Index of the first key that has no binding.
      It is useless to try fkey.start larger than that.  */
@@ -9032,7 +9031,7 @@ static void process_tool_bar_item (Lisp_Object, Lisp_Object, Lisp_Object,
      keybuf with its symbol, or if the sequence starts with a mouse
      click and we need to switch buffers, we jump back here to rebuild
      the initial keymaps from the current buffer.  */
-  current_binding = active_maps (first_event);
+  current_binding = active_maps (mock_input > 0 ? keybuf[0] : Qnil);
 
   /* Start from the beginning in keybuf.  */
   t = 0;
@@ -9279,10 +9278,9 @@ static void process_tool_bar_item (Lisp_Object, Lisp_Object, Lisp_Object,
                }
            }
 
-         if (NILP (first_event))
+         if (t == 0)
            {
-             first_event = key;
-             /* Even if first_event does not specify a particular
+             /* Even if key does not specify a particular
                 window/position, it's important to recompute the maps here
                 since a long time might have passed since we entered
                 read_key_sequence, and a timer (or process-filter or
@@ -9292,7 +9290,7 @@ static void process_tool_bar_item (Lisp_Object, Lisp_Object, Lisp_Object,
                  && (XBUFFER (XWINDOW (selected_window)->contents)
                      != current_buffer))
                Fset_buffer (XWINDOW (selected_window)->contents);
-             current_binding = active_maps (first_event);
+             current_binding = active_maps (key);
            }
 
          GROW_RAW_KEYBUF;


----------------------------------------------------------------
Gesendet mit Telekom Mail <https://t-online.de/email-kostenlos> - kostenlos und sicher für alle!

bug#29104: AW: bug#29104: [PATCH]: xterm-mouse-mode clicks in the modeline dont use the keymap properties of the modeline strings

[Stefan Monnier]

> In a more radical approach and instead of my original patch, one can
> get rid off with the variable first_event all together, see patch
> below. It simplifies the code a little bit, but of course is
> more risky.

Indeed, I got a similar patch (see below) by simply moving your
assignment to first_event from "before the `goto replay_sequence`" to
after it (i.e. after the replay_sequence label).

Your patch looks about as good as mine.  I think they're both better
than your first patch because they fix the problem in a single place
(The Right Place).

I installed mine because it's shorter and looks more "obviously safe"
to me.  Thanks a lot for tracking it down!


        Stefan


diff --git a/src/keyboard.c b/src/keyboard.c
index 399149ae97..6b7a6bfa74 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -8919,7 +8919,6 @@ read_key_sequence (Lisp_Object *keybuf, int bufsize, Lisp_Object prompt,
   ptrdiff_t keys_start;
 
   Lisp_Object current_binding = Qnil;
-  Lisp_Object first_event = Qnil;
 
   /* Index of the first key that has no binding.
      It is useless to try fkey.start larger than that.  */
@@ -9029,6 +9028,7 @@ read_key_sequence (Lisp_Object *keybuf, int bufsize, Lisp_Object prompt,
 
   starting_buffer = current_buffer;
   first_unbound = bufsize + 1;
+  Lisp_Object first_event = mock_input > 0 ? keybuf[0] : Qnil;
 
   /* Build our list of keymaps.
      If we recognize a function key and replace its escape sequence in

bug#29104: AW: bug#29104: [PATCH]: xterm-mouse-mode clicks in the modeline dont use the keymap properties of the modeline strings

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  "29104\@debbugs.gnu.org" <29104-done@debbugs.gnu.org>
> Date: Sat, 18 Nov 2017 23:24:49 -0500
> 
> Your patch looks about as good as mine.  I think they're both better
> than your first patch because they fix the problem in a single place
> (The Right Place).
> 
> I installed mine because it's shorter and looks more "obviously safe"
> to me.  Thanks a lot for tracking it down!

Thanks, but why in the world on the release branch??  I said I was
thinking of installing this on master, not on emacs-26.

bug#29104: AW: bug#29104: [PATCH]: xterm-mouse-mode clicks in the modeline dont use the keymap properties of the modeline strings

[Stefan Monnier]

> Thanks, but why in the world on the release branch??  I said I was
> thinking of installing this on master, not on emacs-26.

Sorry, I didn't notice this part, I just assumed you wanted this on
emacs-26, even tho in retrospect I see it's not particularly urgent.
Do you want me to move it to master?


        Stefan

bug#29104: AW: bug#29104: [PATCH]: xterm-mouse-mode clicks in the modeline dont use the keymap properties of the modeline strings

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: olaf.rogalsky@t-online.de,  29104@debbugs.gnu.org
> Date: Mon, 20 Nov 2017 08:26:10 -0500
> 
> > Thanks, but why in the world on the release branch??  I said I was
> > thinking of installing this on master, not on emacs-26.
> 
> Sorry, I didn't notice this part, I just assumed you wanted this on
> emacs-26, even tho in retrospect I see it's not particularly urgent.

Mistakes happen.  Sorry for not being more clear on that.

> Do you want me to move it to master?

Yes, please, and thanks.

bug#29104: AW: bug#29104: [PATCH]: xterm-mouse-mode clicks in the modeline dont use the keymap properties of the modeline strings

[Stefan Monnier]

> Yes, please, and thanks.

Done,


        Stefan

bug#29104: AW: bug#29104: [PATCH]: xterm-mouse-mode clicks in the modeline dont use the keymap properties of the modeline strings

[Olaf Rogalsky]

> I installed mine because it's shorter and looks more "obviously safe"
> to me.  Thanks a lot for tracking it down!
Thanks for installing the patch and closing the bug!

-- 
Olaf Rogalsky
Schwörhausgasse 5
89073 Ulm
Germany

bug#29104: [PATCH]: xterm-mouse-mode clicks in the modeline dont use the keymap properties of the modeline strings

[Eli Zaretskii]

> From: Olaf Rogalsky <olaf.rogalsky@t-online.de>
> Date: Tue, 07 Nov 2017 00:47:28 +0100
> 
> I would like to give some more details on the patch and which problem it
> tries to solve.
> 
> Ultimately I try to get better support of the mouse under xterm. The
> following features currently do not work with the mouse under xterm:
> 
> 1. Mouse clicks on menu-bar always brings up the left-most menu, but not
>    the menu under the spot.
>    
> 2. A mouse click on the mode-line activates the window, but does not
>    trigger the local-map property on the spot.
> 
> 3. When moving the mouse over some text with a mouse-face property or
>    help-echo property nothing happens, because xterm-mouse-mode does not
>    receive any mouse motion events from the controlling xterm (except
>    during dragging operations).
> 
> I haven't looked into (1), but I am testing a solution for (2) and (3).
> 
> This patch (#29104) addresses (2). It does not implement a new feature,
> but tries to make a bugfix to keyboard.c:read_key_sequence. This
> function is a 900 lines nightmare of spaggheti code, full of global
> state and goto's. At least it is somehow documented.
> 
> But here we go, this is what I understood so far:
> 
> The function reads input characters until some keybinding is found or
> until the sequence does not form a prefix key sequence anymore. If the
> trailing characters, which have been read so far, form a sequence in a
> translation keymap (input-decode-map, key-translation-map,
> local-function-key-map), those trailing characters are replaced by the
> binding of the translation map.  Thereafter a jump back close to the
> beginning of the function is made and all events up to the replacement
> event are reinterpreted again.
> 
> Such jumps ("goto replay_sequence;") take place immediately after the
> lines, which I introduced in the patch.  When replaying the sequence,
> the variable "first_event" will be used to select the currently active
> keymaps (line 9035).  In case, that the first characters are bound in a
> key translation map, first_event still holds (incrorrectly, as I
> presume) the first character of the translation sequence and not its
> binding.
> 
> My patch takes place after a key translation has been performed.  It
> checks, that the key translation is not empty, (or more precisely, that
> the current event sequence is not empty),
> 
>            first_event = mock_input > 0 ? keybuf[0] : Qnil;  
>                          ^^^^^^^^^^^^^^
> 
> and if so, sets the variable first_event to the first event in the
> sequence.
> 
>            first_event = mock_input > 0 ? keybuf[0] : Qnil;  
>                                           ^^^^^^^^^
> 
> Here, all key translations where allready replaced by their
> corresponding binding.  If the current event sequence is empty,
> first_event is set to Qnil, which means that the default active keymaps
> should be used. This would be the case, if the raw key sequence was
> bound to an empty vector in a translation map.
> 
>            first_event = mock_input > 0 ? keybuf[0] : Qnil;  
>                                                       ^^^^
> 
> Now back to my original problem: mouse clicks in the mode line do not
> work correctly. xterm-mouse-mode uses the input-decode-map to translate
> special character sequences coming from xterm (starting with "\e[") into
> proper mouse events.  Without the patch, first_event is set to ?\e.
> Therefore the esc-map is used to find a binding for the mouse event,
> which of cause does not work as intended. With the patch, first_event is
> set to the mouse event and the binding in the local-map property will be
> found.
> 
> Et vola, mouse clicks in the mode line will work :-)

Stefan, any comments?  I'm thinking of installing this on master.

Thanks.

bug#29104: [PATCH]: xterm-mouse-mode clicks in the modeline dont use the keymap properties of the modeline strings

[Stefan Monnier]

>> This patch (#29104) addresses (2). It does not implement a new feature,
>> but tries to make a bugfix to keyboard.c:read_key_sequence. This
>> function is a 900 lines nightmare of spaggheti code, full of global
>> state and goto's. At least it is somehow documented.

Oh, yes, the number of lines has fluctuated a bit, but it's been
a nightmare ever since I first saw it.  I'm impressed you found the
stamina to dig into it.

> Stefan, any comments?  I'm thinking of installing this on master.

I'll take a look at it later this week-end (it will take some time for
me to re-understand the code ;-)


        Stefan