bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[Charles A. Roelli]

From emacs -Q:

C-x SPC
mouse-3 on the space after ";; This"
mouse-3 again on the same spot

You're left with just ";; This".  It looks like the region is being
killed as a normal region (between point and mark) instead of as a
rectangle.

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[Federico Tedin]

I've looked into this bug. Its seems like the problem is in mouse.el,
where line 1610 could be changed from:

(kill-region (mark t) (point)))

to:

(kill-region (mark t) (point) 'region))

Which would make kill-region use the (potentially rectangular) region
instead of BEG and END. The problem is, many mouse-related functions
still use functions such as filter-buffer-substring and delete-region,
which are not rectangular-region aware. Would submitting a patch for
this line only make sense?

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[Charles A. Roelli]

> From: Federico Tedin <federicotedin@gmail.com>
> Date: Sun, 19 Aug 2018 23:26:59 -0300
> 
> I've looked into this bug. Its seems like the problem is in mouse.el,
> where line 1610 could be changed from:
> 
> (kill-region (mark t) (point)))
> 
> to:
> 
> (kill-region (mark t) (point) 'region))
> 
> Which would make kill-region use the (potentially rectangular) region
> instead of BEG and END. The problem is, many mouse-related functions
> still use functions such as filter-buffer-substring and delete-region,
> which are not rectangular-region aware. Would submitting a patch for
> this line only make sense?

Thanks for looking into this.  It would be great to squash all these
problems with one patch to get a consistent behavior.  I will retitle
the bug to make that clearer.

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[Federico Tedin]

> Thanks for looking into this.  It would be great to squash all these
> problems with one patch to get a consistent behavior.  I will retitle
> the bug to make that clearer.

Sorry for the delay, I never received your email for some reason.

I'll check if I can find more problems related to mouse + rectangular
region. If I find
a way to fix them, I'll submit a patch which would also cover the
issue originally brought
up in this bug report.

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[Charles A. Roelli]

> From: Federico Tedin <federicotedin@gmail.com>
> Date: Tue, 11 Sep 2018 21:39:44 -0300
> 
> > Thanks for looking into this.  It would be great to squash all these
> > problems with one patch to get a consistent behavior.  I will retitle
> > the bug to make that clearer.
> 
> Sorry for the delay, I never received your email for some reason.

My bad; it bounced and I forgot to resend it.
 
> I'll check if I can find more problems related to mouse + rectangular
> region. If I find
> a way to fix them, I'll submit a patch which would also cover the
> issue originally brought
> up in this bug report.

Thanks!

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[Federico Tedin]

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

After reading more of the code in mouse.el, I found that
mouse-save-then-kill is the only function that allows using the mouse
to set/resize rectangular regions. I'm attaching a patch that fixes
all three use cases for this function: setting the region initially,
resizing the region, and killing the region. I've also made sure it
works correctly when mouse-drag-copy-region is set to t. Hope it helps.

[-- Attachment #2: mouse.patch --]
[-- Type: text/x-patch, Size: 1490 bytes --]

From 317598a579d95ca34def440d2b7789abdcd1ad47 Mon Sep 17 00:00:00 2001
From: Federico Tedin <federicotedin@gmail.com>
Date: Sat, 22 Sep 2018 16:51:58 -0300
Subject: [PATCH 1/1] Make mouse-save-then-kill work with Rectangle Mark mode

* lisp/mouse.el (mouse-save-then-kill): Make mouse-save-then-kill work
  with rectangular regions, including when mouse-drag-copy-region is
  set to t. (Bug#31240)
---
 lisp/mouse.el | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lisp/mouse.el b/lisp/mouse.el
index d5c132f484..77e2fe7520 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -1606,8 +1606,8 @@ mouse-save-then-kill
       (if mouse-drag-copy-region
           ;; Region already saved in the previous click;
           ;; don't make a duplicate entry, just delete.
-          (delete-region (mark t) (point))
-        (kill-region (mark t) (point)))
+          (funcall region-extract-function 'delete-only)
+        (kill-region (mark t) (point) 'region))
       (setq mouse-selection-click-count 0)
       (setq mouse-save-then-kill-posn nil))
 
@@ -1632,7 +1632,7 @@ mouse-save-then-kill
 	(mouse-set-region-1)
         (when mouse-drag-copy-region
           ;; Region already copied to kill-ring once, so replace.
-          (kill-new (filter-buffer-substring (mark t) (point)) t))
+          (kill-new (funcall region-extract-function nil) t))
 	;; Arrange for a repeated mouse-3 to kill the region.
 	(setq mouse-save-then-kill-posn click-pt)))
 
-- 
2.17.1

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[Charles A. Roelli]

> From: Federico Tedin <federicotedin@gmail.com>
> Date: Sat, 22 Sep 2018 17:05:19 -0300
> 
> After reading more of the code in mouse.el, I found that
> mouse-save-then-kill is the only function that allows using the mouse
> to set/resize rectangular regions. I'm attaching a patch that fixes
> all three use cases for this function: setting the region initially,
> resizing the region, and killing the region. I've also made sure it
> works correctly when mouse-drag-copy-region is set to t. Hope it helps.

Thanks, this looks good.  I think we also need a similar change for
function "mouse-drag-and-drop-region", which is used when the variable
of the same name is non-nil.  For example, the following recipe
exposes it:

M-x set-variable RET mouse-drag-and-drop-region RET t RET
M-< C-SPC M-f C-n C-x SPC
Drag the rectangle region with the mouse, and its shape is ignored

Hopefully we can apply the same kind of change in that function too.

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[Federico Tedin]

> Thanks, this looks good.  I think we also need a similar change for
> function "mouse-drag-and-drop-region", which is used when the variable
> of the same name is non-nil.  For example, the following recipe
> exposes it:
>
> M-x set-variable RET mouse-drag-and-drop-region RET t RET
> M-< C-SPC M-f C-n C-x SPC
> Drag the rectangle region with the mouse, and its shape is ignored
>
> Hopefully we can apply the same kind of change in that function too.

You're right about mouse-drag-and-drop-region, it doesn't work
correctly when using rectangular regions. After taking a look at the
code, I managed to fix two things: the dragged text now has the
correct rectangular contents (but the original text is incorrectly
deleted using a normal region), and the tooltip displaying the dragged
text also shows the correct contents.

To fix the rest of the functionalities, I would need to know the
recommended way of handling some of the details of how
mouse-drag-and-drop-region is implemented:

- The dragged region is tracked using an overlay. From what I
  understand, this is a problem since overlays only handle regions
  with a single beginning and end, and rectangles have one or more of
  those.

- In order to check if the dragged text is read-only, the function
  "next-single-char-property-change" is used. This function has the
  same problem as the overlay, as it assumes the region is contiguous.

I'm thinking both problems could be solved by using a list of overlays
instead of just one, creating them from the result of calling
"region-bounds".  Then, the rest of the function could be adapted to
use the overlay list.

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[Charles A. Roelli]

> From: Federico Tedin <federicotedin@gmail.com>
> Date: Sun, 23 Sep 2018 19:23:13 -0300
>
> You're right about mouse-drag-and-drop-region, it doesn't work
> correctly when using rectangular regions. After taking a look at the
> code, I managed to fix two things: the dragged text now has the
> correct rectangular contents (but the original text is incorrectly
> deleted using a normal region)

Would it be feasible to replace calls to "delete-region" with the
right call to "region-extract-function"?  Even though the current
arguments to "delete-region" in "mouse-drag-and-drop-region" are based
on the position of "mouse-drag-and-drop-overlay", this should not pose
an issue.

> 				, and the tooltip displaying the dragged
> text also shows the correct contents.
> 
> To fix the rest of the functionalities, I would need to know the
> recommended way of handling some of the details of how
> mouse-drag-and-drop-region is implemented:
> 
> - The dragged region is tracked using an overlay. From what I
>   understand, this is a problem since overlays only handle regions
>   with a single beginning and end, and rectangles have one or more of
>   those.

We should be able to keep using the overlay for normal regions, and
adapt mouse-drag-and-drop-region to use "apply-on-rectangle" with the
rectangle bounds as an argument when it needs to do something with a
rectangular region.

> - In order to check if the dragged text is read-only, the function
>   "next-single-char-property-change" is used. This function has the
>   same problem as the overlay, as it assumes the region is contiguous.

I think "apply-on-rectangle" could be helpful here too.
 
> I'm thinking both problems could be solved by using a list of overlays
> instead of just one, creating them from the result of calling
> "region-bounds".  Then, the rest of the function could be adapted to
> use the overlay list.

Yes, this would work too.

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[Federico Tedin]

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

Charles, thanks for your input. I've created a new patch which fixes
the handling of rectangular regions on both mouse-save-then-kill and
mouse-drag-and-drop-region (it includes the changes made in the
previous patch). I'm attaching it below.

[-- Attachment #2: mouse.patch --]
[-- Type: text/x-patch, Size: 8515 bytes --]

From a4a865dbdf60158c2e8036d564311515a08f4830 Mon Sep 17 00:00:00 2001
From: Federico Tedin <federicotedin@gmail.com>
Date: Tue, 25 Sep 2018 21:29:19 -0300
Subject: [PATCH 1/1] Allow two mouse functions to work with Rectangle Mark
 mode

* lisp/mouse.el (mouse-save-then-kill): Make mouse-save-then-kill work
  with rectangular regions, including when mouse-drag-copy-region is
  set to t. (Bug#31240)
  (mouse-drag-and-drop-region): Allow dragging and dropping
  rectangular regions.
---
 lisp/mouse.el | 71 ++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 48 insertions(+), 23 deletions(-)

diff --git a/lisp/mouse.el b/lisp/mouse.el
index d5c132f484..344f39eddc 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -1606,8 +1606,8 @@ mouse-save-then-kill
       (if mouse-drag-copy-region
           ;; Region already saved in the previous click;
           ;; don't make a duplicate entry, just delete.
-          (delete-region (mark t) (point))
-        (kill-region (mark t) (point)))
+          (funcall region-extract-function 'delete-only)
+        (kill-region (mark t) (point) 'region))
       (setq mouse-selection-click-count 0)
       (setq mouse-save-then-kill-posn nil))
 
@@ -1632,7 +1632,7 @@ mouse-save-then-kill
 	(mouse-set-region-1)
         (when mouse-drag-copy-region
           ;; Region already copied to kill-ring once, so replace.
-          (kill-new (filter-buffer-substring (mark t) (point)) t))
+          (kill-new (funcall region-extract-function nil) t))
 	;; Arrange for a repeated mouse-3 to kill the region.
 	(setq mouse-save-then-kill-posn click-pt)))
 
@@ -2408,10 +2408,15 @@ mouse-drag-and-drop-region
          (start (region-beginning))
          (end (region-end))
          (point (point))
+         (region-noncontiguous (region-noncontiguous-p))
          (buffer (current-buffer))
          (window (selected-window))
          (text-from-read-only buffer-read-only)
-         (mouse-drag-and-drop-overlay (make-overlay start end))
+         ;; Use multiple overlays to cover cases where the region is rectangular.
+         (mouse-drag-and-drop-overlays (mapcar (lambda (bounds)
+                                                 (make-overlay (car bounds)
+                                                               (cdr bounds)))
+                                               (region-bounds)))
          point-to-paste
          point-to-paste-read-only
          window-to-paste
@@ -2455,7 +2460,11 @@ mouse-drag-and-drop-region
           ;; Obtain the dragged text in region.  When the loop was
           ;; skipped, value-selection remains nil.
           (unless value-selection
-            (setq value-selection (buffer-substring start end))
+            (setq value-selection (funcall region-extract-function nil))
+            ;; Remove yank-handler property in order to re-insert text using
+            ;; the `insert-rectangle' function later on.
+            (remove-text-properties 0 (length value-selection)
+                                    '(yank-handler) value-selection)
             (when mouse-drag-and-drop-region-show-tooltip
               (let ((text-size mouse-drag-and-drop-region-show-tooltip))
                 (setq text-tooltip
@@ -2468,12 +2477,18 @@ mouse-drag-and-drop-region
                         value-selection))))
 
             ;; Check if selected text is read-only.
-            (setq text-from-read-only (or text-from-read-only
-                                          (get-text-property start 'read-only)
-                                          (not (equal
-                                                (next-single-char-property-change
-                                                 start 'read-only nil end)
-                                                end)))))
+            (setq text-from-read-only
+                  (or text-from-read-only
+                      (get-text-property start 'read-only)
+                      (get-text-property end 'read-only)
+                      (catch 'loop
+                             (dolist (bound (region-bounds))
+                               (unless (equal
+                                        (next-single-char-property-change
+                                         (car bound) 'read-only nil (cdr bound))
+                                        (cdr bound))
+                                 (throw 'loop t)))))))
+
           (setq window-to-paste (posn-window (event-end event)))
           (setq point-to-paste (posn-point (event-end event)))
           ;; Set nil when target buffer is minibuffer.
@@ -2500,12 +2515,12 @@ mouse-drag-and-drop-region
             ;; text will be inserted to inside of the original
             ;; region.
             (setq drag-but-negligible
-                  (and (eq (overlay-buffer mouse-drag-and-drop-overlay)
+                  (and (eq (overlay-buffer (car mouse-drag-and-drop-overlays))
                            buffer-to-paste)
-                       (<= (overlay-start mouse-drag-and-drop-overlay)
+                       (<= (overlay-start (car mouse-drag-and-drop-overlays))
                           point-to-paste)
                        (<= point-to-paste
-                          (overlay-end mouse-drag-and-drop-overlay)))))
+                          (overlay-end (car (last mouse-drag-and-drop-overlays)))))))
 
           ;; Show a tooltip.
           (if mouse-drag-and-drop-region-show-tooltip
@@ -2524,8 +2539,9 @@ mouse-drag-and-drop-region
                                (t
                                 'bar)))
             (when cursor-in-text-area
-              (overlay-put mouse-drag-and-drop-overlay
-                           'face 'mouse-drag-and-drop-region)
+              (dolist (overlay mouse-drag-and-drop-overlays)
+                (overlay-put overlay
+                           'face 'mouse-drag-and-drop-region))
               (deactivate-mark)     ; Maintain region in other window.
               (mouse-set-point event)))))
 
@@ -2590,11 +2606,17 @@ mouse-drag-and-drop-region
           (setq window-exempt window-to-paste)
           (goto-char point-to-paste)
           (push-mark)
-          (insert value-selection)
+
+          (if region-noncontiguous
+              (insert-rectangle (split-string value-selection "\n"))
+            (insert value-selection))
+
           ;; On success, set the text as region on destination buffer.
           (when (not (equal (mark) (point)))
             (setq deactivate-mark nil)
-            (activate-mark))
+            (activate-mark)
+            (when region-noncontiguous
+              (rectangle-mark-mode)))
 
           ;; * SOURCE BUFFER::
           ;; Set back the original text as region or delete the original
@@ -2604,8 +2626,9 @@ mouse-drag-and-drop-region
               ;; remove the original text.
               (when no-modifier-on-drop
                 (let (deactivate-mark)
-                  (delete-region (overlay-start mouse-drag-and-drop-overlay)
-                                 (overlay-end mouse-drag-and-drop-overlay))))
+                  (dolist (overlay mouse-drag-and-drop-overlays)
+                    (delete-region (overlay-start overlay)
+                                   (overlay-end overlay)))))
             ;; When source buffer and destination buffer are different,
             ;; keep (set back the original text as region) or remove the
             ;; original text.
@@ -2615,15 +2638,17 @@ mouse-drag-and-drop-region
             (if mouse-drag-and-drop-region-cut-when-buffers-differ
                 ;; Remove the dragged text from source buffer like
                 ;; operation `cut'.
-                (delete-region (overlay-start mouse-drag-and-drop-overlay)
-                               (overlay-end mouse-drag-and-drop-overlay))
+                (dolist (overlay mouse-drag-and-drop-overlays)
+                    (delete-region (overlay-start overlay)
+                                   (overlay-end overlay)))
               ;; Set back the dragged text as region on source buffer
               ;; like operation `copy'.
               (activate-mark))
             (select-window window-to-paste))))))
 
     ;; Clean up.
-    (delete-overlay mouse-drag-and-drop-overlay)
+    (dolist (overlay mouse-drag-and-drop-overlays)
+      (delete-overlay overlay))
 
     ;; Restore old states but for the window where the drop
     ;; occurred. Restore cursor types for all windows.
-- 
2.17.1

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[Charles A. Roelli]

> From: Federico Tedin <federicotedin@gmail.com>
> Date: Tue, 25 Sep 2018 21:33:35 -0300
> 
> Charles, thanks for your input. I've created a new patch which fixes
> the handling of rectangular regions on both mouse-save-then-kill and
> mouse-drag-and-drop-region (it includes the changes made in the
> previous patch). I'm attaching it below.

Thanks!

I tried this to test the patch:

src/emacs -Q
M-x set-variable RET mouse-drag-and-drop-region RET t
With the mouse, drag from the "n" in "not saved" to the space after "C-x C-f" (in *scratch*)
C-x SPC
Now drag the region until the cursor is on the "a" of "and"

For some reason, the region is not moved, and it gets re-activated as
a normal region instead of a rectangular region.  Maybe I'm missing
something obvious.  I was able to get the dragging of a rectangular
region working sometimes, but not consistently.

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[Federico Tedin]

> Thanks!
>
> I tried this to test the patch:
>
> src/emacs -Q
> M-x set-variable RET mouse-drag-and-drop-region RET t
> With the mouse, drag from the "n" in "not saved" to the space after "C-x C-f" (in *scratch*)
> C-x SPC
> Now drag the region until the cursor is on the "a" of "and"
>
> For some reason, the region is not moved, and it gets re-activated as
> a normal region instead of a rectangular region.  Maybe I'm missing
> something obvious.  I was able to get the dragging of a rectangular
> region working sometimes, but not consistently.

I've tried your test, and it does break my fix, as you mentioned.

The problem was in the criteria used to define the variable
'drag-but-negligible'. The drag action used in your test was being
incorrectly marked as negligible. Because of this, the region was
also re-activated, but not in Rectangle Mark mode (this was also a bug).

I have made a correction where the variable 'drag-but-negligible' is
defined, so dragging a rectangle region outside of itself will no longer
mark it as negligible; and when it _is_ negligible, the region is
re-activated as a rectangle again.

I've also found some cases where the overlay list is not working well
enough to track the selected rectangle. For example, if a buffer
contains the following:

aaaa
BBbb
CCcc

Dragging a 2x2 square starting from the first 'B' (spaces added for
clarity):

 a a a a
[B B]b b
[C C]c c

to the column where the second 'a' is, results in the following:

a B B a a a
b b
c c

In this case, two 'C's are missing in the second line (after the first
'b'). The reason this is happening is the following: when
mouse-drag-and-drop-region is called, the initial overlays are the
following (shown with braces):

 a a a a
{B B}b b
{C C}c c

After the 2x2 square is inserted on the second 'a', the first overlay
is automatically expanded, because characters where inserted between
its start/end:

 a B B a a a
{b C C b}b b
{c c}c c

When the original text is then deleted (by deleting all overlays), the
result is:

a B B a a a
b b
c c

So I think I have two options now: either forbid the user from
dragging a rectangle to a position where the inserted rectangle would
intersect the original rectangle, or find another way to track the
originally selected rectangle in a way it can be accurately deleted
after inserting it in the new position. I guess I'll go with the
second option, since it would make function more useful for users.

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[martin rudalics]

 > So I think I have two options now: either forbid the user from
 > dragging a rectangle to a position where the inserted rectangle would
 > intersect the original rectangle,

IIRC that would be faithful to the original (non-rectangle-dragging)
design.

 > or find another way to track the
 > originally selected rectangle in a way it can be accurately deleted
 > after inserting it in the new position. I guess I'll go with the
 > second option, since it would make function more useful for users.

In that case we should try to make the non-rectangle-dragging behavior
similar.  So if you prefer the second option, please do that and make
it customizable.

Many thanks for working on this, martin

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[Federico Tedin]

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

>  > So I think I have two options now: either forbid the user from
>  > dragging a rectangle to a position where the inserted rectangle would
>  > intersect the original rectangle,
>
> IIRC that would be faithful to the original (non-rectangle-dragging)
> design.

> This second option sounds like it can be quite hard to define.  If you
> decide in the end to prevent the user from dragging the region
> somewhere that would intersect with the dragged region (which, as
> Martin said, is in line with the original design of
> mouse-drag-and-drop-region) that would be fine.

After trying out some solutions and reading these two suggestions, I've
decided to implement the feature like this. The problem of correctly handling
the cases where the dragged rectangular text would overlap with the original
one was more complex than I'd thought, and I have some doubts if the usefulness
of the feature justifies this added complexity.

I'm attaching a new patch with all my changes to mouse.el (and rect.el) so far.
I've created two new helper functions in rect.el to avoid cluttering mouse.el
with more functions.

So, the cases to test out are:

1) Dragging and dropping non-rectangular regions should be exactly the
same as before.
2) Dragging and dropping a rectangle _outside_ of itself should insert
it in the new
position, and then delete the original.
3) Dragging and dropping a rectangle _inside_ of itself should leave
everything unchanged.

After evaluating "(setq mouse-drag-and-drop-region 'shift)":

4) Dragging and dropping a rectangle inside or outside of itself, while holding
the Shift key when dropping, should insert it there, without deleting
the original.

When I say 'outside of itself' I mean that there shouldn't be any
overlapping at all
between the original and the newly inserted rectangles.

[-- Attachment #2: mouse.patch --]
[-- Type: text/x-patch, Size: 11591 bytes --]

From 777aaf732f348b3b7cf41d3366071f602b8cbb0c Mon Sep 17 00:00:00 2001
From: Federico Tedin <federicotedin@gmail.com>
Date: Sat, 29 Sep 2018 20:16:00 -0300
Subject: [PATCH 1/1] Allow two mouse functions to work with Rectangle Mark
 mode

* lisp/mouse.el (mouse-save-then-kill): Make mouse-save-then-kill work
  with rectangular regions, including when mouse-drag-copy-region is
  set to t. (Bug#31240)
  (mouse-drag-and-drop-region): Allow dragging and dropping
  rectangular regions. (Bug#31240)
* rect.el (rectangle-intersect-p): Add a new function.
  (rectangle-position-as-coordinates): Add a new function.
---
 lisp/mouse.el | 92 ++++++++++++++++++++++++++++++++++++---------------
 lisp/rect.el  | 35 ++++++++++++++++++++
 2 files changed, 101 insertions(+), 26 deletions(-)

diff --git a/lisp/mouse.el b/lisp/mouse.el
index cb63ca51c5..b00f38a0f6 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -29,6 +29,8 @@
 
 ;;; Code:
 
+(eval-when-compile (require 'rect))
+
 ;;; Utility functions.
 
 ;; Indent track-mouse like progn.
@@ -1606,8 +1608,8 @@ mouse-save-then-kill
       (if mouse-drag-copy-region
           ;; Region already saved in the previous click;
           ;; don't make a duplicate entry, just delete.
-          (delete-region (mark t) (point))
-        (kill-region (mark t) (point)))
+          (funcall region-extract-function 'delete-only)
+        (kill-region (mark t) (point) 'region))
       (setq mouse-selection-click-count 0)
       (setq mouse-save-then-kill-posn nil))
 
@@ -1632,7 +1634,7 @@ mouse-save-then-kill
 	(mouse-set-region-1)
         (when mouse-drag-copy-region
           ;; Region already copied to kill-ring once, so replace.
-          (kill-new (filter-buffer-substring (mark t) (point)) t))
+          (kill-new (funcall region-extract-function nil) t))
 	;; Arrange for a repeated mouse-3 to kill the region.
 	(setq mouse-save-then-kill-posn click-pt)))
 
@@ -2411,7 +2413,15 @@ mouse-drag-and-drop-region
          (buffer (current-buffer))
          (window (selected-window))
          (text-from-read-only buffer-read-only)
-         (mouse-drag-and-drop-overlay (make-overlay start end))
+         ;; Use multiple overlays to cover cases where the region is rectangular.
+         (mouse-drag-and-drop-overlays (mapcar (lambda (bounds)
+                                                 (make-overlay (car bounds)
+                                                               (cdr bounds)))
+                                               (region-bounds)))
+         (region-noncontiguous (region-noncontiguous-p))
+         (region-width (- (overlay-end (car mouse-drag-and-drop-overlays))
+                          (overlay-start (car mouse-drag-and-drop-overlays))))
+         (region-height (length mouse-drag-and-drop-overlays))
          point-to-paste
          point-to-paste-read-only
          window-to-paste
@@ -2455,7 +2465,11 @@ mouse-drag-and-drop-region
           ;; Obtain the dragged text in region.  When the loop was
           ;; skipped, value-selection remains nil.
           (unless value-selection
-            (setq value-selection (buffer-substring start end))
+            (setq value-selection (funcall region-extract-function nil))
+            ;; Remove yank-handler property in order to re-insert text using
+            ;; the `insert-rectangle' function later on.
+            (remove-text-properties 0 (length value-selection)
+                                    '(yank-handler) value-selection)
             (when mouse-drag-and-drop-region-show-tooltip
               (let ((text-size mouse-drag-and-drop-region-show-tooltip))
                 (setq text-tooltip
@@ -2468,12 +2482,18 @@ mouse-drag-and-drop-region
                         value-selection))))
 
             ;; Check if selected text is read-only.
-            (setq text-from-read-only (or text-from-read-only
-                                          (get-text-property start 'read-only)
-                                          (not (equal
-                                                (next-single-char-property-change
-                                                 start 'read-only nil end)
-                                                end)))))
+            (setq text-from-read-only
+                  (or text-from-read-only
+                      (get-text-property start 'read-only)
+                      (get-text-property end 'read-only)
+                      (catch 'loop
+                             (dolist (bound (region-bounds))
+                               (unless (equal
+                                        (next-single-char-property-change
+                                         (car bound) 'read-only nil (cdr bound))
+                                        (cdr bound))
+                                 (throw 'loop t)))))))
+
           (setq window-to-paste (posn-window (event-end event)))
           (setq point-to-paste (posn-point (event-end event)))
           ;; Set nil when target buffer is minibuffer.
@@ -2500,12 +2520,20 @@ mouse-drag-and-drop-region
             ;; text will be inserted to inside of the original
             ;; region.
             (setq drag-but-negligible
-                  (and (eq (overlay-buffer mouse-drag-and-drop-overlay)
+                  (and (eq (overlay-buffer (car mouse-drag-and-drop-overlays))
                            buffer-to-paste)
-                       (<= (overlay-start mouse-drag-and-drop-overlay)
-                          point-to-paste)
-                       (<= point-to-paste
-                          (overlay-end mouse-drag-and-drop-overlay)))))
+                       (if region-noncontiguous
+                           ;; If the region is rectangular, check if the newly inserted
+                           ;; rectangular text would intersect the already selected
+                           ;; region. If it would, then set "drag-but-negligible" to t.
+                           (let ((size (cons region-width region-height)))
+                             (rectangle-intersect-p
+                              (rectangle-position-as-coordinates start) size
+                              (rectangle-position-as-coordinates point-to-paste) size))
+                         (and (<= (overlay-start (car mouse-drag-and-drop-overlays))
+                                  point-to-paste)
+                              (<= point-to-paste
+                                  (overlay-end (car mouse-drag-and-drop-overlays))))))))
 
           ;; Show a tooltip.
           (if mouse-drag-and-drop-region-show-tooltip
@@ -2524,8 +2552,9 @@ mouse-drag-and-drop-region
                                (t
                                 'bar)))
             (when cursor-in-text-area
-              (overlay-put mouse-drag-and-drop-overlay
-                           'face 'mouse-drag-and-drop-region)
+              (dolist (overlay mouse-drag-and-drop-overlays)
+                (overlay-put overlay
+                           'face 'mouse-drag-and-drop-region))
               (deactivate-mark)     ; Maintain region in other window.
               (mouse-set-point event)))))
 
@@ -2581,7 +2610,9 @@ mouse-drag-and-drop-region
           (select-window window)
           (goto-char point)
           (setq deactivate-mark nil)
-          (activate-mark))
+          (activate-mark)
+          (when region-noncontiguous
+            (rectangle-mark-mode)))
          ;; Modify buffers.
          (t
           ;; * DESTINATION BUFFER::
@@ -2590,11 +2621,17 @@ mouse-drag-and-drop-region
           (setq window-exempt window-to-paste)
           (goto-char point-to-paste)
           (push-mark)
-          (insert value-selection)
+
+          (if region-noncontiguous
+              (insert-rectangle (split-string value-selection "\n"))
+            (insert value-selection))
+
           ;; On success, set the text as region on destination buffer.
           (when (not (equal (mark) (point)))
             (setq deactivate-mark nil)
-            (activate-mark))
+            (activate-mark)
+            (when region-noncontiguous
+              (rectangle-mark-mode)))
 
           ;; * SOURCE BUFFER::
           ;; Set back the original text as region or delete the original
@@ -2604,8 +2641,9 @@ mouse-drag-and-drop-region
               ;; remove the original text.
               (when no-modifier-on-drop
                 (let (deactivate-mark)
-                  (delete-region (overlay-start mouse-drag-and-drop-overlay)
-                                 (overlay-end mouse-drag-and-drop-overlay))))
+                  (dolist (overlay mouse-drag-and-drop-overlays)
+                    (delete-region (overlay-start overlay)
+                                   (overlay-end overlay)))))
             ;; When source buffer and destination buffer are different,
             ;; keep (set back the original text as region) or remove the
             ;; original text.
@@ -2615,15 +2653,17 @@ mouse-drag-and-drop-region
             (if mouse-drag-and-drop-region-cut-when-buffers-differ
                 ;; Remove the dragged text from source buffer like
                 ;; operation `cut'.
-                (delete-region (overlay-start mouse-drag-and-drop-overlay)
-                               (overlay-end mouse-drag-and-drop-overlay))
+                (dolist (overlay mouse-drag-and-drop-overlays)
+                    (delete-region (overlay-start overlay)
+                                   (overlay-end overlay)))
               ;; Set back the dragged text as region on source buffer
               ;; like operation `copy'.
               (activate-mark))
             (select-window window-to-paste))))))
 
     ;; Clean up.
-    (delete-overlay mouse-drag-and-drop-overlay)
+    (dolist (overlay mouse-drag-and-drop-overlays)
+      (delete-overlay overlay))
 
     ;; Restore old states but for the window where the drop
     ;; occurred. Restore cursor types for all windows.
diff --git a/lisp/rect.el b/lisp/rect.el
index 8ccf051ee1..0456242f64 100644
--- a/lisp/rect.el
+++ b/lisp/rect.el
@@ -167,6 +167,41 @@ apply-on-rectangle
                  (<= (point) endpt))))
       final-point)))
 
+(defun rectangle-position-as-coordinates (position)
+  "Return an integer buffer position as a (COL . LINE) coordinate."
+  (save-excursion
+    (goto-char position)
+    (let ((col (current-column))
+          (line (progn
+                  (beginning-of-line)
+                  (count-lines 1 position))))
+      (cons col line))))
+
+(defun rectangle-intersect-p (pos1 size1 pos2 size2)
+  "Return t if the rectangle defined by POS1 and SIZE1 intersects with
+the one defined by POS2 and SIZE2, and return nil if they do not.
+
+POS1 and POS2 should describe the positions of the upper-left
+corners of the first and second rectangles, in the form of (COL . LINE).
+SIZE1 and SIZE2 should describe the dimensions of the first and second
+rectangles, in the form of (WIDTH . HEIGHT)."
+  (let ((x1 (car pos1))
+        (y1 (cdr pos1))
+        (x2 (car pos2))
+        (y2 (cdr pos2))
+        (w1 (car size1))
+        (h1 (cdr size1))
+        (w2 (car size2))
+        (h2 (cdr size2)))
+    (not (or (<= (+ x1 w1)
+                 x2)
+             (<= (+ x2 w2)
+                 x1)
+             (<= (+ y1 h1)
+                 y2)
+             (<= (+ y2 h2)
+                 y1)))))
+
 (defun delete-rectangle-line (startcol endcol fill)
   (when (= (move-to-column startcol (if fill t 'coerce)) startcol)
     (delete-region (point)
-- 
2.17.1

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[martin rudalics]

 > After trying out some solutions and reading these two suggestions, I've
 > decided to implement the feature like this. The problem of correctly handling
 > the cases where the dragged rectangular text would overlap with the original
 > one was more complex than I'd thought, and I have some doubts if the usefulness
 > of the feature justifies this added complexity.
 >
 > I'm attaching a new patch with all my changes to mouse.el (and rect.el) so far.
 > I've created two new helper functions in rect.el to avoid cluttering mouse.el
 > with more functions.
 >
 > So, the cases to test out are:
 >
 > 1) Dragging and dropping non-rectangular regions should be exactly the
 > same as before.
 > 2) Dragging and dropping a rectangle _outside_ of itself should insert
 > it in the new
 > position, and then delete the original.
 > 3) Dragging and dropping a rectangle _inside_ of itself should leave
 > everything unchanged.
 >
 > After evaluating "(setq mouse-drag-and-drop-region 'shift)":
 >
 > 4) Dragging and dropping a rectangle inside or outside of itself, while holding
 > the Shift key when dropping, should insert it there, without deleting
 > the original.
 >
 > When I say 'outside of itself' I mean that there shouldn't be any
 > overlapping at all
 > between the original and the newly inserted rectangles.

Thank you. From what I can tell, the patch correctly addresses the
cases enumerated.  Since I don't use rectangle functions I'd urge
someone who does use them on a more regular basis to test it.

A few issues:

I'd rewrite the doc-string of 'rectangle-position-as-coordinates' as
something like

(defun rectangle-position-as-coordinates (position)
   "Return cons of the column and line values of POSITION.
POSITION specifies a position of the current buffer.  The value
returned is a cons of the current column of POSITION and its line
number."

because doc-strings have to describe all arguments of a functions.
Also,

                   (count-lines 1 position))))

should become

                   (count-lines (point-min) position))))

And I'd rewrite the doc-string of 'rectangle-intersect-p' like

(defun rectangle-intersect-p (pos1 size1 pos2 size2)
   "Return non-nil if two rectangles intersect.
POS1 and POS2 specify the positions of the upper-left corners of
the first and second rectangle as conses of their column and line
values.  SIZE1 and SIZE2 specify the dimensions of the first and
second rectangle, as conses of their width and height measured in
columns and lines."

because the first line of a doc-string must be a complete
sentence.  Also I'd rewrite forms like

              (<= (+ x2 w2)
                  x1)

as

              (<= (+ x2 w2) x1)
	
although this still won't make your patch short enough to qualify as
"tiny change".  So if you haven't done so already, please start the
paperwork process so we can apply this patch.

Thanks again for working on this, martin

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[Charles A. Roelli]

Thanks for the updated patch, it looks good to me.  I have one small
suggestion:

> +(defun rectangle-position-as-coordinates (position)
> +  "Return an integer buffer position as a (COL . LINE) coordinate."
> +  (save-excursion
> +    (goto-char position)
> +    (let ((col (current-column))
> +          (line (progn
> +                  (beginning-of-line)
> +                  (count-lines 1 position))))
> +      (cons col line))))

(beginning-of-line) could be replaced with (forward-line 0), which is
guaranteed to be at the beginning of the line.

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[Federico Tedin]

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

Martin, Charles: Thanks for the suggestions, I've applied them and I'm attaching
the new patch here.

> although this still won't make your patch short enough to qualify as
> "tiny change".  So if you haven't done so already, please start the
> paperwork process so we can apply this patch.

This shouldn't be a problem, my copyright assignment was filed one or two months
ago, and since then I've contributed two patches which have already been merged.

[-- Attachment #2: mouse.patch --]
[-- Type: text/x-patch, Size: 11590 bytes --]

From eb5ad6fa5a2948f5c3249927f94e319d38fa90ba Mon Sep 17 00:00:00 2001
From: Federico Tedin <federicotedin@gmail.com>
Date: Sun, 30 Sep 2018 13:09:10 -0300
Subject: [PATCH] Allow two mouse functions to work with Rectangle Mark mode

* lisp/mouse.el (mouse-save-then-kill): Make mouse-save-then-kill work
  with rectangular regions, including when mouse-drag-copy-region is
  set to t. (Bug#31240)
  (mouse-drag-and-drop-region): Allow dragging and dropping
  rectangular regions. (Bug#31240)
* rect.el (rectangle-intersect-p): Add a new function.
  (rectangle-position-as-coordinates): Add a new function.
---
 lisp/mouse.el | 92 ++++++++++++++++++++++++++++++++++++---------------
 lisp/rect.el  | 33 ++++++++++++++++++
 2 files changed, 99 insertions(+), 26 deletions(-)

diff --git a/lisp/mouse.el b/lisp/mouse.el
index cb63ca51c5..b00f38a0f6 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -29,6 +29,8 @@
 
 ;;; Code:
 
+(eval-when-compile (require 'rect))
+
 ;;; Utility functions.
 
 ;; Indent track-mouse like progn.
@@ -1606,8 +1608,8 @@ mouse-save-then-kill
       (if mouse-drag-copy-region
           ;; Region already saved in the previous click;
           ;; don't make a duplicate entry, just delete.
-          (delete-region (mark t) (point))
-        (kill-region (mark t) (point)))
+          (funcall region-extract-function 'delete-only)
+        (kill-region (mark t) (point) 'region))
       (setq mouse-selection-click-count 0)
       (setq mouse-save-then-kill-posn nil))
 
@@ -1632,7 +1634,7 @@ mouse-save-then-kill
 	(mouse-set-region-1)
         (when mouse-drag-copy-region
           ;; Region already copied to kill-ring once, so replace.
-          (kill-new (filter-buffer-substring (mark t) (point)) t))
+          (kill-new (funcall region-extract-function nil) t))
 	;; Arrange for a repeated mouse-3 to kill the region.
 	(setq mouse-save-then-kill-posn click-pt)))
 
@@ -2411,7 +2413,15 @@ mouse-drag-and-drop-region
          (buffer (current-buffer))
          (window (selected-window))
          (text-from-read-only buffer-read-only)
-         (mouse-drag-and-drop-overlay (make-overlay start end))
+         ;; Use multiple overlays to cover cases where the region is rectangular.
+         (mouse-drag-and-drop-overlays (mapcar (lambda (bounds)
+                                                 (make-overlay (car bounds)
+                                                               (cdr bounds)))
+                                               (region-bounds)))
+         (region-noncontiguous (region-noncontiguous-p))
+         (region-width (- (overlay-end (car mouse-drag-and-drop-overlays))
+                          (overlay-start (car mouse-drag-and-drop-overlays))))
+         (region-height (length mouse-drag-and-drop-overlays))
          point-to-paste
          point-to-paste-read-only
          window-to-paste
@@ -2455,7 +2465,11 @@ mouse-drag-and-drop-region
           ;; Obtain the dragged text in region.  When the loop was
           ;; skipped, value-selection remains nil.
           (unless value-selection
-            (setq value-selection (buffer-substring start end))
+            (setq value-selection (funcall region-extract-function nil))
+            ;; Remove yank-handler property in order to re-insert text using
+            ;; the `insert-rectangle' function later on.
+            (remove-text-properties 0 (length value-selection)
+                                    '(yank-handler) value-selection)
             (when mouse-drag-and-drop-region-show-tooltip
               (let ((text-size mouse-drag-and-drop-region-show-tooltip))
                 (setq text-tooltip
@@ -2468,12 +2482,18 @@ mouse-drag-and-drop-region
                         value-selection))))
 
             ;; Check if selected text is read-only.
-            (setq text-from-read-only (or text-from-read-only
-                                          (get-text-property start 'read-only)
-                                          (not (equal
-                                                (next-single-char-property-change
-                                                 start 'read-only nil end)
-                                                end)))))
+            (setq text-from-read-only
+                  (or text-from-read-only
+                      (get-text-property start 'read-only)
+                      (get-text-property end 'read-only)
+                      (catch 'loop
+                             (dolist (bound (region-bounds))
+                               (unless (equal
+                                        (next-single-char-property-change
+                                         (car bound) 'read-only nil (cdr bound))
+                                        (cdr bound))
+                                 (throw 'loop t)))))))
+
           (setq window-to-paste (posn-window (event-end event)))
           (setq point-to-paste (posn-point (event-end event)))
           ;; Set nil when target buffer is minibuffer.
@@ -2500,12 +2520,20 @@ mouse-drag-and-drop-region
             ;; text will be inserted to inside of the original
             ;; region.
             (setq drag-but-negligible
-                  (and (eq (overlay-buffer mouse-drag-and-drop-overlay)
+                  (and (eq (overlay-buffer (car mouse-drag-and-drop-overlays))
                            buffer-to-paste)
-                       (<= (overlay-start mouse-drag-and-drop-overlay)
-                          point-to-paste)
-                       (<= point-to-paste
-                          (overlay-end mouse-drag-and-drop-overlay)))))
+                       (if region-noncontiguous
+                           ;; If the region is rectangular, check if the newly inserted
+                           ;; rectangular text would intersect the already selected
+                           ;; region. If it would, then set "drag-but-negligible" to t.
+                           (let ((size (cons region-width region-height)))
+                             (rectangle-intersect-p
+                              (rectangle-position-as-coordinates start) size
+                              (rectangle-position-as-coordinates point-to-paste) size))
+                         (and (<= (overlay-start (car mouse-drag-and-drop-overlays))
+                                  point-to-paste)
+                              (<= point-to-paste
+                                  (overlay-end (car mouse-drag-and-drop-overlays))))))))
 
           ;; Show a tooltip.
           (if mouse-drag-and-drop-region-show-tooltip
@@ -2524,8 +2552,9 @@ mouse-drag-and-drop-region
                                (t
                                 'bar)))
             (when cursor-in-text-area
-              (overlay-put mouse-drag-and-drop-overlay
-                           'face 'mouse-drag-and-drop-region)
+              (dolist (overlay mouse-drag-and-drop-overlays)
+                (overlay-put overlay
+                           'face 'mouse-drag-and-drop-region))
               (deactivate-mark)     ; Maintain region in other window.
               (mouse-set-point event)))))
 
@@ -2581,7 +2610,9 @@ mouse-drag-and-drop-region
           (select-window window)
           (goto-char point)
           (setq deactivate-mark nil)
-          (activate-mark))
+          (activate-mark)
+          (when region-noncontiguous
+            (rectangle-mark-mode)))
          ;; Modify buffers.
          (t
           ;; * DESTINATION BUFFER::
@@ -2590,11 +2621,17 @@ mouse-drag-and-drop-region
           (setq window-exempt window-to-paste)
           (goto-char point-to-paste)
           (push-mark)
-          (insert value-selection)
+
+          (if region-noncontiguous
+              (insert-rectangle (split-string value-selection "\n"))
+            (insert value-selection))
+
           ;; On success, set the text as region on destination buffer.
           (when (not (equal (mark) (point)))
             (setq deactivate-mark nil)
-            (activate-mark))
+            (activate-mark)
+            (when region-noncontiguous
+              (rectangle-mark-mode)))
 
           ;; * SOURCE BUFFER::
           ;; Set back the original text as region or delete the original
@@ -2604,8 +2641,9 @@ mouse-drag-and-drop-region
               ;; remove the original text.
               (when no-modifier-on-drop
                 (let (deactivate-mark)
-                  (delete-region (overlay-start mouse-drag-and-drop-overlay)
-                                 (overlay-end mouse-drag-and-drop-overlay))))
+                  (dolist (overlay mouse-drag-and-drop-overlays)
+                    (delete-region (overlay-start overlay)
+                                   (overlay-end overlay)))))
             ;; When source buffer and destination buffer are different,
             ;; keep (set back the original text as region) or remove the
             ;; original text.
@@ -2615,15 +2653,17 @@ mouse-drag-and-drop-region
             (if mouse-drag-and-drop-region-cut-when-buffers-differ
                 ;; Remove the dragged text from source buffer like
                 ;; operation `cut'.
-                (delete-region (overlay-start mouse-drag-and-drop-overlay)
-                               (overlay-end mouse-drag-and-drop-overlay))
+                (dolist (overlay mouse-drag-and-drop-overlays)
+                    (delete-region (overlay-start overlay)
+                                   (overlay-end overlay)))
               ;; Set back the dragged text as region on source buffer
               ;; like operation `copy'.
               (activate-mark))
             (select-window window-to-paste))))))
 
     ;; Clean up.
-    (delete-overlay mouse-drag-and-drop-overlay)
+    (dolist (overlay mouse-drag-and-drop-overlays)
+      (delete-overlay overlay))
 
     ;; Restore old states but for the window where the drop
     ;; occurred. Restore cursor types for all windows.
diff --git a/lisp/rect.el b/lisp/rect.el
index 8ccf051ee1..cac8be3625 100644
--- a/lisp/rect.el
+++ b/lisp/rect.el
@@ -167,6 +167,39 @@ apply-on-rectangle
                  (<= (point) endpt))))
       final-point)))
 
+(defun rectangle-position-as-coordinates (position)
+   "Return cons of the column and line values of POSITION.
+POSITION specifies a position of the current buffer.  The value
+returned is a cons of the current column of POSITION and its line
+number."
+  (save-excursion
+    (goto-char position)
+    (let ((col (current-column))
+          (line (progn
+                  (forward-line 0)
+                  (count-lines (point-min) position))))
+      (cons col line))))
+
+(defun rectangle-intersect-p (pos1 size1 pos2 size2)
+   "Return non-nil if two rectangles intersect.
+POS1 and POS2 specify the positions of the upper-left corners of
+the first and second rectangle as conses of their column and line
+values.  SIZE1 and SIZE2 specify the dimensions of the first and
+second rectangle, as conses of their width and height measured in
+columns and lines."
+  (let ((x1 (car pos1))
+        (y1 (cdr pos1))
+        (x2 (car pos2))
+        (y2 (cdr pos2))
+        (w1 (car size1))
+        (h1 (cdr size1))
+        (w2 (car size2))
+        (h2 (cdr size2)))
+    (not (or (<= (+ x1 w1) x2)
+             (<= (+ x2 w2) x1)
+             (<= (+ y1 h1) y2)
+             (<= (+ y2 h2) y1)))))
+
 (defun delete-rectangle-line (startcol endcol fill)
   (when (= (move-to-column startcol (if fill t 'coerce)) startcol)
     (delete-region (point)
-- 
2.17.1

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[Eli Zaretskii]

> From: Federico Tedin <federicotedin@gmail.com>
> Date: Sun, 30 Sep 2018 13:20:31 -0300
> Cc: 31240@debbugs.gnu.org
> 
> > although this still won't make your patch short enough to qualify as
> > "tiny change".  So if you haven't done so already, please start the
> > paperwork process so we can apply this patch.
> 
> This shouldn't be a problem, my copyright assignment was filed one or two months
> ago, and since then I've contributed two patches which have already been merged.

Your copyright assignment is already on file.

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[martin rudalics]

Looking again at this part

+          (line (progn
+                  (forward-line 0)
+                  (count-lines (point-min) position))))

I now ask myself why you need to go to the beginning of the line at
all.  'count-lines' doesn't care about the actual position of point.
So you either have to do

                   (count-lines (point-min) (point))

or just do

	          (count-lines (point-min) position)

without going to the beginning of the line because POSITION and the
position at the beginning of its line should be on the same line.

 > This shouldn't be a problem, my copyright assignment was filed one or two months
 > ago, and since then I've contributed two patches which have already been merged.

Great.

martin

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[Federico Tedin]

> Looking again at this part
>
> +          (line (progn
> +                  (forward-line 0)
> +                  (count-lines (point-min) position))))
>
> I now ask myself why you need to go to the beginning of the line at
> all.  'count-lines' doesn't care about the actual position of point.
> So you either have to do
>
>                    (count-lines (point-min) (point))
>
> or just do
>
>                   (count-lines (point-min) position)
>
> without going to the beginning of the line because POSITION and the
> position at the beginning of its line should be on the same line.

The problem with using just (count-lines (point-min) position) is that
the return value
is different when the point is on column 0 and when it is on column 1
or greater. I
needed to be sure that if the position was anywhere on line N, the
result was N (with
N starting at 0).

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[martin rudalics]

 > The problem with using just (count-lines (point-min) position) is that
 > the return value
 > is different when the point is on column 0 and when it is on column 1
 > or greater.

OK.  (count-lines (point-min) (point-min)) returns 0 because START and
END are equal so let's assume you want to special-case that.  Now the
problem with your code is that

+          (line (progn
+                  (forward-line 0)
+                  (count-lines (point-min) position))))

probably doesn't do what you expect: The value of POSITION in the call
of 'count-lines' is the _same_ it was before the 'forward-line' call
because 'forward-line' only changes the value of point but not that of
POSITION.  So if you want a possible return value of 0 you have to
write

+          (line (progn
+                  (forward-line 0)
+                  (count-lines (point-min) (point)))))

instead.

 > I
 > needed to be sure that if the position was anywhere on line N, the
 > result was N (with
 > N starting at 0).

Agreed, once more.  You want to special-case POSITION on the first
line of the buffer.  But go the ends of the following two forms and
type C-x C-e on each:

(let ((position 3))
   (save-excursion
     (goto-char position)
     (forward-line 0)
     (count-lines (point-min) position)))

(let ((position 3))
   (save-excursion
     (goto-char position)
     (forward-line 0)
     (count-lines (point-min) (point))))

Here the first evaluation gets me 1 and the second 0.  I suppose it's
the latter you want.

martin

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[Federico Tedin]

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

> Here the first evaluation gets me 1 and the second 0.  I suppose
> it's the latter you want.

Martin, thanks for your feedback. My use of the variable POSITON was a
mistake on my part, the correct thing to do was calling (point), as
the point had already been set to POSITION (by goto-char), and then
moved by forward-line.

While fixing the mistake, I found the function line-number-at-pos,
which uses the same method I was using (after your correction) to
calculate the line number. I'm attaching a new patch which uses this
function instead.

Slightly related question: is it better for me to keep sending patches
with all my changes and fixes included, or is it better to send an
initial one, and then send additional (smaller) patches with fixes to
the first one?

[-- Attachment #2: mouse.patch --]
[-- Type: text/x-patch, Size: 11521 bytes --]

From 715f56150c513d31b81fdc164e020d7225821964 Mon Sep 17 00:00:00 2001
From: Federico Tedin <federicotedin@gmail.com>
Date: Mon, 1 Oct 2018 18:27:18 -0300
Subject: [PATCH 1/1] Allow two mouse functions to work with Rectangle Mark
 mode

* lisp/mouse.el (mouse-save-then-kill): Make mouse-save-then-kill work
  with rectangular regions, including when mouse-drag-copy-region is
  set to t. (Bug#31240)
  (mouse-drag-and-drop-region): Allow dragging and dropping
  rectangular regions. (Bug#31240)
* rect.el (rectangle-intersect-p): Add a new function.
  (rectangle-position-as-coordinates): Add a new function.
---
 lisp/mouse.el | 92 ++++++++++++++++++++++++++++++++++++---------------
 lisp/rect.el  | 31 +++++++++++++++++
 2 files changed, 97 insertions(+), 26 deletions(-)

diff --git a/lisp/mouse.el b/lisp/mouse.el
index cb63ca51c5..b00f38a0f6 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -29,6 +29,8 @@
 
 ;;; Code:
 
+(eval-when-compile (require 'rect))
+
 ;;; Utility functions.
 
 ;; Indent track-mouse like progn.
@@ -1606,8 +1608,8 @@ mouse-save-then-kill
       (if mouse-drag-copy-region
           ;; Region already saved in the previous click;
           ;; don't make a duplicate entry, just delete.
-          (delete-region (mark t) (point))
-        (kill-region (mark t) (point)))
+          (funcall region-extract-function 'delete-only)
+        (kill-region (mark t) (point) 'region))
       (setq mouse-selection-click-count 0)
       (setq mouse-save-then-kill-posn nil))
 
@@ -1632,7 +1634,7 @@ mouse-save-then-kill
 	(mouse-set-region-1)
         (when mouse-drag-copy-region
           ;; Region already copied to kill-ring once, so replace.
-          (kill-new (filter-buffer-substring (mark t) (point)) t))
+          (kill-new (funcall region-extract-function nil) t))
 	;; Arrange for a repeated mouse-3 to kill the region.
 	(setq mouse-save-then-kill-posn click-pt)))
 
@@ -2411,7 +2413,15 @@ mouse-drag-and-drop-region
          (buffer (current-buffer))
          (window (selected-window))
          (text-from-read-only buffer-read-only)
-         (mouse-drag-and-drop-overlay (make-overlay start end))
+         ;; Use multiple overlays to cover cases where the region is rectangular.
+         (mouse-drag-and-drop-overlays (mapcar (lambda (bounds)
+                                                 (make-overlay (car bounds)
+                                                               (cdr bounds)))
+                                               (region-bounds)))
+         (region-noncontiguous (region-noncontiguous-p))
+         (region-width (- (overlay-end (car mouse-drag-and-drop-overlays))
+                          (overlay-start (car mouse-drag-and-drop-overlays))))
+         (region-height (length mouse-drag-and-drop-overlays))
          point-to-paste
          point-to-paste-read-only
          window-to-paste
@@ -2455,7 +2465,11 @@ mouse-drag-and-drop-region
           ;; Obtain the dragged text in region.  When the loop was
           ;; skipped, value-selection remains nil.
           (unless value-selection
-            (setq value-selection (buffer-substring start end))
+            (setq value-selection (funcall region-extract-function nil))
+            ;; Remove yank-handler property in order to re-insert text using
+            ;; the `insert-rectangle' function later on.
+            (remove-text-properties 0 (length value-selection)
+                                    '(yank-handler) value-selection)
             (when mouse-drag-and-drop-region-show-tooltip
               (let ((text-size mouse-drag-and-drop-region-show-tooltip))
                 (setq text-tooltip
@@ -2468,12 +2482,18 @@ mouse-drag-and-drop-region
                         value-selection))))
 
             ;; Check if selected text is read-only.
-            (setq text-from-read-only (or text-from-read-only
-                                          (get-text-property start 'read-only)
-                                          (not (equal
-                                                (next-single-char-property-change
-                                                 start 'read-only nil end)
-                                                end)))))
+            (setq text-from-read-only
+                  (or text-from-read-only
+                      (get-text-property start 'read-only)
+                      (get-text-property end 'read-only)
+                      (catch 'loop
+                             (dolist (bound (region-bounds))
+                               (unless (equal
+                                        (next-single-char-property-change
+                                         (car bound) 'read-only nil (cdr bound))
+                                        (cdr bound))
+                                 (throw 'loop t)))))))
+
           (setq window-to-paste (posn-window (event-end event)))
           (setq point-to-paste (posn-point (event-end event)))
           ;; Set nil when target buffer is minibuffer.
@@ -2500,12 +2520,20 @@ mouse-drag-and-drop-region
             ;; text will be inserted to inside of the original
             ;; region.
             (setq drag-but-negligible
-                  (and (eq (overlay-buffer mouse-drag-and-drop-overlay)
+                  (and (eq (overlay-buffer (car mouse-drag-and-drop-overlays))
                            buffer-to-paste)
-                       (<= (overlay-start mouse-drag-and-drop-overlay)
-                          point-to-paste)
-                       (<= point-to-paste
-                          (overlay-end mouse-drag-and-drop-overlay)))))
+                       (if region-noncontiguous
+                           ;; If the region is rectangular, check if the newly inserted
+                           ;; rectangular text would intersect the already selected
+                           ;; region. If it would, then set "drag-but-negligible" to t.
+                           (let ((size (cons region-width region-height)))
+                             (rectangle-intersect-p
+                              (rectangle-position-as-coordinates start) size
+                              (rectangle-position-as-coordinates point-to-paste) size))
+                         (and (<= (overlay-start (car mouse-drag-and-drop-overlays))
+                                  point-to-paste)
+                              (<= point-to-paste
+                                  (overlay-end (car mouse-drag-and-drop-overlays))))))))
 
           ;; Show a tooltip.
           (if mouse-drag-and-drop-region-show-tooltip
@@ -2524,8 +2552,9 @@ mouse-drag-and-drop-region
                                (t
                                 'bar)))
             (when cursor-in-text-area
-              (overlay-put mouse-drag-and-drop-overlay
-                           'face 'mouse-drag-and-drop-region)
+              (dolist (overlay mouse-drag-and-drop-overlays)
+                (overlay-put overlay
+                           'face 'mouse-drag-and-drop-region))
               (deactivate-mark)     ; Maintain region in other window.
               (mouse-set-point event)))))
 
@@ -2581,7 +2610,9 @@ mouse-drag-and-drop-region
           (select-window window)
           (goto-char point)
           (setq deactivate-mark nil)
-          (activate-mark))
+          (activate-mark)
+          (when region-noncontiguous
+            (rectangle-mark-mode)))
          ;; Modify buffers.
          (t
           ;; * DESTINATION BUFFER::
@@ -2590,11 +2621,17 @@ mouse-drag-and-drop-region
           (setq window-exempt window-to-paste)
           (goto-char point-to-paste)
           (push-mark)
-          (insert value-selection)
+
+          (if region-noncontiguous
+              (insert-rectangle (split-string value-selection "\n"))
+            (insert value-selection))
+
           ;; On success, set the text as region on destination buffer.
           (when (not (equal (mark) (point)))
             (setq deactivate-mark nil)
-            (activate-mark))
+            (activate-mark)
+            (when region-noncontiguous
+              (rectangle-mark-mode)))
 
           ;; * SOURCE BUFFER::
           ;; Set back the original text as region or delete the original
@@ -2604,8 +2641,9 @@ mouse-drag-and-drop-region
               ;; remove the original text.
               (when no-modifier-on-drop
                 (let (deactivate-mark)
-                  (delete-region (overlay-start mouse-drag-and-drop-overlay)
-                                 (overlay-end mouse-drag-and-drop-overlay))))
+                  (dolist (overlay mouse-drag-and-drop-overlays)
+                    (delete-region (overlay-start overlay)
+                                   (overlay-end overlay)))))
             ;; When source buffer and destination buffer are different,
             ;; keep (set back the original text as region) or remove the
             ;; original text.
@@ -2615,15 +2653,17 @@ mouse-drag-and-drop-region
             (if mouse-drag-and-drop-region-cut-when-buffers-differ
                 ;; Remove the dragged text from source buffer like
                 ;; operation `cut'.
-                (delete-region (overlay-start mouse-drag-and-drop-overlay)
-                               (overlay-end mouse-drag-and-drop-overlay))
+                (dolist (overlay mouse-drag-and-drop-overlays)
+                    (delete-region (overlay-start overlay)
+                                   (overlay-end overlay)))
               ;; Set back the dragged text as region on source buffer
               ;; like operation `copy'.
               (activate-mark))
             (select-window window-to-paste))))))
 
     ;; Clean up.
-    (delete-overlay mouse-drag-and-drop-overlay)
+    (dolist (overlay mouse-drag-and-drop-overlays)
+      (delete-overlay overlay))
 
     ;; Restore old states but for the window where the drop
     ;; occurred. Restore cursor types for all windows.
diff --git a/lisp/rect.el b/lisp/rect.el
index 8ccf051ee1..48db4ffd8f 100644
--- a/lisp/rect.el
+++ b/lisp/rect.el
@@ -167,6 +167,37 @@ apply-on-rectangle
                  (<= (point) endpt))))
       final-point)))
 
+(defun rectangle-position-as-coordinates (position)
+   "Return cons of the column and line values of POSITION.
+POSITION specifies a position of the current buffer.  The value
+returned is a cons of the current column of POSITION and its line
+number."
+  (save-excursion
+    (goto-char position)
+    (let ((col (current-column))
+          (line (1- (line-number-at-pos))))
+      (cons col line))))
+
+(defun rectangle-intersect-p (pos1 size1 pos2 size2)
+   "Return non-nil if two rectangles intersect.
+POS1 and POS2 specify the positions of the upper-left corners of
+the first and second rectangle as conses of their column and line
+values.  SIZE1 and SIZE2 specify the dimensions of the first and
+second rectangle, as conses of their width and height measured in
+columns and lines."
+  (let ((x1 (car pos1))
+        (y1 (cdr pos1))
+        (x2 (car pos2))
+        (y2 (cdr pos2))
+        (w1 (car size1))
+        (h1 (cdr size1))
+        (w2 (car size2))
+        (h2 (cdr size2)))
+    (not (or (<= (+ x1 w1) x2)
+             (<= (+ x2 w2) x1)
+             (<= (+ y1 h1) y2)
+             (<= (+ y2 h2) y1)))))
+
 (defun delete-rectangle-line (startcol endcol fill)
   (when (= (move-to-column startcol (if fill t 'coerce)) startcol)
     (delete-region (point)
-- 
2.17.1

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[martin rudalics]

 > While fixing the mistake, I found the function line-number-at-pos,
 > which uses the same method I was using (after your correction) to
 > calculate the line number. I'm attaching a new patch which uses this
 > function instead.

OK.  I'll install it in a few days if there are no objections.

BTW 'line-number-at-pos' is overly complicated, there is no need for
'start' in it, it should simply do

       (save-excursion
         (goto-char opoint)
         (forward-line 0)
         (1+ (count-lines (point-min) (point)))))))

instead.

 > Slightly related question: is it better for me to keep sending patches
 > with all my changes and fixes included, or is it better to send an
 > initial one, and then send additional (smaller) patches with fixes to
 > the first one?

Personally, I prefer "patches with all changes and fixes included"
because that makes it easy for me to throw them away immediately and
resurrect them any time later.

martin

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[Federico Tedin]

> BTW 'line-number-at-pos' is overly complicated, there is no need for
> 'start' in it, it should simply do
>
>        (save-excursion
>          (goto-char opoint)
>          (forward-line 0)
>          (1+ (count-lines (point-min) (point)))))))
>
> instead.

I agree. On the other hand, I imagined it was better to re-use an existing
function which had maybe dealed with problems I hadn't thought of. Should
I leave the last patch as it is?

> Personally, I prefer "patches with all changes and fixes included"
> because that makes it easy for me to throw them away immediately and
> resurrect them any time later.
>
> martin

Great!

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[martin rudalics]

> I agree. On the other hand, I imagined it was better to re-use an existing
> function which had maybe dealed with problems I hadn't thought of. Should
> I leave the last patch as it is?

Sure.

martin

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[Homeros Misasa]

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

  ;; This [buff]er is fo
  ;; To cr[eate] a file,

When I grab a rectangle as shown above, I cannot move the rectangle by
1 char to the left, because of usage of `rectangle-intersect-p'.
Behavior on moving up, right, and left seems good.

[-- Attachment #2: Type: text/html, Size: 310 bytes --]

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[martin rudalics]

 >    ;; This [buff]er is fo
 >    ;; To cr[eate] a file,
 >
 > When I grab a rectangle as shown above, I cannot move the rectangle by
 > 1 char to the left, because of usage of `rectangle-intersect-p'.
 > Behavior on moving up, right, and left seems good.

Please tell us more precisely what you tried to do.  With

(setq mouse-drag-and-drop-region 'shift)

dragging the rectangle by one "character to the left" and dropping it
with the shift key pressed gets me

;; Thisbuff buffer is for text that is not saved, and for Lisp evaluation.
;; To ceatereate a file, visit it with C-x C-f and enter text in its buffer.

martin

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[Tak Kunihiro]

>> ;; This [buff]er is fo
>> ;; To cr[eate] a file,
>>
>> When I grab a rectangle as shown above, I cannot move the rectangle by
>> 1 char to the left, because of usage of `rectangle-intersect-p'.
>> Behavior on moving up, right, and left seems good.
>
> (setq mouse-drag-and-drop-region 'shift)
>
> dragging the rectangle by one "character to the left" and dropping it
> with the shift key pressed gets me
>
> ;; Thisbuff buffer is for text that is not saved,
> ;; To ceatereate a file, visit it with C-x C-f an

Yes. Dragging the rectangle by one "character to the left" and dropping it
with the shift key pressed work as expected.

start> ;; This [buff]er is for text that is not saved, and for Lisp evaluation.
start> ;; To cr[eate], vie a filsit it with <open> and enter text in its buffer.

goal > ;; Thisbuff buffer is for text that is not saved,
goal > ;; To ceatereate a file, visit it with C-x C-f an

I want to kill the rectangle and yank it to column 7 by dropping it
without the shift key pressed.

       0123456
start> ;; This [buff]er is for text that is not saved,
start> ;; To cr[eate], vie a filsit it with <open> and

goal > ;; This[buff] er is for text that is not saved, and for Lisp evaluation.
goal > ;; To c[eate]r, vie a filsit it with <open> and enter text in its buffer.

When the `inserting' cursor is located at left side of rectangle and on
the same line as the first line of the rectangle, user should be allowed
to move the rectangle, I think.

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[Federico Tedin]

> When the `inserting' cursor is located at left side of rectangle and on
> the same line as the first line of the rectangle, user should be allowed
> to move the rectangle, I think.

The example Homeros tried to run won't work, because the newly intersected
rectangle would intersect the original rectangle. This behaviour is
intentional, as it
is similar to mouse-drag-and-drop-region's behaviour with non-rectangle regions.

However, I think an exception could be made for rectangle operations move the
rectangle horizontally to the left, as Tak said. When the rectangle is
dragged to the left,
there's no way its content could be accidentally added to the overlays
tracking the original
rectangle, so it'll always work correctly (even if
rectangle-intersect-p returns t).

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[martin rudalics]

 > The example Homeros tried to run won't work, because the newly intersected
 > rectangle would intersect the original rectangle. This behaviour is
 > intentional, as it
 > is similar to mouse-drag-and-drop-region's behaviour with non-rectangle regions.
 >
 > However, I think an exception could be made for rectangle operations move the
 > rectangle horizontally to the left, as Tak said. When the rectangle is
 > dragged to the left,
 > there's no way its content could be accidentally added to the overlays
 > tracking the original
 > rectangle, so it'll always work correctly (even if
 > rectangle-intersect-p returns t).

FWIW I see no problems with the following naive amendment

                            (let ((size (cons region-width region-height)))
                              (and (rectangle-intersect-p
                                    (rectangle-position-as-coordinates start) size
                                    (rectangle-position-as-coordinates point-to-paste) size)
                                   (not (= (line-number-at-pos start)
                                           (line-number-at-pos point-to-paste)))))

which also allows shifting the rectangle strictly to the left or
right.  IIUC it's the deletion of the original rectangle after the
copy was inserted on a different line that's causing havoc with
intersections.  But I have not tested all possible variants so please
correct me if I'm wrong.

martin

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[Federico Tedin]

> FWIW I see no problems with the following naive amendment
>
>                             (let ((size (cons region-width region-height)))
>                               (and (rectangle-intersect-p
>                                     (rectangle-position-as-coordinates start) size
>                                     (rectangle-position-as-coordinates point-to-paste) size)
>                                    (not (= (line-number-at-pos start)
>                                            (line-number-at-pos point-to-paste)))))
>
> which also allows shifting the rectangle strictly to the left or
> right.  IIUC it's the deletion of the original rectangle after the
> copy was inserted on a different line that's causing havoc with
> intersections.  But I have not tested all possible variants so please
> correct me if I'm wrong.

The problem with this is that when the rectangle is inserted to the
right of the original one, it is inserted inside the overlays tracking
the selected rectangular region. When the original rectangle is then
deleted (by deleting the overlays), the newly inserted rectangle is
also deleted. You can try this by dragging a rectangle 1 char to the
right.

This problem exists because mouse-drag-and-drop-region first inserts
the dragged contents, and then deletes the original selection. Is
there a reason this has been implemented like this? It sounds like it
would make more sense to first delete the original selection, and then
insert it elsewhere. This would be almost the same as killing and then
yanking some text. That being said, for non-rectangular regions, any
of the two methods works the same.

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[martin rudalics]

 > The problem with this is that when the rectangle is inserted to the
 > right of the original one, it is inserted inside the overlays tracking
 > the selected rectangular region. When the original rectangle is then
 > deleted (by deleting the overlays), the newly inserted rectangle is
 > also deleted. You can try this by dragging a rectangle 1 char to the
 > right.

You're probably right, I have no knowledge of the rectangle code.
Another naive try: Disallow inserting the rectangle in the first row
of the original because that case is almost identical to that of
inserting a contiguous region into itself.

(if region-noncontiguous
     (let ((size (cons region-width region-height)))
       (and (rectangle-intersect-p
             (rectangle-position-as-coordinates start) size
             (rectangle-position-as-coordinates point-to-paste) size)
            (or (not (= (line-number-at-pos start)
                        (line-number-at-pos point-to-paste)))
                (and (>= start point-to-paste)
                     (<= point-to-paste (+ start (car size))))))))

 > This problem exists because mouse-drag-and-drop-region first inserts
 > the dragged contents, and then deletes the original selection. Is
 > there a reason this has been implemented like this? It sounds like it
 > would make more sense to first delete the original selection, and then
 > insert it elsewhere. This would be almost the same as killing and then
 > yanking some text. That being said, for non-rectangular regions, any
 > of the two methods works the same.

Maybe undoing the operation is more conservative the way we do it now:
Currently, a first undo step would restore the original and a second
step would delete the copy.  Otherwise, we would delete the copy first
and then restore the original which would leave us with no clue of
whatever the region contained after the first undo step.  Kunihiro-san
could tell us more about this, maybe.

OTOH if reverting the order yields good results when moving the text
to an intersecting but different line we could special-case the delete
first/copy afterwards behavior on region-noncontiguous.

martin

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[Tak Kunihiro]

>> This problem exists because mouse-drag-and-drop-region first inserts
>> the dragged contents, and then deletes the original selection. Is
>> there a reason this has been implemented like this? It sounds like it
>> would make more sense to first delete the original selection, and then
>> insert it elsewhere. This would be almost the same as killing and then
>> yanking some text. That being said, for non-rectangular regions, any
>> of the two methods works the same.

I appreciate mouse-drag-and-drop-region when I move a sentence because
the sentence never disappears from screen.  Based on this criteria,
mouse-drag-and-drop-region inserts first and delete second.
Practically it is quick, thus order does not matter much.

There are two cases for dragging a sentence, that are ones with (1)
deleting the original selection and (2) keeping it.  Since to insert
is common for two cases, deletion of the original selection comes
later.  If it works, order does not matter much.

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[Federico Tedin]

> (if region-noncontiguous
>      (let ((size (cons region-width region-height)))
>        (and (rectangle-intersect-p
>              (rectangle-position-as-coordinates start) size
>              (rectangle-position-as-coordinates point-to-paste) size)
>             (or (not (= (line-number-at-pos start)
>                         (line-number-at-pos point-to-paste)))
>                 (and (>= start point-to-paste)
>                      (<= point-to-paste (+ start (car size))))))))

Martin, I didn't manage to understand the condition you are testing:

(and (>= start point-to-paste)
     (<= point-to-paste (+ start (car size))))))))

If the first condition is true, the second will also be true (if
point-to-paste is smaller than start, point-to-paste will also be
smaller than start plus a positive integer).

> Maybe undoing the operation is more conservative the way we do it now:
> Currently, a first undo step would restore the original and a second
> step would delete the copy.  Otherwise, we would delete the copy first
> and then restore the original which would leave us with no clue of
> whatever the region contained after the first undo step.  Kunihiro-san
> could tell us more about this, maybe.

> I appreciate mouse-drag-and-drop-region when I move a sentence because
> the sentence never disappears from screen.  Based on this criteria,
> mouse-drag-and-drop-region inserts first and delete second.
> Practically it is quick, thus order does not matter much.

From what I understand, these two explanations refer to the same
thing: it is better to have at all times the dragged region somewhere
in the buffer (even when undoing the drag operation). It would be best
to keep this behaviour in future versions of
mouse-drag-and-drop-region.

> There are two cases for dragging a sentence, that are ones with (1)
> deleting the original selection and (2) keeping it.  Since to insert
> is common for two cases, deletion of the original selection comes
> later.  If it works, order does not matter much.

I also agree with this, it is tidier to leave the optional operation
at the end.

> OTOH if reverting the order yields good results when moving the text
> to an intersecting but different line we could special-case the delete
> first/copy afterwards behavior on region-noncontiguous.

I'll check if I can come up with a better solution that best fits the
points that have been discussed above. If I can't, I'll check how
complex it would be to implement this special case at the end.

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[martin rudalics]

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

 > Martin, I didn't manage to understand the condition you are testing:
 >
 > (and (>= start point-to-paste)
 >       (<= point-to-paste (+ start (car size))))))))

Obviously so.  It should be

(and (<= start point-to-paste)
      (<= point-to-paste (+ start (car size))))))))

I attach a diff of this very chunk against the original trunk version
I have here.  Sorry for my sloppiness.

The idea is to not set drag-but-negligible when point-to-paste is (1)
on the first line of the original region and (2) before its start or
after its end.  To elaborate:

If point-to-paste is on the first line of the original region in
between its start and end, we would try to replace the text by itself
on this and all subsequent lines which is just as silly as replacing a
contiguous region with itself.  So set drag-but-negligible in such a
case.

If point-to-paste is not on the first line of the original region but
the region to insert intersects with the original region, then killing
the original region after inserting the copy might get me some pretty
unintelligible result.  So set drag-but-negligible in that case as
well.

But moving the rectangle strictly to the left or the right on the same
lines it originally occupied seems to work here.  So I would not set
drag-but-negligible in that case.

 > If the first condition is true, the second will also be true (if
 > point-to-paste is smaller than start, point-to-paste will also be
 > smaller than start plus a positive integer).

Indeed.

 >>From what I understand, these two explanations refer to the same
 > thing: it is better to have at all times the dragged region somewhere
 > in the buffer (even when undoing the drag operation). It would be best
 > to keep this behaviour in future versions of
 > mouse-drag-and-drop-region.

This looks like an invariant we could try to preserve.  But I only
tried to guess the rationale of the original design for moving a
contiguous region.

Note that I have never written or used rectangle code.  AFAICT
'insert-rectangle' inserts spaces (via 'move-to-column') when the
target line is too short.  And these insertions can make the result of
a rectangle move look bad when the deletion is done after the copying.
Is that interpretation correct?  Does it always look intelligible when
the deletion is done before the copying?

 > I'll check if I can come up with a better solution that best fits the
 > points that have been discussed above. If I can't, I'll check how
 > complex it would be to implement this special case at the end.

IIUC the code of rect.el nowhere provides an operation that kills a
rectangle and reinserts it in one and the same step not to mention the
possibility that these operations take place in an overlapping area of
one and the same buffer.  So I think that you are in uncharted waters
here and hardly anyone will tell you what to do.

If worse comes to worst, we can always leave your patch as it is now.

martin

[-- Attachment #2: mouse-drag-and-drop-region.diff --]
[-- Type: text/plain, Size: 1911 bytes --]

@@ -2500,12 +2520,24 @@ mouse-drag-and-drop-region
             ;; text will be inserted to inside of the original
             ;; region.
             (setq drag-but-negligible
-                  (and (eq (overlay-buffer mouse-drag-and-drop-overlay)
+                  (and (eq (overlay-buffer (car mouse-drag-and-drop-overlays))
                            buffer-to-paste)
-                       (<= (overlay-start mouse-drag-and-drop-overlay)
-                          point-to-paste)
-                       (<= point-to-paste
-                          (overlay-end mouse-drag-and-drop-overlay)))))
+                       (if region-noncontiguous
+                           ;; If the region is rectangular, check if the newly inserted
+                           ;; rectangular text would intersect the already selected
+                           ;; region. If it would, then set "drag-but-negligible" to t.
+                           (let ((size (cons region-width region-height)))
+                             (and (rectangle-intersect-p
+                                   (rectangle-position-as-coordinates start) size
+                                   (rectangle-position-as-coordinates point-to-paste) size)
+                                  (or (not (= (line-number-at-pos start)
+                                              (line-number-at-pos point-to-paste)))
+                                      (and (<= start point-to-paste)
+                                           (<= point-to-paste (+ start (car size)))))))
+                         (and (<= (overlay-start (car mouse-drag-and-drop-overlays))
+                                  point-to-paste)
+                              (<= point-to-paste
+                                  (overlay-end (car mouse-drag-and-drop-overlays))))))))
 
           ;; Show a tooltip.
           (if mouse-drag-and-drop-region-show-tooltip

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[martin rudalics]

 > It should be
 >
 > (and (<= start point-to-paste)
 >       (<= point-to-paste (+ start (car size))))))))

I forgot to mention that the second conjunct is probably superfluous
here because in that case the rectangles won't intersect in the first
place.  So (<= start point-to-paste) should be sufficient but I have
not tested it.

martin

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[Federico Tedin]

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

> If point-to-paste is on the first line of the original region in
> between its start and end, we would try to replace the text by itself
> on this and all subsequent lines which is just as silly as replacing a
> contiguous region with itself.  So set drag-but-negligible in such a
> case.
>
> If point-to-paste is not on the first line of the original region but
> the region to insert intersects with the original region, then killing
> the original region after inserting the copy might get me some pretty
> unintelligible result.  So set drag-but-negligible in that case as
> well.
>
> But moving the rectangle strictly to the left or the right on the same
> lines it originally occupied seems to work here.  So I would not set
> drag-but-negligible in that case.
>
> I forgot to mention that the second conjunct is probably superfluous
> here because in that case the rectangles won't intersect in the first
> place.  So (<= start point-to-paste) should be sufficient but I have
> not tested it.

Martin, thanks again for your feedback.

As you mentioned, dragging the rectangle to the right of its right
edge will cause rectangle-intersect-p return nil, so that check will
never be evaluated. If I drag the rectangle to the right, but in a way
that it intersects with itself (e.g. one char), then its contents
would be inserted
into the overlays, and it would then be deleted when the original is
deleted (this is why I originally added the intersection check).

If I drag the rectangle to the left in a way it would intersect with
itself, then obviously rectangle-intersect-p will return t. However,
what's interesting about this case is that the insert and delete
operations could be done anyways, as the new content would be inserted
to the left of the overlays, causing them to shift to the right. Then,
the overlays would be deleted, taking with them the original
rectangle. Note that this works even if the drag has a vertical
component (up/down).

So then: moving the rectangle to the left is easy, but moving it to
the right (into itself) is not. My solution was to implement
rectangle-intersection-p to avoid doing the insert and delete if an
intersection was found. One of the conditions you added on your patch
allows moving the rectangle sideways: in my opinion, allowing the user
to move the rectangle only to the left is what's more consistent with
mouse-drag-and-drop-region's behaviour with non-rectangular
regions. You can check this by selecting a word and dragging it one
char to its left: the word is dragged correctly. But if you drag a
word one char to the right, the operation is marked as negligible. It
would make sense that moving a rectangle to the right or to the left
followed the same behaviour.

> Note that I have never written or used rectangle code.  AFAICT
> 'insert-rectangle' inserts spaces (via 'move-to-column') when the
> target line is too short.  And these insertions can make the result of
> a rectangle move look bad when the deletion is done after the copying.
> Is that interpretation correct?  Does it always look intelligible when
> the deletion is done before the copying?

Your guess is probably much better than mine, but I think you're
right. Inserting a rectangle somewhere in a buffer, and then deleting
some text to its left will make it look intelligible. If the deletion
is made before the copying, however, I think the rectangle will always
be inserted correctly (because move-to-column will ensure its content
is aligned correctly).

> IIUC the code of rect.el nowhere provides an operation that kills a
> rectangle and reinserts it in one and the same step not to mention the
> possibility that these operations take place in an overlapping area of
> one and the same buffer.  So I think that you are in uncharted waters
> here and hardly anyone will tell you what to do.
>
> If worse comes to worst, we can always leave your patch as it is now.

At the moment, my current proposal is: allow the user to drag the
rectangle anywhere to the left (on the same line, or on a different
line), or anywhere else where it would not intersect with itself (I'm
attaching a new patch that allows this).

- Federico

[-- Attachment #2: mouse.patch --]
[-- Type: text/x-patch, Size: 12095 bytes --]

From 5888ce9739c3298a5f1939acfaf87158c1023fd9 Mon Sep 17 00:00:00 2001
From: Federico Tedin <federicotedin@gmail.com>
Date: Thu, 11 Oct 2018 21:29:45 -0300
Subject: [PATCH] Allow two mouse functions to work with Rectangle Mark mode

* lisp/mouse.el (mouse-save-then-kill): Make mouse-save-then-kill work
  with rectangular regions, including when mouse-drag-copy-region is
  set to t. (Bug#31240)
  (mouse-drag-and-drop-region): Allow dragging and dropping
  rectangular regions. (Bug#31240)
* rect.el (rectangle-intersect-p): Add a new function.
  (rectangle-position-as-coordinates): Add a new function.
---
 lisp/mouse.el | 99 +++++++++++++++++++++++++++++++++++++--------------
 lisp/rect.el  | 31 ++++++++++++++++
 2 files changed, 104 insertions(+), 26 deletions(-)

diff --git a/lisp/mouse.el b/lisp/mouse.el
index cb63ca51c5..37f4087e5f 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -29,6 +29,8 @@
 
 ;;; Code:
 
+(eval-when-compile (require 'rect))
+
 ;;; Utility functions.
 
 ;; Indent track-mouse like progn.
@@ -1606,8 +1608,8 @@ mouse-save-then-kill
       (if mouse-drag-copy-region
           ;; Region already saved in the previous click;
           ;; don't make a duplicate entry, just delete.
-          (delete-region (mark t) (point))
-        (kill-region (mark t) (point)))
+          (funcall region-extract-function 'delete-only)
+        (kill-region (mark t) (point) 'region))
       (setq mouse-selection-click-count 0)
       (setq mouse-save-then-kill-posn nil))
 
@@ -1632,7 +1634,7 @@ mouse-save-then-kill
 	(mouse-set-region-1)
         (when mouse-drag-copy-region
           ;; Region already copied to kill-ring once, so replace.
-          (kill-new (filter-buffer-substring (mark t) (point)) t))
+          (kill-new (funcall region-extract-function nil) t))
 	;; Arrange for a repeated mouse-3 to kill the region.
 	(setq mouse-save-then-kill-posn click-pt)))
 
@@ -2411,7 +2413,15 @@ mouse-drag-and-drop-region
          (buffer (current-buffer))
          (window (selected-window))
          (text-from-read-only buffer-read-only)
-         (mouse-drag-and-drop-overlay (make-overlay start end))
+         ;; Use multiple overlays to cover cases where the region is rectangular.
+         (mouse-drag-and-drop-overlays (mapcar (lambda (bounds)
+                                                 (make-overlay (car bounds)
+                                                               (cdr bounds)))
+                                               (region-bounds)))
+         (region-noncontiguous (region-noncontiguous-p))
+         (region-width (- (overlay-end (car mouse-drag-and-drop-overlays))
+                          (overlay-start (car mouse-drag-and-drop-overlays))))
+         (region-height (length mouse-drag-and-drop-overlays))
          point-to-paste
          point-to-paste-read-only
          window-to-paste
@@ -2455,7 +2465,11 @@ mouse-drag-and-drop-region
           ;; Obtain the dragged text in region.  When the loop was
           ;; skipped, value-selection remains nil.
           (unless value-selection
-            (setq value-selection (buffer-substring start end))
+            (setq value-selection (funcall region-extract-function nil))
+            ;; Remove yank-handler property in order to re-insert text using
+            ;; the `insert-rectangle' function later on.
+            (remove-text-properties 0 (length value-selection)
+                                    '(yank-handler) value-selection)
             (when mouse-drag-and-drop-region-show-tooltip
               (let ((text-size mouse-drag-and-drop-region-show-tooltip))
                 (setq text-tooltip
@@ -2468,12 +2482,18 @@ mouse-drag-and-drop-region
                         value-selection))))
 
             ;; Check if selected text is read-only.
-            (setq text-from-read-only (or text-from-read-only
-                                          (get-text-property start 'read-only)
-                                          (not (equal
-                                                (next-single-char-property-change
-                                                 start 'read-only nil end)
-                                                end)))))
+            (setq text-from-read-only
+                  (or text-from-read-only
+                      (get-text-property start 'read-only)
+                      (get-text-property end 'read-only)
+                      (catch 'loop
+                             (dolist (bound (region-bounds))
+                               (unless (equal
+                                        (next-single-char-property-change
+                                         (car bound) 'read-only nil (cdr bound))
+                                        (cdr bound))
+                                 (throw 'loop t)))))))
+
           (setq window-to-paste (posn-window (event-end event)))
           (setq point-to-paste (posn-point (event-end event)))
           ;; Set nil when target buffer is minibuffer.
@@ -2500,12 +2520,27 @@ mouse-drag-and-drop-region
             ;; text will be inserted to inside of the original
             ;; region.
             (setq drag-but-negligible
-                  (and (eq (overlay-buffer mouse-drag-and-drop-overlay)
+                  (and (eq (overlay-buffer (car mouse-drag-and-drop-overlays))
                            buffer-to-paste)
-                       (<= (overlay-start mouse-drag-and-drop-overlay)
-                          point-to-paste)
-                       (<= point-to-paste
-                          (overlay-end mouse-drag-and-drop-overlay)))))
+                       (if region-noncontiguous
+                           ;; If the region is rectangular, check if the newly inserted
+                           ;; rectangular text would intersect the already selected
+                           ;; region. If it would, then set "drag-but-negligible" to t.
+                           ;; As a special case, allow dragging the region freely anywhere
+                           ;; to the left, as this will never trigger its contents to be
+                           ;; inserted into the overlays tracking it.
+                           (let ((size (cons region-width region-height))
+                                 (start-coordinates (rectangle-position-as-coordinates start))
+                                 (point-to-paste-coordinates (rectangle-position-as-coordinates point-to-paste)))
+                             (and (rectangle-intersect-p
+                                   start-coordinates size
+                                   point-to-paste-coordinates size)
+                                  (not (<= (car point-to-paste-coordinates)
+                                           (car start-coordinates)))))
+                         (and (<= (overlay-start (car mouse-drag-and-drop-overlays))
+                                  point-to-paste)
+                              (<= point-to-paste
+                                  (overlay-end (car mouse-drag-and-drop-overlays))))))))
 
           ;; Show a tooltip.
           (if mouse-drag-and-drop-region-show-tooltip
@@ -2524,8 +2559,9 @@ mouse-drag-and-drop-region
                                (t
                                 'bar)))
             (when cursor-in-text-area
-              (overlay-put mouse-drag-and-drop-overlay
-                           'face 'mouse-drag-and-drop-region)
+              (dolist (overlay mouse-drag-and-drop-overlays)
+                (overlay-put overlay
+                           'face 'mouse-drag-and-drop-region))
               (deactivate-mark)     ; Maintain region in other window.
               (mouse-set-point event)))))
 
@@ -2581,7 +2617,9 @@ mouse-drag-and-drop-region
           (select-window window)
           (goto-char point)
           (setq deactivate-mark nil)
-          (activate-mark))
+          (activate-mark)
+          (when region-noncontiguous
+            (rectangle-mark-mode)))
          ;; Modify buffers.
          (t
           ;; * DESTINATION BUFFER::
@@ -2590,11 +2628,17 @@ mouse-drag-and-drop-region
           (setq window-exempt window-to-paste)
           (goto-char point-to-paste)
           (push-mark)
-          (insert value-selection)
+
+          (if region-noncontiguous
+              (insert-rectangle (split-string value-selection "\n"))
+            (insert value-selection))
+
           ;; On success, set the text as region on destination buffer.
           (when (not (equal (mark) (point)))
             (setq deactivate-mark nil)
-            (activate-mark))
+            (activate-mark)
+            (when region-noncontiguous
+              (rectangle-mark-mode)))
 
           ;; * SOURCE BUFFER::
           ;; Set back the original text as region or delete the original
@@ -2604,8 +2648,9 @@ mouse-drag-and-drop-region
               ;; remove the original text.
               (when no-modifier-on-drop
                 (let (deactivate-mark)
-                  (delete-region (overlay-start mouse-drag-and-drop-overlay)
-                                 (overlay-end mouse-drag-and-drop-overlay))))
+                  (dolist (overlay mouse-drag-and-drop-overlays)
+                    (delete-region (overlay-start overlay)
+                                   (overlay-end overlay)))))
             ;; When source buffer and destination buffer are different,
             ;; keep (set back the original text as region) or remove the
             ;; original text.
@@ -2615,15 +2660,17 @@ mouse-drag-and-drop-region
             (if mouse-drag-and-drop-region-cut-when-buffers-differ
                 ;; Remove the dragged text from source buffer like
                 ;; operation `cut'.
-                (delete-region (overlay-start mouse-drag-and-drop-overlay)
-                               (overlay-end mouse-drag-and-drop-overlay))
+                (dolist (overlay mouse-drag-and-drop-overlays)
+                    (delete-region (overlay-start overlay)
+                                   (overlay-end overlay)))
               ;; Set back the dragged text as region on source buffer
               ;; like operation `copy'.
               (activate-mark))
             (select-window window-to-paste))))))
 
     ;; Clean up.
-    (delete-overlay mouse-drag-and-drop-overlay)
+    (dolist (overlay mouse-drag-and-drop-overlays)
+      (delete-overlay overlay))
 
     ;; Restore old states but for the window where the drop
     ;; occurred. Restore cursor types for all windows.
diff --git a/lisp/rect.el b/lisp/rect.el
index 8ccf051ee1..48db4ffd8f 100644
--- a/lisp/rect.el
+++ b/lisp/rect.el
@@ -167,6 +167,37 @@ apply-on-rectangle
                  (<= (point) endpt))))
       final-point)))
 
+(defun rectangle-position-as-coordinates (position)
+   "Return cons of the column and line values of POSITION.
+POSITION specifies a position of the current buffer.  The value
+returned is a cons of the current column of POSITION and its line
+number."
+  (save-excursion
+    (goto-char position)
+    (let ((col (current-column))
+          (line (1- (line-number-at-pos))))
+      (cons col line))))
+
+(defun rectangle-intersect-p (pos1 size1 pos2 size2)
+   "Return non-nil if two rectangles intersect.
+POS1 and POS2 specify the positions of the upper-left corners of
+the first and second rectangle as conses of their column and line
+values.  SIZE1 and SIZE2 specify the dimensions of the first and
+second rectangle, as conses of their width and height measured in
+columns and lines."
+  (let ((x1 (car pos1))
+        (y1 (cdr pos1))
+        (x2 (car pos2))
+        (y2 (cdr pos2))
+        (w1 (car size1))
+        (h1 (cdr size1))
+        (w2 (car size2))
+        (h2 (cdr size2)))
+    (not (or (<= (+ x1 w1) x2)
+             (<= (+ x2 w2) x1)
+             (<= (+ y1 h1) y2)
+             (<= (+ y2 h2) y1)))))
+
 (defun delete-rectangle-line (startcol endcol fill)
   (when (= (move-to-column startcol (if fill t 'coerce)) startcol)
     (delete-region (point)
-- 
2.17.1

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[martin rudalics]

 > At the moment, my current proposal is: allow the user to drag the
 > rectangle anywhere to the left (on the same line, or on a different
 > line), or anywhere else where it would not intersect with itself (I'm
 > attaching a new patch that allows this).

Thanks.  The following case looks a bit problematic here.  Suppose my
rectangle in an Elisp buffer is

;;;;;;;;;;;;;;;;;;;
;;;;;;RECT;;;;;;;;;
;;;;;;RECT;;;;;;;;;
;;;;;;RECT;;;;;;;;;
;;;;;;RECT;;;;;;;;;
;;;;;;RECT;;;;;;;;;
;;;;;;;;;;;;;;;;;;;

If I now drag it up by one line and four columns to the right (so that
it just does not intersect with itself) I get

;;;;;;;;;;RECT;;;;;;;;;
;;;;;;RECT;;;;;;;;;
;;;;;;RECT;;;;;;;;;
;;;;;;RECT;;;;;;;;;
;;;;;;RECT;;;;;;;;;
;;;;;;;;;;;;;;;
;;;;;;;;;;;;;;;;;;;

which is somehow counterintuitive.  Any ideas?

Thanks, martin

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[Federico Tedin]

> If I now drag it up by one line and four columns to the right (so that
> it just does not intersect with itself) I get
>
> ;;;;;;;;;;RECT;;;;;;;;;
> ;;;;;;RECT;;;;;;;;;
> ;;;;;;RECT;;;;;;;;;
> ;;;;;;RECT;;;;;;;;;
> ;;;;;;RECT;;;;;;;;;
> ;;;;;;;;;;;;;;;
> ;;;;;;;;;;;;;;;;;;;
>
> which is somehow counterintuitive.  Any ideas?

This is because after inserting the rectangle, the original rectangle
(which is to the left of the inserted one) is deleted. When it is
deleted, then the last 4 lines of the inserted rectangle are shifted
to the left:

1) Inserted new rectangle:

;;;;;;;;;;RECT;;;;;;;;;
;;;;;;RECTRECT;;;;;;;;;
;;;;;;RECTRECT;;;;;;;;;
;;;;;;RECTRECT;;;;;;;;;
;;;;;;RECTRECT;;;;;;;;;
;;;;;;RECT;;;;;;;;;
;;;;;;;;;;;;;;;;;;;

2) Deleted the original one:

;;;;;;;;;;RECT;;;;;;;;;  <--- this line isn't shifted
;;;;;;RECT;;;;;;;;;
;;;;;;RECT;;;;;;;;;
;;;;;;RECT;;;;;;;;;
;;;;;;RECT;;;;;;;;;
;;;;;;;;;;;;;;;
;;;;;;;;;;;;;;;;;;;

I've thought up two ways of dealing with this problem:

A) Forbid the user from dragging the rectangle to the right when the
new rectangle is *not* completly above or completly below the original
one. As an exception, allow the user to drag the rectangle purely to
the right (same line). In this option, the drag operation in your
example would not be permitted.  I think this option would be a bit
confusing for users (and too restrictive).

B) Add a variable mouse-drag-and-drop-rectangle-fill, initially set to
nil. When the variable's value is nil, the behaviour of dragging and
dropping rectangles is exactly the same as it is now. When it is set
to a non-nil value (for example, " "), then replace all characters of
the original rectangle with the variable's value. In this case, your
example would look like this:

1) M-: (setq mouse-drag-and-drop-rectangle-fill " ")

2) Inserted new rectangle:

