* bug#14176: 24.3.50; `bookmark-completing-read': prompt and return value for "" DEFAULT
@ 2013-04-10 22:04 Drew Adams
2013-04-11 5:59 ` Thierry Volpiatto
0 siblings, 1 reply; 10+ messages in thread
From: Drew Adams @ 2013-04-10 22:04 UTC (permalink / raw)
To: 14176
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@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'
^ permalink raw reply [flat|nested] 10+ messages in thread* bug#14176: 24.3.50; `bookmark-completing-read': prompt and return value for "" DEFAULT
2013-04-10 22:04 bug#14176: 24.3.50; `bookmark-completing-read': prompt and return value for "" DEFAULT Drew Adams
@ 2013-04-11 5:59 ` Thierry Volpiatto
2013-04-11 14:03 ` Stefan Monnier
2013-04-11 21:32 ` Drew Adams
0 siblings, 2 replies; 10+ messages in thread
From: Thierry Volpiatto @ 2013-04-11 5:59 UTC (permalink / raw)
To: 14176
Hi Drew,
"Drew Adams" <drew.adams@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@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
^ permalink raw reply related [flat|nested] 10+ messages in thread* bug#14176: 24.3.50; `bookmark-completing-read': prompt and return value for "" DEFAULT
2013-04-11 5:59 ` Thierry Volpiatto
@ 2013-04-11 14:03 ` Stefan Monnier
2013-04-11 15:10 ` Thierry Volpiatto
2013-04-11 21:32 ` Drew Adams
1 sibling, 1 reply; 10+ messages in thread
From: Stefan Monnier @ 2013-04-11 14:03 UTC (permalink / raw)
To: Thierry Volpiatto; +Cc: 14176
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread* bug#14176: 24.3.50; `bookmark-completing-read': prompt and return value for "" DEFAULT
2013-04-11 14:03 ` Stefan Monnier
@ 2013-04-11 15:10 ` Thierry Volpiatto
0 siblings, 0 replies; 10+ messages in thread
From: Thierry Volpiatto @ 2013-04-11 15:10 UTC (permalink / raw)
To: 14176
Stefan Monnier <monnier@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
^ permalink raw reply [flat|nested] 10+ messages in thread
* bug#14176: 24.3.50; `bookmark-completing-read': prompt and return value for "" DEFAULT
2013-04-11 5:59 ` Thierry Volpiatto
2013-04-11 14:03 ` Stefan Monnier
@ 2013-04-11 21:32 ` Drew Adams
2013-04-12 5:38 ` Thierry Volpiatto
1 sibling, 1 reply; 10+ messages in thread
From: Drew Adams @ 2013-04-11 21:32 UTC (permalink / raw)
To: 'Thierry Volpiatto', 14176
> > 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.
^ permalink raw reply [flat|nested] 10+ messages in thread* bug#14176: 24.3.50; `bookmark-completing-read': prompt and return value for "" DEFAULT
2013-04-11 21:32 ` Drew Adams
@ 2013-04-12 5:38 ` Thierry Volpiatto
2013-04-12 14:14 ` Drew Adams
0 siblings, 1 reply; 10+ messages in thread
From: Thierry Volpiatto @ 2013-04-12 5:38 UTC (permalink / raw)
To: Drew Adams; +Cc: 14176
"Drew Adams" <drew.adams@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
^ permalink raw reply related [flat|nested] 10+ messages in thread* bug#14176: 24.3.50; `bookmark-completing-read': prompt and return value for "" DEFAULT
2013-04-12 5:38 ` Thierry Volpiatto
@ 2013-04-12 14:14 ` Drew Adams
2013-04-13 8:16 ` Thierry Volpiatto
0 siblings, 1 reply; 10+ messages in thread
From: Drew Adams @ 2013-04-12 14:14 UTC (permalink / raw)
To: 'Thierry Volpiatto'; +Cc: 14176
> > (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.
^ permalink raw reply [flat|nested] 10+ messages in thread* bug#14176: 24.3.50; `bookmark-completing-read': prompt and return value for "" DEFAULT
2013-04-12 14:14 ` Drew Adams
@ 2013-04-13 8:16 ` Thierry Volpiatto
2013-04-13 15:46 ` Drew Adams
2013-04-19 5:12 ` Stefan Monnier
0 siblings, 2 replies; 10+ messages in thread
From: Thierry Volpiatto @ 2013-04-13 8:16 UTC (permalink / raw)
To: Drew Adams; +Cc: 14176
"Drew Adams" <drew.adams@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
^ permalink raw reply related [flat|nested] 10+ messages in thread* bug#14176: 24.3.50; `bookmark-completing-read': prompt and return value for "" DEFAULT
2013-04-13 8:16 ` Thierry Volpiatto
@ 2013-04-13 15:46 ` Drew Adams
2013-04-19 5:12 ` Stefan Monnier
1 sibling, 0 replies; 10+ messages in thread
From: Drew Adams @ 2013-04-13 15:46 UTC (permalink / raw)
To: 'Thierry Volpiatto'; +Cc: 14176
> 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').
^ permalink raw reply [flat|nested] 10+ messages in thread
* bug#14176: 24.3.50; `bookmark-completing-read': prompt and return value for "" DEFAULT
2013-04-13 8:16 ` Thierry Volpiatto
2013-04-13 15:46 ` Drew Adams
@ 2013-04-19 5:12 ` Stefan Monnier
1 sibling, 0 replies; 10+ messages in thread
From: Stefan Monnier @ 2013-04-19 5:12 UTC (permalink / raw)
To: Thierry Volpiatto; +Cc: 14176-done
Thanks, installed,
Stefan
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-04-19 5:12 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-10 22:04 bug#14176: 24.3.50; `bookmark-completing-read': prompt and return value for "" DEFAULT Drew Adams
2013-04-11 5:59 ` Thierry Volpiatto
2013-04-11 14:03 ` Stefan Monnier
2013-04-11 15:10 ` Thierry Volpiatto
2013-04-11 21:32 ` Drew Adams
2013-04-12 5:38 ` Thierry Volpiatto
2013-04-12 14:14 ` Drew Adams
2013-04-13 8:16 ` Thierry Volpiatto
2013-04-13 15:46 ` Drew Adams
2013-04-19 5:12 ` Stefan Monnier
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).