GNU bug report logs -
#14176
24.3.50; `bookmark-completing-read': prompt and return value for "" DEFAULT
Previous Next
Reported by: "Drew Adams" <drew.adams <at> oracle.com>
Date: Wed, 10 Apr 2013 22:09:01 UTC
Severity: normal
Found in version 24.3.50
Done: Stefan Monnier <monnier <at> iro.umontreal.ca>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 14176 in the body.
You can then email your comments to 14176 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#14176; Package
emacs.
(Wed, 10 Apr 2013 22:09:01 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
"Drew Adams" <drew.adams <at> oracle.com>:
New bug report received and forwarded. Copy sent to
bug-gnu-emacs <at> gnu.org.
(Wed, 10 Apr 2013 22:09:02 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
1. Do not insert DEFAULT, in parens, into the PROMPT if DEFAULT is "".
2. Doc string should mention the return value ("") for empty input ("")
when DEFAULT is nil.
In GNU Emacs 24.3.50.1 (i386-mingw-nt5.1.2600)
of 2013-04-02 on ODIEONE
Bzr revision: 112212 cyd <at> gnu.org-20130402033331-sqegwhqh7u1o0ars
Windowing system distributor `Microsoft Corp.', version 5.1.2600
Configured using:
`configure --with-gcc (4.7) --no-opt --enable-checking --cflags
-IC:/Devel/emacs/build/include --ldflags -LC:/Devel/emacs/build/lib'
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#14176; Package
emacs.
(Thu, 11 Apr 2013 06:04:06 GMT)
Full text and
rfc822 format available.
Message #8 received at submit <at> debbugs.gnu.org (full text, mbox):
Hi Drew,
"Drew Adams" <drew.adams <at> oracle.com> writes:
> 1. Do not insert DEFAULT, in parens, into the PROMPT if DEFAULT is "".
>
> 2. Doc string should mention the return value ("") for empty input ("")
> when DEFAULT is nil.
Isn't there a bad usage of `completing-read' here, IMO the
DEFAULT arg of `completing-read' should be used instead of:
,----
| (if (string-equal "" str) default str)
`----
Also why is 'default' let-bounded ?
Here a patch:
diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index c1d8a4a..8698821 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -437,22 +437,21 @@ the empty string."
'string-lessp)
(bookmark-all-names)))
(let* ((completion-ignore-case bookmark-completion-ignore-case)
- (default default)
(prompt (concat prompt (if default
(format " (%s): " default)
- ": ")))
- (str
- (completing-read prompt
- (lambda (string pred action)
- (if (eq action 'metadata)
- '(metadata (category . bookmark))
- (complete-with-action
- action bookmark-alist string pred)))
- nil
- 0
- nil
- 'bookmark-history)))
- (if (string-equal "" str) default str))))
+ ": "))))
+ (completing-read prompt
+ (lambda (string pred action)
+ (if (eq action 'metadata)
+ '(metadata (category . bookmark))
+ (complete-with-action
+ action bookmark-alist string pred)))
+ nil
+ 0
+ nil
+ 'bookmark-history
+ default))))
+
(defmacro bookmark-maybe-historicize-string (string)
> In GNU Emacs 24.3.50.1 (i386-mingw-nt5.1.2600)
> of 2013-04-02 on ODIEONE
> Bzr revision: 112212 cyd <at> gnu.org-20130402033331-sqegwhqh7u1o0ars
> Windowing system distributor `Microsoft Corp.', version 5.1.2600
> Configured using:
> `configure --with-gcc (4.7) --no-opt --enable-checking --cflags
> -IC:/Devel/emacs/build/include --ldflags -LC:/Devel/emacs/build/lib'
>
>
>
>
>
>
--
Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#14176; Package
emacs.
(Thu, 11 Apr 2013 14:08:13 GMT)
Full text and
rfc822 format available.
Message #11 received at 14176 <at> debbugs.gnu.org (full text, mbox):
> Isn't there a bad usage of `completing-read' here, IMO the
> DEFAULT arg of `completing-read' should be used instead of:
> ,----
> | (if (string-equal "" str) default str)
> `----
Sounds like a left-over from previous code, indeed.
> Also why is 'default' let-bounded ?
> Here a patch:
Feel free to install it (assuming you've confirmed it works, that is ;-)
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#14176; Package
emacs.
(Thu, 11 Apr 2013 15:15:03 GMT)
Full text and
rfc822 format available.
Message #14 received at submit <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
> Feel free to install it (assuming you've confirmed it works, that is ;-)
Please apply, as you know I have no bzr installation here, thanks.
--
Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#14176; Package
emacs.
(Thu, 11 Apr 2013 21:37:02 GMT)
Full text and
rfc822 format available.
Message #17 received at 14176 <at> debbugs.gnu.org (full text, mbox):
> > 1. Do not insert DEFAULT, in parens, into the PROMPT if
> > DEFAULT is "".
> >
> > 2. Doc string should mention the return value ("") for
> > empty input ("") when DEFAULT is nil.
>
> Here a patch:...
Wrt #1:
No, that patch does not work:
M-: (bookmark-completing-read "Bookmark" "")
The prompt still shows empty parens: "Bookmark (): ".
The prompt should be handled like this (or equivalent):
(if (and default (not (equal "" default)))
; ^^^^^^^^^^^^^^^^^^^^^^^^
(concat prompt
(format " (%s): "
(if (consp default)
(car default)
default)
default))
(concat prompt ": "))
(The handling of a cons DEFAULT is appropriate since Emacs 23. But that minor
enhancement is really separate from this bug report. #1 is just about empty
parens in the prompt.)
Wrt #2:
a. I misspoke a bit. The behavior was that nil (not "") was returned when
DEFAULT is nil and the user enters empty input. IMO, the value returned should
be "" (which Thierry's patch fixes, BTW).
Given that correction, this (new) behavior should be pointed out in the doc
string. That is #2 of the bug report.
IOW, the function should always return a string, and that string should be empty
("") if DEFAULT is nil and the user input is empty. And we should point out
this behavior explicitly in the doc string, for clarity.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#14176; Package
emacs.
(Fri, 12 Apr 2013 05:43:02 GMT)
Full text and
rfc822 format available.
Message #20 received at 14176 <at> debbugs.gnu.org (full text, mbox):
"Drew Adams" <drew.adams <at> oracle.com> writes:
> The prompt still shows empty parens: "Bookmark (): ".
diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index c1d8a4a..ad1609b 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -437,22 +437,18 @@ the empty string."
'string-lessp)
(bookmark-all-names)))
(let* ((completion-ignore-case bookmark-completion-ignore-case)
- (default default)
+ (default (unless (string= "" default) default))
(prompt (concat prompt (if default
(format " (%s): " default)
- ": ")))
- (str
- (completing-read prompt
- (lambda (string pred action)
- (if (eq action 'metadata)
- '(metadata (category . bookmark))
- (complete-with-action
- action bookmark-alist string pred)))
- nil
- 0
- nil
- 'bookmark-history)))
- (if (string-equal "" str) default str))))
+ ": "))))
+ (completing-read prompt
+ (lambda (string pred action)
+ (if (eq action 'metadata)
+ '(metadata (category . bookmark))
+ (complete-with-action
+ action bookmark-alist string pred)))
+ nil 0 nil 'bookmark-history default))))
+
(defmacro bookmark-maybe-historicize-string (string)
> (The handling of a cons DEFAULT is appropriate since Emacs 23. But that minor
> enhancement is really separate from this bug report. #1 is just about empty
> parens in the prompt.)
Don't see why DEFAULT would be a cons, a bookmark name is a string, so IMO
DEFAULT should be a string or nil.
>
> Wrt #2:
>
> a. I misspoke a bit. The behavior was that nil (not "") was returned when
> DEFAULT is nil and the user enters empty input. IMO, the value returned should
> be "" (which Thierry's patch fixes, BTW).
>
> Given that correction, this (new) behavior should be pointed out in the doc
> string. That is #2 of the bug report.
>
> IOW, the function should always return a string, and that string should be empty
> ("") if DEFAULT is nil and the user input is empty.
(bookmark-completing-read "test" "")
=> ""
(bookmark-completing-read "test")
=> ""
(bookmark-completing-read "test" "foo")
=> "foo"
> And we should point out this behavior explicitly in the doc string,
> for clarity.
Maybe you can provide a patch ?...
Thanks.
--
Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#14176; Package
emacs.
(Fri, 12 Apr 2013 14:19:05 GMT)
Full text and
rfc822 format available.
Message #23 received at 14176 <at> debbugs.gnu.org (full text, mbox):
> > (The handling of a cons DEFAULT is appropriate since Emacs
> > 23. But that minor enhancement is really separate from
> > this bug report. #1 is just about empty parens in the prompt.)
>
> Don't see why DEFAULT would be a cons, a bookmark name is a
> string, so IMO DEFAULT should be a string or nil.
As I said, it is not important for this bug report. But the DEF arg of
`completing-read' can in general be a list of default values. No reason to lose
that generality here. That's all.
> > Wrt #2:
> >
> > a. I misspoke a bit. The behavior was that nil (not "") was
> > returned when DEFAULT is nil and the user enters empty input. IMO,
> > the value returned should be "" (which Thierry's patch fixes, BTW).
> >
> > Given that correction, this (new) behavior should be
> > pointed out in the doc string. That is #2 of the bug report.
> >
> > IOW, the function should always return a string, and that
> > string should be empty ("") if DEFAULT is nil and the user input
> > is empty.
>
> (bookmark-completing-read "test") => ""
That is the correct behavior, IMO. It was not the behavior before the patch.
It returned nil before (i.e., it still returns nil in trunk and in 24.3).
> > And we should point out this behavior explicitly in the doc string,
> > for clarity.
>
> Maybe you can provide a patch ?...
For the doc string:
DEFAULT is a string to return if the user input is empty.
If DEFAULT is nil (absent) then return "" for empty input.
Or if a cons DEFAULT is accepted then this is all we need:
DEFAULT is as for `completing-read'.
Or if you don't want such indirection and a cons DEFAULT is accepted:
DEFAULT is a string or a list of strings. If the user input is empty
then return the string (or the first string in the list). If DEFAULT
is nil (absent) then return "" for empty input.
The last one is more explicit. This might be helpful, since returning ""
instead of nil for empty input is an incompatible change.
Prior to Emacs 22, `(nil)' was returned for empty input. Since Emacs 22, `nil'
is returned. With this correction, `""' will be returned.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#14176; Package
emacs.
(Sat, 13 Apr 2013 08:21:02 GMT)
Full text and
rfc822 format available.
Message #26 received at 14176 <at> debbugs.gnu.org (full text, mbox):
"Drew Adams" <drew.adams <at> oracle.com> writes:
> For the doc string:
>
> DEFAULT is a string to return if the user input is empty.
> If DEFAULT is nil (absent) then return "" for empty input.
Here with the docstring modified, if you want to modify the docstring
(or something else), provide a patch to allow Stefan to apply the
changes.
Thanks.
diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index c1d8a4a..c98ad0c 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -427,8 +427,8 @@ just return it."
"Prompting with PROMPT, read a bookmark name in completion.
PROMPT will get a \": \" stuck on the end no matter what, so you
probably don't want to include one yourself.
-Optional second arg DEFAULT is a string to return if the user enters
-the empty string."
+Optional arg DEFAULT is a string to return if the user input is empty.
+If DEFAULT is nil then return empty string for empty input."
(bookmark-maybe-load-default-file) ; paranoia
(if (listp last-nonmenu-event)
(bookmark-menu-popup-paned-menu t prompt
@@ -437,22 +437,17 @@ the empty string."
'string-lessp)
(bookmark-all-names)))
(let* ((completion-ignore-case bookmark-completion-ignore-case)
- (default default)
+ (default (unless (string= "" default) default))
(prompt (concat prompt (if default
(format " (%s): " default)
- ": ")))
- (str
- (completing-read prompt
- (lambda (string pred action)
- (if (eq action 'metadata)
- '(metadata (category . bookmark))
- (complete-with-action
- action bookmark-alist string pred)))
- nil
- 0
- nil
- 'bookmark-history)))
- (if (string-equal "" str) default str))))
+ ": "))))
+ (completing-read prompt
+ (lambda (string pred action)
+ (if (eq action 'metadata)
+ '(metadata (category . bookmark))
+ (complete-with-action
+ action bookmark-alist string pred)))
+ nil 0 nil 'bookmark-history default))))
(defmacro bookmark-maybe-historicize-string (string)
--
Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#14176; Package
emacs.
(Sat, 13 Apr 2013 15:51:01 GMT)
Full text and
rfc822 format available.
Message #29 received at 14176 <at> debbugs.gnu.org (full text, mbox):
> Here with the docstring modified, if you want to modify the docstring
> (or something else), provide a patch to allow Stefan to apply the
> changes.
Looks good to me, thanks. I would still suggest that we use
(if (consp default) (car default) default)
instead of just `default', in the `format' sexp, to allow for a cons DEFAULT
value.
There is no reason not to, IMO. A caller might pass, for example, a list of (a)
the current bookmark, (b) a bookmark mentioned in text at point, (c) the
bookmark whose target location is closest to point... Or a caller might pass a
list of bookmarks with targets in the current buffer/file. And so on.
But that enhancement, though trivial, is outside this bug report. And if/when
we do that, it would also be good to update the doc string to mention that
DEFAULT can be a cons (as in `completing-read').
Reply sent
to
Stefan Monnier <monnier <at> iro.umontreal.ca>:
You have taken responsibility.
(Fri, 19 Apr 2013 05:17:02 GMT)
Full text and
rfc822 format available.
Notification sent
to
"Drew Adams" <drew.adams <at> oracle.com>:
bug acknowledged by developer.
(Fri, 19 Apr 2013 05:17:02 GMT)
Full text and
rfc822 format available.
Message #34 received at 14176-done <at> debbugs.gnu.org (full text, mbox):
Thanks, installed,
Stefan
bug archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org.
(Fri, 17 May 2013 11:24:06 GMT)
Full text and
rfc822 format available.
This bug report was last modified 12 years and 30 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.