-
Notifications
You must be signed in to change notification settings - Fork 33
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 selectrum--completing-read-handler-alist #156
Add selectrum--completing-read-handler-alist #156
Conversation
1bbd98b
to
5974ea6
Compare
0b320d2
to
022a673
Compare
022a673
to
4b0124a
Compare
550a8e7
to
2772233
Compare
a0d19c5
to
50c1e6f
Compare
de819c0
to
588bb98
Compare
588bb98
to
78c03ce
Compare
Sorry, may not get to this for a while, I just started work etc etc. Don't worry, it'll be in my inbox until then though :) |
(Feel free to make executive decisions if you would like, otherwise I'll get to all of these threads eventually.) |
Okay, I thought I would ask for review because you might have different views how to handle this. I will investigate the described issue a bit more and try to find a good solution. As the delegation is all internal it's easy to change how we do this later on. I wish you the best for your new job! |
3c79d00
to
77c85a2
Compare
77c85a2
to
ea42453
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a reasonable approach!
"Get symbol for completion COLLECTION. | ||
The symbol is used to identify COLLECTION in | ||
`selectrum--completing-read-handler-alist'" | ||
(cond ((and (memq real-this-command |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way we can avoid relying on real-this-command
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know of a better alternative. We could remove that check for real-this-command
entirely, though. I put it there because I wanted to narrow the scope down to the cases I have tested. Removing duplicates shouldn't break anything but removing the directory entries maybe. This is another example of a table which assumes to be used by the default completion paradigm. The elc
and directory candidates show only up when there are no other candidates left (in default completion that means you already have inserted the lib name and then press TAB again to see the elc
and dir entry for that lib in the completion list).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. Would it be possible to condition the entire alist only on the value of this-command
(or real-this-command
)? That would make it easier for the end user to change, as well as simplify logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One problem is that this-command
and real-this-command
can be unreliable (#98). That is also the reason that ivy has chosen to let you pass the :caller
argument for each command. But maybe we could avoid that by tracking commands as proposed in #98. But I think generally dispatching based on completion table would probably be better. It works for all commands which use the same table (regardless of the command which called it). But it usually requires the completion table to be a named function so it's likely we have to fallback to detect the command, too. The case in question is a bit special unfortunately because it's a completion table that gets created by
(apply-partially
'locate-file-completion-table ...
and the arguments passed vary by command.
That would make it easier for the end user to change, as well as simplify logic
I didn't intend the alist to be changed by users and made it a private variable. Would you prefer to have it public? I hope we can fix cases which aren't properly supported now and only use this alist for some corner cases or special handling to improve integration of dynamic completion tables with selectrum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. This entire situation is somewhat distasteful, but I suppose there is no perfect solution. Your approach seems fine.
This addresses #136 and #139 by adding the discussed
selectrum--completing-read-handler-alist
. Later we could also use this variable to solve #114 where you had the idea to have a customizable list of table symbols that are known to be static.As discovered in #136 other commands which locate libs suffer from the same issue (for Emacs version < 28). I refactored the code to reuse the code of
read-library-name
and delegate all those commands to the function which handles shadows in load path correctly. One downside is that currently the different suffixes passed tolocate-file-completion-table
are ignored (simply passing them toselectrum--locate-library-completions
doesn't work) and so the commands don't differentiate bettween.el
,.el.gz
and.elc
. But before you couldn't either so removing the duplicates and showing load path shadows instead is an improvement. Except forload-library
where you might want to differentiate between.el
and.elc
the variants make not much sense to me anyway.The problem with the completion table seems to be that it doesn't return all those candidates at once. With default completion
.elc
and.gz
variants are only shown after you have already completed to the library name you want and invoke completion again.