bug#23007: 24.5; buggy interactive search with middle click

bug#23007: 24.5; buggy interactive search with middle click

[Vincent Lefevre]

1. Start Emacs with its own UI under X Window.

2. Type: M-<
   Here the cursor is at the beginning of the buffer, which contains
   some text.

3. In some other window, select some text (e.g. the letter "n").

4. Type: C-s
   This starts the interactive search. It says:
   I-search:

5. Click with the middle button near the bottom of the window.
   This pastes the text at the end of the buffer.

6. Type: C-_
   This removes the pasted text and moves the cursor back at the
   beginning of the buffer.

7. Type: C-s
   This starts the interactive search again. But this time, it says:
   I-search:  [No previous search string]

8. Type: e
   This searches for "e". All the occurrences of "e" are highlighted.

9. Type: <down>
   This is no longer the interactive search, but the occurrences of "e"
   are still highlighted.

Typing other letters brings back to the interactive search.



In GNU Emacs 24.5.1 (x86_64-pc-linux-gnu, GTK+ Version 3.18.6)
 of 2016-01-22 on binet, modified by Debian
Windowing system distributor `The X.Org Foundation', version 11.0.11801000
System Description:	Debian GNU/Linux stable-updates (sid)

Configured using:
 `configure --build x86_64-linux-gnu --prefix=/usr
 --sharedstatedir=/var/lib --libexecdir=/usr/lib
 --localstatedir=/var/lib --infodir=/usr/share/info
 --mandir=/usr/share/man --with-pop=yes
 --enable-locallisppath=/etc/emacs24:/etc/emacs:/usr/local/share/emacs/24.5/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/24.5/site-lisp:/usr/share/emacs/site-lisp
 --build x86_64-linux-gnu --prefix=/usr --sharedstatedir=/var/lib
 --libexecdir=/usr/lib --localstatedir=/var/lib
 --infodir=/usr/share/info --mandir=/usr/share/man --with-pop=yes
 --enable-locallisppath=/etc/emacs24:/etc/emacs:/usr/local/share/emacs/24.5/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/24.5/site-lisp:/usr/share/emacs/site-lisp
 --with-x=yes --with-x-toolkit=gtk3 --with-toolkit-scroll-bars
 'CFLAGS=-g -O2 -fstack-protector-strong -Wformat
 -Werror=format-security -Wall' 'CPPFLAGS=-Wdate-time
 -D_FORTIFY_SOURCE=2' LDFLAGS=-Wl,-z,relro'

Important settings:
  value of $LC_COLLATE: POSIX
  value of $LC_CTYPE: en_US.UTF-8
  value of $LC_TIME: en_DK
  value of $LANG: POSIX
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  display-time-mode: t
  show-paren-mode: t
  tooltip-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent messages:
Loading /etc/emacs/site-start.d/50latex-cjk-common.el (source)...done
Loading /etc/emacs/site-start.d/50latex-cjk-thai.el (source)...done
Loading /etc/emacs/site-start.d/50psvn.el (source)...done
Loading /etc/emacs/site-start.d/50python-docutils.el (source)...done
Loading /etc/emacs/site-start.d/50rnc-mode.el (source)...done
Loading /etc/emacs/site-start.d/50texlive-lang-english.el (source)...done
Loading /home/vinc17/share/emacs/site-lisp/mutteditor.el (source)...done
Loading time...done
For information about GNU Emacs and the GNU system, type C-h C-a.
user-error: Beginning of history; no preceding item

Load-path shadows:
/usr/share/emacs/24.5/site-lisp/debian-startup hides /usr/share/emacs/site-lisp/debian-startup
/usr/share/emacs24/site-lisp/cmake-data/cmake-mode hides /usr/share/emacs/site-lisp/cmake-mode
/usr/share/emacs/site-lisp/rst hides /usr/share/emacs/24.5/lisp/textmodes/rst
/usr/share/emacs24/site-lisp/latex-cjk-thai/thai-word hides /usr/share/emacs/24.5/lisp/language/thai-word

