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

Avoid sorting completion candidates #190

Closed
nemethf opened this issue Dec 21, 2018 · 7 comments
Closed

Avoid sorting completion candidates #190

nemethf opened this issue Dec 21, 2018 · 7 comments

Comments

@nemethf
Copy link
Collaborator

nemethf commented Dec 21, 2018

It seems pyls returns completions in a sensible order: class methods starting with an underscore are at the end. Is it possible to keep the server order in completion-at-point and in company-complete?

A bit indendent of the question, but I tried to look into this, and I think there's a bug in eglot around this line:

eglot/eglot.el

Line 1858 in 85711cc

:display-sort-function

minibuffer-completion-help sets afun like this:

             (afun (or (completion-metadata-get all-md 'annotation-function)
                       (plist-get completion-extra-properties
                                  :annotation-function)
                       completion-annotate-function))

but plist-get part is missing for sort-fun

                (let ((sort-fun (completion-metadata-get
                                 all-md 'display-sort-function)))

so the lambda function of line 1859 set at eglot-completion-at-point seems to never get called.

@joaotavora
Copy link
Owner

Thanks for the report. Either this is a bug in Emacs itself (it should be fetching :display-sort-function too) or eglot should do the sorting somewhere else. I'll look into it, but there's one thing I don't understand. If completions are being returned in a sensible order and Eglot isn't doing anything, why isn't this working?

@nemethf
Copy link
Collaborator Author

nemethf commented Dec 22, 2018

I think Eglot isn't doing anything, but for me it should actively do nothing :) This is the relevant part of minibuffer-completion-help:

          (setq completions
                ;; FIXME: This function is for the output of all-completions,
                ;; not completion-all-completions.  Often it's the same, but
                ;; not always.
                (let ((sort-fun (completion-metadata-get
                                 all-md 'display-sort-function)))
                  (if sort-fun
                      (funcall sort-fun completions)
                    (sort completions 'string-lessp))))

So if sort-fun is not set, m-c-h will sort the completions anyway. Eglot sending an identity function (lambda (x) x) would be fine.

Company has different logic for the display order, which might be adjusted without configuring/changing eglot, but I still don't understand that part.

Thank you.

@joaotavora
Copy link
Owner

Zooming out from the code for a little while, what are you seeing here in practice?

  1. when using company
  2. when using completion-at-point

Are any of these two situations correct.

The fix seems to lie in Eglot, but first I need a clear, code-free, description of what is happening in situations 1 and 2.

joaotavora added a commit that referenced this issue Dec 22, 2018
The :display-sort-function property in capf functions (vs the
'display-sort-function metadata property) doesn't appear to anything,
as pointed out by Felicián Németh.

* eglot.el (eglot-completion-at-point): Move location of sort function.
@joaotavora
Copy link
Owner

joaotavora commented Dec 22, 2018

@nemethf please try out the entirely untested e43399b, and report here if it works for you. EDIT: actually, that should be 59745dc

joaotavora added a commit that referenced this issue Dec 22, 2018
The :display-sort-function property in capf functions (vs the
'display-sort-function metadata property) doesn't appear to anything,
as pointed out by Felicián Németh.

* eglot.el (eglot-completion-at-point): Move location of sort function.
@fbergroth
Copy link
Contributor

I think label should be used as fallback (instead of the empty string).

	/**
	 * A string that should be used when comparing this item
	 * with other items. When `falsy` the label is used.
	 */
	sortText?: string;

@nemethf
Copy link
Collaborator Author

nemethf commented Dec 23, 2018

Obviously, it was a mistake from me to talk about two separate things at the very beginning.


  1. Sensible ordering in python. Here is tiny demonstration what I see and what I want to see. Let's have a python file containing just one line without a newline character at the end:
"hello".

If I open this file and press M-> C-M-i, I see the following in Completions.

Click on a completion to select it.
In this buffer, type RET to select the completion near point.

Possible completions are:
__add__ str 	__class__ str
__contains__ str 	__delattr__ str
__doc__ str 	__eq__ str
__format__ str 	__ge__ str
__getattribute__ str 	__getitem__ str
__getnewargs__ str 	__getslice__ str
__gt__ str 	__hash__ str
__init__ str 	__le__ str
__len__ str 	__lt__ str
__mod__ str 	__mul__ str
__ne__ str 	__new__ str
__reduce__ str 	__reduce_ex__ str
__repr__ str 	__rmod__ str
__rmul__ str 	__setattr__ str
__sizeof__ str 	__str__ str
__subclasshook__ str 	_formatter_field_name_split str
_formatter_parser str 	capitalize str
center str 	count str
decode str 	encode str
endswith str 	expandtabs str
find str 	format str
index str 	isalnum str
isalpha str 	isdigit str
islower str 	isspace str
istitle str 	isupper str
join str 	ljust str
lower str 	lstrip str
partition str 	replace str
rfind str 	rindex str
rjust str 	rpartition str
rsplit str 	rstrip str
split str 	splitlines str
startswith str 	strip str
swapcase str 	title str
translate str 	upper str
zfill str

I get the same order with company (i.e. opening the file and press M-> DEL .)
The order I'd like to see is something like this: capitalize, center, count, decode, encode, endswith, expandtabs, find, format, index, isalnum, isalpha, isdigit, islower, isspace, istitle, isupper, join, ljust, lower, lstrip, partition, replace, rfind, rindex, rjust, rpartition, rsplit, rstrip, split, splitlines, startswith, strip, swapcase, title, translate, upper, zfill, add, class, contains, delattr, doc, eq, format, ge, getattribute, getitem, getnewargs, getslice, gt, hash, init, le, len, lt, mod, mul, ne, new, reduce, reduce_ex, repr, rmod, rmul, setattr, sizeof, str, subclasshook, _formatter_field_name_split, _formatter_parser

If my analysis is correct, pyls returns the available completions in my desired order.


  1. completion-at-point doesn't use eglot's sort function. This might by fixed with 59745dc, but it's hard to see which function does the sorting just by looking at the output. ... ... Now I've modified eglot to sort in reverse order. completion-at-point does not show the results in reverse order, so this does not seem to be fixed.

This is what I used for my test:

diff --git a/eglot.el b/eglot.el
index e5a45c1..44fa3f1 100644
--- a/eglot.el
+++ b/eglot.el
@@ -1837,7 +1837,7 @@ is not active."
                   (put-text-property 0 1 'eglot--lsp-completion all completion)
                   completion))
               (sort items
-                    (lambda (a b)
+                    (lambda (b a)
                       (string-lessp
                        (or (plist-get a :sortText) "")
                        (or (plist-get b :sortText) "")))))))))

Sorry for the confusion and thanks again

joaotavora added a commit that referenced this issue Dec 23, 2018
The :display-sort-function property in capf functions (vs the
'display-sort-function metadata property) doesn't appear to anything,
as pointed out by Felicián Németh.

* eglot.el (eglot-completion-at-point): Move location of sort function.
@joaotavora
Copy link
Owner

So I think I fixed this. A couple of points.

  1. There seems to be something clearly sub-optimal in Emacs itself. It's not easy at all for users of completion-at-point-functions to tell users of this variable which sort function to use.
  2. If the completion function does nothing, then both completion-at-point and company will sort lexicographically, thus potentially destroying any order from the LSP server.

I will probably e opening a bug report about 1. I think that either :completion-sort-function should be allowed as a property, or completion-table-dynamic should/could allow metadata to be specified.

There has to be a very thorough understanding of Emacs's completion mechanism before tackling things like #181. It will be even more complicated for LSP servers returning incomplete completion results.

bhankas pushed a commit to bhankas/emacs that referenced this issue Sep 18, 2022
* eglot.el (eglot-completion-at-point): Complicate severely.
bhankas pushed a commit to bhankas/emacs that referenced this issue Sep 19, 2022
* eglot.el (eglot-completion-at-point): Complicate severely.
bhankas pushed a commit to bhankas/emacs that referenced this issue Sep 19, 2022
* eglot.el (eglot-completion-at-point): Complicate severely.

#190: joaotavora/eglot#190
jollaitbot pushed a commit to sailfishos-mirror/emacs that referenced this issue Oct 12, 2022
* eglot.el (eglot-completion-at-point): Complicate severely.

GitHub-reference: fix joaotavora/eglot#190
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

No branches or pull requests

3 participants