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

Invoke the correct sorting function from ivy-sorting-functions-alist #899

Closed
wants to merge 1 commit into from

Conversation

torstenmarek
Copy link
Contributor

Instead of using the function from ivy-state, ivy--reset-state called (ivy--sort-function t). This is now fixed.

Some of the expressions involved in finding the correct sorting function have been simplified, and the documentation for the :sort parameter of ivy-read has been fixed.

@abo-abo
Copy link
Owner

abo-abo commented Feb 22, 2017

I don't agree with the documentation change. The sort parameter is still either nil or t, it's not a sorting function.

@torstenmarek
Copy link
Contributor Author

torstenmarek commented Feb 22, 2017

Then I seem to misunderstand something. In the original code,

(let ((sort (assoc this-command ivy-sort-functions-alist)))
                  (if sort
                      (ivy--sort-function (car sort))
                    (or (ivy--sort-function t) t)))))))

will only assign t to :sort when ivy-sort-functions-alist does not contain an entry for t or this-command. Not sure if this is intended. Otherwise, it's a function from ivy-sort-functions-alist or nil.

So it seems like the intended behavior of :sort is:

  1. nil, no sorting
  2. t, default sorting (by looking up the caller or collection in ivy--sort-function?)
  3. function, use as comparator for sorting

Note that the call to ivy-read in ivy-completing-read doesn't set :caller, so 3. doesn't work in this case.

Will update the patch once this is cleared up.

@abo-abo
Copy link
Owner

abo-abo commented Feb 22, 2017

Grepping through the sources, the :sort parameter is only t or nil, never a function.
The idea is that :sort is intended for users, not developers. The developer already has the candidates in hand and can sort them and pass them as collection.
The default is :sort nil, for performance reasons. And :sort t is connected to user's setting in ivy-sort-functions-alist.

@abo-abo
Copy link
Owner

abo-abo commented Feb 22, 2017

So the developer that really wants a certain custom sorting function, applies it himself and passes it as collection, and then :sort nil (the default) so that the user doesn't override his sort.

@torstenmarek
Copy link
Contributor Author

:sort is a function when ivy-read is invoked from ivy-completing-read:

Line 1782:

:sort
            (let ((sort (assoc this-command ivy-sort-functions-alist)))
                 (if sort
                      (ivy--sort-function (car sort))
                    (or (ivy--sort-function t) t)))))))

(ivy--sort-function (car sort)) returns the function associated with it. My problem is that I have registered a sorting function for a command, and without this patch, it's not being honored when ivy-completing-read is invoked via completing-read.

@abo-abo
Copy link
Owner

abo-abo commented Feb 22, 2017

Could we make a change in ivy-completing-read that sets :sort t unconditionally and instead passes this-command as :caller? Would that fix your problem?

@torstenmarek
Copy link
Contributor Author

torstenmarek commented Feb 22, 2017 via email

…ivy-completing-read.

When calling ivy-read from ivy-completing-read, always pass in :sort t and
:caller this-command. In ivy--reset-state, look up the sorting function
using caller.
@torstenmarek
Copy link
Contributor Author

Updated, PTAL

@abo-abo abo-abo closed this in 90aaf8b Feb 24, 2017
@abo-abo
Copy link
Owner

abo-abo commented Feb 24, 2017

Thanks, let try it for now, see if there are any regressions.
Please look into getting an Emacs Copyright Assignment for more contributions.

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

2 participants