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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