;;;;;;;;;;RECT;;;;;;;;;
;;;;;;RECTRECT;;;;;;;;;
;;;;;;RECTRECT;;;;;;;;;
;;;;;;RECTRECT;;;;;;;;;
;;;;;;RECTRECT;;;;;;;;;
;;;;;;RECT;;;;;;;;;
;;;;;;;;;;;;;;;;;;;

3) Deleted the original one:

;;;;;;;;;;RECT;;;;;;;;;
;;;;;;    RECT;;;;;;;;;
;;;;;;    RECT;;;;;;;;;
;;;;;;    RECT;;;;;;;;;
;;;;;;    RECT;;;;;;;;;
;;;;;;    ;;;;;;;;;
;;;;;;;;;;;;;;;;;;;

I can't think of any other way of ensuring the inserted rectangle
retains its shape after being inserted, if there are pending delete
operations to its left.

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[martin rudalics]

 > This is because after inserting the rectangle, the original rectangle
 > (which is to the left of the inserted one) is deleted. When it is
 > deleted, then the last 4 lines of the inserted rectangle are shifted
 > to the left:
 >
 > 1) Inserted new rectangle:
 >
 > ;;;;;;;;;;RECT;;;;;;;;;
 > ;;;;;;RECTRECT;;;;;;;;;
 > ;;;;;;RECTRECT;;;;;;;;;
 > ;;;;;;RECTRECT;;;;;;;;;
 > ;;;;;;RECTRECT;;;;;;;;;
 > ;;;;;;RECT;;;;;;;;;
 > ;;;;;;;;;;;;;;;;;;;
 >
 > 2) Deleted the original one:
 >
 > ;;;;;;;;;;RECT;;;;;;;;;  <--- this line isn't shifted
 > ;;;;;;RECT;;;;;;;;;
 > ;;;;;;RECT;;;;;;;;;
 > ;;;;;;RECT;;;;;;;;;
 > ;;;;;;RECT;;;;;;;;;
 > ;;;;;;;;;;;;;;;
 > ;;;;;;;;;;;;;;;;;;;

