[PATCH v3 1/3] Improve interaction between mouse-drag-region and scroll-margin

[PATCH v3 1/3] Improve interaction between mouse-drag-region and scroll-margin

[Yuri D'Elia]

* lisp/mouse.el (mouse-drag-track): Disable both scroll-margin and
auto-hscroll-mode in mouse-drag-region and do not re-enable them until
dragging is over, making selections work as expected when inside the
margins.

Since mouse-drag-region is initiated on the initial down-mouse event, we
_shouldn't_ scroll the point until up-mouse is finally received, as it
would otherwise interfere with the regular mouse-1 event. If the point
is moved on down-mouse an immediate motion event results, jump-starting
the drag.

Complicating things, as the tracking is done in a transient map, a
let-bound scroll-margin is not sufficient. We must save/restore its
value (as similarly done already for auto-hscroll-mode).

The point will still moved according to scroll-margin due to mouse-1,
but now it's done only when releasing the mouse button. Simply clicking
inside the scroll-margin region now behaves as expected.

This partially addresses Bug#42150 (previous discussion at Bug#9541).

Clicking inside the scroll-margin region still occasionally causes some
regions to be selected, although it doesn't cause the buffer to scroll
uncontrollably anymore.
---
 lisp/mouse.el | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/lisp/mouse.el b/lisp/mouse.el
index 9f1417f42..a6c999594 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -1578,8 +1578,7 @@ mouse-drag-track
   (mouse-minibuffer-check start-event)
   (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))
+  (let* ((start-posn (event-start start-event))
 	 (start-point (posn-point start-posn))
 	 (start-window (posn-window start-posn))
 	 (_ (with-current-buffer (window-buffer start-window)
@@ -1601,12 +1600,20 @@ mouse-drag-track
 		   ;; Don't count the mode line.
 		   (1- (nth 3 bounds))))
 	 (click-count (1- (event-click-count start-event)))
-	 ;; Suppress automatic hscrolling, because that is a nuisance
-	 ;; when setting point near the right fringe (but see below).
+         ;; Save original automatic scrolling behavior (see below).
 	 (auto-hscroll-mode-saved auto-hscroll-mode)
+	 (scroll-margin-saved scroll-margin)
          (old-track-mouse track-mouse))
 
     (setq mouse-selection-click-count click-count)
+
+    ;; Suppress automatic scrolling near the edges while tracking
+    ;; movement, as it interferes with the natural dragging behavior
+    ;; (point will unexpectedly be moved beneath the pointer, making
+    ;; selections in auto-scrolling margins impossible).
+    (setq auto-hscroll-mode nil)
+    (setq scroll-margin 0)
+
     ;; In case the down click is in the middle of some intangible text,
     ;; use the end of that text, and put it in START-POINT.
     (if (< (point) start-point)
@@ -1625,7 +1632,6 @@ mouse-drag-track
 
     (setf (terminal-parameter nil 'mouse-drag-start) start-event)
     (setq track-mouse t)
-    (setq auto-hscroll-mode nil)
 
     (set-transient-map
      (let ((map (make-sparse-keymap)))
@@ -1636,8 +1642,6 @@ mouse-drag-track
            (let* ((end (event-end event))
                   (end-point (posn-point end)))
              (unless (eq end-point start-point)
-               ;; As soon as the user moves, we can re-enable auto-hscroll.
-               (setq auto-hscroll-mode auto-hscroll-mode-saved)
                ;; And remember that we have moved, so mouse-set-region can know
                ;; its event is really a drag event.
                (setcar start-event 'mouse-movement))
@@ -1658,6 +1662,7 @@ mouse-drag-track
      t (lambda ()
          (setq track-mouse old-track-mouse)
          (setq auto-hscroll-mode auto-hscroll-mode-saved)
+         (setq scroll-margin scroll-margin-saved)
          ;; Don't deactivate the mark when the context menu was invoked
          ;; by down-mouse-3 immediately after down-mouse-1 and without
          ;; releasing the mouse button with mouse-1. This allows to use
-- 
2.33.0

[PATCH v3 2/3] Perform cleanup on errors in mouse-drag-track

[Yuri D'Elia]

* lisp/mouse.el (mouse-drag-track): Correctly reset original values
changed during execution if errors occur.
---
 lisp/mouse.el | 134 ++++++++++++++++++++++++++------------------------
 1 file changed, 70 insertions(+), 64 deletions(-)

diff --git a/lisp/mouse.el b/lisp/mouse.el
index a6c999594..72251fdfa 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -1603,75 +1603,81 @@ mouse-drag-track
          ;; Save original automatic scrolling behavior (see below).
 	 (auto-hscroll-mode-saved auto-hscroll-mode)
 	 (scroll-margin-saved scroll-margin)
-         (old-track-mouse track-mouse))
+         (old-track-mouse track-mouse)
+         (cleanup (lambda ()
+                    (setq track-mouse old-track-mouse)
+                    (setq auto-hscroll-mode auto-hscroll-mode-saved)
+                    (setq scroll-margin scroll-margin-saved))))
+    (condition-case err
+        (progn
+          (setq mouse-selection-click-count click-count)
 
-    (setq mouse-selection-click-count click-count)
+          ;; Suppress automatic scrolling near the edges while tracking
+          ;; movement, as it interferes with the natural dragging behavior
+          ;; (point will unexpectedly be moved beneath the pointer, making
+          ;; selections in auto-scrolling margins impossible).
+          (setq auto-hscroll-mode nil)
+          (setq scroll-margin 0)
 
-    ;; Suppress automatic scrolling near the edges while tracking
-    ;; movement, as it interferes with the natural dragging behavior
-    ;; (point will unexpectedly be moved beneath the pointer, making
-    ;; selections in auto-scrolling margins impossible).
-    (setq auto-hscroll-mode nil)
-    (setq scroll-margin 0)
+          ;; In case the down click is in the middle of some intangible text,
+          ;; use the end of that text, and put it in START-POINT.
+          (if (< (point) start-point)
+	      (goto-char start-point))
+          (setq start-point (point))
 
-    ;; In case the down click is in the middle of some intangible text,
-    ;; use the end of that text, and put it in START-POINT.
-    (if (< (point) start-point)
-	(goto-char start-point))
-    (setq start-point (point))
+          ;; Activate the region, using `mouse-start-end' to determine where
+          ;; to put point and mark (e.g., double-click will select a word).
+          (setq-local transient-mark-mode
+                      (if (eq transient-mark-mode 'lambda)
+                          '(only)
+                        (cons 'only transient-mark-mode)))
+          (let ((range (mouse-start-end start-point start-point click-count)))
+            (push-mark (nth 0 range) t t)
+            (goto-char (nth 1 range)))
 
-    ;; Activate the region, using `mouse-start-end' to determine where
-    ;; to put point and mark (e.g., double-click will select a word).
-    (setq-local transient-mark-mode
-                (if (eq transient-mark-mode 'lambda)
-                    '(only)
-                  (cons 'only transient-mark-mode)))
-    (let ((range (mouse-start-end start-point start-point click-count)))
-      (push-mark (nth 0 range) t t)
-      (goto-char (nth 1 range)))
+          (setf (terminal-parameter nil 'mouse-drag-start) start-event)
+          (setq track-mouse t)
 
-    (setf (terminal-parameter nil 'mouse-drag-start) start-event)
-    (setq track-mouse t)
-
-    (set-transient-map
-     (let ((map (make-sparse-keymap)))
-       (define-key map [switch-frame] #'ignore)
-       (define-key map [select-window] #'ignore)
-       (define-key map [mouse-movement]
-         (lambda (event) (interactive "e")
-           (let* ((end (event-end event))
-                  (end-point (posn-point end)))
-             (unless (eq end-point start-point)
-               ;; And remember that we have moved, so mouse-set-region can know
-               ;; its event is really a drag event.
-               (setcar start-event 'mouse-movement))
-             (if (and (eq (posn-window end) start-window)
-                      (integer-or-marker-p end-point))
-                 (mouse--drag-set-mark-and-point start-point
-                                                 end-point click-count)
-               (let ((mouse-row (cdr (cdr (mouse-position)))))
-                 (cond
-                  ((null mouse-row))
-                  ((< mouse-row top)
-                   (mouse-scroll-subr start-window (- mouse-row top)
-                                      nil start-point))
-                  ((>= mouse-row bottom)
-                   (mouse-scroll-subr start-window (1+ (- mouse-row bottom))
-                                      nil start-point))))))))
-       map)
-     t (lambda ()
-         (setq track-mouse old-track-mouse)
-         (setq auto-hscroll-mode auto-hscroll-mode-saved)
-         (setq scroll-margin scroll-margin-saved)
-         ;; Don't deactivate the mark when the context menu was invoked
-         ;; by down-mouse-3 immediately after down-mouse-1 and without
-         ;; releasing the mouse button with mouse-1. This allows to use
-         ;; region-related context menu to operate on the selected region.
-         (unless (and context-menu-mode
-                      (eq (car-safe (aref (this-command-keys-vector) 0))
-                          'down-mouse-3))
-           (deactivate-mark)
-           (pop-mark))))))
+          (set-transient-map
+           (let ((map (make-sparse-keymap)))
+             (define-key map [switch-frame] #'ignore)
+             (define-key map [select-window] #'ignore)
+             (define-key map [mouse-movement]
+               (lambda (event) (interactive "e")
+                 (let* ((end (event-end event))
+                        (end-point (posn-point end)))
+                   (unless (eq end-point start-point)
+                     ;; And remember that we have moved, so mouse-set-region can know
+                     ;; its event is really a drag event.
+                     (setcar start-event 'mouse-movement))
+                   (if (and (eq (posn-window end) start-window)
+                            (integer-or-marker-p end-point))
+                       (mouse--drag-set-mark-and-point start-point
+                                                       end-point click-count)
+                     (let ((mouse-row (cdr (cdr (mouse-position)))))
+                       (cond
+                        ((null mouse-row))
+                        ((< mouse-row top)
+                         (mouse-scroll-subr start-window (- mouse-row top)
+                                            nil start-point))
+                        ((>= mouse-row bottom)
+                         (mouse-scroll-subr start-window (1+ (- mouse-row bottom))
+                                            nil start-point))))))))
+             map)
+           t (lambda ()
+               (funcall cleanup)
+               ;; Don't deactivate the mark when the context menu was invoked
+               ;; by down-mouse-3 immediately after down-mouse-1 and without
+               ;; releasing the mouse button with mouse-1. This allows to use
+               ;; region-related context menu to operate on the selected region.
+               (unless (and context-menu-mode
+                            (eq (car-safe (aref (this-command-keys-vector) 0))
+                                'down-mouse-3))
+                 (deactivate-mark)
+                 (pop-mark)))))
+      ;; Cleanup on errors
+      (error (funcall cleanup)
+             (signal (car err) (cdr err))))))
 
 (defun mouse--drag-set-mark-and-point (start click click-count)
   (let* ((range (mouse-start-end start click click-count))
-- 
2.33.0

[PATCH v3 3/3] Document the improved scroll-margin behavior

[Yuri D'Elia]

* etc/NEWS: document the improved `scroll-margin' behavior.
---
 etc/NEWS | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/etc/NEWS b/etc/NEWS
index f211a9867..3bc84c911 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1063,6 +1063,14 @@ in text mode.  The cursor still only actually blinks in GUI frames.
 It serves as a local counterpart for 'show-paren-mode', allowing you
 to toggle it separately in different buffers.
 
++++
+** Improved mouse behavior with auto-scrolling modes.
+When clicking inside the `scroll-margin' or `hscroll-margin' region
+the point is now moved only when releasing the mouse button.  This no
+longer results in a bogus selection, unless the mouse has been
+effectively dragged.
+
+
 \f
 * Changes in Specialized Modes and Packages in Emacs 28.1
 
-- 
2.33.0

Re: [PATCH v3 1/3] Improve interaction between mouse-drag-region and scroll-margin

[Yuri D'Elia]

On Sat, Sep 25 2021, Yuri D'Elia wrote:
> Clicking inside the scroll-margin region still occasionally causes some
> regions to be selected, ....

I spent some extra time digging into this specific edge case that was
remaining, as I still consider it buggy.

To sum up: now the click and drag behave as expected within the
scroll-margin region, but I *still* noticed sometimes a word or a line
would unexpectedly be selected.

Turns out the code is correct. It's a much dumber problem:

When clicking inside the scroll-margin region, the point is moved and
the buffer is scrolled. So far so good.

So I would just click again on the same spot for testing. This would
scroll more. But it the mouse hasn't moved, so this event would *also*
count as a double-click (and triple, if done in succession) if done fast
enough.

It's technically correct, since you *might* be trying to do a legitimate
double-click. However, if you click within any autoscroll region, the
buffer is scrolled after releasing the button. Such attempt results in
two scrolls, or more. Having this select /anything/ and at the same time
move it away from view is broken IMHO.

My thinking would be that if scrolling happens between the down-mouse
and up-mouse event, we shouldn't count subsequent clicks as repeated.

However the repeat count seems to come from a higher level which I'm not
familiar with. I'm not sure how would I approach such a fix.

Ideas?

Re: [PATCH v3 1/3] Improve interaction between mouse-drag-region and scroll-margin

[Yuri D'Elia]

On Sat, Sep 25 2021, Yuri D'Elia wrote:
> * lisp/mouse.el (mouse-drag-track): Disable both scroll-margin and
> auto-hscroll-mode in mouse-drag-region and do not re-enable them until
> dragging is over, making selections work as expected when inside the
> margins.
<...>

I'll bang the iron while it's hot. Anything missing from this series to
prevent merging?

Copyright assignment is clear on my side.

Re: [PATCH v3 1/3] Improve interaction between mouse-drag-region and scroll-margin

[Lars Ingebrigtsen]

Yuri D'Elia <wavexx@thregr.org> writes:

> I'll bang the iron while it's hot. Anything missing from this series to
> prevent merging?

I didn't quite understand what this was supposed to fix.  Do you have a
recipe to reproduce the misbehaviour in this area?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: [PATCH v3 1/3] Improve interaction between mouse-drag-region and scroll-margin

[Yuri D'Elia]

On Wed, Sep 29 2021, Lars Ingebrigtsen wrote:
> Yuri D'Elia <wavexx@thregr.org> writes:
>
>> I'll bang the iron while it's hot. Anything missing from this series to
>> prevent merging?
>
> I didn't quite understand what this was supposed to fix.  Do you have a
> recipe to reproduce the misbehaviour in this area?

Sure thing.

Set a large scroll-margin on a buffer (say, 10).

Then try to click in the margin area (top 10 visible lines, or bottom
10).

Then try to drag.

Then try with the patch applied.