[Lldb-commits] [PATCH] D81001: [lldb] Display autosuggestion part in gray if there is one possible suggestion

Mon Jul 27 02:35:33 PDT 2020

labath added a comment.

In D81001#2172658 <https://reviews.llvm.org/D81001#2172658>, @gedatsu217 wrote:

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

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

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81001/new/

https://reviews.llvm.org/D81001