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.