query-replace and command-history

query-replace and command-history

[Ben North]

I would like to propose the following patch to replace.el, which
modifies the behaviour of `query-replace' and friends slightly,
when `transient-mark-mode' is turned on.

Problem: Suppose I mark out a region of text, and perform a
`query-replace', changing "foo" into "bar".  Suppose I then want
to perform this same change on a different region.  I mark out
the other region, hit `repeat-complex-command', and am prompted
with

Redo: (query-replace "foo" "bar" nil 1383 1730)

Here, 1383--1730 is where the region was when I performed the
first `query-replace'.  What I would prefer is to be prompted by
something more like

Redo: (query-replace "foo" "bar" nil (region-beginning) (region-end))

I think this would be more natural behaviour.  Also, it would
match the behaviour for interactive commands which either use
the "r" specifier, or explicitly call `region-beginning' or
`region-end' in their `interactive' form.

Solution: The patch below defines new functions,
`region-beginning-safe' and `region-end-safe' which are roughly
analogous to `car-safe'.  `region-beginning-safe' returns the
beginning of the region if it is active, and nil otherwise.  It
always returns nil if `transient-mark-mode' is nil.  Similarly
for `region-end-safe'.

(There may be a more natural file in which to define the
`region-beginning-safe' and `region-end-safe' functions.)

The patch modifies `query-replace-read-args' to use these new
functions, to reduce code duplication.

The patch also defines a new function
`replace-revise-command-history', which modifies the `car' of
`command-history' so that the fourth and fifth arguments are
changed to calls to `region-beginning-safe' and
`region-end-safe'.  It only does this if the `car' of
`command-history' is a call to the currently-executing function,
as extracted from `backtrace-frame'.  This could probably be
fooled by people being suitably perverse, but it does perform
some sanity checks.

The patch then inserts calls to `replace-revise-command-history'
at the start of these functions:
   query-replace
   query-replace-regexp
   query-replace-regexp-eval
   map-query-replace-regexp
   replace-string
   replace-regexp

I have found this new behaviour very useful, and would
appreciate comments.  I would like to thank Mark Hulme-Jones for
helpful suggestions during development of this patch.

Secondly, while developing this patch, I found a bug in
`map-query-replace-regexp'.  If it is called with
`transient-mark-mode' t, and the region active, it fails with

copy-marker: Wrong type argument: number-or-marker-p, nil

The second patch below (a one-line change) just fixes this bug,
in case the developers wish to fix this bug but have questions
about the main patch.  The bug-fix is included in the first
(command-history) patch.  Both patches are against the 1.163
revision.

Thanks,

Ben.


- - - - 8< - - - - NEW BEHAVIOUR PATCH - - - - 8< - - - -
--- HEAD--replace.el	2003-02-20 11:48:20.000000000 +0000
+++ replace.el	2003-02-20 15:51:31.000000000 +0000
@@ -64,6 +64,25 @@
   :group 'matching
   :version "21.4")

+(defun region-beginning-safe ()
+  "Return the beginning of region, or nil if region not active.
+If `transient-mark-mode' is nil, return nil."
+  (and transient-mark-mode mark-active (region-beginning)))
+
+(defun region-end-safe ()
+  "Return the end of region, or nil if region not active.
+If `transient-mark-mode' is nil, return nil."
+  (and transient-mark-mode mark-active (region-end)))
+
+(defun replace-revise-command-history ()
+  (let ((frame-of-caller (backtrace-frame 3))
+        (ch-top (car-safe command-history)))
+    (when (and (equal (car (cdr frame-of-caller))
+                      (car ch-top))
+               (>= (length ch-top) 6))
+      (setcar (nthcdr 4 ch-top) '(region-beginning-safe))
+      (setcar (nthcdr 5 ch-top) '(region-end-safe)))))
+
 (defun query-replace-read-args (string regexp-flag &optional noerror)
   (unless noerror
     (barf-if-buffer-read-only))
@@ -87,9 +106,7 @@
     (setq to (read-from-minibuffer (format "%s %s with: " string from)
 				   nil nil nil
 				   query-replace-to-history-variable from t))
-    (if (and transient-mark-mode mark-active)
-	(list from to current-prefix-arg (region-beginning) (region-end))
-      (list from to current-prefix-arg nil nil))))
+    (list from to current-prefix-arg (region-beginning-safe) (region-end-safe))))

 (defun query-replace (from-string to-string &optional delimited start end)
   "Replace some occurrences of FROM-STRING with TO-STRING.
@@ -117,6 +134,7 @@

 To customize possible responses, change the \"bindings\" in `query-replace-map'."
   (interactive (query-replace-read-args "Query replace" nil))
+  (replace-revise-command-history)
   (perform-replace from-string to-string t nil delimited nil nil start end))

 (define-key esc-map "%" 'query-replace)
@@ -149,6 +167,7 @@
 and `\\=\\N' (where N is a digit) stands for
  whatever what matched the Nth `\\(...\\)' in REGEXP."
   (interactive (query-replace-read-args "Query replace regexp" t))
+  (replace-revise-command-history)
   (perform-replace regexp to-string t t delimited nil nil start end))
 (define-key esc-map [?\C-%] 'query-replace-regexp)

@@ -198,6 +217,7 @@
      ;; and the user might enter a single token.
      (replace-match-string-symbols to)
      (list from (car to) current-prefix-arg start end)))
+  (replace-revise-command-history)
   (perform-replace regexp (cons 'replace-eval-replacement to-expr)
 		   t t delimited nil nil start end))

@@ -236,7 +256,8 @@
 		       from)
 	       nil nil nil
 	       'query-replace-history from t))
-     (list from to start end current-prefix-arg)))
+     (list from to (prefix-numeric-value current-prefix-arg) start end)))
+  (replace-revise-command-history)
   (let (replacements)
     (if (listp to-strings)
 	(setq replacements to-strings)
@@ -278,6 +299,7 @@
 \(You may need a more complex loop if FROM-STRING can match the null string
 and TO-STRING is also null.)"
   (interactive (query-replace-read-args "Replace string" nil))
+  (replace-revise-command-history)
   (perform-replace from-string to-string nil nil delimited nil nil start end))

 (defun replace-regexp (regexp to-string &optional delimited start end)
@@ -305,6 +327,7 @@
     (replace-match TO-STRING nil nil))
 which will run faster and will not set the mark or print anything."
   (interactive (query-replace-read-args "Replace regexp" t))
+  (replace-revise-command-history)
   (perform-replace regexp to-string nil t delimited nil nil start end))

 \f
- - - - 8< - - - - END OF NEW BEHAVIOUR PATCH - - - - 8< - - - -


- - - - 8< - - - - BUGFIX PATCH - - - - 8< - - - -
--- HEAD--replace.el	2003-02-20 11:48:20.000000000 +0000
+++ bugfix--replace.el	2003-02-20 15:53:29.000000000 +0000
@@ -236,7 +236,7 @@
 		       from)
 	       nil nil nil
 	       'query-replace-history from t))
-     (list from to start end current-prefix-arg)))
+     (list from to (prefix-numeric-value current-prefix-arg) start end)))
   (let (replacements)
     (if (listp to-strings)
 	(setq replacements to-strings)
- - - - 8< - - - - END OF BUGFIX PATCH - - - - 8< - - - -




----
This e-mail messages has been scanned by MailScanner and is believed to be
clean of dangerous content and virus'.
http://www.mailscanner.info

Re: query-replace and command-history

[Richard Stallman]

I will fix this in a cleaner way.  Thanks for reporting the bug.

Re: query-replace and command-history

[Ben North]

I recently wrote:
> I would like to propose the following patch to replace.el, which
> modifies the behaviour of `query-replace' and friends slightly,
> when `transient-mark-mode' is turned on.
>
> [ Messy patch which hacks around with `command-history' directly ]

RMS replied:
> I will fix this in a cleaner way.

I've just looked at the most recent code in Savannah, and the
fix is indeed cleaner.  I think one small bug remains, though.
If you give a prefix argument to `map-query-replace-regexp' by
pressing C-u (as opposed to C-u 4), you get a

    if: Wrong type argument: number-or-marker-p, (4)

error.

This patch fixes that:

diff -u HEAD-replace.el new-replace.el
--- HEAD-replace.el     2003-02-26 14:08:57.000000000 +0000
+++ new-replace.el      2003-02-26 14:39:15.000000000 +0000
@@ -252,7 +252,7 @@
                       from)
               nil nil nil
               'query-replace-history from t))
-     (list from to current-prefix-arg
+     (list from to (prefix-numeric-value current-prefix-arg)
           (if (and transient-mark-mode mark-active)
               (region-beginning))
           (if (and transient-mark-mode mark-active)

Thanks,

Ben.




----
This e-mail messages has been scanned by MailScanner and is believed to be
clean of dangerous content and virus'.
http://www.mailscanner.info