-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
Fix various cooked read issues #17556
Conversation
src/host/readDataCooked.cpp
Outdated
@@ -1134,7 +1178,10 @@ | |||
const auto& line = lines.at(i + pagerContentTop); | |||
|
|||
// Skip lines that aren't marked as dirty. | |||
if (line.dirtyBegOffset >= line.text.size()) | |||
// We use dirtyBegColumn instead of dirtyBegOffset to test for dirtiness, because a line that has 1 column | |||
// of space for layout and was asked to fit a wide glyph will have no text, but stil be "dirty". |
Check failure
Code scanning / check-spelling
Unrecognized Spelling
ff17a2a
to
04c04b7
Compare
@@ -445,7 +445,6 @@ size_t TextBuffer::FitTextIntoColumns(const std::wstring_view& chars, til::Coord | |||
while (dist < len) | |||
{ | |||
cwd.GraphemeNext(state, chars); | |||
dist += state.len; |
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.
We shouldn't mark this grapheme as consumed (dist
) if we'll early return in the if
right below.
{ | ||
pagerPromptEnd.x = 0; | ||
pagerPromptEnd.y++; | ||
} |
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.
Modifying pagerPromptEnd
like this throws off the if
condition right after this one.
@@ -960,12 +955,14 @@ void COOKED_READ_DATA::_redisplay() | |||
auto& line = lines.back(); | |||
|
|||
// CSI K may be expensive, so use spaces if we can. | |||
if (remaining <= 8) | |||
if (remaining <= 16) |
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 felt like 16 would be more appropriate for word-wise deletions.
if (it != end && column < columnLimit) | ||
{ | ||
output.append(columnLimit - column, L' '); | ||
column = columnLimit; | ||
} |
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.
Removing this stops us from inserting padding spaces manually (that's the job of the terminal). But with the if
condition gone, we need to move the column = columnLimit;
elsewhere of course, because otherwise the caller doesn't know that we line-wrapped.
as narrow glyphs which broke line layout.
padded with whitespace which broke Ctrl+A + Ctrl+C in conhost.
parts of the viewport with leftover text and not clear it away.
its first two characters.
Closes #17554