Page MenuHomePhabricator

[lldb] Display autosuggestion part in gray if there is one possible suggestion
ClosedPublic

Authored by gedatsu217 on Jun 2 2020, 6:47 AM.

Details

Summary

I implemented autosuggestion if there is one possible suggestion.
I set the keybinds for every character. When a character is typed, Editline::TypedCharacter is called.
Then, autosuggestion part is displayed in gray, and you can actually input by typing C-k.
Editline::Autosuggest is a function for finding completion, and it is like Editline::TabCommand now, but I will add more features to it.

Testing does not work well in my environment, so I can't confirm that it goes well, sorry. I am dealing with it now.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Remove extra parts in Editline::TabCommand in Editline.cpp.

Add a test for the autosuggestion.

This now looks good to me. I'll give the others a chance to take a look too but beside that I think this is ready to go IMHO.

The test looks wrong to me. If the feature is supposed to be off by default, then how can it test anything without first turning the feature on? Either the feature isn't really off, or the test is broken. I would expect it to be the latter, because the test sequence seems incorrect to me. I would expect to see some newlines in the strings being sent, otherwise all of this will just get concatenated into a single command.

Also, this only seems to test the ^F functionality. It does not check that the command is printed in gray _before_ the user hits ^F.

lldb/source/Core/IOHandler.cpp
204

Why does this switch Optional<string> result to a by-ref string argument. We have both code which uses an empty string to signify failure, and code which does that with None. Both have their advantages and disadvantages, and I don't have any strong objections to either one, but I certainly don't see an advantage in using both in the same patch.

lldb/source/Host/common/Editline.cpp
1226

These don't have anything to do with indenting. Looks like a leftover from the copy paste from line 1255

teemperor requested changes to this revision.Fri, Jul 17, 9:24 AM

The test looks wrong to me. If the feature is supposed to be off by default, then how can it test anything without first turning the feature on? Either the feature isn't really off, or the test is broken. I would expect it to be the latter, because the test sequence seems incorrect to me. I would expect to see some newlines in the strings being sent, otherwise all of this will just get concatenated into a single command.
Also, this only seems to test the ^F functionality. It does not check that the command is printed in gray _before_ the user hits ^F.

Yeah, and the test obviously doesn't pass.

Alright, let's try to fix this.

@gedatsu217 Not sure if you uploaded the wrong diff, but that test doesn't pass. It would be a nice test if if worked though. But as we anyway need another iteration, let's also extend the test while we're at it.