Features:
(shadow sort gnus-util mail-extr warnings emacsbug message format-spec
rfc822 mml easymenu mml-sec mm-decode mm-bodies mm-encode mail-parse
rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045
ietf-drums mm-util help-fns mail-prsvr mail-utils time cus-start
cus-load paren cc-styles cc-align cc-engine cc-vars cc-defs edmacro
kmacro cl-loaddefs cl-lib time-date tooltip electric uniquify ediff-hook
vc-hooks lisp-float-type mwheel x-win x-dnd tool-bar dnd fontset image
regexp-opt fringe tabulated-list newcomment lisp-mode prog-mode register
page menu-bar rfn-eshadow timer select scroll-bar mouse jit-lock
font-lock syntax facemenu font-core frame cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese hebrew
greek romanian slovak czech european ethiopic indian cyrillic chinese
case-table epa-hook jka-cmpr-hook help simple abbrev minibuffer nadvice
loaddefs button faces cus-face macroexp files text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote make-network-process dbusbind
gfilenotify dynamic-setting system-font-setting font-render-setting
move-toolbar gtk x-toolkit x multi-tty emacs)

Memory information:
((conses 16 92473 4397)
 (symbols 48 20392 0)
 (miscs 40 44 93)
 (strings 32 16021 4470)
 (string-bytes 1 471882)
 (vectors 16 10303)
 (vector-slots 8 396854 4370)
 (floats 8 70 88)
 (intervals 56 230 0)
 (buffers 960 12)
 (heap 1024 32308 1033))

bug#23007: 24.5; buggy interactive search with middle click

[Alex]

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

tags 23007 patch
quit

Vincent Lefevre <vincent@vinc17.net> writes:

> 1. Start Emacs with its own UI under X Window.
>
> 2. Type: M-<
>    Here the cursor is at the beginning of the buffer, which contains
>    some text.
>
> 3. In some other window, select some text (e.g. the letter "n").
>
> 4. Type: C-s
>    This starts the interactive search. It says:
>    I-search:
>
> 5. Click with the middle button near the bottom of the window.
>    This pastes the text at the end of the buffer.
>
> 6. Type: C-_
>    This removes the pasted text and moves the cursor back at the
>    beginning of the buffer.
>
> 7. Type: C-s
>    This starts the interactive search again. But this time, it says:
>    I-search:  [No previous search string]
>
> 8. Type: e
>    This searches for "e". All the occurrences of "e" are highlighted.
>
> 9. Type: <down>
>    This is no longer the interactive search, but the occurrences of "e"
>    are still highlighted.
>
> Typing other letters brings back to the interactive search.

This is because clicking the middle button during isearch calls
isearch-mouse-2, which calls the usual mouse-yank-primary, which calls
isearch-done to finish isearch. Problem is, it let-binds
overriding-terminal-local-map around this call, which counteracts
isearch-done setting this variable to nil. The end result is having quit
isearch, but with the overriding map still in place.

I have attached a simple patch below. Eli, if it's satisfactory, would
you commit it for me?

TIA


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-patch, Size: 1562 bytes --]

From 33251d03d1d2298b29ae2a35ecccb8ae41e8bdd4 Mon Sep 17 00:00:00 2001
From: Alexander Gramiak <agrambot@gmail.com>
Date: Mon, 29 May 2017 13:43:23 -0600
Subject: [PATCH] Limit scope of local overriding-terminal-local-map

The function `binding' may call isearch-done, which globally sets
overriding-terminal-local-map to nil (Bug #23007).
* lisp/isearch.el (isearch-mouse-2): Don't bind
overriding-terminal-local-map around the call to `binding'.
---
 lisp/isearch.el | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/lisp/isearch.el b/lisp/isearch.el
index c34739d638..992794e43a 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -2036,14 +2036,15 @@ isearch-mouse-2
 Otherwise invoke whatever the calling mouse-2 command sequence
 is bound to outside of Isearch."
   (interactive "e")
-  (let* ((w (posn-window (event-start click)))
-	 (overriding-terminal-local-map nil)
-	 (binding (key-binding (this-command-keys-vector) t)))
+  (let ((w (posn-window (event-start click)))
+        binding)
     (if (and (window-minibuffer-p w)
-	     (not (minibuffer-window-active-p w))) ; in echo area
-	(isearch-yank-x-selection)
+             (not (minibuffer-window-active-p w))) ; in echo area
+        (isearch-yank-x-selection)
+      (let ((overriding-terminal-local-map nil))
+        (setq binding (key-binding (this-command-keys-vector) t)))
       (when (functionp binding)
-	(call-interactively binding)))))
+        (call-interactively binding)))))
 
 (declare-function xterm--pasted-text "term/xterm" ())
 
