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

Add counsel-bookmarked-directory #1657

Closed
wants to merge 1 commit into from

Conversation

akirak
Copy link
Contributor

@akirak akirak commented Jul 8, 2018

This command was originally named ivy-bookmarked-directory, but I got a suggestion to add it as an alternative command, so I added this to counsel package. If you don't mind, I would like you to add this command.

counsel.el Outdated
@@ -2063,6 +2063,65 @@ By default `counsel-bookmark' opens a dired buffer for directories."
("r" ,(counsel--apply-bookmark-fn #'counsel-find-file-as-root)
"open as root")))

;;** `counsel-bookmarked-directory'
(defcustom counsel-bookmarked-directory-default-name "%s"
"Default bookmark name created by `counsel-bookmarked-directory-add."
Copy link
Collaborator

@basil-conto basil-conto Jul 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither the variable name nor the docstring make it clear that the value should/can be a format string. I'm not familiar with how it's used yet, but at first glance a name like counsel-bookmarked-directory-format or similar would seem more intuitive.

Copy link
Contributor Author

@akirak akirak Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I simply omitted this custom variable. Thanks.

counsel.el Outdated
(substring (expand-file-name "b" parent) i (1+ i))))

(defconst counsel-bookmarked-directory-separator
(counsel-bookmarked-directory--path-separator))
Copy link
Collaborator

@basil-conto basil-conto Jul 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use the function ivy--dirname-p instead of this. Even on non-POSIX systems, Emacs understands POSIX filenames, so Elisp programs should almost never have to deal with path separators, which is very brittle; see (elisp) File Names.

counsel.el Outdated
(defun counsel-bookmarked-directory--candidates ()
"Get a list of bookmarked directories sorted by file path."
(bookmark-maybe-load-default-file)
(cl-sort (cl-remove-if-not
Copy link
Collaborator

@basil-conto basil-conto Jul 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use the built-in sort instead of cl-sort here.

counsel.el Outdated
(bookmark-maybe-load-default-file)
(cl-sort (cl-remove-if-not
(lambda (filename)
(string-suffix-p counsel-bookmarked-directory-separator filename))
Copy link
Collaborator

@basil-conto basil-conto Jul 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given my previous comment, this lambda can be replaced with #'ivy--dirname-p.

counsel.el Outdated

;;;###autoload
(defun counsel-bookmarked-directory-add (dir name)
"Add a bookmark to DIR with NAME."
Copy link
Collaborator

@basil-conto basil-conto Jul 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstring should mention what values DIR and NAME default to when called interactively.

Copy link
Contributor Author

@akirak akirak Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the function.

counsel.el Outdated
path)
(format counsel-bookmarked-directory-default-name
path)))))
(let* ((record `((filename . ,(abbreviate-file-name (file-name-as-directory (expand-file-name dir))))
Copy link
Collaborator

@basil-conto basil-conto Jul 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use let instead of let* here.

counsel.el Outdated
(read-from-minibuffer (format "Name of the new bookmark to %s: "
path)
(format counsel-bookmarked-directory-default-name
path)))))
Copy link
Collaborator

@basil-conto basil-conto Jul 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are DIR and NAME separate arguments, rather than a single file name argument?

Why is read-from-minibuffer being used instead of read-directory-name?

If counsel-bookmarked-directory-default-name is always going to be formatted based on default-directory, then what's its purpose? Manipulating file names as strings (e.g. by interpolation) is usually a bad idea, so why not provide a user option counsel-bookmarked-directory-default of type 'directory which will act as the second argument to read-directory-name instead?

counsel.el Outdated
current value of `default-directory'."
(interactive)
(if current-prefix-arg
(call-interactively 'counsel-bookmarked-directory-add)
Copy link
Collaborator

@basil-conto basil-conto Jul 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Use #' instead of '.

counsel.el Outdated
(ivy-read "Bookmarked directory: "
(counsel-bookmarked-directory--candidates)
:caller 'counsel-bookmarked-directory
:action 'dired)))
Copy link
Collaborator

@basil-conto basil-conto Jul 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

counsel.el Outdated
:action 'dired)))

(ivy-set-actions 'counsel-bookmarked-directory
'(("j" dired-other-window "dired-other-window")
Copy link
Collaborator

@basil-conto basil-conto Jul 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually Counsel just calls this "other window".

@akirak akirak force-pushed the bookmarked-directory branch from 52856ae to 00fda1d Compare Jul 9, 2018
@akirak
Copy link
Contributor Author

@akirak akirak commented Jul 9, 2018

Thanks for the review. I pushed a new commit that would fix those issues.

counsel.el Outdated
(ivy-read "Bookmarked directory: "
(counsel-bookmarked-directory--candidates)
:caller 'counsel-bookmarked-directory
:action 'dired)))
Copy link
Collaborator

@basil-conto basil-conto Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: use #' instead of '.

counsel.el Outdated
This function prompts for the bookmark name, which defaults to the
abbreviate path of the directory.

When called-interactively, DIR defaults to he current value of
Copy link
Collaborator

@basil-conto basil-conto Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: the instead of he.

counsel.el Outdated
(default-name abbr-path)
(name (read-string (format "Name of the new directory bookmark [%s]: "
default-name)
nil nil default-name))
Copy link
Collaborator

@basil-conto basil-conto Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I understand now that this is just meant to be a bookmark name, rather than a directory, which is why read-directory-name isn't used. Either way, no need for both abbr-path and default-name; pick a single descriptive variable name, e.g. abbr-dir, and use that consistently. In fact, I would reuse the argument dir:

(setq dir (abbreviate-file-name (expand-file-name dir)))
(let ((name (read-string (format "Name of new directory bookmark [%s]: " dir)
                         nil nil dir))
      (record `((filename . ,(file-name-as-directory dir))
                (front-context-string)
                (rear-context-string))))
  (bookmark-maybe-load-default-file)
  (bookmark-store name record nil))

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Jul 9, 2018

By the way, these are significant changes (more than ~15 lines), so you need to assign copyright to the FSF in order for them to be merged. Have you already done so? If not, are you willing to? See https://github.com/abo-abo/swiper/blob/master/CONTRIBUTING.org#copyright-assignment for some more details on this.

I'm also wondering whether the copyright story is at all affected by the fact that this code already exists as a separate package ivy-bookmarked-directory.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Jul 9, 2018

I'm also wondering whether the copyright story is at all affected by the fact that this code already exists as a separate package ivy-bookmarked-directory.

This isn't an issue as long as we have CA on the commits that we merge here. But yes, if someone without a CA commits to upstream, then we couldn't merge it here.

counsel.el Outdated
(bookmark-maybe-load-default-file)
(sort (cl-remove-if-not
#'ivy--dirname-p
(mapcar #'bookmark-get-filename bookmark-alist))
Copy link
Owner

@abo-abo abo-abo Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please wrap with (delq nil).

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Aug 9, 2018

@akirak Are you willing to start or have you already started the copyright assignment process?

@akirak
Copy link
Contributor Author

@akirak akirak commented Aug 10, 2018

I sent a signed PDF two weeks ago, and the registration process has been just completed!

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Aug 13, 2018

@akirak Merged, thanks. I didn't merge the counsel-bookmarked-directory-add for now; I'm not convinced that it belongs in counsel, since it simply calls read-string with no completion. And it's very similar to the built in C-x r m (bookmark-set).

Can you explain why you added it and the advantages over bookmark-set? If it's really needed, open a new PR.

basil-conto added a commit to basil-conto/swiper that referenced this issue Aug 13, 2018
counsel.el (counsel-bookmarked-directory):
Load required feature and declare its definitions.

Re: abo-abo#1657
@akirak
Copy link
Contributor Author

@akirak akirak commented Aug 14, 2018

counsel-bookmarked-directory-add adds a bookmark to the directory rather than to the file. This behaves differently from bookmark-set in many cases, e.g. when the current buffer is a file. Maybe it should be named counsel-add-directory-bookmark, since it is now in counsel package and it needs only counsel- as its naming prefix.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Aug 14, 2018

But how is it different when you're in a dired buffer and call bookmark-set?

@akirak
Copy link
Contributor Author

@akirak akirak commented Aug 14, 2018

It doesn't make much difference in a dired buffer, but it gives an abbreviate absolute path as the default name.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Aug 14, 2018

It doesn't make much difference in a dired buffer

Then I'm not convinced we should have it. If I want to bookmark a directory, I always do it from a dired buffer.

@akirak
Copy link
Contributor Author

@akirak akirak commented Aug 14, 2018

I see. I don't mind if the command won't be added to counsel package. Thank you for your comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants