read-regexp

read-regexp

[Eli Zaretskii]

This function accepts an optional argument DEFAULT, but it does not
add its value to the list of defaults it passes to
read-from-minibuffer.  Is that intentional, and if so, what is the
reason for this behavior?

Also, it sounds like DEFAULT must be a single string or nil, it cannot
be a list of strings, as in other functions that read from the
minibuffer.  Again, is that on purpose?

(Btw, it would be good if the doc string gave answers to these
questions.)

TIA

Re: read-regexp

[Juri Linkov]

> This function accepts an optional argument DEFAULT, but it does not
> add its value to the list of defaults it passes to
> read-from-minibuffer.  Is that intentional, and if so, what is the
> reason for this behavior?
>
> Also, it sounds like DEFAULT must be a single string or nil, it cannot
> be a list of strings, as in other functions that read from the
> minibuffer.  Again, is that on purpose?
>
> (Btw, it would be good if the doc string gave answers to these
> questions.)

The problem is in the ambiguous name of the argument DEFAULT.
It was intended to provide exactly the same functionality as the
argument STRING-DESCRIBING-DEFAULT of the function `read-face-name',
i.e. to display in the minibuffer prompt what default the function
will use if the user types RET.

I thought that the name STRING-DESCRIBING-DEFAULT is too long and clumsy
but had no better idea.  Unless you want to propose a better name, I will
rename the argument DEFAULT of `read-regexp' to the same name
STRING-DESCRIBING-DEFAULT as below with docstring fixes:

Index: lisp/replace.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/replace.el,v
retrieving revision 1.276
diff -c -r1.276 replace.el
*** lisp/replace.el	12 Sep 2008 00:37:41 -0000	1.276
--- lisp/replace.el	18 Oct 2008 16:17:48 -0000
***************
*** 522,532 ****
  Maximum length of the history list is determined by the value
  of `history-length', which see.")
  
! (defun read-regexp (prompt &optional default)
    "Read regexp as a string using the regexp history and some useful defaults.
  Prompt for a regular expression with PROMPT (without a colon and
! space) in the minibuffer.  The optional string argument DEFAULT
! provides the basic default value, that is returned on typing RET.
  Additional defaults are the string at point, the last isearch regexp,
  the last isearch string, and the last replacement regexp."
    (let* ((defaults
--- 526,537 ----
  Maximum length of the history list is determined by the value
  of `history-length', which see.")
  
! (defun read-regexp (prompt &optional string-describing-default)
    "Read regexp as a string using the regexp history and some useful defaults.
  Prompt for a regular expression with PROMPT (without a colon and
! space) in the minibuffer.  The optional argument STRING-DESCRIBING-DEFAULT
! provides the value to display in the minibuffer prompt that is
! returned if the user just types RET.
  Additional defaults are the string at point, the last isearch regexp,
  the last isearch string, and the last replacement regexp."
    (let* ((defaults
***************
*** 544,555 ****
  	 (history-add-new-input nil)
  	 (input
  	  (read-from-minibuffer
! 	   (if default
! 	       (format "%s (default %s): " prompt (query-replace-descr default))
  	     (format "%s: " prompt))
  	   nil nil nil 'regexp-history defaults t)))
      (if (equal input "")
! 	default
        (prog1 input
  	(add-to-history 'regexp-history input)))))
  
--- 549,561 ----
  	 (history-add-new-input nil)
  	 (input
  	  (read-from-minibuffer
! 	   (if string-describing-default
! 	       (format "%s (default %s): " prompt
! 		       (query-replace-descr string-describing-default))
  	     (format "%s: " prompt))
  	   nil nil nil 'regexp-history defaults t)))
      (if (equal input "")
! 	string-describing-default
        (prog1 input
  	(add-to-history 'regexp-history input)))))
  
-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: read-regexp

[Stefan Monnier]

> The problem is in the ambiguous name of the argument DEFAULT.
> It was intended to provide exactly the same functionality as the
> argument STRING-DESCRIBING-DEFAULT of the function `read-face-name',
> i.e. to display in the minibuffer prompt what default the function
> will use if the user types RET.

The question is rather: why isn't that string available via M-n as
defaults usually do?  I think adding that regexp to M-n is a better
option, and that makes the use of the name `default'
completely justified.


        Stefan

Re: read-regexp

[Juri Linkov]

>> The problem is in the ambiguous name of the argument DEFAULT.
>> It was intended to provide exactly the same functionality as the
>> argument STRING-DESCRIBING-DEFAULT of the function `read-face-name',
>> i.e. to display in the minibuffer prompt what default the function
>> will use if the user types RET.
>
> The question is rather: why isn't that string available via M-n as
> defaults usually do?  I think adding that regexp to M-n is a better
> option, and that makes the use of the name `default'
> completely justified.

This string is not available via M-n due to historical requirements of
`occur'.  It used to display the last element of regexp-history
in the prompt and use it when the user types RET.  Other commands
that use read-regexp historically don't require such a string.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: read-regexp

[Stefan Monnier]

>>> The problem is in the ambiguous name of the argument DEFAULT.
>>> It was intended to provide exactly the same functionality as the
>>> argument STRING-DESCRIBING-DEFAULT of the function `read-face-name',
>>> i.e. to display in the minibuffer prompt what default the function
>>> will use if the user types RET.
>> 
>> The question is rather: why isn't that string available via M-n as
>> defaults usually do?  I think adding that regexp to M-n is a better
>> option, and that makes the use of the name `default'
>> completely justified.

> This string is not available via M-n due to historical requirements of
> `occur'.  It used to display the last element of regexp-history
> in the prompt and use it when the user types RET.  Other commands
> that use read-regexp historically don't require such a string.

I still don't understand.  Your occur explanation explains why it's
displayed in the prompt and used as default value, but not why it's not
in the M-n.


        Stefan

Re: read-regexp

[Juri Linkov]

>>>> The problem is in the ambiguous name of the argument DEFAULT.
>>>> It was intended to provide exactly the same functionality as the
>>>> argument STRING-DESCRIBING-DEFAULT of the function `read-face-name',
>>>> i.e. to display in the minibuffer prompt what default the function
>>>> will use if the user types RET.
>>>
>>> The question is rather: why isn't that string available via M-n as
>>> defaults usually do?  I think adding that regexp to M-n is a better
>>> option, and that makes the use of the name `default'
>>> completely justified.
>
>> This string is not available via M-n due to historical requirements of
>> `occur'.  It used to display the last element of regexp-history
>> in the prompt and use it when the user types RET.  Other commands
>> that use read-regexp historically don't require such a string.
>
> I still don't understand.  Your occur explanation explains why it's
> displayed in the prompt and used as default value, but not why it's not
> in the M-n.

It's not in the M-n because the last history element is available in the
history list via M-p.  Since the same value is already available via
M-p and RET, we can avoid putting it to the third place - M-n, thus
making access to other values on M-n more easy with less keystrokes.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: read-regexp

[Eli Zaretskii]

> From: Juri Linkov <juri@jurta.org>
> Cc: emacs-devel@gnu.org
> Date: Sat, 18 Oct 2008 19:19:45 +0300
> 
> The problem is in the ambiguous name of the argument DEFAULT.

No, the problem is also with the "Additional defaults are ..." part,
which makes it sound like DEFAULT is treated like those "additional
defaults".  But in fact, it isn't.

> I thought that the name STRING-DESCRIBING-DEFAULT is too long and clumsy
> but had no better idea.  Unless you want to propose a better name, I will
> rename the argument DEFAULT of `read-regexp' to the same name
> STRING-DESCRIBING-DEFAULT as below with docstring fixes:

DEFAULT-VALUE sounds good enough to me, but please also make sure that
the doc string does not hint it is somehow related to the "additional
defaults".  In fact, I suggest to rephrase the last sentence of the
doc string such that it doesn't use the word "defaults", but instead
explains that those are values you pass to read-from-minibuffer for
making them available via M-n.

Thanks.

Re: read-regexp

[Juri Linkov]

>> I thought that the name STRING-DESCRIBING-DEFAULT is too long and clumsy
>> but had no better idea.  Unless you want to propose a better name, I will
>> rename the argument DEFAULT of `read-regexp' to the same name
>> STRING-DESCRIBING-DEFAULT as below with docstring fixes:
>
> DEFAULT-VALUE sounds good enough to me, but please also make sure that
> the doc string does not hint it is somehow related to the "additional
> defaults".  In fact, I suggest to rephrase the last sentence of the
> doc string such that it doesn't use the word "defaults", but instead
> explains that those are values you pass to read-from-minibuffer for
> making them available via M-n.

Ok, this could be rephrased as:

  "Values available via M-n are the string at point, the last isearch
   regexp, the last isearch string, and the last replacement regexp."

-- 
Juri Linkov
http://www.jurta.org/emacs/

Word Delimited Query Replace (was: read-regexp)

[Juri Linkov]

BTW, I discovered a UI problem: to prepare the patch it was necessary
to rename the string "default" but not its plural form "defaults" in
read-regexp.  So I selected the region around the body of read-regexp
and typed `C-s default C-u M-%'.  The C-u prefix argument of query-replace
means to replace only matches surrounded by word boundaries, but while
implementing isearch-query-replace a few years ago we missed this case,
so currently isearch-query-replace just ignores it.

The patch below fixes this problem by adding the prefix argument
DELIMITED to isearch-query-replace and isearch-query-replace-regexp that
has the same meaning as the prefix argument DELIMITED of query-replace
and query-replace-regexp.

Also I found another problem: there is no indication in the query-replace
minibuffer prompts about the fact that replacements are word-delimited.
This is unlike the regexp mode where there is such indication.
So in addition to currently existing possible replacement prompts:

    Query replace regexp
    Query replace regexp in region

the patch below adds similar prompts when the word-delimited argument
is non-nil:

    Query replace word
    Query replace word in region

Index: lisp/isearch.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/isearch.el,v
retrieving revision 1.330
diff -c -r1.330 isearch.el
*** lisp/isearch.el	25 Sep 2008 17:06:22 -0000	1.330
--- lisp/isearch.el	18 Oct 2008 16:21:40 -0000
***************
*** 1333,1341 ****
    (sit-for 1)
    (isearch-update))
  
! (defun isearch-query-replace (&optional regexp-flag)
!   "Start `query-replace' with string to replace from last search string."
!   (interactive)
    (barf-if-buffer-read-only)
    (if regexp-flag (setq isearch-regexp t))
    (let ((case-fold-search isearch-case-fold-search)
--- 1333,1344 ----
    (sit-for 1)
    (isearch-update))
  
! (defun isearch-query-replace (&optional delimited regexp-flag)
!   "Start `query-replace' with string to replace from last search string.
! The arg DELIMITED (prefix arg if interactive), if non-nil, means replace
! only matches surrounded by word boundaries."
!   (interactive
!    (list current-prefix-arg))
    (barf-if-buffer-read-only)
    (if regexp-flag (setq isearch-regexp t))
    (let ((case-fold-search isearch-case-fold-search)
***************
*** 1356,1371 ****
       isearch-string
       (query-replace-read-to
        isearch-string
!       (if isearch-regexp "Query replace regexp" "Query replace")
        isearch-regexp)
!      t isearch-regexp isearch-word nil nil
       (if (and transient-mark-mode mark-active) (region-beginning))
       (if (and transient-mark-mode mark-active) (region-end)))))
  
! (defun isearch-query-replace-regexp ()
!   "Start `query-replace-regexp' with string to replace from last search string."
!   (interactive)
!   (isearch-query-replace t))
  
  (defun isearch-occur (regexp &optional nlines)
    "Run `occur' with regexp to search from the current search string.
--- 1359,1380 ----
       isearch-string
       (query-replace-read-to
        isearch-string
!       (concat "Query replace"
! 	      (if (or delimited isearch-word) " word" "")
! 	      (if isearch-regexp " regexp" "")
! 	      (if (and transient-mark-mode mark-active) " in region" ""))
        isearch-regexp)
!      t isearch-regexp (or delimited isearch-word) nil nil
       (if (and transient-mark-mode mark-active) (region-beginning))
       (if (and transient-mark-mode mark-active) (region-end)))))
  
! (defun isearch-query-replace-regexp (&optional delimited)
!   "Start `query-replace-regexp' with string to replace from last search string.
! The arg DELIMITED (prefix arg if interactive), if non-nil, means replace
! only matches surrounded by word boundaries."
!   (interactive
!    (list current-prefix-arg))
!   (isearch-query-replace delimited t))
  
  (defun isearch-occur (regexp &optional nlines)
    "Run `occur' with regexp to search from the current search string.

Index: lisp/replace.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/replace.el,v
retrieving revision 1.276
diff -u -w -b -r1.276 replace.el
--- lisp/replace.el	12 Sep 2008 00:37:41 -0000	1.276
+++ lisp/replace.el	18 Oct 2008 16:21:50 -0000
@@ -223,11 +223,12 @@
 Fourth and fifth arg START and END specify the region to operate on.
 
 To customize possible responses, change the \"bindings\" in `query-replace-map'."
-  (interactive (let ((common
+  (interactive
+   (let ((common
 		      (query-replace-read-args
-		       (if (and transient-mark-mode mark-active)
-			 "Query replace in region"
-			 "Query replace")
+	   (concat "Query replace"
+		   (if current-prefix-arg " word" "")
+		   (if (and transient-mark-mode mark-active) " in region" ""))
 			 nil)))
 		 (list (nth 0 common) (nth 1 common) (nth 2 common)
 		       ;; These are done separately here
@@ -289,9 +290,10 @@
   (interactive
    (let ((common
 	  (query-replace-read-args
-	   (if (and transient-mark-mode mark-active)
-	       "Query replace regexp in region"
-	     "Query replace regexp")
+	   (concat "Query replace"
+		   (if current-prefix-arg " word" "")
+		   " regexp"
+		   (if (and transient-mark-mode mark-active) " in region" ""))
 	   t)))
      (list (nth 0 common) (nth 1 common) (nth 2 common)
 	   ;; These are done separately here
@@ -447,9 +449,10 @@
   (interactive
    (let ((common
 	  (query-replace-read-args
-	   (if (and transient-mark-mode mark-active)
-	       "Replace string in region"
-	     "Replace string")
+	   (concat "Replace"
+		   (if current-prefix-arg " word" "")
+		   " string"
+		   (if (and transient-mark-mode mark-active) " in region" ""))
 	   nil)))
      (list (nth 0 common) (nth 1 common) (nth 2 common)
 	   (if (and transient-mark-mode mark-active)
@@ -504,9 +507,10 @@
   (interactive
    (let ((common
 	  (query-replace-read-args
-	   (if (and transient-mark-mode mark-active)
-	       "Replace regexp in region"
-	     "Replace regexp")
+	   (concat "Replace"
+		   (if current-prefix-arg " word" "")
+		   " regexp"
+		   (if (and transient-mark-mode mark-active) " in region" ""))
 	   t)))
      (list (nth 0 common) (nth 1 common) (nth 2 common)
 	   (if (and transient-mark-mode mark-active)
@@ -1714,6 +1720,7 @@
 			 (with-output-to-temp-buffer "*Help*"
 			   (princ
 			    (concat "Query replacing "
+				    (if delimited-flag "word " "")
 				    (if regexp-flag "regexp " "")
 				    from-string " with "
 				    next-replacement ".\n\n"

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Word Delimited Query Replace

[Stefan Monnier]

> BTW, I discovered a UI problem: to prepare the patch it was necessary
> to rename the string "default" but not its plural form "defaults" in
> read-regexp.  So I selected the region around the body of read-regexp
> and typed `C-s default C-u M-%'.  The C-u prefix argument of query-replace
> means to replace only matches surrounded by word boundaries, but while
> implementing isearch-query-replace a few years ago we missed this case,
> so currently isearch-query-replace just ignores it.

IIUC the right way to do it is to use M-s w during the search to toggle
isearch-word, after which M-% should DTRT (just like M-% in regexp
isearch requests a query-replace-regexp).

Also, are you sure your patch works?  As far as I can tell, C-u
currently leaves isearch, and I don't see anything in your patch that
would change it.


        Stefan

Re: Word Delimited Query Replace

[Juri Linkov]

>> BTW, I discovered a UI problem: to prepare the patch it was necessary
>> to rename the string "default" but not its plural form "defaults" in
>> read-regexp.  So I selected the region around the body of read-regexp
>> and typed `C-s default C-u M-%'.  The C-u prefix argument of query-replace
>> means to replace only matches surrounded by word boundaries, but while
>> implementing isearch-query-replace a few years ago we missed this case,
>> so currently isearch-query-replace just ignores it.
>
> IIUC the right way to do it is to use M-s w during the search to toggle
> isearch-word, after which M-% should DTRT (just like M-% in regexp
> isearch requests a query-replace-regexp).

Currently, M-s w is broken, and I'll will try to fix it.  But even after it
is fixed, I think it would be good to provide two ways to run word replacement
from isearch, exactly as we already provide two ways to run regexp replacement:

regexp search/replace: C-M-s M-% and C-s C-M-%

  word search/replace: M-s w M-% and C-s C-u M-%

> Also, are you sure your patch works?  As far as I can tell, C-u
> currently leaves isearch, and I don't see anything in your patch that
> would change it.

I have isearch-allow-scroll=t, and it allows C-u to not leave isearch.
Perhaps this should be the default behavior.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Word Delimited Query Replace

[Stefan Monnier]

> Currently, M-s w is broken, and I'll will try to fix it.  But even
> after it is fixed, I think it would be good to provide two ways to run
> word replacement from isearch, exactly as we already provide two ways
> to run regexp replacement:
>   regexp search/replace: C-M-s M-% and C-s C-M-%
>     word search/replace: M-s w M-% and C-s C-u M-%

I'd rather discourage the second alternatives.  After all, C-s C-M-% is
rather odd since it'll use the string you searched as a regexp, so if
you were searching for the string "a*b" the replacement will not apply
to what isearch matched.

>> Also, are you sure your patch works?  As far as I can tell, C-u
>> currently leaves isearch, and I don't see anything in your patch that
>> would change it.
> I have isearch-allow-scroll=t, and it allows C-u to not leave isearch.
> Perhaps this should be the default behavior.

I'd rather not change it.


        Stefan

Re: Word Delimited Query Replace

[Juri Linkov]

>> Currently, M-s w is broken, and I'll will try to fix it.  But even
>> after it is fixed, I think it would be good to provide two ways to run
>> word replacement from isearch, exactly as we already provide two ways
>> to run regexp replacement:
>>   regexp search/replace: C-M-s M-% and C-s C-M-%
>>     word search/replace: M-s w M-% and C-s C-u M-%
>
> I'd rather discourage the second alternatives.  After all, C-s C-M-% is
> rather odd since it'll use the string you searched as a regexp, so if
> you were searching for the string "a*b" the replacement will not apply
> to what isearch matched.

IMHO, it would be worse when the user habitually types C-u M-% to run
word replacement or C-M-% to run regexp replacement and these keys do not
do what the user wants.

We could provide these keys only as a back-up for the case we the user
intuitively types it, and discourage them simply by not advertising in
the documentation.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Word Delimited Query Replace

[Stefan Monnier]

>>> Currently, M-s w is broken, and I'll will try to fix it.  But even
>>> after it is fixed, I think it would be good to provide two ways to run
>>> word replacement from isearch, exactly as we already provide two ways
>>> to run regexp replacement:
>>> regexp search/replace: C-M-s M-% and C-s C-M-%
>>> word search/replace: M-s w M-% and C-s C-u M-%
>> 
>> I'd rather discourage the second alternatives.  After all, C-s C-M-% is
>> rather odd since it'll use the string you searched as a regexp, so if
>> you were searching for the string "a*b" the replacement will not apply
>> to what isearch matched.

> IMHO, it would be worse when the user habitually types C-u M-% to run
> word replacement or C-M-% to run regexp replacement and these keys do not
> do what the user wants.

They still do what the user wants outside of isearch.  But the new
"query-replace from isearch" only works with M-% (which is one of the
advantages I see in this M-%: it provides the same featureset while
using fewer keys: the global M-% and C-M-% can be used for anything
else now).

As it happens, it seems that the problem you found is due to your
setting isearch-allow-scroll=t which caused the C-u to be eaten without
passing it to the query-replace, whereas with isearch-allow-scroll=nil
the C-u exits isearch and is then passed to query-replace correctly.

So I guess your change is OK: in the case that C-u is eaten by
isearch-allow-scroll=t, then the C-u will be passed on to query-replace,
and if it's not, then your change makes no difference and the C-u
exits isearch.

> We could provide these keys only as a back-up for the case we the user
> intuitively types it, and discourage them simply by not advertising in
> the documentation.

Yes, it's probably OK,


        Stefan

Re: Word Delimited Query Replace

[Juri Linkov]

> So I guess your change is OK: in the case that C-u is eaten by
> isearch-allow-scroll=t, then the C-u will be passed on to query-replace,
> and if it's not, then your change makes no difference and the C-u
> exits isearch.
>
>> We could provide these keys only as a back-up for the case we the user
>> intuitively types it, and discourage them simply by not advertising in
>> the documentation.
>
> Yes, it's probably OK,

I've added the detailed information in the doc string of isearch-query-replace.

-- 
Juri Linkov
http://www.jurta.org/emacs/