-- 
2.11.0

bug#23007: 24.5; buggy interactive search with middle click

[npostavs]

Alex <agrambot@gmail.com> writes:

> +      (let ((overriding-terminal-local-map nil))
> +        (setq binding (key-binding (this-command-keys-vector) t)))

IMO, it would be better style to do something like

(let ((binding (let ((overriding-terminal-local-map nil))
                 (key-binding (this-command-keys-vector) t))))
  ...)

bug#23007: 24.5; buggy interactive search with middle click

[Drew Adams]

> > +      (let ((overriding-terminal-local-map nil))
> > +        (setq binding (key-binding (this-command-keys-vector) t)))
> 
> IMO, it would be better style to do something like
> 
> (let ((binding (let ((overriding-terminal-local-map nil))
>                  (key-binding (this-command-keys-vector) t))))
>   ...)
 
(let* ((overriding-terminal-local-map nil)
       (binding (key-binding (this-command-keys-vector) t)))
  ...)

;-)

bug#23007: 24.5; buggy interactive search with middle click

[npostavs]

Drew Adams <drew.adams@oracle.com> writes:

>> > +      (let ((overriding-terminal-local-map nil))
>> > +        (setq binding (key-binding (this-command-keys-vector) t)))
>> 
>> IMO, it would be better style to do something like
>> 
>> (let ((binding (let ((overriding-terminal-local-map nil))
>>                  (key-binding (this-command-keys-vector) t))))
>>   ...)
>  
> (let* ((overriding-terminal-local-map nil)
>        (binding (key-binding (this-command-keys-vector) t)))
>   ...)

Check https://debbugs.gnu.org/cgi/bugreport.cgi?bug=23007#8, we need to
avoid binding `overriding-terminal-local-map' in "...".

bug#23007: 24.5; buggy interactive search with middle click

[Drew Adams]

> >> > +      (let ((overriding-terminal-local-map nil))
> >> > +        (setq binding (key-binding (this-command-keys-vector) t)))
> >>
> >> IMO, it would be better style to do something like
> >>
> >> (let ((binding (let ((overriding-terminal-local-map nil))
> >>                  (key-binding (this-command-keys-vector) t))))
> >>   ...)
> >
> > (let* ((overriding-terminal-local-map nil)
> >        (binding (key-binding (this-command-keys-vector) t)))
> >   ...)
> 
> Check https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__debbugs.gnu.org_cgi_bugreport.cgi-3Fbug-3D23007-
> 238&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PQcxBKCX5YTpkKY057SbK10&r=qZLZosY6GBAaJtO
> G9v8QX7nD4BS9t9s5otgxC3d4IFU&m=eZ-Uw6bQQfS-_CFDf_e3zupcjOhlrCnDnq5pNm6-
> bao&s=e0BQOMr3F7kitlASNZ_EMEy-LvwMn0CsRuZUecvrYZg&e= , we need to
> avoid binding `overriding-terminal-local-map' in "...".

Clearly I, like you, meant that this let is for only the second
`if' clause.  But these are only style differences.

If it were I, I'd also move the let-binding that is used only
in the first `if' clause into the `if' test.  (And I'd ensure
that there is in fact a mark.)

(if (let ((win (posn-window (event-start click))))
      (and (window-minibuffer-p win)
           (not (minibuffer-window-active-p win))
           (mark)))
    (isearch-yank-x-selection)
  (let* ((overriding-terminal-local-map nil)
         (binding (key-binding (this-command-keys-vector) t)))
    (when (functionp binding) (call-interactively binding))))

bug#23007: 24.5; buggy interactive search with middle click

[npostavs]

Drew Adams <drew.adams@oracle.com> writes:

> Clearly I, like you, meant that this let is for only the second
> `if' clause.

Sorry for not being clear, but that's not what I meant:
`overriding-terminal-local-map' must be bound for the `key-binding'
call, and not for the `call-interactively' binding call.  See Alex's
explanation in message #8 and its attached patch.

bug#23007: 24.5; buggy interactive search with middle click

[Drew Adams]

> Sorry for not being clear, but that's not what I meant:
> `overriding-terminal-local-map' must be bound for the `key-binding'
> call, and not for the `call-interactively' binding call.  See Alex's
> explanation in message #8 and its attached patch.

My bad.  Sorry for the noise.

bug#23007: 24.5; buggy interactive search with middle click

[Alex]

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

npostavs@users.sourceforge.net writes:

> Alex <agrambot@gmail.com> writes:
>
>> +      (let ((overriding-terminal-local-map nil))
>> +        (setq binding (key-binding (this-command-keys-vector) t)))
>
> IMO, it would be better style to do something like
>
> (let ((binding (let ((overriding-terminal-local-map nil))
>                  (key-binding (this-command-keys-vector) t))))
>   ...)

That was my first idea, but at the time I thought it was better style to
use the setq version instead. :)

Your version makes the binding more obvious, though, so I updated the
patch to use it. I've attached it below.

PS: I don't immediately see a reason to check for the (mark) in the
condition. I tested by deactivating the mark explicitly, and setting
mark-even-if-inactive to nil in the window, but it still worked
correctly.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch v2 --]
[-- Type: text/x-diff, Size: 1551 bytes --]

From be6236ea77920a123a30b6fb3b4d52602d4d9097 Mon Sep 17 00:00:00 2001
From: Alexander Gramiak <agrambot@gmail.com>
Date: Mon, 29 May 2017 13:43:23 -0600
Subject: [PATCH] Limit scope of local overriding-terminal-local-map

The function `binding' may call isearch-done, which globally sets
overriding-terminal-local-map to nil (Bug#23007).
* lisp/isearch.el (isearch-mouse-2): Don't bind
overriding-terminal-local-map around the call to `binding'.
---
 lisp/isearch.el | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lisp/isearch.el b/lisp/isearch.el
index c34739d638..0d50c9dd9a 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -2036,14 +2036,14 @@ isearch-mouse-2
 Otherwise invoke whatever the calling mouse-2 command sequence
 is bound to outside of Isearch."
   (interactive "e")
-  (let* ((w (posn-window (event-start click)))
-	 (overriding-terminal-local-map nil)
-	 (binding (key-binding (this-command-keys-vector) t)))
+  (let ((w (posn-window (event-start click)))
+        (binding (let ((overriding-terminal-local-map nil))
+                   (key-binding (this-command-keys-vector) t))))
     (if (and (window-minibuffer-p w)
-	     (not (minibuffer-window-active-p w))) ; in echo area
-	(isearch-yank-x-selection)
+             (not (minibuffer-window-active-p w))) ; in echo area
+        (isearch-yank-x-selection)
       (when (functionp binding)
-	(call-interactively binding)))))
+        (call-interactively binding)))))
 
 (declare-function xterm--pasted-text "term/xterm" ())
 
-- 
2.11.0

bug#23007: 24.5; buggy interactive search with middle click

[Drew Adams]

> PS: I don't immediately see a reason to check for the (mark) in the
> condition. I tested by deactivating the mark explicitly, and setting
> mark-even-if-inactive to nil in the window, but it still worked
> correctly.

You probably don't need it.  Sorry if I confused things, again.

[I use it because my code needs to work also in older Emacs versions.
So it first calls `gui-set-selection' to set the primary selection
on the region (when there is a mark).  And it `deactivates-mark'
before then calling `isearch-yank-x-selection'.]

bug#23007: 24.5; buggy interactive search with middle click

[npostavs]

Alex <agrambot@gmail.com> writes:

> npostavs@users.sourceforge.net writes:
>
>> Alex <agrambot@gmail.com> writes:
>>
>>> +      (let ((overriding-terminal-local-map nil))
>>> +        (setq binding (key-binding (this-command-keys-vector) t)))
>>
>> IMO, it would be better style to do something like
>>
>> (let ((binding (let ((overriding-terminal-local-map nil))
>>                  (key-binding (this-command-keys-vector) t))))
>>   ...)
>
> That was my first idea, but at the time I thought it was better style to
> use the setq version instead. :)

Hah, well it is somewhat a question of personal opinion, but generally I
try to avoid setq, if it's not too much trouble.

> Your version makes the binding more obvious, though, so I updated the
> patch to use it. I've attached it below.

Ok, I'll push to master in a couple of days.

bug#23007: 24.5; buggy interactive search with middle click

[npostavs]

tags 23007 fixed
close 23007 26.1
quit

Pushed to master [1: 404273aeac].

[1: 404273aeac]: 2017-06-01 17:56:14 -0400
  Limit scope of local overriding-terminal-local-map
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=404273aeacba39833ae3a38ce6764cc7a636e9d9