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

ivy.el (ivy-immediate-done): Exit with unchanged initial input #1719

Closed
wants to merge 2 commits into from

Conversation

@nxtr
Copy link
Contributor

commented Aug 16, 2018

This change makes ivy-immediate-done (without any editing) to exit
completing-read with return value in same (abbreviated) form and value
as the initial input.

This fix functions read-file-name and read-directory-name to return the
default-filename with ivy-immediate-done, instead of current
input (expanded).

Fixes #1170

nxtr added 2 commits Aug 15, 2018
ivy.el (ivy-immediate-done): Exit with unchanged initial input
This change makes ivy-immediate-done (without any editing) to exit
completing-read with return value in same (abbreviated) form and value
as the initial input.

This fix functions read-file-name and read-directory-name to return the
default-filename with ivy-immediate-done, instead of current
input (expanded).

Fixes #1170
ivy.el (ivy-immediate-done): Add condition on read-file-name
Added condition on read-file-name, it's all about read-file-name
compat after all.

Also, added comments around the rational for compat

It's assumed that any following changes of ivy--directory will be in
expanded form, so ivy-immediate-done will return dirs in expanded
form, just like before.

If that's not the case, one can consider to only return ivy--directory
unchanged if the user exits the minibuffer unchanged.
@abo-abo

This comment has been minimized.

Copy link
Owner

commented Aug 20, 2018

Thanks, the changes look good. However, they are together over 15 lines. Do you have an Emacs CA?

@nxtr

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2018

Great! Not yet, been waiting 7+ days for the form. I'll get back once signed and posted.

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Aug 21, 2018

Thanks.

@nxtr

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2018

Finally, the form is signed, posted and returned.

;; means that `ivy-read' shall return INITIAL-INPUT.
;; `read-file-name-default' `string-equal' return value
;; with provided INITIAL-INPUT to detect that the user
;; choose the default, `default-filename'.

This comment has been minimized.

Copy link
@basil-conto

basil-conto Nov 19, 2018

Collaborator

Nitpick: if default-filename is referring to the eponymous read-file-name argument, then I suggest formatting it as DEFAULT-FILENAME, as is already done with INITIAL-INPUT.

This comment has been minimized.

Copy link
@nxtr

nxtr Nov 19, 2018

Author Contributor

Not quite. IICR, default-filename refers to the (setq default-filename ... ) value in read-file-name-default. Still, good point and the comment wording should be edited and rephrased.

This comment has been minimized.

Copy link
@basil-conto

basil-conto Nov 19, 2018

Collaborator

Not quite. IICR, default-filename refers to the (setq default-filename ... ) value in read-file-name-default.

Okay, but that local variable still corresponds to its enclosing function's positional argument of the same name, no?

Still, good point and the comment wording should be edited and rephrased.

That you added a comment at all is already very useful, but I am always grateful for efforts in improving documentation, thanks!

This comment has been minimized.

Copy link
@nxtr

nxtr Nov 20, 2018

Author Contributor

Okay, but that local variable still corresponds to its enclosing function's positional argument of the same name, no?

Yup, here the focus was more on the value to be returned, not necessarily the provided value, that's all. I have no problem with an upcase.

This comment has been minimized.

Copy link
@basil-conto

basil-conto Nov 20, 2018

Collaborator

Yup, here the focus was more on the value to be returned, not necessarily the provided value, that's all.

Gotcha. In that case I suggest explicitly referring to the function's return value, rather than to some implementation detail.

;; in cased `ivy--directory' started out as
;; INITIAL-INPUT in abbreviated form.
ivy--directory ;unchanged (unexpanded)
(expand-file-name ivy-text ivy--directory))

This comment has been minimized.

Copy link
@basil-conto

basil-conto Nov 19, 2018

Collaborator

Nitpick: I wonder if a cond would avoid the need for nested conditionals?

(setf (ivy-state-current ivy-last)
      (cond ((or (not ivy--directory)
                 (eq (ivy-state-history ivy-last) 'grep-files-history))
             ivy-text)
            ((and (string= ivy-text "")
                  (eq (ivy-state-collection ivy-last)
                      #'read-file-name-internal))
             ;; For `read-file-name' compat, unchanged initial input means
             ;; that `ivy-read' shall return INITIAL-INPUT.
             ;; `read-file-name-default' `string-equal' return value with
             ;; provided INITIAL-INPUT to detect that the user choose the
             ;; default, DEFAULT-FILENAME.  We must return `ivy--directory'
             ;; in unchanged form in cased `ivy--directory' started out as
             ;; INITIAL-INPUT in abbreviated form.
             ivy--directory)          ; Unchanged (unexpanded)
            (t
             (expand-file-name ivy-text ivy--directory)))))
(insert (ivy-state-current ivy-last))

This comment has been minimized.

Copy link
@nxtr

nxtr Nov 19, 2018

Author Contributor

Yup, nice, was more into adding than rewriting at the time.

This comment has been minimized.

Copy link
@basil-conto

basil-conto Nov 19, 2018

Collaborator

I usually collect minor refactors over time, so I'll just add this to my pile, unless someone beats me to it.

@abo-abo abo-abo closed this in c0e4ff0 Nov 19, 2018

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Nov 19, 2018

Thanks.

basil-conto added a commit to basil-conto/swiper that referenced this pull request Nov 20, 2018
Various minor simplifications
ivy.el (ivy-partial, ivy--reset-state, ivy-completing-read):
Simplify.
(ivy-immediate-done): Flatten conditional logic re: abo-abo#1719.
(ivy-read): Avoid traversing entire candidate list.
(ivy-shrink-after-dispatching, ivy-minibuffer-shrink)
(ivy--get-window):
swiper.el (swiper--avy-candidate):
Simplify usage of (selected-window).
(swiper--ivy): De Morgan.
(swiper--face-matcher): Compute string length a single time.
abo-abo added a commit that referenced this pull request Nov 20, 2018
Various minor simplifications
ivy.el (ivy-partial, ivy--reset-state, ivy-completing-read):
Simplify.
(ivy-immediate-done): Flatten conditional logic re: #1719.
(ivy-read): Avoid traversing entire candidate list.
(ivy-shrink-after-dispatching, ivy-minibuffer-shrink)
(ivy--get-window):
swiper.el (swiper--avy-candidate):
Simplify usage of (selected-window).
(swiper--ivy): De Morgan.
(swiper--face-matcher): Compute string length a single time.
abo-abo added a commit that referenced this pull request Jul 17, 2019
ivy.el (ivy-immediate-done): Improve make-directory
1. Don't modify (ivy-state-def ivy-last).
2. "C-M-j" with no input should return (ivy-state-def ivy-last).
3. Update the test, since `read-file-name-default' will detect that
the same (eq) string was returned and return "" instead of the file
name.
4. Add a test that `ivy-read' actually returned the file name.

Re #1170
Re #1719
Fixes #2139
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.