Remove unused live completion code #695
Conversation
| @@ -132,37 +132,6 @@ window.LiveEditor = Backbone.View.extend({ | |||
| type: this.editorType | |||
| }); | |||
|
|
|||
| var tooltipEngine = this.config.editor.tooltipEngine; | |||
There was a problem hiding this comment.
This is the feature from Gigabyte-Giant that was added when live completion was still enabled, to let people turn it off.
| * Initializes the autosuggest functionality and adds/modifies the | ||
| * completers to be applicable to KA. | ||
| */ | ||
| init: function(editor) { |
There was a problem hiding this comment.
The init() and enableLiveCompletion() code all revolve around the live-completion functionality, and do not relate to the documentation pop-up.
(In the React PR, I turned the rest of this file into a React component)
| if (this.enabled) { | ||
| this.doRequestTooltip(e.data); | ||
| } | ||
| this.doRequestTooltip(e.data); |
There was a problem hiding this comment.
I checked git history and this was the previous state.
|
Personally, I really like auto suggestion, and I was disappointed when KA turned it off. This is something that has been requested for Khan Academy Extension, that was working on reenabling. I have a snippet of code that I run that re-enables auto suggestion in the live editor, and the integration with Processing's functions is very helpful. I can confirm that my code snippet no longer works and that this PR is successful in removing extra code in that regard. |
| // our own completer for local variables. | ||
| for (var i = editor.completers.length - 1; i >= 0; i--) { | ||
| if (editor.completers[i] === langTools.keyWordCompleter) { | ||
| editor.completers.splice(i, 1); |
There was a problem hiding this comment.
Where do we turn off completers? If we aren't, won't completions appear for window, document, etc.
There was a problem hiding this comment.
Good Q. The function below this one used to run this code:
this.editor.setOptions({enableLiveAutocompletion:true});
That would tell ACE to turn on completion. (See https://github.com/ajaxorg/ace/blob/v1.1.4/lib/ace/ext/language_tools.js#L190)
Now that we never call that, the live completion doesn't happen.
|
@matthiassaihttam Thanks for chiming in! It's helpful to hear from the community. It'd be great to bring back autocompletion later. Right now, my priority for this codebase is to upgrade its underlying technologies to the modern stack that KA uses (bower to webpack, Backbone to React, etc). That will put everything in a better place so that when/if we do add features, the new code is better integrated. We wrote the Autosuggest code quickly, so it didn't have enough time to be properly architected and tested. |
|
Thanks for the answer; that makes sense. Unless I'm mistaken, here you remove the only reference to |
|
Nice find! I checked git history, and both search-box and language-tools
were added when autocomplete was added. Search-box is still usable today
(cmd-F brings it up), and it is auto-enabled simply by including it in the
build. Language-tools only enables features via explicit options, none of
which remain in our codebase. (Confirmed in ACE docs:
https://github.com/ajaxorg/ace/wiki/Extensions)
I'll remove it in build-paths here, and then hopefully a git conflict upon
merging into my webpack branch will remind me to remove it in the webpack
config. I'll also make a note in my TODOs.
…On Mon, Nov 12, 2018 at 10:00 PM Matthias ***@***.***> wrote:
Thanks for the answer; that makes sense.
Unless I'm mistaken, here
<8d6348e#diff-df673dadecb4aad9e6c054acafd04aebL14>
you remove the only reference to language_tools, so you should be able to
remove this line
<https://github.com/Khan/live-editor/blob/7fe38925bb97b9e642ca39da62097f6e8c46a38b/build-paths.json#L25>
from the build dependencies. This saves 62KB from the final package size.
As far as I know, Ace only uses language_tools to populate their auto
suggestion, and I don't think KA uses that to populate any tooltips.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#695 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AASIUouqtojDUq14EHMHOi-RamQmKb6Yks5uumARgaJpZM4YYRhC>
.
|
High-level description of change
This was another change related to the webpack PR that can actually go straight into master.
ScratchpadAutosuggest includes two types of functionality:
We disabled #1 in 4e028ca and we still use #2.
This PR removes the code associated with #1. The unused code is fairly entangled in other code, so removing it will make future modernization PRs cleaner. It's also very ace-dependent code, so it'd be likely to change if a Monaco upgrade ever happens, eg.
Have you added tests to cover this new/updated code?
I manually checked the demo to ensure documentation pop up still working. There are also 2 autosuggest related tests that still pass.
Risks involved
Hm. This PR removes the code for feature from Gigabyte-Giant to let users turn off autosuggest via a URL query parameter + keyboard shortcut. That was added back when live completion was still part of autosuggest, so I believe its primary purpose no longer exists. I also cannot get it to do anything on live KA. So it seems safe to remove, and if its not, the devs watching this repo might let us know! :)
Are there any dependencies or blockers for merging this?
No
How can we verify that this change works?
Open the PJS demo or cs/new/pjs, type rect(, see documentation pop-up.