First, you need to get the test to actually enable autosuggestions, so you need to launch via self.launch(extra_args=["-o", "settings set show-autosuggestion true"]) (the extra_args are just passed to LLDB's command line).

Also as Pavel said, we should also test that we display the autosuggestion in faint colour. Not sure if there is a good way to test that the text is directly behind the cursor, but we can test that it's there. And I guess we should test that we actually pick the most recent command for the autosuggestion (this is currently wrong and we pick the least recent command).

I extend your test a bit to something like this (which is currently failing as it always completes help frame but not the help apropos).

self.launch(extra_args=["-o", "settings set show-autosuggestion true"]) 
# Common input codes and escape sequences.                                                                        
ctrl_f = "\x06"                                                         
faint_color = "\x1b[2m"                                                 
reset = "\x1b[0m"                                                       
                                                                        
frame_output_needle = "Syntax: frame <subcommand>"                      
# Run 'help frame' once to put it into the command history.             
self.expect("help frame", substrs=[frame_output_needle])                
                                                                        
# Check that LLDB shows the autosuggestion in gray behind the text.     
self.child.send("hel")                                                  
self.child.expect_exact(faint_color + "p frame" + reset)                
                                                                        
# Apply the autosuggestion and press enter. This should print the       
# the 'help frame' output if everything went correctly.                 
self.child.send(ctrl_f + "\n")                                          
self.child.expect_exact(frame_output_needle)                            
                                                                        
# Try autosuggestion using tab and ^f.                                  
# \t makes "help" and ^f makes "help frame". If everything went         
# correct we should see the 'help frame' output again.                  
self.child.send("hel\t" + ctrl_f + "\n")                                
self.child.expect_exact(frame_output_needle)                            
                                                                        
# Try another command.                                                  
apropos_output_needle = "Syntax: apropos <search-word>"                 
# Run 'help frame' once to put it into the command history.             
self.expect("help apropos", substrs=[apropos_output_needle])            
                                                                        
# 'hel' should have an autosuggestion for 'help apropos' now.           
self.child.send("hel")                                                  
# FIXME: This is showing 'frame' instead.                               
self.child.expect_exact(faint_color + "p apropos" + reset)                
                                                                        
# Run the command and expect the 'help apropos' output.                 
self.child.send(ctrl_f + "\n")                                          
self.child.expect_exact(apropos_output_needle)

A few more test cases that come to my mind and you should add:

  • help help frame should not have an autosuggestion to help frame. You can just try to get the autosuggestion for help help frame and check for the error for an invalid command.
  • Pressing Ctrl+F in an empty prompt should do nothing (you can just check for the (lldb) prompt I would say).
  • Pressing Ctrl+F directly after Ctrl+F again should do nothing.
  • Having help frame and help frame var in the command history, help fr should still complete to the most recent one.
  • Entering a1234, then 5 backspaces, then hel and then Ctrl+F should bring up the autosuggestion for help frame.
  • Entering help x, then 1 backspace, then Ctrl+F should bring up the autosuggestion for help frame.
This revision now requires changes to proceed.Fri, Jul 17, 9:24 AM
gedatsu217 marked an inline comment as done.Fri, Jul 17, 1:48 PM

help help frame should not have an autosuggestion to help frame. You can just try to get the autosuggestion for help help frame and check for the error for an invalid command.

Sorry, I do not know what this means. Now, if I execute "help help frame" once, it is suggested. Is this wrong?

lldb/source/Core/IOHandler.cpp
204

Sorry, I do not what you mean. Would you explain it more in detail?

(If None is returned, it is not assigned to "result". So, only is string assigned to "result". Does this answer your question?)

Not sure if there is a good way to test that the text is directly behind the cursor, but we can test that it's there.

I guess that would be done by expecting the appropriate cursor movement command.

self.child.expect_exact(faint_color + "p frame" + reset)

+ "^[[move-cursor-left"+strlen("p frame") (I didn't look up the exact encoding of that)

lldb/source/Core/IOHandler.cpp
204

Ok, let me try to phrase that differently:

  • if GetAutoSuggestionForCommand fails, it returns None
  • if IOHandlerDelegate::IOHandlerSuggestion fails, it sets a by-ref string argument to "" (or rather, leaves it as empty).

The two behaviors are different, and it's not clear to me what justifies that difference. It would be much cleaner and clearer if both were using the same convention. Changing conventions part way like this is confusing and forces one to add conversion code. If IOHandlerSuggestion was using the same convention, then it's implementation would just be return io_handler.GetDebugger().GetCommandInterpreter().GetAutoSuggestionForCommand(line)).

gedatsu217 marked an inline comment as done.Mon, Jul 20, 9:01 AM
gedatsu217 added inline comments.
lldb/source/Core/IOHandler.cpp
204

I understood it.
Even if I change return of IOHandlerSuggestion from void to llvm::Optional<std::string>, I think I have to convert None to empty string in another function eventually, because the final perpose of these function is to make string for autosuggestion.

Therefore, if I change return of CommandInterperter::GetAutoSuggestionForCommand from llvm::Optional<std::string> to just std::string, I think above problem will be solved. What do you think?

In addition to it, I tried the below code, but it did not go well. ("\x1b[nD" moves the cursor n steps to the left.)

self.child.send("hel")
self.child.expect_exact(faint_color + "p frame" + reset + "\x1b[" + str(len("p frame")) + "D")

In the first place, the below code does not go well.

self.child.send("help frame")
self.child.expect_exact("help frame" + "\x1b[0D")

"\x1b[0D" means that the cursor does not move. So, I suspect "\x1b[nD" does not function in pexpect. What do you think?

In addition to it, I tried the below code, but it did not go well. ("\x1b[nD" moves the cursor n steps to the left.)

self.child.send("hel")
self.child.expect_exact(faint_color + "p frame" + reset + "\x1b[" + str(len("p frame")) + "D")

In the first place, the below code does not go well.

self.child.send("help frame")
self.child.expect_exact("help frame" + "\x1b[0D")

"\x1b[0D" means that the cursor does not move. So, I suspect "\x1b[nD" does not function in pexpect. What do you think?

It definitely "functions", but it's possible that lldb/editline output a slightly different sequence with the same effect. For example it might output "\x1b[mG", which would move the cursor to the absolute column m. Other ways of achieving the same thing are possible, but they are more convoluted so I don't expect we are using them. Bottom line: You'll need to check what's the sequence that actually gets output.

lldb/source/Core/IOHandler.cpp
204

Are you sure about that? If I follow the code correctly this eventually gets called in Editline::ApplyAutosuggestCommand

std::string to_add = "";
m_suggestion_callback(line, to_add, m_suggestion_callback_baton);

if (to_add.empty())
  return CC_REFRESH;

el_insertstr(m_editline, to_add.c_str());
return CC_REDISPLAY;

That could be written as:

if (Optional<string> to_add = m_suggestion_callback(line, to_add, m_suggestion_callback_baton)) {
  el_insertstr(m_editline, to_add->c_str());
  return CC_REDISPLAY;
}
return CC_REFRESH;

which is actually shorter and makes the failure case more explicit.

The call in Editline::TypedCharacter could be modified similarly...

Add the test content.

Change return from void to llvm::Optional<std::string> in several functions to make the code cleaner.

Revise the details.

Some quibbles about the test. Otherwise, this looks good to me.

lldb/test/API/iohandler/autosuggestion/TestAutosuggestion.py
34

[1G means move the cursor to the first column, which is not what we have wanted to test here. My guess is that this 1G is the start of the sequence that libedit uses to redraw the prompt (or the entire command line). If that's the case (and we are serious about testing the cursor position), then I guess you'll need to expect that entire sequence. Or possibly, split this into two expect_exact calls so that the second call just checks for the presence of the final sequence which actually puts the cursor in the right place.

53

This won't check anything because an empty string is "everywhere". I thing the best we can do right now is type some more text after the ^F, and then check that _that_ text is executed properly (i.e., that it didn't get appended to some other command).

I checked what's the sequence that actually gets output, and it was like below.

h\x1b[2melp frame\x1b[0m\x1b[1Ghe\x1b[2mlp frame\x1b[0m\x1b[1Gel\x1b[2mp frame\x1b[0m\x1b[1Gl

Is it not good to check these characters in expect_exact?

I checked what's the sequence that actually gets output, and it was like below.

h\x1b[2melp frame\x1b[0m\x1b[1Ghe\x1b[2mlp frame\x1b[0m\x1b[1Gel\x1b[2mp frame\x1b[0m\x1b[1Gl

Is it not good to check these characters in expect_exact?

The problem with that is that it might make the test too strict and susceptible to changes in libedit behavior (either due to different versions or different timings). There are many ways to achieve the same thing after all.
So, I would stick to the tail of the sequence which is printed after l is "pressed".

That said, are you sure this is the right sequence? \x1b[1Gl seems like it should print the l at column one, which does not sound right...

The sequence I got in this test was l\x1b[2mp frame\x1b[0m\x1b[1G\r\x1b[9Cl, which seems more believable. If I am decoding that correctly, it does:

  • l - l is "pressed" and printed
  • \x1b[2mp frame\x1b[0m - lldb prints "p frame" in faint
  • \x1b[1G - libedit moves cursor to column 1
  • \r - moves cursor to column 1 again (not sure why, probably to work around some terminal bugs)
  • \x1b[9C - moves cursor 9 characters forward (strlen("(lldb) he"))
  • l - redraws l

If that sequence works for you, then this is what I'd suggest putting in the test.

That said, are you sure this is the right sequence? \x1b[1Gl seems like it should print the l at column one, which does not sound right...

I thought it did not add up too, but pexpect probably actually outputs these characters.

The sequence I got in this test was l\x1b[2mp frame\x1b[0m\x1b[1G\r\x1b[9Cl, which seems more believable.

It did not go well... However, "l" + faint_color + "p frame" + reset + "\x1b[1G" + "l" passed the test.
This test ensure that the cursor is behind "l", but I do not understand what role "\x1b[1G" plays...

What do you think about this test?

That said, are you sure this is the right sequence? \x1b[1Gl seems like it should print the l at column one, which does not sound right...

I thought it did not add up too, but pexpect probably actually outputs these characters.

The sequence I got in this test was l\x1b[2mp frame\x1b[0m\x1b[1G\r\x1b[9Cl, which seems more believable.

It did not go well... However, "l" + faint_color + "p frame" + reset + "\x1b[1G" + "l" passed the test.
This test ensure that the cursor is behind "l", but I do not understand what role "\x1b[1G" plays...

What do you think about this test?

I think it won't pass on my machine because that's not the sequence I am getting. :(

I can see how different libedit versions could produce different sequences, but I do not understand how can the one you're seeing be "right".

Are you testing this on a mac? Lemme try what I get there...

Yes, I'm testing it on Mac.

By the way, I checked its output on the error message. When the test failed, the characters, buffer (last 100 chars) : ~~~, are displayed as an error message. I thought it was pexpect's output, possibly it is not that output?

labath added a subscriber: labath.Fri, Jul 24, 7:34 AM

That said, are you sure this is the right sequence? \x1b[1Gl seems like it should print the l at column one, which does not sound right...

I thought it did not add up too, but pexpect probably actually outputs these characters.

The sequence I got in this test was l\x1b[2mp frame\x1b[0m\x1b[1G\r\x1b[9Cl, which seems more believable.

It did not go well... However, "l" + faint_color + "p frame" + reset + "\x1b[1G" + "l" passed the test.
This test ensure that the cursor is behind "l", but I do not understand what role "\x1b[1G" plays...

What do you think about this test?

I think it won't pass on my machine because that's not the sequence I am getting. :(

I can see how different libedit versions could produce different sequences, but I do not understand how can the one you're seeing be "right".

Are you testing this on a mac? Lemme try what I get there...

Ok, so I was able to confirm what you are seeing on a mac. I don't fully understand what is happening but the problem is related to the use-color setting. Our pexpect harness starts lldb with colors disabled, and this seems to be causing lldb (or libedit) to go berserk. Indeed if you start lldb without colors manually (lldb --no-use-color), you will see that this feature (on a mac) misbehaves -- the cursor gets moved to the first column just like we expected based on the ansi sequences. On linux everything seems to be working fine even without colors.

Now the question is what to do about it? Did you actually intend for your feature to work with colors disabled? If yes, then we'll need to figure out why things don't work on a mac. If no, then we'll need to customize our test harness to enable launching lldb with colors for the autosuggestion tests. And then we'll need to find the right sequence to expect...

I do not intend for this feature to work with colors disabled.

I found that pexpect output the below sequence, and this passed the test.

self.child.expect_exact("\x1b[" + str(len("(lldb) he") + 1) + "G" + "l" + "\x1b[2m" + "p frame" + "\x1b[0m\x1b[1G" + "l" + "\x1b[1G\x1b[2m" + "(lldb) " + "\x1b[22m\x1b[" + str(len("(lldb) hel") + 1) + "G")

Probably, "(lldb)" is redisplayed every time a character is typed. On the other hand, the character is placed in the designated position.

However, there are two strange points.

  1. When a character is typed, it is placed in the designated position, but later, it is placed again in column one and overwritten by "(lldb)".
  2. About "\x1b[22m". I think this is equal to "\x1b[0m", but the test failed when I replace "\x1b[22m" with "\x1b[0m".

Do you think this is a valid test?

I do not intend for this feature to work with colors disabled.

Right in that case, we should probably disable the feature if colors are disabled.

I found that pexpect output the below sequence, and this passed the test.

self.child.expect_exact("\x1b[" + str(len("(lldb) he") + 1) + "G" + "l" + "\x1b[2m" + "p frame" + "\x1b[0m\x1b[1G" + "l" + "\x1b[1G\x1b[2m" + "(lldb) " + "\x1b[22m\x1b[" + str(len("(lldb) hel") + 1) + "G")

Probably, "(lldb)" is redisplayed every time a character is typed. On the other hand, the character is placed in the designated position.

However, there are two strange points.

  1. When a character is typed, it is placed in the designated position, but later, it is placed again in column one and overwritten by "(lldb)".

Yes, that is true. This is a problem and the prompt drawing is only covering it up.

That said, now that I understand this code better, I believe this is actually a bug on our part that we can fix. In your TypedCharacter function you call MoveCursor(CursorLocation::BlockEnd, CursorLocation::EditingPrompt); after printing the suggested text. This is the thing which moves the cursor to the beginning of the line, and judging by the editline behavior (printing the character at the start of line), this is not what it expects. It seems like the right solution here would be to move the cursor to be just before the character that was added. CursorLocation::EditingCursor moves it just *after* it, so it seems we may need a new constant (BeforeEditingCursor) to perform the desired move.

Alternatively, it may also be possible to move to CursorLocation::EditingCursor with a combination of return CC_NORM. That seemed to work fine in my experiments except for introducing some artefacts when backspacing. It's possible this is another bug that could also be fixed, but I did not look into it.

  1. About "\x1b[22m". I think this is equal to "\x1b[0m", but the test failed when I replace "\x1b[22m" with "\x1b[0m".

The test checks for string equivalence, not functional equivalence. That string is coming from Editline::GetCharacter (ANSI_UNFAINT). That said, since both of these functions are doing the same thing (restoring normal formatting after displaying some custom stuff) it would make sense for them to do it the same way. 0m seems to be more explicit so maybe we should standardize on that? Could you create a patch to change the definition of ANSI_UNFAINT ? (might be worth taking a quick look at git history if there is no good reason for why it uses the color code that it uses)

Also, given the presence of ANSI_(UN)FAINT, I'm not sure what to make of the usage of FormatAnsiTerminalCodes in this patch. This file is already intimately familiar with ansi escape sequences needed move cursors and stuff, so I'm not sure that going through that function actually buys us anything.

Do you think this is a valid test?

With the above caveats, yes. There's also one additional aspect -- even after the above is addressed, we may still need to allow for some variance in the sequence to allow for different libedit versions -- as our test have shown, these can differ sometime.

Could you create a patch to change the definition of ANSI_UNFAINT ? (might be worth taking a quick look at git history if there is no good reason for why it uses the color code that it uses)

Sure. Should I create another patch? (In short, should I create it separately from the current patch?)

Could you create a patch to change the definition of ANSI_UNFAINT ? (might be worth taking a quick look at git history if there is no good reason for why it uses the color code that it uses)

Sure. Should I create another patch? (In short, should I create it separately from the current patch?)

Yes, please.

Revise Editline::ApplyAutosuggestCommand. (Change the return value.)

Revise IOHandlerEditline::IOHandlerEditline (Disable to use autosuggestion if debugger.GetUseColor() is false.)

Fix the test.

Thanks.

When trying this out I did notice one subtle bug related to backspacing. When backspacing over the first character, the command line does not get redrawn, which means the autosuggested text remains on screen. The "deleted" character also remains, and is printed in normal colour, though this is very hard to see (on my terminal) as it is now right under the cursor. Pressing backspace the second time makes the faint text and both deleted characters disappear. I'll try to illustrate it using crude ascii art. Initially the command line is (F = faint, ^ = cursor):

(lldb) help frame
FFFFFF    FFFFFFF
          ^

After the first backspace, the line becomes:

(lldb) help frame
FFFFFF    FFFFFFF
         ^

After the second one:

(lldb) h
FFFFFF    
        ^

I didn't find this effect too disturbing, so I don't think it should be a blocker for this, but it something to keep an eye on, as it may cause problems down the road.

Anyway, LGTM from me.

lldb/test/API/iohandler/autosuggestion/TestAutosuggestion.py
72

Since you have variables for the faint and reset escape codes, maybe you could have a function for the cursor movement? def cursor_horizontal_abs(n): return "\x1b[" + str(n) + "G" ?

Editline::TypedCharacter is called when delete is pressed. (This probably fix the above bug.)

Revise the test.

Sorry, I found the bug. Please do not check this yet.

For example, I execute "help frame variable" and save it as command history. Then, when I type "hel", "helhelp [me variable]" (gray characters are in []) is displayed, probably because of cursor position or CC_NORM.

I am trying to solve this problem, but I have not come up with a solution. If you know how to solve this problem, would you help me?

For example, I execute "help frame variable" and save it as command history. Then, when I type "hel", "helhelp [me variable]" (gray characters are in []) is displayed, probably because of cursor position or CC_NORM.

I am trying to solve this problem, but I have not come up with a solution. If you know how to solve this problem, would you help me?

This works for me (linux, libedit version 20191211.3.1). I don't have any immediate suggestions, but I'd start with looking at the escape sequences produced by libedit and lldb when this happens. That might give a clue as to who is getting confused and why. In particular it may be useful to compare the sequences when this does happen vs. when it does not happen (like why didn't this happen when you just type h or he? Or why doesn't this happen with a different command, etc.).

Fix the cursor position. (Editline::TypedCharacter)

Sorry, I mistakenly uploaded a different file. This is the correct file.

I'm not hitting the same bug here, but I do hit a different bug. If I have a autosuggestion and then type another character that causes a shorter autosuggestion to display, then the old text of the autosuggestion is not deleted from the terminal.

For example:

> ./build/bin/lldb -o "settings set show-autosuggestion true"
(lldb) settings set show-autosuggestion true
(lldb) help frame var
[...]
(lldb) help frame info
[...]
(lldb) help frame v[aro]
  • is the grey text.

"help frame " correctly shows "help frame var info" which is the last command, but typing the "v" will cause that only "help frame var" is left as a valid autosuggestion. However the "o" from "info" is not clear, but only the "inf" part ovewritten with "var".

Tbh, I'm not sure what's the best fix for this. I don't think we can clear all following characters in the line with some control character (correct me if I'm wrong). We could adding spaces into the autosuggestion string to fill the rest of the line though (this way the old characters in the buffer are getting ovewritten).

Although I thought of how to clear all the following characters, I did not come up with it. So, I will try to fill the space. Should I add how many spaces? If I add too many spaces, sequences will be two lines, but if I add a few spaces, I can't handle some of the cases.

Although I thought of how to clear all the following characters, I did not come up with it. So, I will try to fill the space. Should I add how many spaces? If I add too many spaces, sequences will be two lines, but if I add a few spaces, I can't handle some of the cases.

It should be possible to keep track of how long the previous autosuggestion was and then print the difference between the current autosuggestion length and the previous one in spaces to clear the buffer. So, something like spaces_to_print = line.size() + to_add.size - m_previous_autosuggestion_size (and if that's positive we add that many spaces).

Also could you rebase the diff here when you have time (e.g., regenerate the diff from you changes on top of the latest master commit)? It no longer applies at the moment without having to manually resolve a conflict.

lldb/include/lldb/Host/Editline.h
363

Unrelated removed line.

Also could you rebase the diff here when you have time (e.g., regenerate the diff from you changes on top of the latest master commit)? It no longer applies at the moment without having to manually resolve a conflict.

Does this mean that I should generate diff from the current latest master commit of [https://github.com/llvm/llvm-project]? or from my latest commit (local)? Sorry, I am not used to using git:(

Also could you rebase the diff here when you have time (e.g., regenerate the diff from you changes on top of the latest master commit)? It no longer applies at the moment without having to manually resolve a conflict.

Does this mean that I should generate diff from the current latest master commit of [https://github.com/llvm/llvm-project]? or from my latest commit (local)? Sorry, I am not used to using git:(

No worries! From the latest master commit from GitHub.com is what I mean. Someone changed one of the lines your patch changes between your checkout and master and now there is a conflict when applying thes patch. If you git pull on your master branch and then make a new diff, then everyone can apply this patch again without having to manually resolve this conflict over and over again.

Add spaces when a character is typed.

Added some comments on the new code. Also we should be able to see these spaces in the pexpect output, so I guess we should be able to test this too? To make this special case less fragile to test, you can make a new subtest in the Test case by just defining a new function:

@skipIfAsan
@skipIfEditlineSupportMissing
def test_hidden_autosuggestion(self):
@skipIfAsan
@skipIfEditlineSupportMissing
def test_autosuggestion(self):
    self.launch(extra_args=["-o", "settings set show-autosuggestion true", "-o", "settings set use-color true"])
    self.expect("help frame v")
    self.expect("help frame info")
    [type 'help frame v' and check for the three space after the grey part to cover up the "nfo" text]

So, I *believe* that everything that we want to have in this patch has been addressed? There are quite a few comments in this thread so it's hard to say if there are open issues. I played around with the current patch for a bit and I couldn't find anymore issues, so I think this is in pretty good shape in any case.

Let's see if @labath or @JDevlieghere have any more comments, otherwise I would say this can go in and the rest can be fixed as follow up commits.

lldb/include/lldb/Host/Editline.h
377

This probably should be std::size_t type (a negative previous suggestion size seems weird). Also add a comment that this is the length of the previous autosuggestion + user input in bytes.

lldb/source/Host/common/Editline.cpp
1074

Whoever sees this in the future will really wonder why we print a bunch of spaces here, so I think a comment like "Print spaces to hide any remains of a previous longer autosuggestion".

1510

I don't think the value of m_previous_autosuggestion_size should only grow (which is what this if is doing), as this way we just keep printing a longer and longer space buffer over time. Just printing enough to clear the buffer of the previous completion is enough.

Also you can just do this calculation directly after you calculated int spaces_to_print above. This way this code is closer together which hopefully makes this easier to understand.

@skipIfAsan
@skipIfEditlineSupportMissing
def test_hidden_autosuggestion(self):
@skipIfAsan
@skipIfEditlineSupportMissing
def test_autosuggestion(self):
    self.launch(extra_args=["-o", "settings set show-autosuggestion true", "-o", "settings set use-color true"])
    self.expect("help frame v")
    self.expect("help frame info")
    [type 'help frame v' and check for the three space after the grey part to cover up the "nfo" text]

Sorry, would you tell me about this code in more detail? Does this mean that I should make test_hidden_autosuggestion and test if there are spaces there? What is the difference between test_hidden_autosuggestion and test_autosuggestion?

I don't think the value of m_previous_autosuggestion_size should only grow (which is what this if is doing), as this way we just keep printing a longer and longer space buffer over time. Just printing enough to clear the buffer of the previous completion is enough.

If I keep the number of characters of the only previous command, I think there is a problem. For example, If I type "help frame var" → "help frame info" → "help frame v", the remains are hidden. However, If I type "help frame var" → "help frame info" → "help" → "help frame v", the number of characters of "help frame var" is more than that of "help", so "help frame v[aro]" is displayed. What do you think?

Also you can just do this calculation directly after you calculated int spaces_to_print above. This way this code is closer together which hopefully makes this easier to understand.

For example, if the user executes a command directly after using tab-completion, Editline::TypedCharacter is not called, so spaces_to_print is not calculated. That is because I can calculate this here.

By the way, I found a bug again. The gray characters remain when only one character is completed by tab-completion.
For instance, when I type "b" and press tab-key after I execute "breakpoint", b [eakpoint] is displayed. [eakpoint] should not be displayed. This problem will be probably solved by my previous diff (https://reviews.llvm.org/D81001?id=276468). But this diff changes Editline::TabCommand significantly. Would you tell me a good way if any?

@skipIfAsan
@skipIfEditlineSupportMissing
def test_hidden_autosuggestion(self):
@skipIfAsan
@skipIfEditlineSupportMissing
def test_autosuggestion(self):
    self.launch(extra_args=["-o", "settings set show-autosuggestion true", "-o", "settings set use-color true"])
    self.expect("help frame v")
    self.expect("help frame info")
    [type 'help frame v' and check for the three space after the grey part to cover up the "nfo" text]

Sorry, would you tell me about this code in more detail? Does this mean that I should make test_hidden_autosuggestion and test if there are spaces there? What is the difference between test_hidden_autosuggestion and test_autosuggestion?

Every Test*.py file can have multiple test_ methods that are each their own separate test. First the test suite would run test_autosuggestion and then would run test_hidden_autosuggestion, but each is its own isolated test. My point was that the test with the spaces relies on having exactly two commands in that order in the command history to work, so you probably should make a new subtest for this so that the test doesn't break if someone extends the test and runs another command. So, if you *would* add the test for the spaces to the existing test, then if someone would add a command like help frame va to the history it would break.

I don't think the value of m_previous_autosuggestion_size should only grow (which is what this if is doing), as this way we just keep printing a longer and longer space buffer over time. Just printing enough to clear the buffer of the previous completion is enough.

If I keep the number of characters of the only previous command, I think there is a problem. For example, If I type "help frame var" → "help frame info" → "help frame v", the remains are hidden. However, If I type "help frame var" → "help frame info" → "help" → "help frame v", the number of characters of "help frame var" is more than that of "help", so "help frame v[aro]" is displayed. What do you think?

Not sure if I understand your example correctly, but as soon as you type "help frame ", you should have an autosuggestion of "help frame info" and then typing the "v" should clear the "nfo" part. The "help" autosuggestion should not be involved at all in any logic after you typed "help "?

Also you can just do this calculation directly after you calculated int spaces_to_print above. This way this code is closer together which hopefully makes this easier to understand.

For example, if the user executes a command directly after using tab-completion, Editline::TypedCharacter is not called, so spaces_to_print is not calculated. That is because I can calculate this here.

Can you give me an example input where this breaks? I'm not sure how the tab completion would influence until what column we would need to overwrite the line buffer (that should still be the same).

By the way, I found a bug again. The gray characters remain when only one character is completed by tab-completion.
For instance, when I type "b" and press tab-key after I execute "breakpoint", b [eakpoint] is displayed. [eakpoint] should not be displayed. This problem will be probably solved by my previous diff (https://reviews.llvm.org/D81001?id=276468). But this diff changes Editline::TabCommand significantly. Would you tell me a good way if any?

That probably comes from the fact that "b" is a command and we just insert a space when you tab complete it. And it seems the space we insert into the el_insertstr function doesn't seem to overwrite the existing buffer:

switch (completion.GetMode()) {                                             
case CompletionMode::Normal: {                                              
  std::string to_add = completion.GetCompletion();                          
  to_add = to_add.substr(request.GetCursorArgumentPrefix().size());         
  if (request.GetParsedArg().IsQuoted())                                    
    to_add.push_back(request.GetParsedArg().GetQuoteChar());                
  to_add.push_back(' '); <- here we add the space. Changing this to another character seems to correctly overwrite the buffer.                                                   
  el_insertstr(m_editline, to_add.c_str()); <- This space doesn't seem to overwrite the buffer?                                 
  return CC_REFRESH;                                                        
}

Did you try if your previous code fixes this issue?

I don't think the value of m_previous_autosuggestion_size should only grow (which is what this if is doing), as this way we just keep printing a longer and longer space buffer over time. Just printing enough to clear the buffer of the previous completion is enough.

If I keep the number of characters of the only previous command, I think there is a problem. For example, If I type "help frame var" → "help frame info" → "help frame v", the remains are hidden. However, If I type "help frame var" → "help frame info" → "help" → "help frame v", the number of characters of "help frame var" is more than that of "help", so "help frame v[aro]" is displayed. What do you think?

Not sure if I understand your example correctly, but as soon as you type "help frame ", you should have an autosuggestion of "help frame info" and then typing the "v" should clear the "nfo" part. The "help" autosuggestion should not be involved at all in any logic after you typed "help "?

What I mean is that if I should keep the length of characters of the only previous command, following thing occurs.

  1. execution "help frame var" and m_previous_autosuggestion_size = len("help frame var") = 14
  2. execution "help frame info" and m_previous_autosuggestion_size = len("help frame info") = 15
  3. execution "help" and m_previous_autosuggestion_size = len("help") = 4
  4. Typing "help frame v". Then, spaces_to_print = m_previous_autosuggestion_size - line.size() - to_add.getValue().length() = 4 - 12 - 2 < 0. So, spaces are not added. (In short, "help frame v[aro]" is displayed.)

(Maybe, I do not understand what you say. )

Also you can just do this calculation directly after you calculated int spaces_to_print above. This way this code is closer together which hopefully makes this easier to understand.

For example, if the user executes a command directly after using tab-completion, Editline::TypedCharacter is not called, so spaces_to_print is not calculated. That is because I can calculate this here.

Can you give me an example input where this breaks? I'm not sure how the tab completion would influence until what column we would need to overwrite the line buffer (that should still be the same).

If m_previous_autosuggestion_size is calculated in Editline::TypedCharacter,

  1. Execution "help frame info", m_previous_autosuggestion_size = len("help frame info") = 15
  2. Typing "help frame va" and pressing tab-key. "help frame variable" is executed. m_previous_autosuggestion_size = len("help frame va") = 13. (because m_previous_autosuggestion_size is not calculated in Editline::TabCommand.)
  3. Typing "help frame i". Then, spaces_to_print = m_previous_autosuggestion_size - line.size() - to_add.getValue().length() = 13 - 12 - 3 < 0. So, spaces are not added. "help frame i[nfoable] is displayed.

Did you try if your previous code fixes this issue?

Not yet. I will try it.

@teemperor I understand what you say just now. Indeed, your method is more efficient than mine. I'm fixing the code now.

Add test.

Delete gray character if autosuggestion has the only one character. (e.g. "b") (Editline::TabCommand)

Simplify the code.

So the way the issue with the single space is now solved is by doing CC_REDISPLAY when we're only adding the single space? Isn't that also deleting the whole autosuggestion?

I don't think the value of m_previous_autosuggestion_size should only grow (which is what this if is doing), as this way we just keep printing a longer and longer space buffer over time. Just printing enough to clear the buffer of the previous completion is enough.

If I keep the number of characters of the only previous command, I think there is a problem. For example, If I type "help frame var" → "help frame info" → "help frame v", the remains are hidden. However, If I type "help frame var" → "help frame info" → "help" → "help frame v", the number of characters of "help frame var" is more than that of "help", so "help frame v[aro]" is displayed. What do you think?

Not sure if I understand your example correctly, but as soon as you type "help frame ", you should have an autosuggestion of "help frame info" and then typing the "v" should clear the "nfo" part. The "help" autosuggestion should not be involved at all in any logic after you typed "help "?

What I mean is that if I should keep the length of characters of the only previous command, following thing occurs.

  1. execution "help frame var" and m_previous_autosuggestion_size = len("help frame var") = 14
  2. execution "help frame info" and m_previous_autosuggestion_size = len("help frame info") = 15
  3. execution "help" and m_previous_autosuggestion_size = len("help") = 4
  4. Typing "help frame v". Then, spaces_to_print = m_previous_autosuggestion_size - line.size() - to_add.getValue().length() = 4 - 12 - 2 < 0. So, spaces are not added. (In short, "help frame v[aro]" is displayed.)

(Maybe, I do not understand what you say. )

I think you got it :). My point was that m_previous_autosuggestion_size is updated everytime you type a character from what I can see, so you don't see the 4 from step 3. It's more like this from what I can tell:

  1. same as your step 1.
  2. same as your step 2.
  3. same asyour step 3: execution "help" and m_previous_autosuggestion_size = len("help") = 4

3.5 [new step]: Typing "help frame ". m_previous_autosuggestion_size = len("help frame info") = 15. This is because we update the size in TypedCharacter which gets triggered for every character.

  1. Typing "help frame v". Then, spaces_to_print = m_previous_autosuggestion_size - line.size() - to_add.getValue().length() = 15 - 12 - 2 = 1. (which is exactly the space needed to overwrite the 'o').
lldb/source/Host/common/Editline.cpp
1009

I think this can be shortened to just:

  to_add.push_back(' ');
  el_insertstr(m_editline, to_add.c_str());
  // Clear autosuggestion from line buffer if we only added a space.
  if (to_add == " ")
    return CC_REDISPLAY;
  return CC_REFRESH;
}
1079

The (int) cast isn't necessary.

So the way the issue with the single space is now solved is by doing CC_REDISPLAY when we're only adding the single space? Isn't that also deleting the whole autosuggestion?

Yes. CC_REDISPLAY can delete all the gray characters left.

lldb/source/Host/common/Editline.cpp
1079

I found that lldb crashed. Sorry, I should have checked more when I uploaded this.

I use spaces_to_print as size_t, but this sometimes becomes less than zero. So, I have to use this as int.

So the way the issue with the single space is now solved is by doing CC_REDISPLAY when we're only adding the single space? Isn't that also deleting the whole autosuggestion?

Yes. CC_REDISPLAY can delete all the gray characters left.

So, if I would type "b" and then press tab, the autosuggestion would briefly disappear until I type the next character?

lldb/source/Host/common/Editline.cpp
1079

Yeah I see that spaces_to_print can overflow. How about this code instead that avoids all the casting and overflowing and so on:

size_t new_autosuggestion_size = line.size() + to_add->length();
// If the previous command line + autosuggestion was longer than the
// current autosuggestion, make sure that the autosuggestion is overwriting
// the old output by enough adding trailing spaces.
if (new_autosuggestion_size < m_previous_autosuggestion_size) {
  size_t spaces_to_print = m_previous_autosuggestion_size - new_autosuggestion_size;
  std::string spaces = std::string(spaces_to_print, ' ');
  fputs(spaces.c_str(), m_output_file);
}
m_previous_autosuggestion_size = new_autosuggestion_size;

So, if I would type "b" and then press tab, the autosuggestion would briefly disappear until I type the next character?

Yes. Indeed, it may not be good. I'll think of other ways.

So, if I would type "b" and then press tab, the autosuggestion would briefly disappear until I type the next character?

Yes. Indeed, it may not be good. I'll think of other ways.

This seems minor enough that I wouldn't block the whole patch over it, so this can be addresses in a follow-up patch.

Are there any other problems with the current state of the patch that haven't been resolved? (I went over the comments, but it's hard to say what has and hasn't been fixed until now).

Are there any other problems with the current state of the patch that haven't been resolved? (I went over the comments, but it's hard to say what has and hasn't been fixed until now).

Yes, I have solved all the problems.

gedatsu217 updated this revision to Diff 283590.Thu, Aug 6, 6:09 AM

Simplify the code.

Alright, let's see if the others see any other issues, otherwise I think this is finally good to go.

lldb/source/Host/common/Editline.cpp
1011–1014

Without a comment this code is very mysterious to whoever will read it.

add comments.

revise a test(add self.quit()).

I applied the patch locally and gave this another shot. I'm still missing all the common ways I accept auto-suggestions in fish:

  • C-e
  • Right arrow
  • End

but I'm fine with fixing that in another patch assuming that wouldn't require a huge redesign. BTW your original description says you can accept suggestions with C-k, but it's actually C-f.

My terminal isn't honoring the faint escape code, it would be nice to make this configurable like most of the other color options.

As I mentioned in my earlier comment you can't enable auto-suggestions in the current command interpreter. You have to launch with -o settings set show-autosuggestion true or adding it to your `.lldbinit. I think that's fine but I'd be nice if we could fix that.

All these things can be fixed in follow-up patches. I already have a hard time following the discussion here so I'm fine with landing this and improving things incrementally.

lldb/test/API/iohandler/autosuggestion/TestAutosuggestion.py
106

No newline at end of file

teemperor accepted this revision.Mon, Aug 10, 11:36 AM

Also signing this off as I think this is in a good enough state to land. The remaining issues can all be follow-up patches.

Also you might want to change your editor to remove trailing spaces from lines. The test file has a lot of them (I'll just fix that when merging for this patch).

This revision is now accepted and ready to land.Mon, Aug 10, 11:36 AM

Slightly late to the party, but I want to point out that there _is_ an escape code for clearing a line:

CSI n K 	--	Erase in Line --	Erases part of the line. If n is 0 (or missing), clear from cursor to the end of the line. If n is 1, clear from cursor to beginning of the line. If n is 2, clear entire line. Cursor position does not change.

It sounds like it should be sufficient to slap this to the end of the autosuggested text, before the cursor is moved in any way.

That said I'm not sure what exactly will this do in case the suggested text spans multiple lines (because it's longer than the terminal width). It's possible inserting spaces will handle that better, but it's also possible it will not -- I can imagine that creating a new line in an autosuggestion will hopelessly confuse lldb/editline. In either case, I think it'd be good to verify how we handle commands which span multiple lines.

But that can come as a separate patch.

Herald added a project: Restricted Project. · View Herald TranscriptWed, Aug 12, 4:12 AM