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

edit/completion: add sorter #642

Closed
wants to merge 1 commit into from
Closed

Conversation

tw4452852
Copy link
Contributor

@tw4452852 tw4452852 commented Mar 16, 2018

Just got inspiration from sort package, user can setup a comparion function (like the Less method in sort),
and its definition is: func isBefore (seed, a, b string) bool, its result determines whether candidate a should be placed before candidate b.

For issue #580 , a simple directory first sorter would be: [seed a b]{ put ?(test -d $a) }, you can also create any sophisticated sorter for sure.

BTW, now custom sort is a little slow, I will try to figure out the potention reason and improve it.

Thanks.

Signed-off-by: Tw <tw19881113@gmail.com>
}

func sortRawCandidates(ev *eval.Evaler, customSorter eval.Callable, seed string, rs []rawCandidate) ([]rawCandidate, error) {
// default sorter
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I prefer to remove this special case, but seems it is extremely fast compared to the following.

@occivink
Copy link

I have concerns over this approach.

The proposed directory directory sorting involves calling test n*log(n) times which is bound to be slow, especially with a lot of candidates. Even if there was a builtin, it's that many stat() calls that are esentially redundant with what the completer is doing.

In addition, the git add sorting would be tricky to implement, since the information of whether the file is tracked or not is lost after the completer.

@SolitudeSF
Copy link
Contributor

SolitudeSF commented Mar 17, 2018

why cant implicit sorting be disabled and sorting routine just would be explicitly implemented in each completer? is there really a need for another layer?

@tw4452852
Copy link
Contributor Author

@occivink Thanks for your concerns. The proposed directory sorting just a example, i think stat() calls is necessary. For git add, sorter's input is just the completer's result.

@tw4452852
Copy link
Contributor Author

@SolitudeSF , For now we just use the default sorter to sort the candidates, anyway we could add a option to disable sorting entirely. What do you think @xiaq ?

And the sorting routine could be set per completer like the matcher.

@xiaq
Copy link
Member

xiaq commented Mar 17, 2018

Thank you, @tw4452852, but I share @occivink's concern.

@SolitudeSF, decoupling the sorter from the completer is useful. For instance, you can customize the ordering of filename completions without touching the filename completer.

The example you have given is an extreme case, but in general, if you can make do with calling an Elvish function O(1) times you should not call it O(n) times. You can usually rely on the pipeline for communication to reduce the number of calls; the matcher API is an example of how this principle is applied. Instead of calling the matcher for each candidate, it calls the matcher once and feeds the text of all candidates using the pipeline. Granted, to implement such a matcher in Elvish, it's likely that you still need to use each to iterate the input and thus end up calling some function O(n) times anyway. But that's an overhead we can optimize out later.

In our earlier discussion in the user group we have outlined two possible approaches to the sorter API:

  • Feed the sorter function all candidates and use its output as the final candidate list. This approach has the downside that it makes the sorter overly powerful and can potentially be misused.

  • Feed the sorter function all candidates (same as above) and expect it to output a numeric score for each candidate, and use that score for sorting. This API is much simpler, but it makes some simple things hard. For instance, how would you sort things simply by lexicographical order? To do this you need a string -> number mapping that preserves order. You can certainly do this by treating the string as a base-256 number (UTF-8 preserves ordering of codepoints), but that's crazy.

I have just thought of a variant of the second approach that can work, though, and it seems to be the best so far. The sorter still outputs a "score" for each candidate, but it doesn't have to be a numeric score. It can be anything, and the candidates are sorted according to the "score". For instance, if you want candidates to be sorted first by whether they are directories and then by the name, you can use this sorter:

{ each [name]{ put [?(test -d $name) $name] } }

There is one problem with this approach: Elvish doesn't support the notion of comparing, nor sorting anything other than strings and numbers. You can implement something specifically for the sorter of course. For instance, instead of relying on the default list type, you can implement a special sort-key type that has a well-defined ordering. Like a numerical score, it is used purely for scoring the candidates. Then the sorter above will look like

{ each [name]{ sort-key ?(test -d $name) $name } }

Another open question is how to distinguish numerical ordering and lexicographical ordering (Elvish doesn't have separate number types; #456), but that is also relatively easy if you have a dedicated sort-key builtin anyway. You can do something like sort-key [number (get-modified-timestamp $name)] $name, for example. (This is not a very good example because now you cannot use lists directly as arguments sort-key, but you get the idea. A good API for sort-key requires more thought.)

All these said, the completion pipeline still lacks something to make the sorter API more useful. In particular, you almost always want a specific sorter to only apply to a certain "type" of candidates. We do have a notion of "completion type" now, which includes command, index, filename, redir, and argument. The sorter API is already useful, for instance, if you can define a custom ordering for the command type. In fact this is what @notramo asked for in #525.

But the argument type is problematic; it should be finer-grained concept. For instance, the completion candidates for git can include flags, branch names, filenames, etc. And we want at least two things:

  • In the arg-completer API, we should make it possible for the git completer to tag each candidate with types like git-branch, git-flag, filename, etc.

  • You should be able to define sorters for different types.

This can be solved by making the "completion type" part of the candidate structure. Right now it is a property of the whole completion.

I hope I have made my vision clear. If you (still) want to tackle it, I recommend that you start with the last alternative of the sorter API I suggested and implement some version of sort-key. You can leave the support for finer-grained completion types to me.

@krader1961
Copy link
Contributor

@xiaq Ping? This is now almost two years old and seems unlikely to be merged. If it is still an interesting idea perhaps it should be converted into an enhancement issue. Should this PR be closed as rejected?

@xiaq
Copy link
Member

xiaq commented Feb 16, 2020

Thank you, @krader1961. I have filed #915 for the feature and am closing this PR.

@xiaq xiaq closed this Feb 16, 2020
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

5 participants