bug#27897: 25.1; Add REGION-NONCONTIGUOUS-P arg to other replace.el commands

bug#27897: 25.1; Add REGION-NONCONTIGUOUS-P arg to other replace.el commands

[Drew Adams]

Subject line says it all.

The patch is trivial - just do the same thing to commands such as
`replace-string', `replace-regexp', `query-replace-regexp-eval', and
`map-query-replace-regexp' as you did to `query-replace' and
`query-replace-regexp': add REGION-NONCONTIGUOUS-P arg to the
interactive spec the same way, and pass it to `perform-replace' the same
way.

(Shouldn't this have been done in the first place?)


In GNU Emacs 25.1.1 (x86_64-w64-mingw32)
 of 2016-09-17 built on LAPHROAIG
Windowing system distributor 'Microsoft Corp.', version 6.1.7601
Configured using:
 'configure --without-dbus --without-compress-install CFLAGS=-static'

bug#27897: 25.1; Add REGION-NONCONTIGUOUS-P arg to other replace.el commands

[Drew Adams]

ping.

Do you need an explicit patch for this?  Would that make a
difference in getting this done?

> The patch is trivial - just do the same thing to commands such as
> `replace-string', `replace-regexp', `query-replace-regexp-eval', and
> `map-query-replace-regexp' as you did to `query-replace' and
> `query-replace-regexp': add REGION-NONCONTIGUOUS-P arg to the
> interactive spec the same way, and pass it to `perform-replace' the same
> way.
> 
> (Shouldn't this have been done in the first place?)

bug#27897: [PATCH] 25.1; Add REGION-NONCONTIGUOUS-P arg to other replace.el commands

[Drew Adams]

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

Attached is a patch for this.

(I updated doc strings for arg REGION-NONCONTIGUOUS-P.
See also bug #31207, for other doc-string changes that
should be made.)

> ping.
> 
> Do you need an explicit patch for this?  Would that make a
> difference in getting this done?
> 
> > The patch is trivial - just do the same thing to commands such as
> > `replace-string', `replace-regexp', `query-replace-regexp-eval', and
> > `map-query-replace-regexp' as you did to `query-replace' and
> > `query-replace-regexp': add REGION-NONCONTIGUOUS-P arg to the
> > interactive spec the same way, and pass it to `perform-replace' the
> same
> > way.
> >
> > (Shouldn't this have been done in the first place?)

[-- Attachment #2: replace-2018-04-18.patch --]
[-- Type: application/octet-stream, Size: 6369 bytes --]

diff -u replace-2018-04-18.el replace-2018-04-18-PATCHED.el
--- replace-2018-04-18.el	2018-04-18 07:58:34.745852300 -0700
+++ replace-2018-04-18-PATCHED.el	2018-04-18 13:58:44.825710000 -0700
@@ -345,6 +345,9 @@
 
 Fourth and fifth arg START and END specify the region to operate on.
 
+Arguments FROM-STRING, TO-STRING, DELIMITED, START, END, BACKWARD, and
+REGION-NONCONTIGUOUS-P are passed to `perform-replace' (which see).
+
 To customize possible responses, change the bindings in `query-replace-map'."
   (interactive
    (let ((common
@@ -427,7 +430,10 @@
 
 When using those Lisp features interactively in the replacement
 text, TO-STRING is actually made a list instead of a string.
-Use \\[repeat-complex-command] after this command for details."
+Use \\[repeat-complex-command] after this command for details.
+
+Arguments REGEXP, TO-STRING, DELIMITED, START, END, BACKWARD, and
+REGION-NONCONTIGUOUS-P are passed to `perform-replace' (which see)."
   (interactive
    (let ((common
 	  (query-replace-read-args
@@ -450,7 +456,7 @@
 
 (define-key esc-map [?\C-%] 'query-replace-regexp)
 
-(defun query-replace-regexp-eval (regexp to-expr &optional delimited start end)
+(defun query-replace-regexp-eval (regexp to-expr &optional delimited start end region-noncontiguous-p)
   "Replace some things after point matching REGEXP with the result of TO-EXPR.
 
 Interactive use of this function is deprecated in favor of the
@@ -496,7 +502,10 @@
 
 Third arg DELIMITED (prefix arg if interactive), if non-nil, means replace
 only matches that are surrounded by word boundaries.
-Fourth and fifth arg START and END specify the region to operate on."
+Fourth and fifth arg START and END specify the region to operate on.
+
+Arguments REGEXP, DELIMITED, START, END, and REGION-NONCONTIGUOUS-P
+are passed to `perform-replace' (which see)."
   (declare (obsolete "use the `\\,' feature of `query-replace-regexp'
 for interactive calls, and `search-forward-regexp'/`replace-match'
 for Lisp calls." "22.1"))
@@ -518,11 +527,12 @@
        (replace-match-string-symbols to)
        (list from (car to) current-prefix-arg
 	     (if (use-region-p) (region-beginning))
-	     (if (use-region-p) (region-end))))))
+	     (if (use-region-p) (region-end))
+             (and (use-region-p) (region-noncontiguous-p))))))
   (perform-replace regexp (cons 'replace-eval-replacement to-expr)
-		   t 'literal delimited nil nil start end))
+		   t 'literal delimited nil nil start end nil region-noncontiguous-p))
 
-(defun map-query-replace-regexp (regexp to-strings &optional n start end)
+(defun map-query-replace-regexp (regexp to-strings &optional n start end region-noncontiguous-p)
   "Replace some matches for REGEXP with various strings, in rotation.
 The second argument TO-STRINGS contains the replacement strings, separated
 by spaces.  This command works like `query-replace-regexp' except that
@@ -542,7 +552,10 @@
 
 A prefix argument N says to use each replacement string N times
 before rotating to the next.
-Fourth and fifth arg START and END specify the region to operate on."
+Fourth and fifth arg START and END specify the region to operate on.
+
+Arguments REGEXP, START, END, and REGION-NONCONTIGUOUS-P are passed to
+`perform-replace' (which see)."
   (interactive
    (let* ((from (read-regexp "Map query replace (regexp): " nil
 			     query-replace-from-history-variable))
@@ -555,7 +568,8 @@
 	   (and current-prefix-arg
 		(prefix-numeric-value current-prefix-arg))
 	   (if (use-region-p) (region-beginning))
-	   (if (use-region-p) (region-end)))))
+	   (if (use-region-p) (region-end))
+           (and (use-region-p) (region-noncontiguous-p)))))
   (let (replacements)
     (if (listp to-strings)
 	(setq replacements to-strings)
@@ -569,9 +583,9 @@
 				       (1+ (string-match " " to-strings))))
 	  (setq replacements (append replacements (list to-strings))
 		to-strings ""))))
-    (perform-replace regexp replacements t t nil n nil start end)))
+    (perform-replace regexp replacements t t nil n nil start end nil region-noncontiguous-p)))
 
-(defun replace-string (from-string to-string &optional delimited start end backward)
+(defun replace-string (from-string to-string &optional delimited start end backward region-noncontiguous-p)
   "Replace occurrences of FROM-STRING with TO-STRING.
 Preserve case in each match if `case-replace' and `case-fold-search'
 are non-nil and FROM-STRING has no uppercase letters.
@@ -625,10 +639,11 @@
      (list (nth 0 common) (nth 1 common) (nth 2 common)
 	   (if (use-region-p) (region-beginning))
 	   (if (use-region-p) (region-end))
-	   (nth 3 common))))
-  (perform-replace from-string to-string nil nil delimited nil nil start end backward))
+	   (nth 3 common)
+           (and (use-region-p) (region-noncontiguous-p)))))
+  (perform-replace from-string to-string nil nil delimited nil nil start end backward region-noncontiguous-p))
 
-(defun replace-regexp (regexp to-string &optional delimited start end backward)
+(defun replace-regexp (regexp to-string &optional delimited start end backward region-noncontiguous-p)
   "Replace things after point matching REGEXP with TO-STRING.
 Preserve case in each match if `case-replace' and `case-fold-search'
 are non-nil and REGEXP has no uppercase letters.
@@ -701,8 +716,9 @@
      (list (nth 0 common) (nth 1 common) (nth 2 common)
 	   (if (use-region-p) (region-beginning))
 	   (if (use-region-p) (region-end))
-	   (nth 3 common))))
-  (perform-replace regexp to-string nil t delimited nil nil start end backward))
+	   (nth 3 common)
+           (and (use-region-p) (region-noncontiguous-p)))))
+  (perform-replace regexp to-string nil t delimited nil nil start end backward region-noncontiguous-p))
 
 \f
 (defvar regexp-history nil
@@ -2313,7 +2329,12 @@
 containing a function and its first argument.  The function is
 called to generate each replacement like this:
   (funcall (car replacements) (cdr replacements) replace-count)
-It must return a string."
+It must return a string.
+
+Non-nil REGION-NONCONTIGUOUS-P means that the region is composed of
+noncontiguous pieces.  The most common example of this is a
+rectangular region, where the pieces are separated by newline
+characters."
   (or map (setq map query-replace-map))
   (and query-flag minibuffer-auto-raise
        (raise-frame (window-frame (minibuffer-window))))

bug#27897: [PATCH] 25.1; Add REGION-NONCONTIGUOUS-P arg to other replace.el commands

[Juri Linkov]

> -(defun query-replace-regexp-eval (regexp to-expr &optional delimited start end)
> +(defun query-replace-regexp-eval (regexp to-expr &optional delimited start end region-noncontiguous-p)
> -(defun map-query-replace-regexp (regexp to-strings &optional n start end)
> +(defun map-query-replace-regexp (regexp to-strings &optional n start end region-noncontiguous-p)

But why not to add the arg backward like in all other commands?

bug#27897: [PATCH] 25.1; Add REGION-NONCONTIGUOUS-P arg to other replace.el commands

[Drew Adams]

> > -(defun query-replace-regexp-eval (regexp to-expr &optional delimited
> start end)
> > +(defun query-replace-regexp-eval (regexp to-expr &optional delimited
> start end region-noncontiguous-p)
> > -(defun map-query-replace-regexp (regexp to-strings &optional n start
> end)
> > +(defun map-query-replace-regexp (regexp to-strings &optional n start
> end region-noncontiguous-p)
> 
> But why not to add the arg backward like in all other commands?

PLEASE DO, here and elsewhere where it is missing, wherever
it makes sense.

I'm having enough trouble just getting REGION-NONCONTIGUOUS-P
added. ;-)

[And getting arguments described in doc strings (bug #31207).
I was afraid that someone might even complain that I added
some rudimentary mention of other args in this patch.  But
I tried to sneak that in anyway. ;-)]

And some of the commands I added REGION-NONCONTIGUOUS-P to
already had BACKWARD, while others did not.  Should we
assume that whoever did that did it on purpose and wisely?
Dunno.  My purpose here was not to fix missing BACKWARD,
but if you want to think about that and take care of it,
please do - that would be great, along with applying my
patch for REGION-NONCONTIGUOUS-P.

I have the impression that someone added some stuff here
and there without bothering to think more about it and
doing it more systematically.  But maybe not; maybe there
are good reasons why they did what they did.  For
REGION-NONCONTIGUOUS-P, at least, it seems clear to me
that it is useful everywhere I added it.

bug#27897: [PATCH] 25.1; Add REGION-NONCONTIGUOUS-P arg to other replace.el commands

[Juri Linkov]

> And some of the commands I added REGION-NONCONTIGUOUS-P to
> already had BACKWARD, while others did not.  Should we
> assume that whoever did that did it on purpose and wisely?

query-replace-regexp-eval has no BACKWARD arg because
query-replace-regexp-eval is obsolete and should be removed.
map-query-replace-regexp has no BACKWARD arg because
map-query-replace-regexp has special logic for its prefix arg.

> Dunno.  My purpose here was not to fix missing BACKWARD,
> but if you want to think about that and take care of it,
> please do - that would be great, along with applying my
> patch for REGION-NONCONTIGUOUS-P.

Thanks, your patch pushed to master as 75a32f4.

bug#27897: [PATCH] 25.1; Add REGION-NONCONTIGUOUS-P arg to other replace.el commands

[Drew Adams]

> Thanks, your patch pushed to master as 75a32f4.

Thank you.

bug#27897: [PATCH] 25.1; Add REGION-NONCONTIGUOUS-P arg to other replace.el commands

[Drew Adams]

> Thanks, your patch pushed to master as 75a32f4.

BTW, what does that mean, in terms of something I can test.
Does it mean, for example, (> emacs-major-version 26)?

bug#27897: [PATCH] 25.1; Add REGION-NONCONTIGUOUS-P arg to other replace.el commands

[Juri Linkov]

fixed 27897 27.0.50
thanks

>> Thanks, your patch pushed to master as 75a32f4.
>
> BTW, what does that mean, in terms of something I can test.
> Does it mean, for example, (> emacs-major-version 26)?

Thanks for the reminder, now marked as fixed in 27.

bug#27897: [PATCH] 25.1; Add REGION-NONCONTIGUOUS-P arg to other replace.el commands

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Thu, 19 Apr 2018 22:36:53 +0300
> Cc: 27897-done@debbugs.gnu.org
> 
> Thanks, your patch pushed to master as 75a32f4.

I think this needs to be reflected ion NEWS, and perhaps also in the
manuals.

Thanks.

bug#27897: [PATCH] 25.1; Add REGION-NONCONTIGUOUS-P arg to other replace.el commands

[Juri Linkov]

>> Thanks, your patch pushed to master as 75a32f4.
>
> I think this needs to be reflected ion NEWS, and perhaps also in the
> manuals.

query-replace-regexp-eval and map-query-replace-regexp are obscure commands
that don't need to be documented and mentioned in NEWS.  As for the commands
replace-string and replace-regexp, their other arguments also are
not documented.

bug#27897: [PATCH] 25.1; Add REGION-NONCONTIGUOUS-P arg to other replace.el commands

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 27897@debbugs.gnu.org,  drew.adams@oracle.com
> Date: Sat, 21 Apr 2018 22:59:11 +0300
> 
> >> Thanks, your patch pushed to master as 75a32f4.
> >
> > I think this needs to be reflected ion NEWS, and perhaps also in the
> > manuals.
> 
> query-replace-regexp-eval and map-query-replace-regexp are obscure commands
> that don't need to be documented and mentioned in NEWS.  As for the commands
> replace-string and replace-regexp, their other arguments also are
> not documented.

The user-visible changes should be in NEWS.  If you think the manuals
don't need updates on their behalf, mark the NEWS entry with "---".

Thanks.