[lldb] Correctly restore the cursor column after resizing the statusline #145823
Conversation
|
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesThis PR ensures we correctly restore the cursor column after resizing the statusline. To ensure we have space for the statusline, we have to emit a newline to move up everything on screen. The newline causes the cursor to move to the start of the next line, which needs to be undone. Normally, we would use escape codes to save & restore the cursor position, but that doesn't work here, as the cursor position may have (purposely) changed. Instead, we move the cursor up one line using an escape code, but we weren't restoring the column. Interestingly, Editline was able to recover from this issue through the LineInfo struct which contains the buffer and the cursor location, which allows us to compute the column. This PR addresses the bug by relying on the active IOHandler to report its cursor position and if available, use that to restore the cursor column position. Fixes #134064 Full diff: https://github.com/llvm/llvm-project/pull/145823.diff 7 Files Affected:
diff --git a/lldb/include/lldb/Core/Debugger.h b/lldb/include/lldb/Core/Debugger.h
index 2087ef2a11562..0052b042df789 100644
--- a/lldb/include/lldb/Core/Debugger.h
+++ b/lldb/include/lldb/Core/Debugger.h
@@ -133,6 +133,8 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
void SetAsyncExecution(bool async);
+ std::optional<IOHandler::CursorPosition> GetIOHandlerCursorPosition();
+
lldb::FileSP GetInputFileSP() { return m_input_file_sp; }
File &GetInputFile() { return *m_input_file_sp; }
diff --git a/lldb/include/lldb/Core/IOHandler.h b/lldb/include/lldb/Core/IOHandler.h
index 2fb3d7a7c9cc3..d8f5c32106986 100644
--- a/lldb/include/lldb/Core/IOHandler.h
+++ b/lldb/include/lldb/Core/IOHandler.h
@@ -113,6 +113,11 @@ class IOHandler {
virtual const char *GetHelpPrologue() { return nullptr; }
+ using CursorPosition = std::pair<size_t, size_t>;
+ virtual std::optional<CursorPosition> GetCursorPosition() const {
+ return std::nullopt;
+ }
+
int GetInputFD();
int GetOutputFD();
@@ -404,6 +409,8 @@ class IOHandlerEditline : public IOHandler {
void PrintAsync(const char *s, size_t len, bool is_stdout) override;
+ virtual std::optional<CursorPosition> GetCursorPosition() const override;
+
private:
#if LLDB_ENABLE_LIBEDIT
bool IsInputCompleteCallback(Editline *editline, StringList &lines);
diff --git a/lldb/include/lldb/Host/Editline.h b/lldb/include/lldb/Host/Editline.h
index c202a76758e13..388f30c9db15a 100644
--- a/lldb/include/lldb/Host/Editline.h
+++ b/lldb/include/lldb/Host/Editline.h
@@ -267,6 +267,9 @@ class Editline {
size_t GetTerminalHeight() { return m_terminal_height; }
+ using CursorPosition = std::pair<size_t, size_t>;
+ std::optional<CursorPosition> GetCursorPosition();
+
private:
/// Sets the lowest line number for multi-line editing sessions. A value of
/// zero suppresses line number printing in the prompt.
diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index 445baf1f63785..622147b629df8 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger.cpp
@@ -1240,6 +1240,15 @@ void Debugger::DispatchInputEndOfFile() {
reader_sp->GotEOF();
}
+std::optional<IOHandler::CursorPosition>
+Debugger::GetIOHandlerCursorPosition() {
+ std::lock_guard<std::recursive_mutex> guard(m_io_handler_stack.GetMutex());
+ IOHandlerSP reader_sp(m_io_handler_stack.Top());
+ if (reader_sp)
+ return reader_sp->GetCursorPosition();
+ return std::nullopt;
+}
+
void Debugger::ClearIOHandlers() {
// The bottom input reader should be the main debugger input reader. We do
// not want to close that one here.
diff --git a/lldb/source/Core/IOHandler.cpp b/lldb/source/Core/IOHandler.cpp
index 8aac507eaa0c2..d01499649bb3e 100644
--- a/lldb/source/Core/IOHandler.cpp
+++ b/lldb/source/Core/IOHandler.cpp
@@ -634,6 +634,15 @@ void IOHandlerEditline::GotEOF() {
#endif
}
+std::optional<IOHandler::CursorPosition>
+IOHandlerEditline::GetCursorPosition() const {
+#if LLDB_ENABLE_LIBEDIT
+ if (m_editline_up)
+ return m_editline_up->GetCursorPosition();
+#endif
+ return std::nullopt;
+}
+
void IOHandlerEditline::PrintAsync(const char *s, size_t len, bool is_stdout) {
#if LLDB_ENABLE_LIBEDIT
if (m_editline_up) {
diff --git a/lldb/source/Core/Statusline.cpp b/lldb/source/Core/Statusline.cpp
index 8a8640805cac0..6393c71d77ecb 100644
--- a/lldb/source/Core/Statusline.cpp
+++ b/lldb/source/Core/Statusline.cpp
@@ -28,6 +28,7 @@
#define ANSI_TO_START_OF_ROW ESCAPE "[%u;1f"
#define ANSI_REVERSE_VIDEO ESCAPE "[7m"
#define ANSI_UP_ROWS ESCAPE "[%dA"
+#define ANSI_SET_COLUMN_N ESCAPE "[%uG"
using namespace lldb;
using namespace lldb_private;
@@ -102,20 +103,37 @@ void Statusline::UpdateScrollWindow(ScrollWindowMode mode) {
const unsigned scroll_height =
(mode == DisableStatusline) ? m_terminal_height : m_terminal_height - 1;
+ std::optional<IOHandler::CursorPosition> cursor_position =
+ m_debugger.GetIOHandlerCursorPosition();
+
LockedStreamFile locked_stream = stream_sp->Lock();
+
+ if (mode == EnableStatusline) {
+ // Move everything on the screen up to make space for the statusline. This
+ // is going to move the cursor to the start of the next line which we need
+ // to undo.
+ locked_stream << '\n';
+
+ // First move the cursor back up. We can't use ANSI_SAVE/RESTORE_CURSOR
+ // here, because the old and new position differ if everything on the screen
+ // moved up.
+ locked_stream.Printf(ANSI_UP_ROWS, 1);
+
+ // Finally move the cursor back to the correct column, if the IOHandler was
+ // able to tell us where that was.
+ if (cursor_position)
+ locked_stream.Printf(ANSI_SET_COLUMN_N,
+ static_cast<unsigned>(cursor_position->first));
+ }
+
+ // Adjust the scroll window.
locked_stream << ANSI_SAVE_CURSOR;
locked_stream.Printf(ANSI_SET_SCROLL_ROWS, scroll_height);
locked_stream << ANSI_RESTORE_CURSOR;
- switch (mode) {
- case EnableStatusline:
- // Move everything on the screen up.
- locked_stream.Printf(ANSI_UP_ROWS, 1);
- locked_stream << '\n';
- break;
- case DisableStatusline:
+
+ if (mode == DisableStatusline) {
// Clear the screen below to hide the old statusline.
locked_stream << ANSI_CLEAR_BELOW;
- break;
}
}
diff --git a/lldb/source/Host/common/Editline.cpp b/lldb/source/Host/common/Editline.cpp
index b251ded6c3793..b7e7b720443db 100644
--- a/lldb/source/Host/common/Editline.cpp
+++ b/lldb/source/Host/common/Editline.cpp
@@ -122,22 +122,21 @@ static int GetOperation(HistoryOperation op) {
// - The H_FIRST returns the most recent entry in the history.
//
// The naming of the enum entries match the semantic meaning.
- switch(op) {
- case HistoryOperation::Oldest:
- return H_LAST;
- case HistoryOperation::Older:
- return H_NEXT;
- case HistoryOperation::Current:
- return H_CURR;
- case HistoryOperation::Newer:
- return H_PREV;
- case HistoryOperation::Newest:
- return H_FIRST;
+ switch (op) {
+ case HistoryOperation::Oldest:
+ return H_LAST;
+ case HistoryOperation::Older:
+ return H_NEXT;
+ case HistoryOperation::Current:
+ return H_CURR;
+ case HistoryOperation::Newer:
+ return H_PREV;
+ case HistoryOperation::Newest:
+ return H_FIRST;
}
llvm_unreachable("Fully covered switch!");
}
-
EditLineStringType CombineLines(const std::vector<EditLineStringType> &lines) {
EditLineStringStreamType combined_stream;
for (EditLineStringType line : lines) {
@@ -313,8 +312,8 @@ class EditlineHistory {
/// Path to the history file.
std::string m_path;
};
-}
-}
+} // namespace line_editor
+} // namespace lldb_private
// Editline private methods
@@ -398,6 +397,20 @@ int Editline::GetLineIndexForLocation(CursorLocation location, int cursor_row) {
return line;
}
+std::optional<Editline::CursorPosition> Editline::GetCursorPosition() {
+ if (!m_editline)
+ return std::nullopt;
+
+ const LineInfoW *info = el_wline(m_editline);
+ if (!info)
+ return std::nullopt;
+
+ const size_t editline_cursor_col =
+ (int)((info->cursor - info->buffer) + GetPromptWidth()) + 1;
+
+ return {{editline_cursor_col, 0}};
+}
+
void Editline::MoveCursor(CursorLocation from, CursorLocation to) {
const LineInfoW *info = el_wline(m_editline);
int editline_cursor_position =
@@ -1151,7 +1164,8 @@ unsigned char Editline::TabCommand(int ch) {
to_add.push_back(' ');
el_deletestr(m_editline, request.GetCursorArgumentPrefix().size());
el_insertstr(m_editline, to_add.c_str());
- // Clear all the autosuggestion parts if the only single space can be completed.
+ // Clear all the autosuggestion parts if the only single space can be
+ // completed.
if (to_add == " ")
return CC_REDISPLAY;
return CC_REFRESH;
|
JDevlieghere
force-pushed
the
issue-134064
branch
from
45de6a1 to
0c72262
Compare
|
And how exactly does this fix #134064? As in, what steps are happening with this PR compared to before. |
#134064 (comment) has a visualization of the problem, which you've summarized in #145823 (review).
This PR uses the active IOHandler (i.e. Editline) to get the cursor position before we emit the newline and then uses an ANSI escape code to move the cursor back to its original column. I was curious why, depending on the timing, Editline was able to recover from this and this is how Editline does it. TL;DR: Before this patch we only fixed up the row. After this patch we fix up both the row and column. |
This PR ensures we correctly restore the cursor column after resizing the statusline. To ensure we have space for the statusline, we have to emit a newline to move up everything on screen. The newline causes the cursor to move to the start of the next line, which needs to be undone. Normally, we would use escape codes to save & restore the cursor position, but that doesn't work here, as the cursor position may have (purposely) changed. Instead, we move the cursor up one line using an escape code, but we weren't restoring the column. Interestingly, Editline was able to recover from this issue through the LineInfo struct which contains the buffer and the cursor location, which allows us to compute the column. This PR addresses the bug by relying on the active IOHandler to report its cursor position and if available, use that to restore the cursor column position. Fixes llvm#134064
JDevlieghere
force-pushed
the
issue-134064
branch
from
0c72262 to
6dc1ff8
Compare
|
Ok I get what we're doing now but...
This PR uses the active IOHanlder (which is Editline), but you're saying that prior to this PR, Editline was sometimes able to recover. So we were using Editline all along? Confused as to what is Editline the library and what is our Editline code. Do you mean that it's now all done in the same place and that keeps the timing correct? |
I didn't quite get what was causing the confusion so thanks for clarifying.
The challenge is that there is no escape code to move the cursor to its correct location, at least not without knowing what that location is. I was intrigued by the fact that Editline seemingly was able to do that anyway, so I wanted to understand how (2) and especially (3). The former could have been explained by just redrawing the prompt, but I noticed that if your cursor was't right at the end of the prompt, (3) would still do the right thing. The answer was that One solution could have been to indirectly rely on this behavior and ask our Editline wrapper to redraw the prompt or something after we change the scroll window. That's essentially was @labath suggested in #134064 (comment). However that's overkill: instead of using a hammer and redraw the whole thing, we can use the same information (the In other words, we're using the same information that Editline uses to move the cursor to the correct position when drawign the statusline. But we're doing it without having Editline do that for us. |
|
That's true, but it kind of relies on editline not moving the cursor between the time we ask for the location and when we do the statusline dance. In that sense, the hammer approach would be more robust as it would presumably happen while holding the editline mutex. I guess that would be fine in most cases because changing the setting is usually triggered by a CLI command, and that should complete before we start processing the next command... Though, if that's the case, then I'm confused why isn't this working already, and if we couldn't just reorder a couple of calls to make sure the status line is drawn before the prompt. Going back to my list of bad ideas, I think I now know why the first one doesn't work (because we don't know whether |
Understood. Editline does not avoid the situation, it provides a way out of it. |
Holding the editline mutex is a compelling argument for the hammer approach. For completeness, the other reasons I didn't go with this approach were that we already call back from Editline to redraw the statusline after it has done something to clear the screen so you'd get a dance where editline asks the statusline to redraw which asks editline to redraw the prompt. I'll take a look and see what it would take to create a function in Editline that can "redraw the prompt" safely. The
I had considered this but I quickly discarded it after trying it because it looks like very much like a bug. When you interactive resize the terminal we'll get a bunch of |
|
Alternative approach: #146132 |
Yeah, that would definitely be bad, but I feel like it should be avoidable. I take it that's a side effect of implementing resize by disabling and re-enabling the status line. However, does it have to be that way? What if resize was a separate operation that just involved redrawing the status line in the new place? In that case, you shouldn't need to do the |
This PR ensures we correctly restore the cursor column after resizing the statusline. To ensure we have space for the statusline, we have to emit a newline to move up everything on screen. The newline causes the cursor to move to the start of the next line, which needs to be undone.
Normally, we would use escape codes to save & restore the cursor position, but that doesn't work here, as the cursor position may have (purposely) changed. Instead, we move the cursor up one line using an escape code, but we weren't restoring the column.
Interestingly, Editline was able to recover from this issue through the LineInfo struct which contains the buffer and the cursor location, which allows us to compute the column. This PR addresses the bug by relying on the active IOHandler to report its cursor position and if available, use that to restore the cursor column position.
Fixes #134064