-
-
Notifications
You must be signed in to change notification settings - Fork 898
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
CLI Client-side Autocomplete #1329
Conversation
Interesting... How much time takes the execution of this commands to prepare the autocompletion? It does not conflict with the old autocompletion? Great job! |
On F3 about 3sec Currently this autocomplete overrides the native autocompletion. If for some reason the parsing fails it fallsback to the native. The get/dump/etc. commands are slow in the CLI because of the DOM itself. Otherwise the serial stream is much quicker. Since I suppress output while generating the cache, the process is as fast as the serial link. |
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.
Very good work!
src/js/tabs/cli.js
Outdated
* | ||
* @param {jquery} $textarea | ||
*/ | ||
function AutoComplete($textarea) { |
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.
If possible, I think it's better to move the AutoComplete to it's own JS file.
src/js/tabs/cli.js
Outdated
cache.settings.sort(); | ||
cache.commands = Object.keys(cache.commands).sort(); | ||
cache.resources = Object.keys(cache.resourcesCount).sort(); | ||
writeToOutput('Done!<br># Press Tab for command suggestions<br># '); |
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.
The texts of the CLI that come from the firmware are in English, so I think maybe is better to let this in English too but I'm not sure. @mikeller what do you think? Is a good idea to translate this?
Looks interesting, good work @Cleric-K! In general, I am not a fan of injecting stuff into console apps - this introduces uneeded dependency, and will (for example) break if configurator is connected to a custom build that acts different to what configurator expets. For this reason I think making it optional and allowing the user to disable it should be done. Also, adding libraries by adding the library to the repository is deprecated, and should be avoided if possible. Adding an npm dependency on |
OK. I chose I'll add option for disabling in the preferences. @McGiverGim About moving to a separate file: |
Sticking with Re node / npm, the thing to keep in mind is that we still have a considerable percentage of users using the Chrome Web App, and we'll need to stay compatible with it at least for now. This means we can't use |
About the calls to methods of CLI, if they are not too many of them, maybe is better to pass them as callbacks or something similar, to isolate the AutoComplete class. But at first look, I don't see if it will be easy or only complicate things. |
Got it. |
Maybe, but take a look first to see if the final code will be better. I'm on mobile so is difficult to me to see the implications. My message was only a suggestion, if it helps to isolate the code, perfect, but if it will only complicate things, maybe is better to maintain it as is. |
Agreed with @McGiverGim re using callbacks. But in general, splitting up the code in files by function is already an improvement, as it forces developers to think about APIs and stems bad practices such as overly large closures. |
OK. I'll look at it tomorrow. In fact I started by adding the code directly to |
98331e1
to
f5787fc
Compare
So, do we need to localize 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.
I haven't tested it yet, but it seems good to me. I will try to test it tomorrow if I get some time.
Good job!!!
locales/en/messages.json
Outdated
@@ -4154,5 +4154,8 @@ | |||
}, | |||
"flashTab": { | |||
"message": "Update Firmware" | |||
}, | |||
"cliAutoComplete": { | |||
"message": "Client-side CLI AutoComplete" |
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.
This is for "normal" users. They don't understand "client-side" or similar. I think is better to put something like "Advanced AutoComplete in CLI" or "Contextual AutoComplete in CLI", that will be better understood.
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.
I agree. I was going to ask exactly about this but forgot.
@mikeller what would you propose?
I'll push another commit in a while. Found few bugs that affect if turning on/off this feature while the CLI is opened.
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.
I'd also think 'advanced auto-complete' is the right thing from the users' perspective - they don't care much where it is generated.
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.
Good work on replacing jquery.textcomplete with the npm version - this way we'll at least know if there's a security risk that means we have to change versions.
See my comment on the package-lock problem.
package-lock.json
Outdated
@@ -91,7 +91,7 @@ | |||
"string-width": { | |||
"version": "2.1.1", | |||
"resolved": "https://registry.npmjs.org/string-width/-/string-width-2.1.1.tgz", | |||
"integrity": "sha512-nOqH59deCq9SRHlxq1Aw85Jnt4w6KvLKqWVik6oA9ZklXLNIOlqg4F2yrT1MVaTjAqvVwdfeZ7w7aCvJD7ugkw==", | |||
"integrity": "sha1-q5Pyeo3BPSjKyBXEYhQ6bZASrp4=", |
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.
Erg, this is an artefact of an old and broken version of npm being used, or at least cached files downloaded by such a version. Update npm to latest, then follow npm/npm#17749 (comment) to fix it, revert the changes to package-lock.json
, and do an npm i
again.
|
After hundreds and hundreds tests, yesterday I got Anyway, I've added a single retry just in case. If the building fails, it tries silently once more and if that fails too, it produces the |
I tried it today, and while I really wanted this for the last few months while testing dev builds... This implementation does not work for me. Most of the time I want to check a few settings at a time, a lot of features are similary named so a |
@bizmar I'm glad you're bringing this up. I don't know if @McGiverGim and @mikeller had the chance to actually test it already. Different parts of commands have different completing behavior.
So I perfectly understand your experience with the If you want to try the behavior in the way you need it, open As a middle solution we can:
I'll be happy if everyone gives his opinion. |
I've been out today. Tomorrow I will try to test it and give my opinion. |
@Cleric-K: No, I have not yet had the chance to test this - kind of busy with upcoming Betaflight 4.0 at the moment. On a related note, I think that this is a great improvement, but I don't think that it is a good idea to try and squeeze this into the upcoming 10.5.0 release that we'll have to do to go out to support Betaflight 4.0 when it is released. There still are some details that need to be worked out, and merging this early after 10.5.0 is out will give us a lot of time to get and incorporate user feedback. |
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.
Tested, looks good!
@mikeller maybe we can include it in the next release of the Configurator (for Betaflight 4.0), but if the user experience is not polished 100%, maybe we can disable it by default (now is enabled). In this way we can let the users test it and get feedback and suggestions. And enable it for Betaflight 4.1 if at that moment we are happy with it. I will test it in some hours and give my own feedback :) |
I will test all of this maybe tomorrow, to see what happens. Now I'm out of the computer. But adding spaces to fit length 30 does not produce the problem with others elements, I tested it. |
Tests:
So it seems to fail when the command is 29 lenght and one space, 30 in total. @mikeller this strange behaviour. Maybe is a bug in the firmware part of Betaflight? Can you made this test with some F7 (I tested with a MATEKF722SE). What it seems:
|
Thank you! Interesting. Another test that could potentially help a little is opening the serial port in other program, for example PuTTY, activate cli with # and execute the commands. The pc driver can be further tested on Linux. If raw terminal on Linux has the problem too, we can be quite confident that something happens in the mcu. |
I will try to test it, but I'm out of home. In one or two days I will test with putty. |
Other user have tested in putty and it works. In Configurator same problem than me. So it does not seem a firmware problem. |
Hm.. so that leaves it in nw.js/chrome? |
I will test when I can, we can made a trim in the general code of cli.js, after the split, as a workaround if we can't find the origin. But this must go in other PR, because it's clear that is not related with the auto completion, so by the moment for me your code is ok. |
Another suggestion: using F3 is slow. I try to paste something in the text box, but it does not respond (it is loading the autocomplete). Maybe is better to disable the text box with a "Wait please until AutoComplete has finished loading..." or similar message, until the autocomplete has finished. |
@Cleric-K, @McGiverGim: Yes, this issue looks much like other problems we are having on F7 with the USB stack (see betaflight/betaflight#7873). We'll have to look into replacing the library versions with newer (and hopefully better) versions soon-ish. |
Agree. |
Pushed a variant with events. Using jQuery |
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.
This is working great now (save for the problem on F7, but this will need a fix in the firmware.
One little thing: I think it would be nice if, for the commands that enable disable options with <option name>
, -<option name>
(like feature
, beeper
) there was a -
in the autocomplete list.
Can you elaborate: |
No, I think having the features once (to enable them), plus |
There, I think that's what you had in mind. |
I have added the trim to the general code of CLI here: #1388, if you want you can remove the trim in the AutoComplete. |
Looking good now! |
Executes silently various commands on CLI open. Parses the output and build autocomplete cache. Autcomplete hints are displayed in popup lists.
Done, |
Executes silently
help
,dump
,get
on CLI open. Parses the outputand builds autocomplete cache. Autcomplete hints are displayed in
popup lists.
Currently autocompletion is supported for: