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

classic Classic list List threaded Threaded
48 messages Options
123
Reply | Threaded
Open this post in threaded view
|

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.



Reply | Threaded
Open this post in threaded view
|

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?



Reply | Threaded
Open this post in threaded view
|

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

Charles A. Roelli
> From: Federico Tedin <[hidden email]>
> 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.



Reply | Threaded
Open this post in threaded view
|

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

Federico Tedin
In reply to this post by 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.



Reply | Threaded
Open this post in threaded view
|

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

Charles A. Roelli
> From: Federico Tedin <[hidden email]>
> 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!



Reply | Threaded
Open this post in threaded view
|

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

Federico Tedin
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.

mouse.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Charles A. Roelli
> From: Federico Tedin <[hidden email]>
> 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.



Reply | Threaded
Open this post in threaded view
|

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.



Reply | Threaded
Open this post in threaded view
|

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

Charles A. Roelli
> From: Federico Tedin <[hidden email]>
> 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.



Reply | Threaded
Open this post in threaded view
|

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

Federico Tedin
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.

mouse.patch (11K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Charles A. Roelli
> From: Federico Tedin <[hidden email]>
> 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.



Reply | Threaded
Open this post in threaded view
|

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.



Reply | Threaded
Open this post in threaded view
|

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



Reply | Threaded
Open this post in threaded view
|

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

Charles A. Roelli
In reply to this post by Federico Tedin
> From: Federico Tedin <[hidden email]>
> 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.



Reply | Threaded
Open this post in threaded view
|

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

Federico Tedin
In reply to this post by 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.

> 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.

mouse.patch (15K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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



Reply | Threaded
Open this post in threaded view
|

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

Charles A. Roelli
In reply to this post by Federico Tedin
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.



Reply | Threaded
Open this post in threaded view
|

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

Federico Tedin
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.

mouse.patch (15K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> From: Federico Tedin <[hidden email]>
> Date: Sun, 30 Sep 2018 13:20:31 -0300
> Cc: [hidden email]
>
> > 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.



Reply | Threaded
Open this post in threaded view
|

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

martin rudalics
In reply to this post by 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.

 > 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



123