bug#10022: 24.0.91; `isearch-mouse-2' relies on `x-get-selection' - NG for Windows etc.

bug#10022: 24.0.91; `isearch-mouse-2' relies on `x-get-selection' - NG for Windows etc.

[Drew Adams]

No, I am not certain this is a bug - you decide.
 
In `isearch-mode-map', `isearch-mouse-2' is bound to `mouse-2' and
`down-mouse-2' is bound to nil.

`isearch-mouse-2':
 
(defun isearch-mouse-2 (click)
  "Handle mouse-2 in Isearch mode.
For a click in the echo area, invoke `isearch-yank-x-selection'.
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)))
    (if (and (window-minibuffer-p w)
      (not (minibuffer-window-active-p w))) ; in echo area
 (isearch-yank-x-selection)
      (when (functionp binding)
 (call-interactively binding)))))
 
I globally bind `down-mouse-2' to a command of mine,
`mouse-flash-position-or-M-x', which highlights the yank position (or
calls `M-x' in the echo area).  The highlighting intentionally happens
only while the mouse button is pressed.
 
Because of this global binding, I need to change the `isearch-mode-map'
binding of `down-mouse-2' to `ignore'.  Dunno whether a binding of
`ignore' makes better sense for vanilla Emacs here too (e.g., in case
someone binds `down-mouse-2' globally).
 
However, that is not sufficient, to be able to use `mouse-2' with
Isearch.  Perhaps because of some selection setting I have, with my
setup (on MS Windows) `x-get-selection' just returns nil.
 
That means that `isearch-mouse-2' barfs when it calls
`isearch-yank-selection'.  That function tries to yank the string
returned by `x-get-selection', but that function returns nil, not a
string.  The PRIMARY selection is not set, at least in my context.
This function apparently depend ons it being set.  Seems like a
bug, to me.
 
To fix this, I use a hack like this in `isearch-mouse-2'.  Dunno
what the right fix for vanilla Emacs would be.
 
(when (and transient-mark-mode (/= (region-beginning) (region-end)))
  (x-set-selection 'PRIMARY (buffer-substring-no-properties 
                             (region-beginning) (region-end)))
  (deactivate-mark)
  (isearch-yank-x-selection))
 
Consider this only an FYI.  I suspect there is a problem, and that the
`isearch-mouse-2' code should not depend on the PRIMARY selection being
set, but you decide.

In GNU Emacs 24.0.91.1 (i386-mingw-nt5.1.2600) of 2011-11-07 on MARVIN
 Windowing system distributor `Microsoft Corp.', version 5.1.2600
 configured using `configure --with-gcc (4.6) --no-opt --cflags
 -I"D:/devel/emacs/libs/libXpm-3.5.8/include"
 -I"D:/devel/emacs/libs/libXpm-3.5.8/src"
 -I"D:/devel/emacs/libs/libpng-dev_1.4.3-1/include"
 -I"D:/devel/emacs/libs/zlib-dev_1.2.5-2/include"
 -I"D:/devel/emacs/libs/giflib-4.1.4-1/include"
 -I"D:/devel/emacs/libs/jpeg-6b-4/include"
 -I"D:/devel/emacs/libs/tiff-3.8.2-1/include"
 -I"D:/devel/emacs/libs/gnutls-2.10.1/include" --ldflags
 -L"D:/devel/emacs/libs/gnutls-2.10.1/lib"'
 

bug#10022: 24.0.91; `isearch-mouse-2' relies on `x-get-selection' - NG for Windows etc.

[Stefan Monnier]

> No, I am not certain this is a bug - you decide.
> In `isearch-mode-map', `isearch-mouse-2' is bound to `mouse-2' and
> `down-mouse-2' is bound to nil.

You only describe the problems you see in the code, but I can't find
a description of the actual problem you bumped into.


        Stefan

bug#10022: 24.0.91; `isearch-mouse-2' relies on `x-get-selection' - NG for Windows etc.

[Drew Adams]

> You only describe the problems you see in the code, but I can't find
> a description of the actual problem you bumped into.

x-get-selection returning nil.
isearch-yank-x-selection trying to yank that (nil).

bug#10022: 24.0.91; `isearch-mouse-2' relies on `x-get-selection' - NG for Windows etc.

[Stefan Monnier]

>> You only describe the problems you see in the code, but I can't find
>> a description of the actual problem you bumped into.
> x-get-selection returning nil.
> isearch-yank-x-selection trying to yank that (nil).

Steps to reproduce it, please.


        Stefan

bug#10022: 24.0.91; `isearch-mouse-2' relies on `x-get-selection' - NG for Windows etc.

[Drew Adams]

> Steps to reproduce it, please.

As I said in the OP:

 "No, I am not certain this is a bug - you decide.
  ...
  Consider this only an FYI."

In case it helps, digging into this more shows that setting the X selection
explicitly is necessary for the code to work also with older Emacs versions.
That is not necessary for Emacs 24.

FWIW, I use this, which gives users the option to not have to move the mouse to
the echo area to yank the selection.  Option `isearchp-mouse-2-flag' is t by
default.

(defun isearch-mouse-2 (click)
  "Handle `mouse-2' in Isearch mode.
If `isearchp-mouse-2-flag' is non-nil, yank the X selection.
If `isearchp-mouse-2-flag' is nil, yank it only if the `mouse-2' click
is in the echo area.  Otherwise, invoke whatever `mouse-2' is bound to
outside of Isearch."
  (interactive "e")
  (if (not isearchp-mouse-2-flag)
      (let ((win  (posn-window (event-start click)))
            (overriding-terminal-local-map  nil)
            (binding
             (key-binding (this-command-keys-vector) t)))
        (if (and (window-minibuffer-p win)
                 (not (minibuffer-window-active-p win))) ; In echo area
            (isearchp-set-sel-and-yank)
          (when (functionp binding) (call-interactively binding))))
    (when (/= (region-beginning) (region-end))
      (isearchp-set-sel-and-yank))))

(defun isearchp-set-sel-and-yank ()
  "Set X selection and yank it into echo area."
  (x-set-selection 'PRIMARY (buffer-substring-no-properties
                             (region-beginning) (region-end)))
  (deactivate-mark)
  (isearch-yank-x-selection))

If it were not for needing the code to work with older Emacs versions, all that
would be needed would be this:

(defun isearch-mouse-2 (click)
  "..."
  (interactive "e")
  (if (not isearchp-mouse-2-flag)
      (let ((win  (posn-window (event-start click)))
            (overriding-terminal-local-map  nil)
            (binding
             (key-binding (this-command-keys-vector) t)))
        (if (and (window-minibuffer-p win)
                 (not (minibuffer-window-active-p win))) ; In echo area
            (isearch-yank-x-selection)
          (when (functionp binding) (call-interactively binding))))
    (when (/= (region-beginning) (region-end))
      (let ((select-active-regions  t))
        (deactivate-mark)
        (isearch-yank-x-selection)))))

The binding of `select-active-region' is needed, however.  Without that (and
with a customized setting of nil), `get-x-selection' returns nil - the problem I
mentioned earlier.

HTH.

bug#10022: 24.0.91; `isearch-mouse-2' relies on `x-get-selection' - NG for Windows etc.

[Stefan Monnier]

> Because of this global binding, I need to change the `isearch-mode-map'
> binding of `down-mouse-2' to `ignore'.  Dunno whether a binding of
> `ignore' makes better sense for vanilla Emacs here too (e.g., in case
> someone binds `down-mouse-2' globally).

To the extent that isearch only binds mouse-2 in the minibuffer (yes,
it technically also binds it in the main buffer, but to a command that
just delegates to the normal binding), I don't think down-mouse-2 should
default to `ignore', except maybe in the minibuffer.

> That means that `isearch-mouse-2' barfs when it calls
> `isearch-yank-selection'.  That function tries to yank the string
> returned by `x-get-selection', but that function returns nil, not a
> string.  The PRIMARY selection is not set, at least in my context.
> This function apparently depend ons it being set.  Seems like a
> bug, to me.

AFAIK it's called "isearch-yank-x-selection", so it really doesn't seem
like a bug for it to fail when there is no GUI selection (yes, the
choice of "x" to mean "GUI" is unfortunate, but as you know it's
a widespread problem in Emacs).

> (when (and transient-mark-mode (/= (region-beginning) (region-end)))
>   (x-set-selection 'PRIMARY (buffer-substring-no-properties
>                              (region-beginning) (region-end)))
>   (deactivate-mark)
>   (isearch-yank-x-selection))

That presumes the region was meant as a "selection".  I don't see why
isearch should make such an assumption.

> Consider this only an FYI.  I suspect there is a problem, and that the
> `isearch-mouse-2' code should not depend on the PRIMARY selection being
> set, but you decide.

IIUC the intention is for it to perform a task similar to `yank', so it
needs to take the string from some kind of GUI selection, or from the
kill-ring, but taking it straight from the region seems a bit much.

> In case it helps, digging into this more shows that setting the X selection
> explicitly is necessary for the code to work also with older Emacs versions.
> That is not necessary for Emacs 24.

Aha!

> (defun isearch-mouse-2 (click)
>   "..."
>   (interactive "e")
>   (if (not isearchp-mouse-2-flag)
>       (let ((win  (posn-window (event-start click)))
>             (overriding-terminal-local-map  nil)
>             (binding
>              (key-binding (this-command-keys-vector) t)))
>         (if (and (window-minibuffer-p win)
>                  (not (minibuffer-window-active-p win))) ; In echo area
>             (isearch-yank-x-selection)
>           (when (functionp binding) (call-interactively binding))))
>     (when (/= (region-beginning) (region-end))
>       (let ((select-active-regions  t))
>         (deactivate-mark)
>         (isearch-yank-x-selection)))))

> The binding of `select-active-region' is needed, however.
> Without that (and with a customized setting of nil), `get-x-selection'
> returns nil - the problem I mentioned earlier.

But that overrides an explicit request from the user (in her .emacs) to
not automatically consider the region as a selection.
So, while it might make sense when coupled with a new option such as
isearchp-mouse-2-flag, I don't see what Emacs-24.1 could do to help.


        Stefan

bug#10022: 24.0.91; `isearch-mouse-2' relies on `x-get-selection' - NG for Windows etc.

[Drew Adams]

> > Dunno whether a binding of `ignore' makes better sense for
> > vanilla Emacs here too (e.g., in case someone binds
> > `down-mouse-2' globally).
> 
> To the extent that isearch only binds mouse-2 in the minibuffer (yes,
> it technically also binds it in the main buffer, but to a command that
> just delegates to the normal binding), I don't think 
> down-mouse-2 should default to `ignore', except maybe in the minibuffer.

What I meant was that a nil binding means pick up any currently effective (e.g.
global) binding.  If someone binds `down-mouse-2' globally (as I do) then that
binding will be picked for Isearch also, and it might not be appropriate there.

The question is whether you want Isearch to ignore a `down-mouse-2' action or to
pick up any `down-mouse-2' action that might be defined (e.g. globally).  IOW,
should Isearch decide that it wants to impose the `down-mouse-2' behavior or
should it let whatever behavior is defined elsewhere carry over to Isearch as
well.

Your call.

> > That means that `isearch-mouse-2' barfs when it calls
> > `isearch-yank-selection'.  That function tries to yank the string
> > returned by `x-get-selection', but that function returns nil, not a
> > string.  The PRIMARY selection is not set, at least in my context.
> > This function apparently depend ons it being set.  Seems like a
> > bug, to me.
> 
> AFAIK it's called "isearch-yank-x-selection",

Yes, that's what I meant.

> so it really doesn't seem like a bug for it to fail when there is
> no GUI selection

The bug would presumably be that `x-get-selection' returns nil here, but I would
suppose that `isearch-yank-x-selection' might want to handle such an eventuality
more gracefully.  Anyway, there does not seem to be a problem for Emacs 24 -
only older versions.

> > (when (and transient-mark-mode (/= (region-beginning) (region-end)))
> >   (x-set-selection 'PRIMARY (buffer-substring-no-properties
> >                              (region-beginning) (region-end)))
> >   (deactivate-mark)
> >   (isearch-yank-x-selection))
> 
> That presumes the region was meant as a "selection".  I don't see why
> isearch should make such an assumption.

I gave a later version that does not use that - see the definition of
`isearch-mouse-2' I sent.  The part that corresponds to the ordinary, vanilla
behavior is identical to the vanilla Emacs code.  The part that uses the region
as a selection is per user choice and is a separate feature: letting you click
mouse-2 anywhere (not necessarily in the echo area), to insert the region text.

> > Consider this only an FYI.  I suspect there is a problem, 
> > and that the `isearch-mouse-2' code should not depend on the PRIMARY 
> > selection being set, but you decide.
> 
> IIUC the intention is for it to perform a task similar to 
> `yank', so it needs to take the string from some kind of GUI
> selection, or from the kill-ring, but taking it straight from the
> region seems a bit much.

Agreed.  You misunderstood.  Taking it from the region is for a separate,
alternative behavior (user choice).

> > The binding of `select-active-region' is needed, however.
> > Without that (and with a customized setting of nil), 
> > `get-x-selection' returns nil - the problem I mentioned earlier.
> 
> But that overrides an explicit request from the user (in her 
> .emacs) to not automatically consider the region as a selection.

Again, this is per a user choice, via option `isearchp-mouse-2-flag'.  The
intent of such a choice is precisely to use the region as a selection here (and
here only).  Again, this is outside what vanilla Emacs does or should do.

> So, while it might make sense when coupled with a new option such as
> isearchp-mouse-2-flag, I don't see what Emacs-24.1 could do to help.

Agreed.  There is no change needed for vanilla Emacs 24.  The only changes I
have made are (a) for a non-nil option `isearchp-mouse-2-flag' (yank the
non-empty region) and (b) for older Emacs versions (ensure the primary selection
is set, so it can be yanked).

This is why this is all only FYI (except perhaps for the `down-mouse-2' binding
question - your call).

bug#10022: 24.0.91; `isearch-mouse-2' relies on `x-get-selection' - NG for Windows etc.

[Stefan Monnier]

>> To the extent that isearch only binds mouse-2 in the minibuffer (yes,
>> it technically also binds it in the main buffer, but to a command that
>> just delegates to the normal binding), I don't think 
>> down-mouse-2 should default to `ignore', except maybe in the minibuffer.

> What I meant was that a nil binding means pick up any currently
> effective (e.g.  global) binding.  If someone binds `down-mouse-2'
> globally (as I do) then that binding will be picked for Isearch also,
> and it might not be appropriate there.

But as I point out, the mouse-2 binding also runs the global binding
(tho indirectly, and except when in the minibuffer area), so running the
global binding of down-mouse-2 seems like the right thing to do.

>> so it really doesn't seem like a bug for it to fail when there is
>> no GUI selection
> The bug would presumably be that `x-get-selection' returns nil here,

?? What should it return if there's no GUI selection?

> Agreed.  There is no change needed for vanilla Emacs 24.

OK, thanks.

> The only changes I have made are (a) for a non-nil option
> `isearchp-mouse-2-flag' (yank the non-empty region)

As mentioned on gnu.emacs.help (which may not have gotten to
help-gnu-emacs@gnu.org :-( ), users should already be able to get such
behavior with

  (define-key isearch-mode-map [mouse-2] 'isearch-yank-x-selection)

but this wouldn't override select-active-region (which is a separate
issue, although your isearchp-mouse-2-flag controls both).


        Stefan