CLI Client-side Autocomplete #1329

Cleric-K · 2019-03-25T19:37:09Z

Executes silently help, dump, get on CLI open. Parses the output
and builds autocomplete cache. Autcomplete hints are displayed in
popup lists.

Currently autocompletion is supported for:

primary commands
get/set setting names and lookup values
resource names

McGiverGim · 2019-03-25T19:42:14Z

Interesting... How much time takes the execution of this commands to prepare the autocompletion? It does not conflict with the old autocompletion?

Great job!

Cleric-K · 2019-03-25T19:50:14Z

On F3 about 3sec
On F4 less than a second.

Currently this autocomplete overrides the native autocompletion. If for some reason the parsing fails it fallsback to the native.
I was thinking it could be included as option in the upper right gear menu, but the generation is quick enough that I don't think anyone will need to turn it off.

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.

McGiverGim

Very good work!

McGiverGim · 2019-03-25T19:47:00Z

src/js/tabs/cli.js

+ *
+ * @param {jquery} $textarea
+ */
+function AutoComplete($textarea) {


If possible, I think it's better to move the AutoComplete to it's own JS file.

McGiverGim · 2019-03-25T19:50:51Z

src/js/tabs/cli.js

+            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># ');


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?

mikeller · 2019-03-25T20:00:47Z

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 jquery.textcomplete (or rather textcomplete, since the former is deprecated), and then referencing the minified version included in that package, is the preferred way.

Cleric-K · 2019-03-25T20:14:23Z

OK.
So I'll look into the npm stuff. I'm still little confused about the bridging between node.js and browser/js.

I chose jquery.texcomplete (even though is deprecated) because it has a little clearer api (especially since we use jquery). The new textcomplete uses classes but I'll take a look.

I'll add option for disabling in the preferences.

@McGiverGim About moving to a separate file: AutoComplete calls methods in TABS.cli and also globals in cli.js (for example writeToOutput()). Would it be OK if these functions are called across files?

mikeller · 2019-03-25T20:21:38Z

@Cleric-K:

Sticking with jquery.textcomplete is ok if this is a better fit for us - I just wanted to point out that it is listed as deprecated.

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 require. Luckily npm packages often include a self-contained, often minified, 'browser ready' version that can be directly referenced from web pages, and these 'browser ready' versions can be included in the Chrome Web App (and built into the NW.js version) just like they are included in JS called in web pages.

McGiverGim · 2019-03-25T20:33:53Z

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.
The idea is not too let grow too much the JS file, it will grow continuously making it hard to maintain.

Cleric-K · 2019-03-25T20:39:27Z

Got it.
I guess it should go into the workers directory?

McGiverGim · 2019-03-25T20:48:04Z

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.

mikeller · 2019-03-25T21:01:54Z

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.

Cleric-K · 2019-03-25T21:04:14Z

OK. I'll look at it tomorrow.

In fact I started by adding the code directly to TABS.cli but later decided to separate it as a class just to package the autocomplete data into a more monolithic whole (instead of adding bunch of properties to TABS.cli) - now everything is inside the TABS.cli.autoComplete object.
But other than that they are still quite interrelated - autocomplete calls the send and write functions, the read function of cli has to decide whether to send incoming data to autoComplete for cache building or direct to output... so it's hard to imagine one without the other.
If it was written in Java I guess AutoComplete would be a nested class.

Cleric-K · 2019-03-26T14:42:21Z

CliAutoComplete code moved in a separate file
Preference for enabling (on by default)
textcomplete through npm. I'm still using jquery-textcomplete. I tried textcomplete and it works but there's one very annoying issue. The jq version has the nice feature that the popup list can be scrolled not only with UP/DOWN keys but also with PAGEUP/PAGEDOWN. This is missing in the newer version and it's very annoying since we can easily get long lists (with get/set) and trying to move with PAGEUP/PAGEDOWN moves the cursor in the <textarea> instead. We can work around this by conditionally preventDefault() for these keys if the list is open but it's getting ugly. Plus the jquery version looks a little nicer in code. Thus I suggest we stick with the jq version at this point. The Api is practically the same so if something forces us in the future to upgrade only few lines will have to be changed.
Tests fixed

So, do we need to localize the Building ... message in the CLI window?
The FC's cli output is always in English so we may conform with that.

McGiverGim

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!!!

McGiverGim · 2019-03-26T15:43:05Z

locales/en/messages.json

@@ -4154,5 +4154,8 @@
    },
    "flashTab": {
        "message": "Update Firmware"
+    },
+    "cliAutoComplete": {
+        "message": "Client-side CLI AutoComplete"


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.

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.

I'd also think 'advanced auto-complete' is the right thing from the users' perspective - they don't care much where it is generated.

mikeller

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.

mikeller · 2019-03-26T16:50:30Z

package-lock.json

@@ -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=",


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.

Cleric-K · 2019-03-26T20:51:06Z

Preferences text changed to Advanced CLI AutoComplete
package-lock.json fixed
Added beeper and feature command support. I doubt many people configure these through CLI, but anyways, it was just a few lines of code to support them.

Cleric-K · 2019-03-27T14:35:30Z

After hundreds and hundreds tests, yesterday I got Failed building once. I couldn't understand why it happened, neither I could reproduce it.
The strange things was that I got in the console the Failed! message, but there was no Building... message (which should precede it). I couldn't figure a code path that could lead to such a result.

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 Failed message and fallsback to native autocomplete.

bizmar · 2019-03-27T17:56:04Z

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 get brings them all up at once, like get RPM, get dyn, get d_min.. this implementation forces you to select one full name.. I would like auto complete to be invoked by pressing the tab key, or similar.

Cleric-K · 2019-03-27T18:49:00Z

@bizmar I'm glad you're bringing this up.
So far the discussion was focused more on the source code level. I was expecting to comment on the actual UI experience, because I'm far from the thought that it is perfect.

I don't know if @McGiverGim and @mikeller had the chance to actually test it already.

Different parts of commands have different completing behavior.

primary command - For this I have chosen to autocomplete only by prefix, since most commands are single worded and it would be rare someone for example to need execute status but start typing tus. Also, currently the list won't open by itself - only on Tab (I guess that's what you expect from get)
get/set settings names - for these I chose to show suggestions by matching anywhere in the string, since that's how people are used to work already (when getting values like you say - get rpm, etc.).
For this part I chose the autocomplete to popup automatically after the third character. My sole motivation for this is just to make the user aware that such thing exists. Many people have no experience with consoles (Linux for example) and they simply would not know that such behavior can be expected from the Tab key.
set values - this pops up automatically. Especially for the string lookup values I think there's no downside to that. Matching is also anywhere.
resource, feature, beeper - these show suggestions after the first character. Again I don't see any downside to this.

So I perfectly understand your experience with the get command. Technically it does not force you to choose single command - you can hit ESC and it will close the popup, then hitting ENTER will execute the get with incomplete name. Of course this is cumbersome and requires intuition about the ESC key which most users in our GUI age probably don't have.

If you want to try the behavior in the way you need it, open src\js\CliAutoComplete.js, go to lines 239 and 250, and change the last argument of the twosearcher() calls (currently 3) to some large number. This is the min chars that have to be input before the popup is opened.

As a middle solution we can:

make get not open automatically
make set open automatically after third character. This makes more sense, since set requires single name (although it can be executed with partial name and it behaves like get but probably not many use it in this way).

I'll be happy if everyone gives his opinion.

McGiverGim · 2019-03-27T18:52:12Z

I've been out today. Tomorrow I will try to test it and give my opinion.

mikeller · 2019-03-27T20:23:49Z

@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.

mikeller

Tested, looks good!

McGiverGim · 2019-03-28T11:23:28Z

@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 :)

McGiverGim · 2019-04-12T19:49:48Z

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.

McGiverGim · 2019-04-13T06:29:21Z

Tests:

Manual adding an space at the end: fails, set debug_mode = gyro_scaled tested with autocomplete disabled and it did not work. Adding two spaces work again.
RANGEFINDER and ITERM_RELAX with space fail
RPM_FILTER with two spaces, work
RX_FRSKY_SPI work, with and without space

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:

We send the "bad" commands to the FC, but the FC does not respond. It stores them. When a "good" command is sent, then it executes all the stored and responds to all.
F4 does not seem to have this problem, only F7.

Cleric-K · 2019-04-13T08:02:17Z

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.
If it fails here too we can be sure that it is not js fault. It can be effect in windows drivers or at the mcu hal layer.

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.

McGiverGim · 2019-04-13T11:41:53Z

I will try to test it, but I'm out of home. In one or two days I will test with putty.

McGiverGim · 2019-04-13T13:54:53Z

Other user have tested in putty and it works. In Configurator same problem than me. So it does not seem a firmware problem.

Cleric-K · 2019-04-13T14:36:49Z

Hm.. so that leaves it in nw.js/chrome?
Maybe if we try to add chrome.serial.flush() after sending this will confirm ..

McGiverGim · 2019-04-13T18:32:37Z

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.

McGiverGim · 2019-04-14T08:25:23Z

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.

mikeller · 2019-04-14T08:33:02Z

@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.

Cleric-K · 2019-04-14T08:38:12Z

Agree.
Maybe the proper way to do it would be to make events for start and stop building in the CliAutoComplete object and make TABS.cli listen for them and disable/modify placehodler of the <textarea>. This will be slightly more code but code will be more decoupled (rather than CliAutoComplete manipulating the <textarea> directly. What do you think?

Cleric-K · 2019-04-14T09:48:17Z

Pushed a variant with events. Using jQuery .on/.off/.trigger.

mikeller

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.

Cleric-K · 2019-04-14T12:11:26Z

Can you elaborate:
on a empty argument and pressed Tab, what would you like the list to look like?
list + all the features twice - once without and once with -?

mikeller · 2019-04-14T12:35:11Z

No, I think having the features once (to enable them), plus list and - should be enough. Then, after the user entered -, the features (sans list and -) should be listed.

Cleric-K · 2019-04-14T13:56:46Z

There, I think that's what you had in mind.

McGiverGim · 2019-04-15T09:58:23Z

I have added the trim to the general code of CLI here: #1388, if you want you can remove the trim in the AutoComplete.

mikeller · 2019-04-15T10:07:31Z

Looking good now!

Executes silently various commands on CLI open. Parses the output and build autocomplete cache. Autcomplete hints are displayed in popup lists.

Cleric-K · 2019-04-15T11:51:18Z

Done, trim() removed.

Cleric-K force-pushed the autocomplete branch from d99cd5e to 5ff614c Compare March 25, 2019 19:42

McGiverGim reviewed Mar 25, 2019

View reviewed changes

Cleric-K force-pushed the autocomplete branch 3 times, most recently from 98331e1 to f5787fc Compare March 26, 2019 14:22

McGiverGim previously approved these changes Mar 26, 2019

View reviewed changes

mikeller requested changes Mar 26, 2019

View reviewed changes

Cleric-K dismissed McGiverGim’s stale review via 9181bdd March 26, 2019 20:47

Cleric-K force-pushed the autocomplete branch from f5787fc to 9181bdd Compare March 26, 2019 20:47

Cleric-K force-pushed the autocomplete branch from 9181bdd to 5c53f44 Compare March 27, 2019 14:29

mikeller added this to the 10.6.0 milestone Mar 27, 2019

mikeller previously approved these changes Mar 28, 2019

View reviewed changes

Cleric-K force-pushed the autocomplete branch from 1a96539 to 852c924 Compare April 14, 2019 09:47

mikeller previously approved these changes Apr 14, 2019

View reviewed changes

Cleric-K dismissed mikeller’s stale review via 29c7703 April 14, 2019 13:55

Cleric-K force-pushed the autocomplete branch from 852c924 to 29c7703 Compare April 14, 2019 13:55

McGiverGim mentioned this pull request Apr 15, 2019

Workaround for bug in CLI with 29 lenght + space #1388

Merged

mikeller previously approved these changes Apr 15, 2019

View reviewed changes

CLI Client-side Autocomplete

f96fc0e

Executes silently various commands on CLI open. Parses the output and build autocomplete cache. Autcomplete hints are displayed in popup lists.

Cleric-K dismissed mikeller’s stale review via f96fc0e April 15, 2019 11:47

Cleric-K force-pushed the autocomplete branch from 29c7703 to f96fc0e Compare April 15, 2019 11:47

mikeller approved these changes Apr 15, 2019

View reviewed changes

mikeller merged commit a5c942c into betaflight:master Apr 15, 2019

IllusionFpv mentioned this pull request Aug 24, 2019

Motors stop after entering in CLI while running (for check dshot telemetry) #1597

Closed

mikeller added the RN: FEATURE label Sep 15, 2019

CLI Client-side Autocomplete #1329

CLI Client-side Autocomplete #1329

Conversation

Cleric-K commented Mar 25, 2019 • edited Loading

McGiverGim commented Mar 25, 2019

Cleric-K commented Mar 25, 2019

McGiverGim left a comment

Choose a reason for hiding this comment

McGiverGim Mar 25, 2019

Choose a reason for hiding this comment

McGiverGim Mar 25, 2019

Choose a reason for hiding this comment

mikeller commented Mar 25, 2019

Cleric-K commented Mar 25, 2019

mikeller commented Mar 25, 2019

McGiverGim commented Mar 25, 2019

Cleric-K commented Mar 25, 2019

McGiverGim commented Mar 25, 2019

mikeller commented Mar 25, 2019

Cleric-K commented Mar 25, 2019

Cleric-K commented Mar 26, 2019

McGiverGim left a comment

Choose a reason for hiding this comment

McGiverGim Mar 26, 2019

Choose a reason for hiding this comment

Cleric-K Mar 26, 2019

Choose a reason for hiding this comment

mikeller Mar 26, 2019

Choose a reason for hiding this comment

mikeller left a comment

Choose a reason for hiding this comment

mikeller Mar 26, 2019

Choose a reason for hiding this comment

Cleric-K commented Mar 26, 2019

Cleric-K commented Mar 27, 2019

bizmar commented Mar 27, 2019

Cleric-K commented Mar 27, 2019

McGiverGim commented Mar 27, 2019

mikeller commented Mar 27, 2019

mikeller left a comment

Choose a reason for hiding this comment

McGiverGim commented Mar 28, 2019

McGiverGim commented Apr 12, 2019

McGiverGim commented Apr 13, 2019

Cleric-K commented Apr 13, 2019

McGiverGim commented Apr 13, 2019

McGiverGim commented Apr 13, 2019

Cleric-K commented Apr 13, 2019

McGiverGim commented Apr 13, 2019

McGiverGim commented Apr 14, 2019

mikeller commented Apr 14, 2019

Cleric-K commented Apr 14, 2019

Cleric-K commented Apr 14, 2019

mikeller left a comment

Choose a reason for hiding this comment

Cleric-K commented Apr 14, 2019

mikeller commented Apr 14, 2019

Cleric-K commented Apr 14, 2019

McGiverGim commented Apr 15, 2019

mikeller commented Apr 15, 2019

Cleric-K commented Apr 15, 2019

Cleric-K commented Mar 25, 2019 •

edited

Loading