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

Make sure foo/ directory goes before foo-bar/ directory #1315

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@neojski
Contributor

neojski commented Nov 23, 2017

I turned #723 into a pull request. Thank you @hmelman.

@manuel-uberti

This comment has been minimized.

Contributor

manuel-uberti commented Nov 24, 2017

If I understood correctly the change, I have one question: could this be made customizable somehow?
At least for those who prefer plain alphabetical sorting.

@basil-conto

This comment has been minimized.

Contributor

basil-conto commented Nov 24, 2017

How about something like this?

diff --git a/ivy.el b/ivy.el
index fd2d130..11ff086 100644
--- a/ivy.el
+++ b/ivy.el
@@ -1345,12 +1345,23 @@ ivy-avy
        (substring-no-properties
         (nth (- (line-number-at-pos candidate) 2) ivy--old-cands))))))
 
+(defcustom ivy-sort-directory-file-names nil
+  "When non-nil, sort directories by their filenames.
+This means any trailing directory separator is ignored, which
+affects lexicographic sort.
+
+This option is used by `ivy-sort-file-function-default'."
+  :group 'ivy
+  :type 'boolean)
+
 (defun ivy-sort-file-function-default (x y)
   "Compare two files X and Y.
 Prioritize directories."
   (if (get-text-property 0 'dirp x)
       (if (get-text-property 0 'dirp y)
-          (string< x y)
+          (if ivy-sort-directory-file-names
+              (string< (directory-file-name x) (directory-file-name y))
+            (string< x y))
         t)
     (if (get-text-property 0 'dirp y)
         nil

The alternative to the user option would be providing a separate sorting function.

FWIW, I think sorting by directory filename (i.e. ignoring trailing directory separators) should be the default.

@manuel-uberti

This comment has been minimized.

Contributor

manuel-uberti commented Nov 24, 2017

Yes, @basil-conto, that looks more reasonable to me. :-)

@abo-abo

This comment has been minimized.

Owner

abo-abo commented Nov 24, 2017

FWIW, I think sorting by directory filename (i.e. ignoring trailing directory separators) should be the default.

Please PR, it will be more clear for me to see what you mean.

Right now, the sorting is in a perfect spot for me. I use this setting for dired:

(setq dired-listing-switches "-laGh1v --group-directories-first")

counsel-find-file returns the results in exactly the same order as dired with the above setting.

@basil-conto

This comment has been minimized.

Contributor

basil-conto commented Nov 24, 2017

counsel-find-file returns the results in exactly the same order as dired with the above setting

In many cases it does, but not in all:

2017-11-24-165235_1600x900_scrot

Steps to reproduce:

  1. make plain
  2. M-: (setq dired-listing-switches "-laGh1v --group-directories-first") RET
  3. M-: (dired (make-temp-file "" t)) RET
  4. + foo RET
  5. + foo+ C-M-j
  6. + foo# RET
  7. g
  8. C-x C-f
    • Expected result: File completion with subdirectories in the order foo, foo#, foo+
    • Actual result: File completion with subdirectories in the order foo#, foo+, foo

This happens because (all-completions "" 'read-file-name-internal) in ivy--sorted-files returns directory file names with a trailing separator /, and this is considered by ivy-sort-file-function-default when sorting lexicographically (note that (< ?# ?+ ?/) is t).

I'm not against making this configurable, but I think the correct default behaviour should be to pass directories through directory-file-name before comparing them with string-lessp in ivy-sort-file-function-default.

In other words, I propose either

diff --git a/ivy.el b/ivy.el
index ca80da4..1d2e696 100644
--- a/ivy.el
+++ b/ivy.el
@@ -1350,7 +1350,7 @@ ivy-sort-file-function-default
 Prioritize directories."
   (if (get-text-property 0 'dirp x)
       (if (get-text-property 0 'dirp y)
-          (string< x y)
+          (string< (directory-file-name x) (directory-file-name y))
         t)
     (if (get-text-property 0 'dirp y)
         nil

or

diff --git a/ivy.el b/ivy.el
index fd2d130..11ff086 100644
--- a/ivy.el
+++ b/ivy.el
@@ -1345,12 +1345,23 @@ ivy-avy
        (substring-no-properties
         (nth (- (line-number-at-pos candidate) 2) ivy--old-cands))))))
 
+(defcustom ivy-sort-directory-file-names t
+  "When non-nil, sort directories by their filenames.
+This means any trailing directory separator is ignored, which
+affects lexicographic sort.
+
+This option is used by `ivy-sort-file-function-default'."
+  :group 'ivy
+  :type 'boolean)
+
 (defun ivy-sort-file-function-default (x y)
   "Compare two files X and Y.
 Prioritize directories."
   (if (get-text-property 0 'dirp x)
       (if (get-text-property 0 'dirp y)
-          (string< x y)
+          (if ivy-sort-directory-file-names
+              (string< (directory-file-name x) (directory-file-name y))
+            (string< x y))
         t)
     (if (get-text-property 0 'dirp y)
         nil

where ivy-sort-directory-file-names defaults to t.

@basil-conto

This comment has been minimized.

Contributor

basil-conto commented Nov 24, 2017

Please PR

If you don't mind, I'd rather keep all relevant discussion in a single PR. My suggestion is the same as that of @neojski, only I'm recommending calling directory-file-name over manipulating substrings. If the consensus is that this sort order should be configurable, as @manuel-uberti requests, then we should either a) introduce a user option to control this, as per my second example; b) provide an alternative to ivy-sort-file-function-default and tell users to put that in their ivy-sort-functions-alist; or c) tell users to write their own sort function and put that in their ivy-sort-functions-alist. I think option (b) is the only one I don't like, because it leads to more code duplication in ivy.el.

@neojski

This comment has been minimized.

Contributor

neojski commented Nov 25, 2017

@basil-conto, thank you for adding more details explaining what this PR is about :-) I changed the code to use directory-file-name.

I didn't make this configurable as I treated this as fixing a bug as ivy doesn't do the same as:

(setq dired-listing-switches "-laGh1v --group-directories-first")

@abo-abo abo-abo closed this in 9901417 Nov 25, 2017

@abo-abo

This comment has been minimized.

Owner

abo-abo commented Nov 25, 2017

Thanks to all for contributing.

@neojski

This comment has been minimized.

Contributor

neojski commented Nov 25, 2017

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment