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 optional getSuggestionDetailsOnSelect to provider api #925

Merged
merged 2 commits into from Nov 27, 2017

Conversation

Projects
None yet
3 participants
@leroix
Contributor

leroix commented Nov 16, 2017

This adds an optional hook to the provider api:

.getSuggestionDetailsOnSelect(suggestion)

This is useful in providers like the language server providers because getting additional details about the suggestion can be expensive.

It takes one of the suggestions previously received via .getSuggestions as its argument and returns a Promise of a suggestion to replace it with. The AutocompleteManager will call this function if it's available on the provider when the user selects or focuses a particular item in the completion list.

I'm on the fence about the name. Internally, we call this action "select", but that sounds like the user is choosing this suggestion. We call choosing the selection "confirming". It could be .getSuggestionDetailsOnSelect.

Here's an example of the UX this enables:
lazy-loaded-autocomplete-content

Another thing I'm not sure about: should the left/right labels go away when the selection moves on?

/cc @atom/maintainers

@damieng we can still potentially do a hook like getSuggestionDetailsOnScroll, but I thought I'd just do this as a first pass because it seemed like the more important one.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 16, 2017

Contributor

Seems like we may as well keep details once we have fetched them but maybe it is a bit weird.

Contributor

nathansobo commented Nov 16, 2017

Seems like we may as well keep details once we have fetched them but maybe it is a bit weird.

@damieng

This comment has been minimized.

Show comment
Hide comment
@damieng

damieng Nov 16, 2017

Contributor

Cool, will try this out tomorrow and see if I can wire it up. Nice job!

Yeah it does look a bit weird with one resolving on it's own but should be less of a problem when we get some kind of visible event where we can pre-resolve just the visible options.

Contributor

damieng commented Nov 16, 2017

Cool, will try this out tomorrow and see if I can wire it up. Nice job!

Yeah it does look a bit weird with one resolving on it's own but should be less of a problem when we get some kind of visible event where we can pre-resolve just the visible options.

@damieng damieng referenced this pull request Nov 16, 2017

Closed

completionItem/resolve #7

@leroix

This comment has been minimized.

Show comment
Hide comment
@leroix

leroix Nov 18, 2017

Contributor

@damieng I'm actually thinking that perhaps we don't call getSuggestionDetailsOnFocus on the first item in the list. I'm worried about how this results in rendering the first item twice on every keystroke. I think we should leave it up to the provider to resolve the details about the first suggestion in what they return in getSuggestions.

Also, I have performance worries about the "visible event" you mentioned because of the repeated renderings. I think probably the provider should just look at the config setting to determine how many suggestions will be visible and go ahead and resolve those suggestions upfront on the getSuggestions call. If that's too many suggestions, perhaps the provider could just resolve the first few upfront, and the user would have to move down the list to see the details of the other suggestions.

Does anyone have any opinions on the .getSuggestionDetailsOnFocus vs .getSuggestionDetailsOnSelect naming?

Contributor

leroix commented Nov 18, 2017

@damieng I'm actually thinking that perhaps we don't call getSuggestionDetailsOnFocus on the first item in the list. I'm worried about how this results in rendering the first item twice on every keystroke. I think we should leave it up to the provider to resolve the details about the first suggestion in what they return in getSuggestions.

Also, I have performance worries about the "visible event" you mentioned because of the repeated renderings. I think probably the provider should just look at the config setting to determine how many suggestions will be visible and go ahead and resolve those suggestions upfront on the getSuggestions call. If that's too many suggestions, perhaps the provider could just resolve the first few upfront, and the user would have to move down the list to see the details of the other suggestions.

Does anyone have any opinions on the .getSuggestionDetailsOnFocus vs .getSuggestionDetailsOnSelect naming?

@damieng

This comment has been minimized.

Show comment
Hide comment
@damieng

damieng Nov 18, 2017

Contributor

Not sure I understand the first keystroke problem.

The problem with doing them all upfront on a single call is we can't return any then until the last visible one is resolved. I quite like the idea of showing the initial list as fast as possible then filling in the details.

I wouldn't worry about calling it multiple times - I'm caching the results and when you ask to resolve something I've already resolved I just return the same instance.

The new name for the method is probably more accurate, +1

Contributor

damieng commented Nov 18, 2017

Not sure I understand the first keystroke problem.

The problem with doing them all upfront on a single call is we can't return any then until the last visible one is resolved. I quite like the idea of showing the initial list as fast as possible then filling in the details.

I wouldn't worry about calling it multiple times - I'm caching the results and when you ask to resolve something I've already resolved I just return the same instance.

The new name for the method is probably more accurate, +1

@leroix

This comment has been minimized.

Show comment
Hide comment
@leroix

leroix Nov 19, 2017

Contributor

@damieng the way this branch is working currently is that we call getSuggestions, render the suggestion list, call getSuggestionDetailsOnFocus (if available) for the first item in the list, and re-render the first item. We've done a lot in autocomplete-plus to try to minimize the scripting/rendering work that happens on every keystroke as much as we possibly can. The reason is that key strokes can come as close as 30ms apart, and if there's still scripting/rendering going on when the next keystroke occurs, it leads to a janky typing experience.

The alternative would be to have the language server provider resolve the first item in the list right away as part of the getSuggestions call, but you're right--we wouldn't be able to show the list until /resolve completes. My intuition is that this will only take around a millisecond. Do you have some experience with how long the language servers usually take to respond to completion and resolve requests? There's also, of course, time spent parsing JSON.

Contributor

leroix commented Nov 19, 2017

@damieng the way this branch is working currently is that we call getSuggestions, render the suggestion list, call getSuggestionDetailsOnFocus (if available) for the first item in the list, and re-render the first item. We've done a lot in autocomplete-plus to try to minimize the scripting/rendering work that happens on every keystroke as much as we possibly can. The reason is that key strokes can come as close as 30ms apart, and if there's still scripting/rendering going on when the next keystroke occurs, it leads to a janky typing experience.

The alternative would be to have the language server provider resolve the first item in the list right away as part of the getSuggestions call, but you're right--we wouldn't be able to show the list until /resolve completes. My intuition is that this will only take around a millisecond. Do you have some experience with how long the language servers usually take to respond to completion and resolve requests? There's also, of course, time spent parsing JSON.

@damieng

This comment has been minimized.

Show comment
Hide comment
@damieng

damieng Nov 19, 2017

Contributor
Contributor

damieng commented Nov 19, 2017

@leroix leroix changed the title from Add optional getSuggestionDetailsOnFocus to provider api to Add optional getSuggestionDetailsOnSelect to provider api Nov 26, 2017

@leroix leroix merged commit 9c855a7 into master Nov 27, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@leroix leroix deleted the suggestion-details branch Nov 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment