bug#19829: 25.0.50; query-replace in rectangle regions do not honor boundaries

[Juri Linkov <juri@linkov.net>]

>> +  (lambda (delete &optional positions)
>
> No: additional *value*, not additional *argument*.

Sorry, I misread your previous message.  Then maybe the argument `delete'
should be renamed to something else, but I have no idea for another name.

>> +  (let (start end)
>> +    (move-to-column startcol)
>> +    (setq start (point))
>> +    (move-to-column endcol)
>> +    (setq end (point))
>> +    (setcdr positions (cons (cons start end) (cdr positions)))))
>
> Aka
>      (move-to-column startcol)
>      (let ((start (point)))
>        (move-to-column endcol)
>        (push (cons start (point)) (cdr positions)))
>
>> +    (apply-on-rectangle 'extract-rectangle-position
>                           ^^^
>                           #'

Done in the new patch.

>> +         ;; Use local binding in add-function below.
>> +         (isearch-filter-predicate isearch-filter-predicate)
>> +         (rectangular-region-positions nil)
>
> replace.el should not know about rectangles and shouldn't check
> rectangle-mark-mode.  It should simply always call
> region-extract-function to get the various elements that make up the
> "region".

This is true generally, but query-replace is a very special command.
It already does many unorthodox things like putting markers on
region boundaries, and then deactivating the mark.  What is more,
it needs to use rectangle boundaries in isearch-filter-predicate,
so lazy-highlight will match only strings it's going to replace.

I propose to start with simpler commands that operate on region,
and add support for rectangular regions to them to see how this
feature develops, and then optimize query-replace based on
this experience.

diff --git a/lisp/simple.el b/lisp/simple.el
index 25293ed..76d84c9 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -958,13 +958,19 @@ (defcustom delete-active-region t
 (defvar region-extract-function
   (lambda (delete)
     (when (region-beginning)
-      (if (eq delete 'delete-only)
-          (delete-region (region-beginning) (region-end))
-        (filter-buffer-substring (region-beginning) (region-end) delete))))
+      (cond
+       ((eq delete 'positions)
+        (list (cons (region-beginning) (region-end))))
+       ((eq delete 'delete-only)
+        (delete-region (region-beginning) (region-end)))
+       (t
+        (filter-buffer-substring (region-beginning) (region-end) delete)))))
   "Function to get the region's content.
 Called with one argument DELETE.
 If DELETE is `delete-only', then only delete the region and the return value
 is undefined.  If DELETE is nil, just return the content as a string.
+If DELETE is `positions', then don't delete, but just return the
+positions of the region as a list of (START . END) boundaries.
 If anything else, delete the region and return its content as a string.")
 
 (defun delete-backward-char (n &optional killflag)
diff --git a/lisp/rect.el b/lisp/rect.el
index c5a5486..271f720 100644
--- a/lisp/rect.el
+++ b/lisp/rect.el
@@ -257,6 +257,19 @@ (defun extract-rectangle (start end)
     (apply-on-rectangle 'extract-rectangle-line start end lines)
     (nreverse (cdr lines))))
 
+(defun extract-rectangle-positions (start end)
+  "Return the positions of the rectangle with corners at START and END.
+Return it as a list of (START . END) boundaries, one for each line of
+the rectangle."
+  (let (positions)
+    (apply-on-rectangle
+     (lambda (startcol endcol)
+       (move-to-column startcol)
+       (push (cons (prog1 (point) (move-to-column endcol)) (point))
+	     positions))
+     start end)
+    (nreverse positions)))
+
 (defvar killed-rectangle nil
   "Rectangle for `yank-rectangle' to insert.")
 
@@ -681,8 +694,12 @@ (defun rectangle-previous-line (&optional n)
 
 
 (defun rectangle--extract-region (orig &optional delete)
-  (if (not rectangle-mark-mode)
-      (funcall orig delete)
+  (cond
+   ((not rectangle-mark-mode)
+    (funcall orig delete))
+   ((eq delete 'positions)
+    (extract-rectangle-positions (region-beginning) (region-end)))
+   (t
     (let* ((strs (funcall (if delete
                               #'delete-extract-rectangle
                             #'extract-rectangle)
@@ -696,7 +713,7 @@ (defun rectangle--extract-region (orig &optional delete)
         (put-text-property 0 (length str) 'yank-handler
                            `(rectangle--insert-for-yank ,strs t)
                            str)
-        str))))
+        str)))))
 
 (defun rectangle--insert-for-yank (strs)
   (push (point) buffer-undo-list)
diff --git a/lisp/emulation/cua-rect.el b/lisp/emulation/cua-rect.el
index ea8b524..044939f 100644
--- a/lisp/emulation/cua-rect.el
+++ b/lisp/emulation/cua-rect.el
@@ -666,6 +666,22 @@ (defun cua--extract-rectangle ()
     	     (setq rect (cons row rect))))))
     (nreverse rect)))
 
+(defun cua--extract-rectangle-positions ()
+  (let (rect)
+    (if (not (cua--rectangle-virtual-edges))
+        (cua--rectangle-operation nil nil nil nil nil ; do not tabify
+          (lambda (s e _l _r)
+             (setq rect (cons (cons s e) rect))))
+      (cua--rectangle-operation nil 1 nil nil nil ; do not tabify
+        (lambda (s e l r _v)
+           (goto-char s)
+           (move-to-column l)
+           (setq s (point))
+           (move-to-column r)
+           (setq e (point))
+           (setq rect (cons (cons s e) rect)))))
+    (nreverse rect)))
+
 (defun cua--insert-rectangle (rect &optional below paste-column line-count)
   ;; Insert rectangle as insert-rectangle, but don't set mark and exit with
   ;; point at either next to top right or below bottom left corner
@@ -1405,8 +1421,12 @@ (defun cua--rectangle-highlight-for-redisplay (orig &rest args)
 
 (defun cua--rectangle-region-extract (orig &optional delete)
   (cond
-   ((not cua--rectangle) (funcall orig delete))
-   ((eq delete 'delete-only) (cua--delete-rectangle))
+   ((not cua--rectangle)
+    (funcall orig delete))
+   ((eq delete 'positions)
+    (cua--extract-rectangle-positions))
+   ((eq delete 'delete-only)
+    (cua--delete-rectangle))
    (t
     (let* ((strs (cua--extract-rectangle))
            (str (mapconcat #'identity strs "\n")))



diff --git a/lisp/replace.el b/lisp/replace.el
index e0636e0..f2fee8c 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -2089,6 +2089,9 @@ (defun perform-replace (from-string replacements
 
          ;; If non-nil, it is marker saying where in the buffer to stop.
          (limit nil)
+         ;; Use local binding in add-function below.
+         (isearch-filter-predicate isearch-filter-predicate)
+         (rectangular-region-positions nil)
 
          ;; Data for the next match.  If a cons, it has the same format as
          ;; (match-data); otherwise it is t if a match is possible at point.
@@ -2101,6 +2104,24 @@ (defun perform-replace (from-string replacements
                       "Query replacing %s with %s: (\\<query-replace-map>\\[help] for help) ")
                      minibuffer-prompt-properties))))
 
+    ;; If rectangle is active, operate on rectangular region.
+    (when (and (boundp 'rectangle-mark-mode) rectangle-mark-mode)
+      (setq rectangular-region-positions
+            (mapcar (lambda (position)
+                      (cons (copy-marker (car position))
+                            (copy-marker (cdr position))))
+                    (funcall region-extract-function 'positions)))
+      (add-function :after-while isearch-filter-predicate
+                    (lambda (start end)
+                      (delq nil (mapcar
+                                 (lambda (positions)
+                                   (and
+                                    (>= start (car positions))
+                                    (<= start (cdr positions))
+                                    (>= end   (car positions))
+                                    (<= end   (cdr positions))))
+                                 rectangular-region-positions)))))
+
     ;; If region is active, in Transient Mark mode, operate on region.
     (if backward
 	(when end