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