Re: [Emacs-diffs] master 134ba45: Allow two mouse functions to work with Rectangle Mark mode

Re: [Emacs-diffs] master 134ba45: Allow two mouse functions to work with Rectangle Mark mode

[Stefan Monnier]

>     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)
>     (rectangle-position-as-coordinates): New functions.

Cool, thanks.

> @@ -2455,7 +2466,11 @@ is copied instead of being cut."
>            ;; 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

I don't understand the comment: why is it necessary to remove the
yank-handler in order to insert the text via insert-rectangle?

> @@ -2468,12 +2483,18 @@ is copied instead of being cut."
>                          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.

Why do we use start/end additionally to checking each chunk?
Also why check (get-text-property end 'read-only) which looks at the
read-only property of the char right *after* the region?

I'd have expected instead something like

    (setq text-from-read-only
          (or text-from-read-only
              (catch 'loop
                (dolist (bound (region-bounds))
                  (unless (and (null (get-text-property (car bound) 'read-only))
                               (equal (next-single-char-property-change
                                       (car bound) 'read-only nil (cdr bound))
                                      (cdr bound)))
                    (throw 'loop t)))))))

Also I notice that this code (the old version, your new version, and
the version I just wrote above) have the odd property of using get-text-property
in one place and next-single-char-property-change in the other, meaning
that one part only check text-properties while the other checks both
text-properties and overlays.

IIRC `read-only` is only special in text-properties and not in overlays,
in which case we could maybe use just

    (setq text-from-read-only
          (or text-from-read-only
              (catch 'loop
                (dolist (bound (region-bounds))
                  (unless (text-property-not-all
                           (car bound) (cdr bound) 'read-only nil)
                    (throw 'loop t)))))))

> @@ -2581,7 +2624,9 @@ is copied instead of being cut."
>            (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::

Hmm... while it's true that currently, I don't know of a package which
provides non contiguous regions other than rectangular regions, there
hopefully will be such a package in the future (which lets you build
arbitrary non-contiguous region, chunk by chunk, or create
a non-contiguous region holding all matches of a particular regexp).

Maybe we need to extend the API of noncontiguous regions so you can
tell which "special mode" is associated with it (if any)?

> @@ -2590,11 +2635,17 @@ is copied instead of being cut."
>            (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))

Hmm... I think if you use insert-for-yank (and don't strip
the yank-handler earlier), then it will just work for both the
contiguous and the rectangular cases.

>            ;; 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)))

Same as above.  Maybe those 3 lines should be moved to their own
little function.

> +... The value
> +returned is a cons of the current column of POSITION and its line
> +number."

We usually write this using some kind of "pattern".
E.g. "Return a value of the form (COLUMN . LINE)".

> +  (save-excursion
> +    (goto-char position)
> +    (let ((col (current-column))
> +          (line (1- (line-number-at-pos))))
> +      (cons col line))))

Why "1-"?  AFAIK you only compare those line numbers between themselves,
so this "1-" doesn't make any difference to your code and simply causes
this code to return "non-standard" line numbers.

> +(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."

Same as above, we could write it as 

    POS1 and POS2 specify the positions of the upper-left corners of
    the first and second rectangle as conses of the form (COLUMN . LINE).
    SIZE1 and SIZE2 specify the dimensions of the first and
    second rectangle, as conses of the form (WIDTH . HEIGHT)."

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


        Stefan

RE: [Emacs-diffs] master 134ba45: Allow two mouse functions to work with Rectangle Mark mode

[Drew Adams]

> Hmm... while it's true that currently, I don't know of a package which
> provides non contiguous regions other than rectangular regions,

zones.el does.

https://www.emacswiki.org/emacs/download/zones.el

https://www.emacswiki.org/emacs/Zones

> there hopefully will be such a package in the future (which lets you build
> arbitrary non-contiguous region, chunk by chunk, or create
> a non-contiguous region holding all matches of a particular regexp).

zones.el lets you do these things.

> Maybe we need to extend the API of noncontiguous regions so you can
> tell which "special mode" is associated with it (if any)?

I don't understand the question. Could you elaborate?
Searching your msg for "special" didn't help.

Re: [Emacs-diffs] master 134ba45: Allow two mouse functions to work with Rectangle Mark mode

[Stefan Monnier]

>> Hmm... while it's true that currently, I don't know of a package which
>> provides non contiguous regions other than rectangular regions,
> zones.el does.

Could we add it to elpa.git?

>> Maybe we need to extend the API of noncontiguous regions so you can
>> tell which "special mode" is associated with it (if any)?
> I don't understand the question. Could you elaborate?
> Searching your msg for "special" didn't help.

The code I quoted said:

        (when region-noncontiguous
          (rectangle-mark-mode)))

I don't know why he needs to enable rectangle-mark-mode here, but if
it's needed in the case of rectangular regions (apparently his code
assumes that "noncontiguous" is synonym with "rectangular") maybe some
other mode might be needed for other noncontinuous regions, in which
case, the region should maybe carry with itself its associated mode.

But to know what to do, we first need to know more about why he has code
like the above.


        Stefan

Re: [Emacs-diffs] master 134ba45: Allow two mouse functions to work with Rectangle Mark mode

[Federico Tedin]

Stefan, I'm addressing your feedback below:

> I don't understand the comment: why is it necessary to remove the
> yank-handler in order to insert the text via insert-rectangle?

When I wrote the patch, I thought the only way of inserting a
rectangle was through insert-rectangle, which receives a list of
strings without the yank-handler property. I now see that when killing
and yanking a rectangular region, insert-for-yank calls
rectangle--insert-for-yank, which then calls insert-rectangle with the
string list.

> Why do we use start/end additionally to checking each chunk?
> Also why check (get-text-property end 'read-only) which looks at the
> read-only property of the char right *after* the region?
>
> I'd have expected instead something like
>
>     (setq text-from-read-only
>           (or text-from-read-only
>               (catch 'loop
>                 (dolist (bound (region-bounds))
>                   (unless (and (null (get-text-property (car bound) 'read-only))
>                                (equal (next-single-char-property-change
>                                        (car bound) 'read-only nil (cdr bound))
>                                       (cdr bound)))
>                     (throw 'loop t)))))))
>
> Also I notice that this code (the old version, your new version, and
> the version I just wrote above) have the odd property of using get-text-property
> in one place and next-single-char-property-change in the other, meaning
> that one part only check text-properties while the other checks both
> text-properties and overlays.
>
> IIRC `read-only` is only special in text-properties and not in overlays,
> in which case we could maybe use just
>
>     (setq text-from-read-only
>           (or text-from-read-only
>               (catch 'loop
>                 (dolist (bound (region-bounds))
>                   (unless (text-property-not-all
>                            (car bound) (cdr bound) 'read-only nil)
>                     (throw 'loop t)))))))

I assumed (get-text-property start 'read-only) was an optimization to
cover cases where the region was large, and at least the beggining of
it was read only. I added (get-text-property end 'read-only) to
complement this check, but on the ending. I see now though that my
addition is incorrect (as you mentioned) because it checks the char
not contained in the region.

I'm interested in testing you last example for setting
text-from-read-only, as it is more readable than the others (and runs
the same checks).

> Hmm... while it's true that currently, I don't know of a package which
> provides non contiguous regions other than rectangular regions, there
> hopefully will be such a package in the future (which lets you build
> arbitrary non-contiguous region, chunk by chunk, or create
> a non-contiguous region holding all matches of a particular regexp).
>
> Maybe we need to extend the API of noncontiguous regions so you can
> tell which "special mode" is associated with it (if any)?

I agree with your last point. If the region's "special mode" could be
determined somehow, then different actions could be taken depending on
its value. In my patch, I treated noncontiguous regions as always
rectangular.

> Hmm... I think if you use insert-for-yank (and don't strip
> the yank-handler earlier), then it will just work for both the
> contiguous and the rectangular cases.

Just tested this and yes, it works for both cases. As you mentioned, I
should keep the yank-handler and just use insert-for-yank.

> Same as above.  Maybe those 3 lines should be moved to their own
> little function.

I'm not sure of what the function could do. Would it call
activate-mark and then enable a special region mode? I imagine it
would need to receive a parameter indicating what mode to choose.

> We usually write this using some kind of "pattern".
> E.g. "Return a value of the form (COLUMN . LINE)".

I'll apply the change you suggested here.

> Why "1-"?  AFAIK you only compare those line numbers between themselves,
> so this "1-" doesn't make any difference to your code and simply causes
> this code to return "non-standard" line numbers.

I wanted the "coordinates" to start at (0, 0) as I thought it would be
easier to reason about when comparing them to other coordinates. As
you said, the "1-" could be removed and the rest of the code would
work the same.

> Same as above, we could write it as
>
>     POS1 and POS2 specify the positions of the upper-left corners of
>     the first and second rectangle as conses of the form (COLUMN . LINE).
>     SIZE1 and SIZE2 specify the dimensions of the first and
>     second rectangle, as conses of the form (WIDTH . HEIGHT)."

I'll also apply the change you suggested here.

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

You are right, I overlooked this. Using an overlay list then doesn't
seem like a good option for tracking the contents of rectangular
regions. Maybe I could change the code to make it look more like the
original, and use rectangle--* functions with 'start' and 'end' where
its needed. To do this, however, I would need to know the preferred
way of checking the region's geometry. Right now I'm using
'region-noncontiguous-p' and assuming that t means rectangular, which
could break things in the future.

Answering to your other message:

> The code I quoted said:
>
>         (when region-noncontiguous
>           (rectangle-mark-mode)))
>
> I don't know why he needs to enable rectangle-mark-mode here, but if
> it's needed in the case of rectangular regions (apparently his code
> assumes that "noncontiguous" is synonym with "rectangular") maybe some
> other mode might be needed for other noncontinuous regions, in which
> case, the region should maybe carry with itself its associated mode.
>
> But to know what to do, we first need to know more about why he has code
> like the above.

The code you mentioned is needed to ensure that when the user selects
a rectangular region and drags it, the resulting content is selected
with rectangular-mark-mode enabled.  The same goes for when the drag
operation fails. In both cases, the user expects that after dragging
(or trying to drag) a rectangle, the resulting selected region will
also be rectangular.

If there was a way of getting the region's mode, then the problems in
mouse-drag-and-drop-region would be easier to fix, and would also
allow implementing drag operations for other region modes.  I'm not
sure of how to continue from here. Should I create a new patch fixing
the problems you mentioned, or should I wait until there's a better
way of determining the region's mode?

Thanks for your feedback.

Re: [Emacs-diffs] master 134ba45: Allow two mouse functions to work with Rectangle Mark mode

[Stefan Monnier]

> I assumed (get-text-property start 'read-only) was an optimization to
> cover cases where the region was large, and at least the beggining of
> it was read only. I added (get-text-property end 'read-only) to
> complement this check, but on the ending.

I see.  No, the (get-text-property start 'read-only) was there because
the next test only verified if there's a *change* somewhere, so in order
to make sure it's nil everywhere you needed both: make sure it's nil
somewhere (in this case, at the beginning) and then make sure there's no
change anywhere.

>> Same as above.  Maybe those 3 lines should be moved to their own
>> little function.
>
> I'm not sure of what the function could do. Would it call
> activate-mark and then enable a special region mode? I imagine it
> would need to receive a parameter indicating what mode to choose.

Not sure what it should do, indeed, but AFAIK it does the same at both
places.  My point was only to combine those two places to avoid code
duplication: what the content should be is orthogonal.

>> Why "1-"?  AFAIK you only compare those line numbers between themselves,
>> so this "1-" doesn't make any difference to your code and simply causes
>> this code to return "non-standard" line numbers.
>
> I wanted the "coordinates" to start at (0, 0) as I thought it would be
> easier to reason about when comparing them to other coordinates.

For better or worse, we use (0, 1) as "origin" and I think it's better
to stick to this odd convention than to muddle the water even further.

>> 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).
>
> You are right, I overlooked this. Using an overlay list then doesn't
> seem like a good option for tracking the contents of rectangular
> regions.

Not sure why you think so.
You can probably just use.

    (save-excursion
      (let ((ol (car mouse-drag-and-drop-overlays)))
        (goto-char (overlay-end ol))
        (- (current-column)
           (progn (goto-char (overlay-start ol)) (current-column)))))

> Maybe I could change the code to make it look more like the
> original, and use rectangle--* functions with 'start' and 'end' where
> its needed. To do this, however, I would need to know the preferred
> way of checking the region's geometry. Right now I'm using
> 'region-noncontiguous-p' and assuming that t means rectangular, which
> could break things in the future.

Maybe we should try and figure out what is special about rectangles, and
what should the code do for non-rectangular non-contiguous regions.
That might help us decide how best to get the needed information.

> The code you mentioned is needed to ensure that when the user selects
> a rectangular region and drags it, the resulting content is selected
> with rectangular-mark-mode enabled.  The same goes for when the drag
> operation fails. In both cases, the user expects that after dragging
> (or trying to drag) a rectangle, the resulting selected region will
> also be rectangular.

Hmm... how 'bout attacking the problem as follows.  There are 2 cases:
1- the drag failed, so presumably nothing was changed.  In that case,
   naive thinks, there's nothing to do, the region is still active (and
   rectangle-mark-mode is still active if applicable).  If not why not?
   In any case, if the region is not active any more (and
   rectangle-mark-mode is hence also deactivated), there should be a way
   to undo a region deactivation, i.e. "reactivate the region".
2- the drag succeeded, so the text was just inserted somewhere with
   insert-for-yank.  Here as well, we just need some way to "activate"
   that which we just inserted.

So I think what we need is a generic `reactivate-region` command and
some way for it to work correctly for noncontiguous regions (and the
insert-for-yank code should be able to set it up so that reactivating
the region after an insert-for-yank would activate it in the proper
mode as well).

IIUC, this should solve your problems and be generally useful, right?


        Stefan

Re: [Emacs-diffs] master 134ba45: Allow two mouse functions to work with Rectangle Mark mode

[Federico Tedin]

> > I assumed (get-text-property start 'read-only) was an optimization to
> > cover cases where the region was large, and at least the beginning of
> > it was read only. I added (get-text-property end 'read-only) to
> > complement this check, but on the ending.
>
> I see.  No, the (get-text-property start 'read-only) was there because
> the next test only verified if there's a *change* somewhere, so in order
> to make sure it's nil everywhere you needed both: make sure it's nil
> somewhere (in this case, at the beginning) and then make sure there's no
> change anywhere.

I see. Understood, then.

> >> Same as above.  Maybe those 3 lines should be moved to their own
> >> little function.
> >
> > I'm not sure of what the function could do. Would it call
> > activate-mark and then enable a special region mode? I imagine it
> > would need to receive a parameter indicating what mode to choose.
>
> Not sure what it should do, indeed, but AFAIK it does the same at both
> places.  My point was only to combine those two places to avoid code
> duplication: what the content should be is orthogonal.

I agree.

> >> Why "1-"?  AFAIK you only compare those line numbers between themselves,
> >> so this "1-" doesn't make any difference to your code and simply causes
> >> this code to return "non-standard" line numbers.
> >
> > I wanted the "coordinates" to start at (0, 0) as I thought it would be
> > easier to reason about when comparing them to other coordinates.
>
> For better or worse, we use (0, 1) as "origin" and I think it's better
> to stick to this odd convention than to muddle the water even further.

No problem, I'll remove the call to "1-".

> >> 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).
> >
> > You are right, I overlooked this. Using an overlay list then doesn't
> > seem like a good option for tracking the contents of rectangular
> > regions.
>
> Not sure why you think so.
> You can probably just use.
>
>     (save-excursion
>       (let ((ol (car mouse-drag-and-drop-overlays)))
>         (goto-char (overlay-end ol))
>         (- (current-column)
>            (progn (goto-char (overlay-start ol)) (current-column)))))

I thought you were actually referring to another problem. Consider the
following example, where a buffer contains the text (spaces added for
clarity):

 a
 b b
 c c c
 d d d d

...and the user selects the following rectangular region:

[a      ]
[b b    ]
[c c c  ]
[d d d d]

In this case, the first overlay will actually only contain "a", so
subtracting (current-column) in its end and start will yield 1. This
makes sense, as evaluating (car (region-bounds)) yields (1 . 2).

Something I also realized is that the variables 'region-width' and
'region-height' are 1) rectangular region-specific and 2) are only
used when checking if the region is being dragged into itself (setq
drag-but-neglibible ...). Maybe their definition could be moved to
that specific part of the code (and only define them when the region
is rectangular). Also, region-width could be calculated using 'start'
and 'end', probably using one of the functions in rect.el. Some of the
functions in rect.el are not commented so I'll have to read them
carefully to see if they can be used for this.

On the other hand, using overlays to delete the original selection
(whether it is rectangular or not) works fine, so maybe keeping them for
that purpose is a good idea.

> > Maybe I could change the code to make it look more like the
> > original, and use rectangle--* functions with 'start' and 'end' where
> > its needed. To do this, however, I would need to know the preferred
> > way of checking the region's geometry. Right now I'm using
> > 'region-noncontiguous-p' and assuming that t means rectangular, which
> > could break things in the future.
>
> Maybe we should try and figure out what is special about rectangles, and
> what should the code do for non-rectangular non-contiguous regions.
> That might help us decide how best to get the needed information.
>
> > The code you mentioned is needed to ensure that when the user selects
> > a rectangular region and drags it, the resulting content is selected
> > with rectangular-mark-mode enabled.  The same goes for when the drag
> > operation fails. In both cases, the user expects that after dragging
> > (or trying to drag) a rectangle, the resulting selected region will
> > also be rectangular.
>
> Hmm... how 'bout attacking the problem as follows.  There are 2 cases:
> 1- the drag failed, so presumably nothing was changed.  In that case,
>    naive thinks, there's nothing to do, the region is still active (and
>    rectangle-mark-mode is still active if applicable).  If not why not?
>    In any case, if the region is not active any more (and
>    rectangle-mark-mode is hence also deactivated), there should be a way
>    to undo a region deactivation, i.e. "reactivate the region".
> 2- the drag succeeded, so the text was just inserted somewhere with
>    insert-for-yank.  Here as well, we just need some way to "activate"
>    that which we just inserted.
>
> So I think what we need is a generic `reactivate-region` command and
> some way for it to work correctly for non contiguous regions (and the
> insert-for-yank code should be able to set it up so that reactivating
> the region after an insert-for-yank would activate it in the proper
> mode as well).
>
> IIUC, this should solve your problems and be generally useful, right?

I believe the only parts of the code that is rectangle-specific are:

1) Checking if the drag is negligible
2) Inserting the text in the new position
3) Re-activating the region after a successful or failed drag

Then, the command you propose would be helpful for point 3. Points 1
and 2 would still need to be handled in a specific way, depending on
the region's "mode". For that, we would need a way to get the current
region's "mode" (or the one that was active when
mouse-drag-and-drop-region was called).

Re: [Emacs-diffs] master 134ba45: Allow two mouse functions to work with Rectangle Mark mode

[Federico Tedin]

A small correction:
> I believe the only parts of the code that is rectangle-specific are:

With this, I mean "the only parts of the code that behave differently depending
on what the region's 'mode' (normal, rectangle, etc.) are..."

Re: [Emacs-diffs] master 134ba45: Allow two mouse functions to work with Rectangle Mark mode

[Stefan Monnier]

> I thought you were actually referring to another problem. Consider the
> following example, where a buffer contains the text (spaces added for
> clarity):
>
>  a
>  b b
>  c c c
>  d d d d
>
> ...and the user selects the following rectangular region:
>
> [a      ]
> [b b    ]
> [c c c  ]
> [d d d d]
>
> In this case, the first overlay will actually only contain "a", so
> subtracting (current-column) in its end and start will yield 1. This
> makes sense, as evaluating (car (region-bounds)) yields (1 . 2).

Ah, right, that's yet another problem, indeed.

> Something I also realized is that the variables 'region-width' and
> 'region-height' are 1) rectangular region-specific

Sorry for not mentioning it earlier.

> and 2) are only used when checking if the region is being dragged into
> itself (setq drag-but-neglibible ...).

I don't understand enough of the "dragged into itself" test to know what
should be done here.  I think part of the issue is that for rectangular
regions, the "insert-for-yank" will actually not just insert, so we
can't just test "is insertion-point inside region-bounds?" (which could
be easily implemented in a generic way).  But at the same time, why
should we disallow dragging the rectangle to a place that overlaps its
original location?

> Maybe their definition could be moved to
> that specific part of the code (and only define them when the region
> is rectangular).

If that can be done, that sounds good.

> I believe the only parts of the code that is rectangle-specific are:
>
> 1) Checking if the drag is negligible
> 2) Inserting the text in the new position
> 3) Re-activating the region after a successful or failed drag

insert-for-yank takes care of (2), doesn't it?


        Stefan

Re: [Emacs-diffs] master 134ba45: Allow two mouse functions to work with Rectangle Mark mode

[martin rudalics]

 > I don't understand enough of the "dragged into itself" test to know what
 > should be done here.  I think part of the issue is that for rectangular
 > regions, the "insert-for-yank" will actually not just insert, so we
 > can't just test "is insertion-point inside region-bounds?" (which could
 > be easily implemented in a generic way).  But at the same time, why
 > should we disallow dragging the rectangle to a place that overlaps its
 > original location?

AFAICT, the drag into itself case is different for contiguous and
non-contiguous regions.  For the former we want to avoid changing
anything because we would clutter undo with actions that remove and
re-add the same portion of text at the same position.  For rectangles
we allow to move the rectangle such that the buffer regions spanned by
the beginning of the first and the end of the last region of original
and dragged rectangle may overlap while the individual regions do not
overlap.

martin

Re: [Emacs-diffs] master 134ba45: Allow two mouse functions to work with Rectangle Mark mode

[Federico Tedin]

> > I thought you were actually referring to another problem. Consider the
> > following example, where a buffer contains the text (spaces added for
> > clarity):
> >
> >  a
> >  b b
> >  c c c
> >  d d d d
> >
> > ...and the user selects the following rectangular region:
> >
> > [a      ]
> > [b b    ]
> > [c c c  ]
> > [d d d d]
> >
> > In this case, the first overlay will actually only contain "a", so
> > subtracting (current-column) in its end and start will yield 1. This
> > makes sense, as evaluating (car (region-bounds)) yields (1 . 2).
>
> Ah, right, that's yet another problem, indeed.
>
> > Something I also realised is that the variables 'region-width' and
> > 'region-height' are 1) rectangular region-specific
>
> Sorry for not mentioning it earlier.

No problem, I should've noticed it when I added it in.

> > and 2) are only used when checking if the region is being dragged into
> > itself (setq drag-but-negligible ...).
>
> I don't understand enough of the "dragged into itself" test to know what
> should be done here.  I think part of the issue is that for rectangular
> regions, the "insert-for-yank" will actually not just insert, so we
> can't just test "is insertion-point inside region-bounds?" (which could
> be easily implemented in a generic way).  But at the same time, why
> should we disallow dragging the rectangle to a place that overlaps its
> original location?

Here I'll refer to Martin's explanation, and add a bit more.

For contiguous regions (i.e. the original code),
mouse-drag-and-drop-region does the following during a drag operation
(with some details omitted):

1) Create an overlay with start and end equal to the region's
2) Get the string contents of the region and save them in 'value-selection'
3) Check if the drag is negligible: if the point where the user
   dragged the region lies *inside* the overlay, then the operation is
   marked as negligible.

Then, if the drag is not negligible:

4) 'value-selection' is inserted where the user dragged the region
5) The original selection is deleted using delete-region, using the
   overlay's start and end as parameters

If the check for 'drag-but-negligible' wasn't there, then the
following could happen: the user could drag the region into itself,
then the contents would be inserted, and then both the original and
the newly inserted contents would be deleted with the call to
delete-region (because the text would have been inserted *inside* the
overlay).

With rectangular regions, I chose to use a list of overlays to represent
the selected region. In my mind this made sense as I could keep the code
that dealt with overlays when checking for read-only properties or when
calling delete-region (with the overlay's start and end). The problem with
this is that, in some cases, a rectangle can be dragged in a way where *some*
parts of it are re-inserted into the overlays tracking the original selection.
For example:

Select a square:

 a a a
[b b]b
[c c]c

Insert it at point marked with "|":

 a|a a
[b b]b
[c c]c

 a b b a a
[b c c b]b
[c c]c

Call delete-region using the two overlays' boundaries:

a b b a a
b
c

...which probably isn't what the user was expecting. By adding
'rectangle-intersect-p', and checking for a possible intersection
between the original rectangular region and the potentially newly
inserted rectangle, these cases could be avoided.

The check for drag-but-negligible is easier to understand for contiguous
regions: if dragging a region into itself was somehow possible, the text
would remain unchanged, so the operation wouldn't make sense even from
the user's perspective. With rectangles, however, it is harder to define
which cases 'make sense'. For example, what should happen if a rectangle
is dragged one column to the right, into itself?

I think mouse-drag-and-drop-region could be greatly improved if we
somehow implemented a way of checking the region's "type" (as we were
talking) and if the results of dragging a rectangle were well
defined and specified, from a user's perspective.

> > Maybe their definition could be moved to
> > that specific part of the code (and only define them when the region
> > is rectangular).
>
> If that can be done, that sounds good.
>
> > I believe the only parts of the code that is rectangle-specific are:
> >
> > 1) Checking if the drag is negligible
> > 2) Inserting the text in the new position
> > 3) Re-activating the region after a successful or failed drag
>
> insert-for-yank takes care of (2), doesn't it?

Yes, you're right. insert-for-yank is already "region type"-aware
(because it uses the yank-handler property).

Re: [Emacs-diffs] master 134ba45: Allow two mouse functions to work with Rectangle Mark mode

[Federico Tedin]

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

Stefan, I've prepared a patch that fixes most of the problems you
found with my commit.

I also fixed the case where a rectangle was dragged to its exact
current location, the operation was not being marked as negligible and
the result was that both the original and inserted rectangles where
being deleted.

Although this patch tidies up mouse-drag-and-drop-region, the problem
of always treating noncontiguous regions as rectangles still remains.
I have limited experience writing Emacs Lisp so I am not sure what the
options are to solve this, but I'm interested in participating in the
process of thinking/implementing a solution.

Thanks.

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

From a5e351511f3c199c7fe49bf1ffff251536d30328 Mon Sep 17 00:00:00 2001
From: Federico Tedin <federicotedin@gmail.com>
Date: Thu, 25 Oct 2018 19:57:40 -0300
Subject: [PATCH 1/1] Better handling of rectangular regions in
 mouse-drag-and-drop-region

* lisp/mouse.el (mouse-drag-and-drop-region): Use insert-for-yank for
  insertion, remove rectangular-region-specific variables.
* lisp/rect.el (rectangle-dimensions): New function.
---
 lisp/mouse.el | 36 +++++++++++-------------------------
 lisp/rect.el  | 22 +++++++++++++++-------
 2 files changed, 26 insertions(+), 32 deletions(-)

diff --git a/lisp/mouse.el b/lisp/mouse.el
index 44cca4c868..e23e4f1ccd 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -2413,16 +2413,13 @@ mouse-drag-and-drop-region
          (buffer (current-buffer))
          (window (selected-window))
          (text-from-read-only buffer-read-only)
-         ;; Use multiple overlays to cover cases where the region is
-         ;; rectangular.
+         ;; Use multiple overlays to cover cases where the region has more
+         ;; than one boundary.
          (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
@@ -2467,10 +2464,6 @@ mouse-drag-and-drop-region
           ;; skipped, value-selection remains nil.
           (unless value-selection
             (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
@@ -2485,15 +2478,11 @@ mouse-drag-and-drop-region
             ;; Check if selected text is read-only.
             (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)))))))
+                        (dolist (bound (region-bounds))
+                          (when (text-property-any
+                                   (car bound) (cdr bound) 'read-only t)
+                            (throw 'loop t)))))))
 
           (setq window-to-paste (posn-window (event-end event)))
           (setq point-to-paste (posn-point (event-end event)))
@@ -2531,16 +2520,16 @@ mouse-drag-and-drop-region
                   (and (eq (overlay-buffer (car mouse-drag-and-drop-overlays))
                            buffer-to-paste)
                        (if region-noncontiguous
-                           (let ((size (cons region-width region-height))
+                           (let ((dimensions (rectangle-dimensions start end))
                                  (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)
+                                   start-coordinates dimensions
+                                   point-to-paste-coordinates dimensions)
+                                  (not (< (car point-to-paste-coordinates)
                                            (car start-coordinates)))))
                          (and (<= (overlay-start
                                    (car mouse-drag-and-drop-overlays))
@@ -2635,10 +2624,7 @@ mouse-drag-and-drop-region
           (setq window-exempt window-to-paste)
           (goto-char point-to-paste)
           (push-mark)
-
-          (if region-noncontiguous
-              (insert-rectangle (split-string value-selection "\n"))
-            (insert value-selection))
+          (insert-for-yank value-selection)
 
           ;; On success, set the text as region on destination buffer.
           (when (not (equal (mark) (point)))
diff --git a/lisp/rect.el b/lisp/rect.el
index 48db4ffd8f..6b6906ac89 100644
--- a/lisp/rect.el
+++ b/lisp/rect.el
@@ -170,21 +170,19 @@ apply-on-rectangle
 (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."
+returned has the form (COLUMN . LINE)."
   (save-excursion
     (goto-char position)
     (let ((col (current-column))
-          (line (1- (line-number-at-pos))))
+          (line (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."
+the first and second rectangles as conses of the form (COLUMN . LINE).
+SIZE1 and SIZE2 specify the dimensions of the first and second
+rectangles, as conses of the form (WIDTH . HEIGHT)."
   (let ((x1 (car pos1))
         (y1 (cdr pos1))
         (x2 (car pos2))
@@ -198,6 +196,16 @@ rectangle-intersect-p
              (<= (+ y1 h1) y2)
              (<= (+ y2 h2) y1)))))
 
+(defun rectangle-dimensions (start end)
+  "Return the dimensions of the rectangle with corners at START
+and END. The returned value has the form of (WIDTH . HEIGHT)."
+  (save-excursion
+    (let* ((height (1+ (abs (- (line-number-at-pos end)
+                               (line-number-at-pos start)))))
+           (cols (rectangle--pos-cols start end))
+           (width (abs (- (cdr cols) (car cols)))))
+      (cons width height))))
+
 (defun delete-rectangle-line (startcol endcol fill)
   (when (= (move-to-column startcol (if fill t 'coerce)) startcol)
     (delete-region (point)
-- 
2.17.1

Re: [Emacs-diffs] master 134ba45: Allow two mouse functions to work with Rectangle Mark mode

[Stefan Monnier]

> Stefan, I've prepared a patch that fixes most of the problems you
> found with my commit.

Thanks, installed (after changing text-property-any with
text-property-not-all so we also detect non-t non-nil values as meaning
"read-only").

> Although this patch tidies up mouse-drag-and-drop-region, the problem
> of always treating noncontiguous regions as rectangles still remains.
> I have limited experience writing Emacs Lisp so I am not sure what the
> options are to solve this, but I'm interested in participating in the
> process of thinking/implementing a solution.

AFAICT the next step is to introduce a function `reactivate-mark`.
This function will need to know what kind of mark was earlier activated,
so we will need to store the "kind of mark" somewhere.
I guess the easiest is to introduce a new var
`reactivate-mark-function` (so reactivate-mark doesn't need to do much
more than funcall it).

Then we'll want to change a few places in Elisp to set/reset this var.


        Stefan

Re: [Emacs-diffs] master 134ba45: Allow two mouse functions to work with Rectangle Mark mode

[Federico Tedin]

> Thanks, installed (after changing text-property-any with
> text-property-not-all so we also detect non-t non-nil values as meaning
> "read-only").

Thanks.

> AFAICT the next step is to introduce a function `reactivate-mark`.
> This function will need to know what kind of mark was earlier activated,
> so we will need to store the "kind of mark" somewhere.
> I guess the easiest is to introduce a new var
> `reactivate-mark-function` (so reactivate-mark doesn't need to do much
> more than funcall it).
>
> Then we'll want to change a few places in Elisp to set/reset this var.

Could we store the region "type" instead of a function instead? For
example, 'normal for normal regions, 'rectangle for rectangular
regions, etc. The reason I propose this is because, at least in
mouse-drag-and-drop-region, more than one parts of the function depend
on the region type:

- Checking if the drag is negligible
- Re-activating the region after the drag operation

By implementing `reactivate-mark-function` alone, the problem of not
knowing the region type would still remain. If we store the region
type instead, then the value could be read directly, and
`reactivate-mark` could be implemented as:

(defun reactivate-mark ()
  (pcase region-type
    ('normal ...)
    ('rectangle ...)
     ...))

Would that make sense? Other functions depending on the region type could
be implemented in a similar fashion.

Re: [Emacs-diffs] master 134ba45: Allow two mouse functions to work with Rectangle Mark mode

[Stefan Monnier]

> By implementing `reactivate-mark-function` alone, the problem of not
> knowing the region type would still remain. If we store the region
> type instead, then the value could be read directly, and
> `reactivate-mark` could be implemented as:
>
> (defun reactivate-mark ()
>   (pcase region-type
>     ('normal ...)
>     ('rectangle ...)
>      ...))

We don't want the code to list the finite set of known region types, I think.

We could try and make it a cl-defgeneric, but we need something on which
to dispatch.


        Stefan

Re: [Emacs-diffs] master 134ba45: Allow two mouse functions to work with Rectangle Mark mode

[Federico Tedin]

> We don't want the code to list the finite set of known region types, I think.
>
> We could try and make it a cl-defgeneric, but we need something on which
> to dispatch.

Right, to use cl-defgeneric, the region would need to be represented
with different types of objects depending on the region type (from
what I understand about the mechanics of cl-defgeneric).

If we implemented the `reactivate-mark-function` variable instead (and
the function `reactivate-mark`), would it be OK to have code like
this?

(pcase reactivate-mark-function
  ('normal-reactivate-mark-function ...)
  ('rectangle-reactivate-mark-function ...))

I can imagine using something like that in mouse-drag-and-drop-region,
when deciding how to calculate if the drag was negligible.

Re: [Emacs-diffs] master 134ba45: Allow two mouse functions to work with Rectangle Mark mode

[Stefan Monnier]

> Right, to use cl-defgeneric, the region would need to be represented
> with different types of objects depending on the region type (from
> what I understand about the mechanics of cl-defgeneric).

The dispatch can also be done based on a particular value.  E.g.

    (cl-defmethod reactivate-mark ((marktype (eql t)))
      ;; Normal mark, nothing special to do.
      )

    (cl-defmethod reactivate-mark ((marktype (eql rectangle)))
      ;; Rectangular mark
      (rectangle-mark-mode))

    ...


-- Stefan

Re: [Emacs-diffs] master 134ba45: Allow two mouse functions to work with Rectangle Mark mode

[Stefan Monnier]

> If we implemented the `reactivate-mark-function` variable instead (and
> the function `reactivate-mark`), would it be OK to have code like this?
>
> (pcase reactivate-mark-function
>   ('normal-reactivate-mark-function ...)
>   ('rectangle-reactivate-mark-function ...))

It would be evidence that the design isn't very good: a function should
be treated as a black box with which the only thing you can do is to
call it.

You generally shouldn't look at it (e.g. with an equality test) or care
about its name.


        Stefan

Re: [Emacs-diffs] master 134ba45: Allow two mouse functions to work with Rectangle Mark mode

[Federico Tedin]

>> If we implemented the `reactivate-mark-function` variable instead (and
>> the function `reactivate-mark`), would it be OK to have code like this?
>>
>> (pcase reactivate-mark-function
>>   ('normal-reactivate-mark-function ...)
>>   ('rectangle-reactivate-mark-function ...))
>
> It would be evidence that the design isn't very good: a function should
> be treated as a black box with which the only thing you can do is to
> call it.

Yes, that is true. Starting from the example code you sent earlier,
would this make more sense? I added (activate-mark) as I
imagine that a call to `reactivate-mark' would always be preceded by a
call to `activate-mark' anyways:

(cl-defmethod reactivate-mark ((marktype (eql t)))
 (activate-mark))

(cl-defmethod reactivate-mark ((marktype (eql rectangle)))
 ;; Rectangular mark
 (activate-mark)
 (rectangle-mark-mode))

Then, in mouse.el the following could be added:

(cl-defmethod drag-negligible-p ((marktype (eql t)))
 ...)

(cl-defmethod drag-negligible-p ((marktype (eql rectangle)))
 ...)

Other questions I have are:
- Where would `marktype' be defined?
- When activating an alternate region mode (e.g. rectangle-region-mode),
would the previous mark type be stored and then set back, or would it
simply go back to the default? (t)


- Federico

Re: [Emacs-diffs] master 134ba45: Allow two mouse functions to work with Rectangle Mark mode

[Stefan Monnier]

> Yes, that is true. Starting from the example code you sent earlier,
> would this make more sense? I added (activate-mark) as I
> imagine that a call to `reactivate-mark' would always be preceded by a
> call to `activate-mark' anyways:
>
> (cl-defmethod reactivate-mark ((marktype (eql t)))
>  (activate-mark))
>
> (cl-defmethod reactivate-mark ((marktype (eql rectangle)))
>  ;; Rectangular mark
>  (activate-mark)
>  (rectangle-mark-mode))

That seems to go in the right direction, yes.

> Then, in mouse.el the following could be added:
>
> (cl-defmethod drag-negligible-p ((marktype (eql t)))
>  ...)
>
> (cl-defmethod drag-negligible-p ((marktype (eql rectangle)))
>  ...)

Exactly (tho the name should clarify that we're talking about something
related to the mark).

> Other questions I have are:
> - Where would `marktype' be defined?

I think it should be the object stored in `mark-active`.

We might be able to merge reactivate-mark and activate-mark into
a single mark-activate (and use the "mark-" prefix in all other methods)
which by default just does (setq mark-active <thearg>).

> - When activating an alternate region mode (e.g. rectangle-region-mode),
> would the previous mark type be stored and then set back, or would it
> simply go back to the default? (t)

I don't understand the question: when would we st it back (to t or to
some saved value)?


        Stefan

Re: [Emacs-diffs] master 134ba45: Allow two mouse functions to work with Rectangle Mark mode

[Federico Tedin]

> I don't understand the question: when would we st it back (to t or to
> some saved value)?

Sorry if I wasn't being clear enough. What I meant is the following:
Let's we have three region modes: normal, rectangle and zones
(https://www.emacswiki.org/emacs/Zones).
If I activate rectangle region mode first and then zones region mode,
after I deactivate zones region mode,
should the rectangle region mode be activated, or simply go back to
normal? We don't have that problem right
now because there are only two region 'modes' in Emacs at the moment
(that I know of).

Re: [Emacs-diffs] master 134ba45: Allow two mouse functions to work with Rectangle Mark mode

[Stefan Monnier]

>> I don't understand the question: when would we st it back (to t or to
>> some saved value)?
> Sorry if I wasn't being clear enough. What I meant is the following:
> Let's we have three region modes: normal, rectangle and zones
> (https://www.emacswiki.org/emacs/Zones).
> If I activate rectangle region mode first and then zones region mode,
> after I deactivate zones region mode,
> should the rectangle region mode be activated, or simply go back to
> normal? We don't have that problem right
> now because there are only two region 'modes' in Emacs at the moment
> (that I know of).

I think the generic infrastructure shouldn't care: it should be up to
the specific "deactivate" command you use to decide whether to set
mark-active to nil (i.e. really deactivate), to t, or to a value it had
earlier (via reactivate).


        Stefan

Re: [Emacs-diffs] master 134ba45: Allow two mouse functions to work with Rectangle Mark mode

[Federico Tedin]

> I think the generic infrastructure shouldn't care: it should be up to
> the specific "deactivate" command you use to decide whether to set
> mark-active to nil (i.e. really deactivate), to t, or to a value it had
> earlier (via reactivate).

Understood. I'll try to find the time in the following weeks to create
an initial patch that covers what we've discussed in this
thread. Thanks for the advice!