-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
feat: inline autocompletion #5084
feat: inline autocompletion #5084
Conversation
4be9945
to
d064720
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.
Looks very good, but overall the choice of replacing the existing autocompletion instead of augmenting it, is unexpected for me. Some users may want the behavior of chrome devtools where it both shows the completion inline and a normal popup.
Also i think it would be better to keep this in a separate extension instead of adding to language_tools, because the vast majority of users would not be using this.
And another question, what should happen if the line has an autopaired brace inserted? E.g. if user writes if (
editor inserts closing )
and completer returns a result for the whole if block?
Yes, that's a very good point, I am working on a new revision to reuse the current autocompletion instead of a new one and show the popup with inline text. However inline-text only mode requires slightly different navigation and rules, so I think the two different types of completer makes sense (inline-only, and popup with optional inline text).
Sure, removing it from there. |
Currently when I tested, no autopaired brace was inserted while the autocomplete was active. However the behaviour is not handled explicitly at the moment.
|
Added optional ghost-text preview to popup-based autocompletion (disabled by default) New external inline-autocompletion widget which supports ghost-text only autocompletion
d064720
to
3ef14dd
Compare
Update: enabled optional inline completion, and moved the inline-only autocompletion to a separate extension. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #5084 +/- ##
==========================================
+ Coverage 86.20% 86.60% +0.39%
==========================================
Files 548 555 +7
Lines 41704 42931 +1227
Branches 6564 6697 +133
==========================================
+ Hits 35953 37181 +1228
+ Misses 5751 5750 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 16 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
the last issue seems to be caused by a bug in addToken which should do Line 1665 in e0d4ea9
|
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.
Left some comments. Would it be possible to add this to the kitchen sink so we can play around with it too?
src/autocomplete/inline_test.js
Outdated
document.body.appendChild(el); | ||
var renderer = new VirtualRenderer(el); | ||
var session = new EditSession(""); | ||
editor = new Editor(renderer, session); |
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.
Would it make sense to test with more than one editor? I interpret it as being a specific use case we want to support based on AceInline
's implementation
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.
Yes, this needs to support multiple editors, as Autocomplete uses it, which could use a shared editor instance if the sharedPopups
config parameter is set to true
Added the inline autocompletion to the kitchen sink demo Added kitchen sink demo option to enable inline preview for popup-based autocomplete Inline completion and command bar tooltip are changed to be editor scoped
Co-authored-by: André Oliveira <[email protected]>
I added it to the kitchen sink so it can be tested. I also added an option which overrides the |
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.
Looks good to me. Nice set of changes and the addition to the kitchen sink was very much appreciated :).
There is only one detail/edge case that I'd like to leave a note here of:
- when showing inline autocompletion suggestions that contain multiple suggested lines and they go beyond the bottom of the editor, specifically when using the default autocompletion interaction (e.g.:
⌥
+Space
for MacOS, not⌥
+C
), the blur behaviour of the auto-completion popup that is to stop showing the suggestion makes it so it's not possible to scroll lower in the editor and see all suggested lines.- I don't find this to be a blocker to this PR
* Inline ghost-text autocompletion Added optional ghost-text preview to popup-based autocompletion (disabled by default) New external inline-autocompletion widget which supports ghost-text only autocompletion * Autocomplete bugfixes Added the inline autocompletion to the kitchen sink demo Added kitchen sink demo option to enable inline preview for popup-based autocomplete Inline completion and command bar tooltip are changed to be editor scoped * Update src/ext/inline_autocomplete.js Co-authored-by: André Oliveira <[email protected]> * Fix styling and add cross-editor tests * Fix for popup display positioning for autocompletion * Add popup display unit tests --------- Co-authored-by: André Oliveira <[email protected]>
Issue #, if available: N/A
Description of changes:
Inline autocompletion feature.
Minor fixes to ghost text display.
Snippet manager preview text.
The popup-based autocompletion is augmented with optional inline-code rendering. This feature is turned off by default, so no unexpected ghost text appears for users in the new version. It can be turned on with the
inlineEnabled
boolean flag in theAutocomplete
widget.This autocompletion displays the inline completions on a best effort basis (if the completion's prefix allows it), as the autocomplete settings might not be fully compatible with inline completion (eg: matches based on caption not prefix might cannot be rendered inline).
If the inline completions are enabled, the popup position is dynamically updated whenever it would overlay a multi-line inline completion suggestion.
An other external widget was created which can be used for inline-only completion. Here, all the completions are guaranteed to be compatible with inline rendering, and the commands are set up in a way to promote more horizontal (left-right, rather than up-down) controls for autocompletion. This widget could be used as is, to allow both style of completions, or could be used as a reference implementation for integrators.
This external inline autocompletion widget works similarly to the previous popup-based autocompletion with a few differences:
exactMatch
, and does not try to match oncaption
value)Keybindings:
Alt/Option-C
Alt/Option-[
Alt/Option-]
Tab
orCtrl/Cmd-Right
Esc
The popup and the inline style autocompletions are mutually exclusive, only one is attached at a time (they share the same
editor.completer
property). However, they can seamlessly switch when both of them are configured to different keybindings.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.