bug#24030: 25.0.95; mouse-drag-region regression

bug#24030: 25.0.95; mouse-drag-region regression

[Alex]

If you attempt to create a region by dragging the mouse in a
never-before-focused window, then the region isn't shown until letting
go of the mouse button.

Steps to reproduce:

emacs -Q
C-h f help RET
Click and drag the mouse in the help window

This doesn't work in the pretest, but it did in Emacs 24.5.


In GNU Emacs 25.0.95.1 (x86_64-redhat-linux-gnu, GTK+ Version 3.20.6)
 of 2016-07-12 built on buildvm-07.phx2.fedoraproject.org
Windowing system distributor 'Fedora Project', version 11.0.11803000
Configured using:
 'configure --build=x86_64-redhat-linux-gnu
 --host=x86_64-redhat-linux-gnu --program-prefix=
 --disable-dependency-tracking --prefix=/usr --exec-prefix=/usr
 --bindir=/usr/bin --sbindir=/usr/sbin --sysconfdir=/etc
 --datadir=/usr/share --includedir=/usr/include --libdir=/usr/lib64
 --libexecdir=/usr/libexec --localstatedir=/var
 --sharedstatedir=/var/lib --mandir=/usr/share/man
 --infodir=/usr/share/info --with-dbus --with-gif --with-jpeg --with-png
 --with-rsvg --with-tiff --with-xft --with-xpm --with-x-toolkit=gtk3
 --with-gpm=no --with-xwidgets build_alias=x86_64-redhat-linux-gnu
 host_alias=x86_64-redhat-linux-gnu 'CFLAGS=-DMAIL_USE_LOCKF -O2 -g
 -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2
 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4
 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1
 -m64 -mtune=generic' LDFLAGS=-Wl,-z,relro
 PKG_CONFIG_PATH=:/usr/lib64/pkgconfig:/usr/share/pkgconfig'

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND DBUS GCONF GSETTINGS NOTIFY
ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 XWIDGETS

Important settings:
  value of $LC_CTYPE: en_CA.utf8
  value of $LANG: en_CA.utf8
  value of $XMODIFIERS: @im=none
  locale-coding-system: utf-8-unix

Major mode: Help

bug#24030: 25.0.95; mouse-drag-region regression

[Eli Zaretskii]

> From: Alex <agrambot@gmail.com>
> Date: Tue, 19 Jul 2016 16:11:48 -0600
> 
> If you attempt to create a region by dragging the mouse in a
> never-before-focused window, then the region isn't shown until letting
> go of the mouse button.

More accurately, it's not the window that was never the selected one,
it's the buffer that is displayed in it.  To see that, after selecting
the window, switch back to the other window (showing *scratch* in the
recipe), then type the same C-h f command again, and you will see the
problem, even though the window was already selected.  That's because
each C-h f erases and fills up the *Help* buffer again.

AFAICT, the root cause is the fact that region-active-p returns nil
under the specific circumstances of the recipe.  Why it returns nil is
less clear; AFAICS it's because mark-active is nil after the first
click of the mouse, when redisplay--update-region-highlight is called
from the display engine.  And mark-active is nil because some code
that I was unable to track down sets deactivate-mark to non-nil, so
the command loop deactivates the mark.

I hope someone else will be able to pick up where I left off, and find
the offending code.

> This doesn't work in the pretest, but it did in Emacs 24.5.

Right.

Btw, debugging such core features which are implemented in Lisp that
is called from the display engine is a nightmare, because there's no
easy way of displaying values of key Lisp variables, for the obvious
reasons.  My conclusion is that such refactoring brings significant
maintenance headaches, which need to be carefully considered as part
of similar decisions in the future.  If someone knows of handy
techniques to make this easier, I'd love to hear about them.

Thanks.

bug#24030: 25.0.95; mouse-drag-region regression

[Alex]

Eli Zaretskii <eliz@gnu.org> writes:

> More accurately, it's not the window that was never the selected one,
> it's the buffer that is displayed in it.

Ah, I see.

> And mark-active is nil because some code
> that I was unable to track down sets deactivate-mark to non-nil, so
> the command loop deactivates the mark.

It appears that it's somewhere within select-window or something it
calls. After posn-set-point calls select-window in 24.5, deactivate-mark
is nil -- but it's t in 25.1.

#+BEGIN_SRC elisp
(defun test ()
  (message "before: %s" deactivate-mark)
  (select-window (cadr (window-list)))
  (message "after: %s" deactivate-mark))
#+END_SRC

In the case where the target window contains the new help buffer,
deactivate-mark will be nil in 24.5 and t in 25.1.
> Thanks.

bug#24030: 25.0.95; mouse-drag-region regression

[Eli Zaretskii]

> From: Alex <agrambot@gmail.com>
> Cc: 24030@debbugs.gnu.org
> Date: Fri, 22 Jul 2016 00:09:22 -0600
> 
> > And mark-active is nil because some code
> > that I was unable to track down sets deactivate-mark to non-nil, so
> > the command loop deactivates the mark.
> 
> It appears that it's somewhere within select-window or something it
> calls. After posn-set-point calls select-window in 24.5, deactivate-mark
> is nil -- but it's t in 25.1.
> 
> #+BEGIN_SRC elisp
> (defun test ()
>   (message "before: %s" deactivate-mark)
>   (select-window (cadr (window-list)))
>   (message "after: %s" deactivate-mark))
> #+END_SRC
> 
> In the case where the target window contains the new help buffer,
> deactivate-mark will be nil in 24.5 and t in 25.1.

Thanks.

The reason for the above is that in Emacs 25 deactivate-mark was made
a variable that becomes buffer-local when set.  And inserting text
into a buffer, which happens when describe-function inserts the
documentation into *Help*, was made to set the this variable in a way
that creates a buffer-local binding for it.  That is why select-window
gives deactivate-mark a non-nil value: select-window makes the
window's buffer the current one, which assigns buffer-local values to
all of it variables, including deactivate-mark.  Then this
buffer-local value is being examined by mouse-drag-region.

I think the problem is that the command loop fails to reset the
buffer-local value of this variable, so we must add something to
keyboard.c, where the global value is reset after each command.

I'm CC'ing Stefan, who made the above-mentioned changes, in the hope
that he will have some ideas for how to fix this regression.

bug#24030: 25.0.95; mouse-drag-region regression

[Stefan Monnier]

> that creates a buffer-local binding for it.  That is why select-window
> gives deactivate-mark a non-nil value: select-window makes the
> window's buffer the current one, which assigns buffer-local values to
> all of it variables, including deactivate-mark.  Then this
> buffer-local value is being examined by mouse-drag-region.

Indeed, this is a problem here because of have a "stale" setting of
deactivate-mark.

Maybe something like the patch below will do?

Another option is to make the deactivate-mark function reset
deactivate-mark to nil (which would seem to make a lot of sense in
itself) and then to call deactivate-mark at that point or to move the
earlier deactivate-mark to after mouse-set-point.

Of course, maybe there should be a more thorough handling of stale
deactivate-mark settings.  IOW change all places that set
deactivate-mark to non-nil so they also record the affected buffer and
then change the command loop so that it calls deactivate-mark in all
those buffers where deactivate-mark was set as non-nil.


        Stefan


diff --git a/lisp/mouse.el b/lisp/mouse.el
index 27f2acb..6175d77 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -811,6 +811,7 @@ mouse-drag-track
          ;; window, now let's jump to the place of the event, where things
          ;; are happening.
          (_ (mouse-set-point start-event))
+         (_ (kill-local-variable 'deactivate-mark))
          (echo-keystrokes 0)
 	 (start-posn (event-start start-event))
 	 (start-point (posn-point start-posn))

bug#24030: 25.0.95; mouse-drag-region regression

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Alex <agrambot@gmail.com>,  24030@debbugs.gnu.org
> Date: Sat, 23 Jul 2016 13:17:09 -0400
> 
> > that creates a buffer-local binding for it.  That is why select-window
> > gives deactivate-mark a non-nil value: select-window makes the
> > window's buffer the current one, which assigns buffer-local values to
> > all of it variables, including deactivate-mark.  Then this
> > buffer-local value is being examined by mouse-drag-region.
> 
> Indeed, this is a problem here because of have a "stale" setting of
> deactivate-mark.

How did it become stale, though?  It was supposed to be reset by the
command loop when the "C-h f" command returned.  Why wasn't it?

bug#24030: 25.0.95; mouse-drag-region regression

[Stefan Monnier]

> How did it become stale, though?  It was supposed to be reset by the
> command loop when the "C-h f" command returned.  Why wasn't it?

Because it wasn't the current-buffer when the command ended.


        Stefan

bug#24030: 25.0.95; mouse-drag-region regression

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: agrambot@gmail.com,  24030@debbugs.gnu.org
> Date: Sat, 23 Jul 2016 17:16:37 -0400
> 
> > that creates a buffer-local binding for it.  That is why select-window
> > gives deactivate-mark a non-nil value: select-window makes the
> > window's buffer the current one, which assigns buffer-local values to
> > all of it variables, including deactivate-mark.  Then this
> > buffer-local value is being examined by mouse-drag-region.
> 
> Indeed, this is a problem here because of have a "stale" setting of
> deactivate-mark.
> 
> Maybe something like the patch below will do?

It didn't solve the problem here.  Did it work for you?

> Another option is to make the deactivate-mark function reset
> deactivate-mark to nil (which would seem to make a lot of sense in
> itself) and then to call deactivate-mark at that point or to move the
> earlier deactivate-mark to after mouse-set-point.

Neither of these seems to solve the problem.

What do you think about the alternative patch below?  It does seem to
solve the problem here.

> Of course, maybe there should be a more thorough handling of stale
> deactivate-mark settings.  IOW change all places that set
> deactivate-mark to non-nil so they also record the affected buffer and
> then change the command loop so that it calls deactivate-mark in all
> those buffers where deactivate-mark was set as non-nil.

I envision complications with this when recursive-edit is used.

> > How did it become stale, though?  It was supposed to be reset by the
> > command loop when the "C-h f" command returned.  Why wasn't it?
> 
> Because it wasn't the current-buffer when the command ended.

But that means the whole handling of deactivate-mark is now quite
fragile, isn't it?  A command can change the current buffer any number
of times, touching several buffers, and change back before it returns
to the command loop, and Emacs will be none the wiser.

Here's the proposed patch:

--- lisp/mouse.el~	2016-07-20 09:52:16.559875700 +0300
+++ lisp/mouse.el	2016-07-24 17:14:34.469052800 +0300
@@ -815,14 +815,16 @@ The region will be defined with mark and
   (setq mouse-selection-click-count-buffer (current-buffer))
   (deactivate-mark)
   (let* ((scroll-margin 0) ; Avoid margin scrolling (Bug#9541).
+	 (start-posn (event-start start-event))
+	 (start-point (posn-point start-posn))
+	 (start-window (posn-window start-posn))
+	 (_ (with-selected-window start-window
+	      (setq deactivate-mark nil)))
          ;; We've recorded what we needed from the current buffer and
          ;; window, now let's jump to the place of the event, where things
          ;; are happening.
          (_ (mouse-set-point start-event))
          (echo-keystrokes 0)
-	 (start-posn (event-start start-event))
-	 (start-point (posn-point start-posn))
-	 (start-window (posn-window start-posn))
 	 (bounds (window-edges start-window))
 	 (make-cursor-line-fully-visible nil)
 	 (top (nth 1 bounds))

bug#24030: 25.0.95; mouse-drag-region regression

[Stefan Monnier]

> --- lisp/mouse.el~	2016-07-20 09:52:16.559875700 +0300
> +++ lisp/mouse.el	2016-07-24 17:14:34.469052800 +0300
> @@ -815,14 +815,16 @@ The region will be defined with mark and
>    (setq mouse-selection-click-count-buffer (current-buffer))
>    (deactivate-mark)
>    (let* ((scroll-margin 0) ; Avoid margin scrolling (Bug#9541).
> +	 (start-posn (event-start start-event))
> +	 (start-point (posn-point start-posn))
> +	 (start-window (posn-window start-posn))
> +	 (_ (with-selected-window start-window
> +	      (setq deactivate-mark nil)))
>           ;; We've recorded what we needed from the current buffer and
>           ;; window, now let's jump to the place of the event, where things
>           ;; are happening.
>           (_ (mouse-set-point start-event))
>           (echo-keystrokes 0)
> -	 (start-posn (event-start start-event))
> -	 (start-point (posn-point start-posn))
> -	 (start-window (posn-window start-posn))
>  	 (bounds (window-edges start-window))
>  	 (make-cursor-line-fully-visible nil)
>  	 (top (nth 1 bounds))

Looks fine to me, tho I don't think we should use with-selected-window
here, but only with-current-buffer.


        Stefan

bug#24030: 25.0.95; mouse-drag-region regression

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: agrambot@gmail.com,  24030@debbugs.gnu.org
> Date: Sun, 24 Jul 2016 11:02:38 -0400
> 
> Looks fine to me, tho I don't think we should use with-selected-window
> here, but only with-current-buffer.

How is the latter different in this particular context?

bug#24030: 25.0.95; mouse-drag-region regression

[Stefan Monnier]

>> Looks fine to me, tho I don't think we should use with-selected-window
>> here, but only with-current-buffer.
> How is the latter different in this particular context?

The end result will probably be very similar in practice, but if you
look at the definition of with-selected-window and of all the functions
it calls, you'll see that with-current-buffer is super-extra lean
in comparison.  And in the current situation all we care about is to set
the buffer-local value of a variable, so the window is of no importance
(other than to find the relevant buffer).


        Stefan

bug#24030: 25.0.95; mouse-drag-region regression

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: agrambot@gmail.com,  24030@debbugs.gnu.org
> Date: Sun, 24 Jul 2016 16:16:01 -0400
> 
> >> Looks fine to me, tho I don't think we should use with-selected-window
> >> here, but only with-current-buffer.
> > How is the latter different in this particular context?
> 
> The end result will probably be very similar in practice, but if you
> look at the definition of with-selected-window and of all the functions
> it calls, you'll see that with-current-buffer is super-extra lean
> in comparison.  And in the current situation all we care about is to set
> the buffer-local value of a variable, so the window is of no importance
> (other than to find the relevant buffer).

Makes sense, thanks.  Modified to use with-current-buffer and pushed
to master.

bug#24030: 25.0.95; mouse-drag-region regression

[Alex]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> Cc: agrambot@gmail.com,  24030@debbugs.gnu.org
>> Date: Sat, 23 Jul 2016 17:16:37 -0400
>> 
>> > that creates a buffer-local binding for it.  That is why select-window
>> > gives deactivate-mark a non-nil value: select-window makes the
>> > window's buffer the current one, which assigns buffer-local values to
>> > all of it variables, including deactivate-mark.  Then this
>> > buffer-local value is being examined by mouse-drag-region.
>> 
>> Indeed, this is a problem here because of have a "stale" setting of
>> deactivate-mark.
>> 
>> Maybe something like the patch below will do?
>
> It didn't solve the problem here.  Did it work for you?

FWIW both of the patches worked for me.