Thanks for the explanation.

 > I've thought up two ways of dealing with this problem:
 >
 > A) Forbid the user from dragging the rectangle to the right when the
 > new rectangle is *not* completly above or completly below the original
 > one. As an exception, allow the user to drag the rectangle purely to
 > the right (same line). In this option, the drag operation in your
 > example would not be permitted.  I think this option would be a bit
 > confusing for users (and too restrictive).

Agreed.

 > B) Add a variable mouse-drag-and-drop-rectangle-fill, initially set to
 > nil. When the variable's value is nil, the behaviour of dragging and
 > dropping rectangles is exactly the same as it is now. When it is set
 > to a non-nil value (for example, " "), then replace all characters of
 > the original rectangle with the variable's value. In this case, your
 > example would look like this:
 >
 > 1) M-: (setq mouse-drag-and-drop-rectangle-fill " ")
 >
 > 2) Inserted new rectangle:
 >
 > ;;;;;;;;;;RECT;;;;;;;;;
 > ;;;;;;RECTRECT;;;;;;;;;
 > ;;;;;;RECTRECT;;;;;;;;;
 > ;;;;;;RECTRECT;;;;;;;;;
 > ;;;;;;RECTRECT;;;;;;;;;
 > ;;;;;;RECT;;;;;;;;;
 > ;;;;;;;;;;;;;;;;;;;
 >
 > 3) Deleted the original one:
 >
 > ;;;;;;;;;;RECT;;;;;;;;;
 > ;;;;;;    RECT;;;;;;;;;
 > ;;;;;;    RECT;;;;;;;;;
 > ;;;;;;    RECT;;;;;;;;;
 > ;;;;;;    RECT;;;;;;;;;
 > ;;;;;;    ;;;;;;;;;
 > ;;;;;;;;;;;;;;;;;;;
 >
 > I can't think of any other way of ensuring the inserted rectangle
 > retains its shape after being inserted, if there are pending delete
 > operations to its left.

Let's keep B) in mind for the case that someone comes up with a real
use case where this would be needed.  Mine was just a constructed one
that looked convincing to me to to be reported.

The only remaining problem I have now is that we are far beyond the 80
columns limit for code.  I came up with truncating the assignment to
'drag-but-negligible' as follows

             (setq drag-but-negligible
                   (and (eq (overlay-buffer (car mouse-drag-and-drop-overlays))
                            buffer-to-paste)
                        (if region-noncontiguous
                            ;; If the region is rectangular, check if
                            ;; the newly inserted rectangular text
                            ;; would intersect the already selected
                            ;; region. If it would, then set
                            ;; "drag-but-negligible" to t.  As a
                            ;; special case, allow dragging the region
                            ;; freely anywhere to the left, as this
                            ;; will never trigger its contents to be
                            ;; inserted into the overlays tracking it.
                            (let ((size (cons region-width region-height))
                                  (start-coordinates
                                   (rectangle-position-as-coordinates start))
                                  (point-to-paste-coordinates
                                   (rectangle-position-as-coordinates
                                    point-to-paste)))
                              (and (rectangle-intersect-p
                                    start-coordinates size
                                    point-to-paste-coordinates size)
                                   (not (<= (car point-to-paste-coordinates)
                                            (car start-coordinates)))))
                          (and (<= (overlay-start
                                    (car mouse-drag-and-drop-overlays))
                                   point-to-paste)
                               (<= point-to-paste
                                   (overlay-end
                                    (car mouse-drag-and-drop-overlays))))))))

but that's ugly.  If you have any ideas how to handle that better (for
example, by moving the comment up by a few lines) I'll do that.
Otherwise, I'll have to use that form above.

Thanks, martin

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[Federico Tedin]

> Let's keep B) in mind for the case that someone comes up with a real
> use case where this would be needed.  Mine was just a constructed one
> that looked convincing to me to to be reported.

I agree that option B) seems better. Should I implement it now. or
would it be better to wait for other opinions?

> The only remaining problem I have now is that we are far beyond the 80
> columns limit for code.  I came up with truncating the assignment to
> 'drag-but-negligible' as follows
> ...
> but that's ugly.  If you have any ideas how to handle that better (for
> example, by moving the comment up by a few lines) I'll do that.
> Otherwise, I'll have to use that form above.

I started re-indenting some lines to see if I could make them fit in 80 columns,
but then I realized the results were exactly the same as yours. I just ended up
moving the comment instead:


            ;; Check if "drag but negligible".  Operation "drag but
            ;; negligible" is defined as drag-and-drop the text to
            ;; the original region.  When modifier is pressed, the
            ;; text will be inserted to inside of the original
            ;; region.
            ;; If the region is rectangular, check if the newly inserted
            ;; rectangular text would intersect the already selected
            ;; region. If it would, then set "drag-but-negligible" to t.
            ;; As a special case, allow dragging the region freely anywhere
            ;; to the left, as this will never trigger its contents to be
            ;; inserted into the overlays tracking it.
             (setq drag-but-negligible
                   (and (eq (overlay-buffer (car mouse-drag-and-drop-overlays))
                            buffer-to-paste)
                        (if region-noncontiguous
                            (let ((size (cons region-width region-height))
                                  (start-coordinates
                                   (rectangle-position-as-coordinates start))
                                  (point-to-paste-coordinates
                                   (rectangle-position-as-coordinates
                                    point-to-paste)))
                              (and (rectangle-intersect-p
                                    start-coordinates size
                                    point-to-paste-coordinates size)
                                   (not (<= (car point-to-paste-coordinates)
                                            (car start-coordinates)))))
                          (and (<= (overlay-start
                                    (car mouse-drag-and-drop-overlays))
                                   point-to-paste)
                               (<= point-to-paste
                                   (overlay-end
                                    (car mouse-drag-and-drop-overlays))))))))

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[martin rudalics]

 > I agree that option B) seems better. Should I implement it now. or
 > would it be better to wait for other opinions?

Let's wait.

 > I started re-indenting some lines to see if I could make them fit in 80 columns,
 > but then I realized the results were exactly the same as yours. I just ended up
 > moving the comment instead:

OK.  If there are no further objections I'll install that.

Thanks, martin

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[martin rudalics]

 > OK.  If there are no further objections I'll install that.

Done with commit 134ba45bf0c11048c44a46c11d5dc8da12ca4d3e on master.

Please close bug#31240 if nothing else remains to be done.

Thanks for the work, martin

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[Federico Tedin]

> Please close bug#31240 if nothing else remains to be done.

Is this something I should do? (If so, how?)

> Thanks for the work, martin

Thanks for your help.

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[martin rudalics]

 >> Please close bug#31240 if nothing else remains to be done.
 >
 > Is this something I should do? (If so, how?)

By replying to 31240-done@debbugs.gnu.org as I'm trying to do right
now.

BTW, did you look into Stefan's pretensions?  In particular his remark

     BTW, your "width" is computed AFAICT as

	(- (overlay-end (car mouse-drag-and-drop-overlays))
	   (overlay-start (car mouse-drag-and-drop-overlays)))

     which is a char-distance and not a column-distance (big difference in
     the presence of TABs and double-width chars).

sounds worthy of examination.

martin

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[Federico Tedin]

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

Martin, I’ve been searching for Stefan’s message and I am unable to find it
in the bug’s thread.  I’m also subscribed to emacs-devel but wasn’t able to
find it there either. From what you mentioned, however, I can see the
mistake of using overlays when the region is rectangular. To propose a fix
should I file a new bug report, or send an email to emacs-devel?  Thanks.

[-- Attachment #2: Type: text/html, Size: 406 bytes --]

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[Tak Kunihiro]

> I attach a diff of this very chunk against the original trunk version
> I have here.

I confirmed that I can move the rectangle to the left horizontally.

bug#31240: 26.1; mouse-save-then-kill does not kill rectangles

[Charles A. Roelli]

> From: Federico Tedin <federicotedin@gmail.com>
> Date: Thu, 27 Sep 2018 20:45:42 -0300

> > For some reason, the region is not moved, and it gets re-activated as
> > a normal region instead of a rectangular region.  Maybe I'm missing
> > something obvious.  I was able to get the dragging of a rectangular
> > region working sometimes, but not consistently.
> 
> I've tried your test, and it does break my fix, as you mentioned.
> 
> The problem was in the criteria used to define the variable
> 'drag-but-negligible'. The drag action used in your test was being
> incorrectly marked as negligible. Because of this, the region was
> also re-activated, but not in Rectangle Mark mode (this was also a bug).
> 
> I have made a correction where the variable 'drag-but-negligible' is
> defined, so dragging a rectangle region outside of itself will no longer
> mark it as negligible; and when it _is_ negligible, the region is
> re-activated as a rectangle again.

Thanks, sounds good.
 
> I've also found some cases where the overlay list is not working well
> enough to track the selected rectangle. For example, if a buffer
> contains the following:
> 
> aaaa
> BBbb
> CCcc
> 
> Dragging a 2x2 square starting from the first 'B' (spaces added for
> clarity):
> 
>  a a a a
> [B B]b b
> [C C]c c
> 
> to the column where the second 'a' is, results in the following:
> 
> a B B a a a
> b b
> c c
> 
> In this case, two 'C's are missing in the second line (after the first
> 'b'). The reason this is happening is the following: when
> mouse-drag-and-drop-region is called, the initial overlays are the
> following (shown with braces):
> 
>  a a a a
> {B B}b b
> {C C}c c
> 
> After the 2x2 square is inserted on the second 'a', the first overlay
> is automatically expanded, because characters where inserted between
> its start/end:
> 
>  a B B a a a
> {b C C b}b b
> {c c}c c
> 
> When the original text is then deleted (by deleting all overlays), the
> result is:
> 
> a B B a a a
> b b
> c c
> 
> So I think I have two options now: either forbid the user from
> dragging a rectangle to a position where the inserted rectangle would
> intersect the original rectangle, or find another way to track the
> originally selected rectangle in a way it can be accurately deleted
> after inserting it in the new position. I guess I'll go with the
> second option, since it would make function more useful for users.

This second option sounds like it can be quite hard to define.  If you
decide in the end to prevent the user from dragging the region
somewhere that would intersect with the dragged region (which, as
Martin said, is in line with the original design of
mouse-drag-and-drop-region) that would be fine.