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

Preselect current file when showing list of files #1363

Merged
merged 2 commits into from Jan 13, 2018

Conversation

@neojski
Copy link
Contributor

commented Dec 9, 2017

Motivation is when you're browsing files in a directory one by one you want an easy way to go to the next one. With this change you only press ctrl-n.
Even without this example seems like a better default compared to ..

This commit also does tiny refactor to remove duplicate code.

counsel.el Outdated
@@ -1695,6 +1695,14 @@ Skip some dotfiles unless `ivy-text' requires them."
(defvar counsel-find-file-speedup-remote t
"Speed up opening remote files by disabling `find-file-hook' for them.")

(defun preselect-file ()

This comment has been minimized.

Copy link
@dieggsy

dieggsy Dec 9, 2017

Contributor

This should probably be called counsel--preselect-file

This comment has been minimized.

Copy link
@neojski

neojski Dec 9, 2017

Author Contributor

Oh, right, fixing.

@dieggsy

This comment has been minimized.

Copy link
Contributor

commented Dec 9, 2017

I think it would be good to make this optional in any case.

@neojski neojski force-pushed the neojski:preselect-current-file branch 2 times, most recently Dec 9, 2017

@neojski

This comment has been minimized.

Copy link
Contributor Author

commented Dec 9, 2017

@dieggsy, I added an option to turn my modification off.

counsel.el Outdated
(let ((f (ffap-guesser)))
(when f (expand-file-name f))))
(when counsel-preselect-current-file
(when buffer-file-name (file-name-nondirectory buffer-file-name)))))

This comment has been minimized.

Copy link
@basil-conto

basil-conto Dec 9, 2017

Collaborator

This last operand of or can be written as

(and counsel-preselect-current-file
     buffer-file-name
     (file-name-nondirectory buffer-file-name))

Either way, please add a docstring!

This comment has been minimized.

Copy link
@neojski

neojski Dec 10, 2017

Author Contributor

Added docstring. I'm not convinced by (and x y z) being more readable but if that's more idiomatic lisp I can change it.

This comment has been minimized.

Copy link
@basil-conto

basil-conto Dec 10, 2017

Collaborator

My opinion may not count for much, but since we're interested in the final expression's value and not control flow, doubly nested when forms are a big no-no. Please use and, which is not only more idiomatic Elisp but both logically and syntactically simpler.

This comment has been minimized.

Copy link
@neojski

neojski Dec 10, 2017

Author Contributor

"big no-no" sounds very serious. I'll change it to and, then. Thanks for adding some motivation as why this is.

This comment has been minimized.

Copy link
@basil-conto

basil-conto Dec 10, 2017

Collaborator

"big no-no" sounds very serious

I didn't intend for it to sound so serious/harmful, sorry about that. My argument can be summarised as "it's not idiomatic and it's more complex."

This comment has been minimized.

Copy link
@neojski

neojski Dec 10, 2017

Author Contributor

No worries :-)
I appreciate your comments and will gladly learn from you.

counsel.el Outdated
@@ -1654,6 +1654,11 @@ Does not list the currently checked out one."
:type 'boolean
:group 'ivy)

(defcustom counsel-preselect-current-file t
"When non-nil, preselects current file in list of candidates."

This comment has been minimized.

Copy link
@basil-conto

basil-conto Dec 9, 2017

Collaborator

Nitpick: "preselect" instead of "preselects", as this defcustom is not the one performing the preselection.

This comment has been minimized.

Copy link
@neojski

neojski Dec 10, 2017

Author Contributor

Fixed below.

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Dec 10, 2017

Thanks, the change looks fine. Just make counsel-preselect-current-file off by default and add some small docstring to the new function.

This change is over 15 lines. Are you willing to get an Emacs Copyright Assignment? See README for more info.

@neojski neojski force-pushed the neojski:preselect-current-file branch Dec 10, 2017

@neojski

This comment has been minimized.

Copy link
Contributor Author

commented Dec 10, 2017

I just sent an email to assign@gnu.org regarding Emacs Copyright Assignement. Do we have to wait until they reply to merge this PR?

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Dec 10, 2017

Do we have to wait until they reply to merge this PR?

Yes, unfortunately. Hopefully, it won't take long. Thanks for the effort.

@neojski neojski force-pushed the neojski:preselect-current-file branch Dec 10, 2017

@neojski

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2018

Ping @abo-abo, I have the Emacs Copyright Assignment now. Can we get this PR merged?

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Jan 9, 2018

Thanks. Could you please resolve the merge conflict and update the PR?

counsel.el Outdated
@@ -1695,6 +1700,17 @@ Skip some dotfiles unless `ivy-text' requires them."
(defvar counsel-find-file-speedup-remote t
"Speed up opening remote files by disabling `find-file-hook' for them.")

(defun counsel--preselect-file ()
"Chooses the file to preselect in find-file like completions depending on customizations"

This comment has been minimized.

Copy link
@basil-conto

basil-conto Jan 9, 2018

Collaborator

How about something like

  "Return candidate to preselect during filename completion.
The preselect behaviour can be customized via user options
`counsel-find-file-at-point' and
`counsel-preselect-current-file', which see."

This comment has been minimized.

Copy link
@basil-conto

basil-conto Jan 9, 2018

Collaborator

My suggestion is basically to try and follow the conventions listed in (elisp) Documentation Tips.

@neojski neojski force-pushed the neojski:preselect-current-file branch to 5d9aedb Jan 13, 2018

@neojski

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2018

@basil-conto, @abo-abo, I updated the PR. Thanks!

@abo-abo abo-abo merged commit 5d9aedb into abo-abo:master Jan 13, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abo-abo

This comment has been minimized.

Copy link
Owner

commented Jan 13, 2018

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.