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

Should completions be sorted by name? #1792

Open
Techatrix opened this issue Feb 24, 2024 · 5 comments
Open

Should completions be sorted by name? #1792

Techatrix opened this issue Feb 24, 2024 · 5 comments
Labels
enhancement New feature or request question Further information is requested

Comments

@Techatrix
Copy link
Member

Completions are sorted by the label since 1805837. Grouping them based on what kind (keyword, function, variable, ...) they are seems reasonable but sorted them alphabetically? You often group/order declarations in a way to make them easier to understand which gets undone by the sorting.

relevant code:

for (completions) |*c| {
const prefix = kindToSortScore(c.kind.?) orelse continue;
c.sortText = try std.fmt.allocPrint(arena, "{s}{s}", .{ prefix, c.label });
if (source_index < source.len and source[source_index] == '(') {
c.insertText = c.label;
c.insertTextFormat = .PlainText;
}
}

@Techatrix Techatrix added enhancement New feature or request question Further information is requested labels Feb 24, 2024
@nektro
Copy link
Contributor

nektro commented Feb 24, 2024

imo they should be by 1) Leven distance from the test text or 2) source order of fields.
on a personal note, ngl I got surprised by this because I thought editors took care of the ordering for you

@SuperAuguste
Copy link
Member

I agree that Levenshtein distance and order of fields could both be interesting metrics to try sorting by.

On editor handling ordering:

Editors do handle sorting for you as the spec says:

[...] the client is responsible for filtering and sorting. [...] However servers can enforce different behavior by setting a filterText / sortText

We override the sort text so we order certain types of completions above others. This gives us more control but means we have to implement more ourselves vs the editor implementing it for us.

@btipling
Copy link

btipling commented Mar 30, 2024

image
I would expect entity here to appear before an import statement for a file I'm not currently using. I'm not sure if this is what is being discussed in this issue, but seemed similar, an ordering issue on a completion. I think it would save me a lot of time in aggregate, as I use entity a lot in my ECS based game and it always appears beneath the import. I'm also assuming this thing in my screenshot is ZLS related, sorry if it's not. I assumed it is because it's zig. I don't really know how vs code extensions work.

@Techatrix
Copy link
Member Author

I would expect entity here to appear before an import statement for a file I'm not currently using.

If the import statement is unused then why not remove it so it doesn't appear in the completions?

I do agree that entity should come first but for a different reason. I assume that in your code entity which is of type u64 can coerce to the second parameter type of has_id which is entity_t. This means that calling ecs.has_id(...,entity,...) is valid while ecs.has_id(...,entities,...) isn't so it becomes clear which one should be prioritized.

This is a far more complex (but better) mechanism than what this issue is about so created a separate issue in #1845.

@btipling
Copy link

Yes entities is a file struct. and the argument to flecs entity_t is coerced from u64 in zig. You are correct that I should remove unused imports, but I often forget to, but I guess them appearing in completion should be a reminder! I have removed it. Thanks for creating the other issue I will follow that one too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants