Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly handle list and nil default in ivy-completing-read #1049

Merged
merged 8 commits into from Jul 20, 2017

Conversation

@DarwinAwardWinner
Copy link
Contributor

@DarwinAwardWinner DarwinAwardWinner commented Jun 5, 2017

This pull request allows ivy-completing-read to handle a DEF argument that is a list or nil consistently with completing-read-default. It also adds tests for this behavior, including tests for the case where DEF is not an element of COLLECTION.

@DarwinAwardWinner
Copy link
Contributor Author

@DarwinAwardWinner DarwinAwardWinner commented Jun 5, 2017

Should fix #1041 and also address #636 even more.

@DarwinAwardWinner DarwinAwardWinner force-pushed the DarwinAwardWinner:nil-default branch Jun 5, 2017
@DarwinAwardWinner
Copy link
Contributor Author

@DarwinAwardWinner DarwinAwardWinner commented Jun 5, 2017

By the way, I have prevoiusly completed the Emacs copyright assignment.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Jun 6, 2017

Thanks for the work, a large portion of it is definitely an improvement.

But this I don't like:

(should
 (equal ""
        (ivy-with '(ivy-completing-read "Pick: " '("a" "b" "c") nil t nil nil nil)
                  "RET")))

It means that even this common code will have "" as the unnecessary and (always) unwanted default:

(ivy-completing-read "Pick: " '("a" "b" "c"))

Probably the best way to go is to set ivy-completing-read-default-is-empty-string to nil by default and update the last two tests accordingly.

abo-abo added a commit that referenced this pull request Jun 6, 2017
@DarwinAwardWinner
Copy link
Contributor Author

@DarwinAwardWinner DarwinAwardWinner commented Jun 6, 2017

The problem it that it's not always unwanted. Some functions call completing-read and handle the default themselves if it returns "". The docstring for completing-read specifically mentions this behavior (emphasis mine):

If the input is null, ‘completing-read’ returns DEF, or the first element of the list of default values, or an empty string if DEF is nil, regardless of the value of REQUIRE-MATCH.

So if you don't allow it to work like this, you'll break some functions that work like this (same example from here) :

(defun pick-a-fruit ()
  (interactive)
  (let* ((default "banana")
         (prompt (format "Pick a fruit (default %s): " default))
         (collection '("apple" "banana" "cherry"))
         (selection (completing-read prompt collection nil t)))
    (when (string= selection "")
      (setq selection default))
    (message "You selected %s" selection)))

Using ivy would effectively change the default to "apple" rather than "banana", despite the fact that the prompt clearly specifies that the default is banana. You'd have to use C-M-j to get "banana". I think it's unfortunate that completing-read works in this way, but it can't really be changed now. As a compromise, this behavior could be enabled only when REQUIRE-MATCH is non-nil, since otherwise you can already enter an empty string with C-M-j.

Alternatively, you could default ivy-completing-read-default-is-empty-string to nil and then implement a whitelist of commands that do expect "" as the implicit default and only set ivy-completing-read-default-is-empty-string to non-nil for those commands. You can steal the current list from ido-ubiquitous 3.x. Just pick out all the commands that are marked as enable-old from here. (This is going away in ido-ubiquitous version 4.x in favor of the behavior described above.)

@DarwinAwardWinner
Copy link
Contributor Author

@DarwinAwardWinner DarwinAwardWinner commented Jun 6, 2017

Of course, the other option is to just disable ivy for those commands.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Jun 7, 2017

It's possible to do, see ivy-completing-read-handlers-alist.

This maintains compatibility with completing-read-default, which
effectively treats a nil DEF as the empty string.

Since this may not always be desired, there is also a variable to
allow suppression of this behavior named
ivy-completing-read-default-is-empty-string.
@DarwinAwardWinner DarwinAwardWinner force-pushed the DarwinAwardWinner:nil-default branch to 9f74310 Jul 19, 2017
Now "ivy-completing-read" itself does nothing special when DEF is nil,
but there is a new function
"ivy-completing-read-with-empty-string-def" that treats a nil DEF as
equivalent to the empty string for compatibility with
"completing-read-default". This new function can be used in
"ivy-completing-read-handlers-alist" to override ivy behavior for
specific commands.
Previously, if a function in ivy-completing-read-handlers-alist called
ivy-completing-read, this would result in infinite recursion. However,
now if ivy-completing-read is called recursively without incrementing
"(minibuffer-depth)", it ignores all handlers on the second call,
breaking the recursion.

The result of all this is that functions in
ivy-completing-read-handlers-alist can now safely call
ivy-completing-read.
This includes testing for infinite recursion.
@DarwinAwardWinner
Copy link
Contributor Author

@DarwinAwardWinner DarwinAwardWinner commented Jul 20, 2017

Ok, instead of having a variable to control default handling, I just implemented a new function called ivy-completing-read-with-empty-string-def which implements the compatible behavior, while leaving ivy-completing-read alone. Then I added some entries to ivy-completing-read-handlers-alist using this function, whereupon I realized that having a handler that itself calls ivy-completing-read causes infinite recursion. So I fixed that as well. Lastly, I added a test for ivy-completing-read-handlers-alist.

With these changes, ivy should now work with all of the functions that I know of that expect this special behavior from completing-read (the offending packages are listed in my comment here).

@DarwinAwardWinner
Copy link
Contributor Author

@DarwinAwardWinner DarwinAwardWinner commented Jul 20, 2017

According to the CI tests, Emacs 24.3 doesn't seem to like cl-letf*. I'll investigate this.

In Emacs 24.4, "symbol-function" was changed to return nil when called
on a symbol with no function. Previously, it threw a void-function
error. This commit backports that fix so that cl-letf can work
properly in ivy-test.el. The fix is only installed when running the
tests.
@DarwinAwardWinner
Copy link
Contributor Author

@DarwinAwardWinner DarwinAwardWinner commented Jul 20, 2017

The root cause was a bug in symbol-function that was fixed in 24.4. I backported the fix just for the test, and now everything passes.

@abo-abo abo-abo merged commit 1760860 into abo-abo:master Jul 20, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Jul 20, 2017

Thanks for your work. Very detailed and thorough:)

Specifically, if DEF is nil, it is treated the same as if DEF was
the empty string. This mimics the behavior of
`completing-read-refault'. This function can therefore be used in
place of `icy-completing-read' for commands that rely on this

This comment has been minimized.

@basil-conto

basil-conto Jul 21, 2017
Collaborator

s/icy/ivy/

This comment has been minimized.

@abo-abo

abo-abo Jul 21, 2017
Owner

haha, I'll fix it later

basil-conto added a commit to basil-conto/swiper that referenced this pull request Nov 15, 2018
(ivy-completing-read-handlers-alist): Do not pass empty string as
DEF argument to completing-read in webjump command, as this adds a
bogus empty candidate to the webjump-sites collection.

Re: abo-abo#1049
abo-abo added a commit that referenced this pull request Nov 18, 2018
(ivy-completing-read-handlers-alist): Do not pass empty string as
DEF argument to completing-read in webjump command, as this adds a
bogus empty candidate to the webjump-sites collection.

Re: #1049
Fixes #1802
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants