[lldb] Use the terminal height for paging editline completions #119914
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesCurrently, we arbitrarily paginate editline completions to 40 elements. On large terminals, that leaves some real-estate unused. On small terminals, it's pretty annoying to not see the first completions. We can address both issues by using the terminal height for pagination. This builds on the improvements of #116456. Full diff: https://github.com/llvm/llvm-project/pull/119914.diff 10 Files Affected:
diff --git a/lldb/include/lldb/API/SBDebugger.h b/lldb/include/lldb/API/SBDebugger.h
index 1f42ec3cdc7d51..787bd040dd15bb 100644
--- a/lldb/include/lldb/API/SBDebugger.h
+++ b/lldb/include/lldb/API/SBDebugger.h
@@ -382,6 +382,10 @@ class LLDB_API SBDebugger {
void SetTerminalWidth(uint32_t term_width);
+ uint32_t GetTerminalHeight() const;
+
+ void SetTerminalHeight(uint32_t term_height);
+
lldb::user_id_t GetID();
const char *GetPrompt() const;
diff --git a/lldb/include/lldb/Core/Debugger.h b/lldb/include/lldb/Core/Debugger.h
index 1d5f2fcc20626c..70f4c4216221c6 100644
--- a/lldb/include/lldb/Core/Debugger.h
+++ b/lldb/include/lldb/Core/Debugger.h
@@ -280,6 +280,10 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
bool SetTerminalWidth(uint64_t term_width);
+ uint64_t GetTerminalHeight() const;
+
+ bool SetTerminalHeight(uint64_t term_height);
+
llvm::StringRef GetPrompt() const;
llvm::StringRef GetPromptAnsiPrefix() const;
diff --git a/lldb/include/lldb/Host/Editline.h b/lldb/include/lldb/Host/Editline.h
index e8e8a6c0d4f67e..26deba38f8471c 100644
--- a/lldb/include/lldb/Host/Editline.h
+++ b/lldb/include/lldb/Host/Editline.h
@@ -240,6 +240,8 @@ class Editline {
size_t GetTerminalWidth() { return m_terminal_width; }
+ size_t GetTerminalHeight() { return m_terminal_height; }
+
private:
/// Sets the lowest line number for multi-line editing sessions. A value of
/// zero suppresses line number printing in the prompt.
@@ -373,6 +375,7 @@ class Editline {
std::vector<EditLineStringType> m_input_lines;
EditorStatus m_editor_status;
int m_terminal_width = 0;
+ int m_terminal_height = 0;
int m_base_line_number = 0;
unsigned m_current_line_index = 0;
int m_current_line_rows = -1;
diff --git a/lldb/source/API/SBDebugger.cpp b/lldb/source/API/SBDebugger.cpp
index 4efec747aacff1..4e6b22492a0d1c 100644
--- a/lldb/source/API/SBDebugger.cpp
+++ b/lldb/source/API/SBDebugger.cpp
@@ -1405,6 +1405,19 @@ void SBDebugger::SetTerminalWidth(uint32_t term_width) {
m_opaque_sp->SetTerminalWidth(term_width);
}
+uint32_t SBDebugger::GetTerminalHeight() const {
+ LLDB_INSTRUMENT_VA(this);
+
+ return (m_opaque_sp ? m_opaque_sp->GetTerminalWidth() : 0);
+}
+
+void SBDebugger::SetTerminalHeight(uint32_t term_height) {
+ LLDB_INSTRUMENT_VA(this, term_height);
+
+ if (m_opaque_sp)
+ m_opaque_sp->SetTerminalHeight(term_height);
+}
+
const char *SBDebugger::GetPrompt() const {
LLDB_INSTRUMENT_VA(this);
diff --git a/lldb/source/Core/CoreProperties.td b/lldb/source/Core/CoreProperties.td
index e11aad2660b461..d3816c3070bbc5 100644
--- a/lldb/source/Core/CoreProperties.td
+++ b/lldb/source/Core/CoreProperties.td
@@ -136,6 +136,10 @@ let Definition = "debugger" in {
Global,
DefaultUnsignedValue<80>,
Desc<"The maximum number of columns to use for displaying text.">;
+ def TerminalHeight: Property<"term-height", "UInt64">,
+ Global,
+ DefaultUnsignedValue<24>,
+ Desc<"The number of rows used for displaying text.">;
def ThreadFormat: Property<"thread-format", "FormatEntity">,
Global,
DefaultStringValue<"thread #${thread.index}: tid = ${thread.id%tid}{, ${frame.pc}}{ ${module.file.basename}{`${function.name-with-args}{${frame.no-debug}${function.pc-offset}}}}{ at ${ansi.fg.cyan}${line.file.basename}${ansi.normal}:${ansi.fg.yellow}${line.number}${ansi.normal}{:${ansi.fg.yellow}${line.column}${ansi.normal}}}{, name = ${ansi.fg.green}'${thread.name}'${ansi.normal}}{, queue = ${ansi.fg.green}'${thread.queue}'${ansi.normal}}{, activity = ${ansi.fg.green}'${thread.info.activity.name}'${ansi.normal}}{, ${thread.info.trace_messages} messages}{, stop reason = ${ansi.fg.red}${thread.stop-reason}${ansi.normal}}{\\\\nReturn value: ${thread.return-value}}{\\\\nCompleted expression: ${thread.completed-expression}}\\\\n">,
diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index bb0a78790a9649..6ceb209269c9e7 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger.cpp
@@ -371,11 +371,29 @@ uint64_t Debugger::GetTerminalWidth() const {
}
bool Debugger::SetTerminalWidth(uint64_t term_width) {
+ const uint32_t idx = ePropertyTerminalWidth;
+ const bool success = SetPropertyAtIndex(idx, term_width);
+
if (auto handler_sp = m_io_handler_stack.Top())
handler_sp->TerminalSizeChanged();
- const uint32_t idx = ePropertyTerminalWidth;
- return SetPropertyAtIndex(idx, term_width);
+ return success;
+}
+
+uint64_t Debugger::GetTerminalHeight() const {
+ const uint32_t idx = ePropertyTerminalHeight;
+ return GetPropertyAtIndexAs<uint64_t>(
+ idx, g_debugger_properties[idx].default_uint_value);
+}
+
+bool Debugger::SetTerminalHeight(uint64_t term_height) {
+ const uint32_t idx = ePropertyTerminalHeight;
+ const bool success = SetPropertyAtIndex(idx, term_height);
+
+ if (auto handler_sp = m_io_handler_stack.Top())
+ handler_sp->TerminalSizeChanged();
+
+ return success;
}
bool Debugger::GetUseExternalEditor() const {
@@ -919,7 +937,11 @@ Debugger::Debugger(lldb::LogOutputCallback log_callback, void *baton)
m_collection_sp->GetPropertyAtIndexAsOptionValueUInt64(
ePropertyTerminalWidth);
term_width->SetMinimumValue(10);
- term_width->SetMaximumValue(1024);
+
+ OptionValueUInt64 *term_height =
+ m_collection_sp->GetPropertyAtIndexAsOptionValueUInt64(
+ ePropertyTerminalHeight);
+ term_height->SetMinimumValue(10);
// Turn off use-color if this is a dumb terminal.
const char *term = getenv("TERM");
diff --git a/lldb/source/Host/common/Editline.cpp b/lldb/source/Host/common/Editline.cpp
index c9e890304ae1ec..78164e2cfb2c25 100644
--- a/lldb/source/Host/common/Editline.cpp
+++ b/lldb/source/Host/common/Editline.cpp
@@ -44,6 +44,7 @@ using namespace lldb_private::line_editor;
#define ANSI_SET_COLUMN_N ESCAPE "[%dG"
#define ANSI_UP_N_ROWS ESCAPE "[%dA"
#define ANSI_DOWN_N_ROWS ESCAPE "[%dB"
+#define ANSI_CLEAR_BELOW ESCAPE "[J"
#if LLDB_EDITLINE_USE_WCHAR
@@ -924,10 +925,11 @@ unsigned char Editline::BufferEndCommand(int ch) {
/// Prints completions and their descriptions to the given file. Only the
/// completions in the interval [start, end) are printed.
-static void
+static size_t
PrintCompletion(FILE *output_file,
llvm::ArrayRef<CompletionResult::Completion> results,
- size_t max_completion_length, size_t max_length) {
+ size_t max_completion_length, size_t max_length,
+ std::optional<size_t> max_height = std::nullopt) {
constexpr size_t ellipsis_length = 3;
constexpr size_t padding_length = 8;
constexpr size_t separator_length = 4;
@@ -935,7 +937,14 @@ PrintCompletion(FILE *output_file,
const size_t description_col =
std::min(max_completion_length + padding_length, max_length);
+ size_t lines_printed = 0;
+ size_t results_printed = 0;
for (const CompletionResult::Completion &c : results) {
+ if (max_height && lines_printed == *max_height)
+ break;
+
+ results_printed++;
+
if (c.GetCompletion().empty())
continue;
@@ -956,6 +965,7 @@ PrintCompletion(FILE *output_file,
fprintf(output_file, "%.*s...\n",
static_cast<int>(max_length - padding_length - ellipsis_length),
c.GetCompletion().c_str());
+ lines_printed++;
continue;
}
@@ -964,6 +974,7 @@ PrintCompletion(FILE *output_file,
if (c.GetDescription().empty() ||
description_col + separator_length + ellipsis_length >= max_length) {
fprintf(output_file, "\n");
+ lines_printed++;
continue;
}
@@ -1000,14 +1011,17 @@ PrintCompletion(FILE *output_file,
if (position + description_length < max_length) {
fprintf(output_file, "%.*s\n", static_cast<int>(description_length),
line.data());
+ lines_printed++;
} else {
fprintf(output_file, "%.*s...\n",
static_cast<int>(max_length - position - ellipsis_length),
line.data());
+ lines_printed++;
continue;
}
}
}
+ return results_printed;
}
void Editline::DisplayCompletions(
@@ -1016,7 +1030,11 @@ void Editline::DisplayCompletions(
fprintf(editline.m_output_file,
"\n" ANSI_CLEAR_BELOW "Available completions:\n");
- const size_t page_size = 40;
+
+ /// Account for the current line, the line showing "Available completions"
+ /// before and the line saying "More" after.
+ const size_t page_size = editline.GetTerminalHeight() - 3;
+
bool all = false;
auto longest =
@@ -1026,21 +1044,15 @@ void Editline::DisplayCompletions(
const size_t max_len = longest->GetCompletion().size();
- if (results.size() < page_size) {
- PrintCompletion(editline.m_output_file, results, max_len,
- editline.GetTerminalWidth());
- return;
- }
-
size_t cur_pos = 0;
while (cur_pos < results.size()) {
size_t remaining = results.size() - cur_pos;
size_t next_size = all ? remaining : std::min(page_size, remaining);
- PrintCompletion(editline.m_output_file, results.slice(cur_pos, next_size),
- max_len, editline.GetTerminalWidth());
-
- cur_pos += next_size;
+ cur_pos += PrintCompletion(
+ editline.m_output_file, results.slice(cur_pos, next_size), max_len,
+ editline.GetTerminalWidth(),
+ all ? std::nullopt : std::optional<size_t>(page_size));
if (cur_pos >= results.size())
break;
@@ -1525,6 +1537,13 @@ void Editline::ApplyTerminalSizeChange() {
m_terminal_width = INT_MAX;
m_current_line_rows = 1;
}
+
+ int rows;
+ if (el_get(m_editline, EL_GETTC, "li", &rows, nullptr) == 0) {
+ m_terminal_height = rows;
+ } else {
+ m_terminal_height = INT_MAX;
+ }
}
const char *Editline::GetPrompt() { return m_set_prompt.c_str(); }
diff --git a/lldb/test/API/terminal/TestEditlineCompletions.py b/lldb/test/API/terminal/TestEditlineCompletions.py
index 7fa6f95c130c64..b4ea0f39ec10c5 100644
--- a/lldb/test/API/terminal/TestEditlineCompletions.py
+++ b/lldb/test/API/terminal/TestEditlineCompletions.py
@@ -62,3 +62,37 @@ def test_multiline_description(self):
" kdp-remote is an abbreviation for 'process conn..."
)
self.child.expect(" kill -- Terminate the current target process.")
+
+ @skipIfAsan
+ @skipIfEditlineSupportMissing
+ def test_completion_pagination(self):
+ """Test that we use the terminal height for pagination."""
+ self.launch(dimensions=(10, 30))
+ self.child.send("_regexp-\t")
+ self.child.expect("Available completions:")
+ self.child.expect(" _regexp-attach")
+ self.child.expect(" _regexp-break")
+ self.child.expect(" _regexp-bt")
+ self.child.expect(" _regexp-display")
+ self.child.expect(" _regexp-down")
+ self.child.expect(" _regexp-env")
+ self.child.expect(" _regexp-jump")
+ self.child.expect("More")
+
+ @skipIfAsan
+ @skipIfEditlineSupportMissing
+ def test_completion_multiline_pagination(self):
+ """Test that we use the terminal height for pagination and account for multi-line descriptions."""
+ self.launch(dimensions=(6, 72))
+ self.child.send("k\t")
+ self.child.expect("Available completions:")
+ self.child.expect(
+ " kdp-remote -- Connect to a process via remote KDP server."
+ )
+ self.child.expect(
+ " If no UDP port is specified, port 41139 is assu..."
+ )
+ self.child.expect(
+ " kdp-remote is an abbreviation for 'process conn..."
+ )
+ self.child.expect("More")
diff --git a/lldb/tools/driver/Driver.cpp b/lldb/tools/driver/Driver.cpp
index 98c3643f75c97b..771663eb7bf0af 100644
--- a/lldb/tools/driver/Driver.cpp
+++ b/lldb/tools/driver/Driver.cpp
@@ -451,6 +451,8 @@ int Driver::MainLoop() {
::ioctl(STDIN_FILENO, TIOCGWINSZ, &window_size) == 0) {
if (window_size.ws_col > 0)
m_debugger.SetTerminalWidth(window_size.ws_col);
+ if (window_size.ws_row > 0)
+ m_debugger.SetTerminalHeight(window_size.ws_row);
}
SBCommandInterpreter sb_interpreter = m_debugger.GetCommandInterpreter();
@@ -627,8 +629,9 @@ int Driver::MainLoop() {
return sb_interpreter.GetQuitStatus();
}
-void Driver::ResizeWindow(unsigned short col) {
+void Driver::ResizeWindow(unsigned short col, unsigned short row) {
GetDebugger().SetTerminalWidth(col);
+ GetDebugger().SetTerminalHeight(row);
}
void sigwinch_handler(int signo) {
@@ -636,7 +639,7 @@ void sigwinch_handler(int signo) {
if ((isatty(STDIN_FILENO) != 0) &&
::ioctl(STDIN_FILENO, TIOCGWINSZ, &window_size) == 0) {
if ((window_size.ws_col > 0) && g_driver != nullptr) {
- g_driver->ResizeWindow(window_size.ws_col);
+ g_driver->ResizeWindow(window_size.ws_col, window_size.ws_row);
}
}
}
diff --git a/lldb/tools/driver/Driver.h b/lldb/tools/driver/Driver.h
index 83e0d8a41cfdb1..ed400ca43575d4 100644
--- a/lldb/tools/driver/Driver.h
+++ b/lldb/tools/driver/Driver.h
@@ -92,7 +92,7 @@ class Driver : public lldb::SBBroadcaster {
lldb::SBDebugger &GetDebugger() { return m_debugger; }
- void ResizeWindow(unsigned short col);
+ void ResizeWindow(unsigned short col, unsigned short row);
private:
lldb::SBDebugger m_debugger;
|
Currently, we arbitrarily paginate editline completions to 40 elements. On large terminals, that leaves some real-estate unused. On small terminals, it's pretty annoying to not see the first completions. We can address both issues by using the terminal height for pagination.
JDevlieghere
force-pushed
the
editline-pagination
branch
from
b789e1d to
f6fdfaf
Compare
JDevlieghere
commented
Dec 13, 2024
| @@ -919,7 +937,11 @@ Debugger::Debugger(lldb::LogOutputCallback log_callback, void *baton) | |||
| m_collection_sp->GetPropertyAtIndexAsOptionValueUInt64( | |||
| ePropertyTerminalWidth); | |||
| term_width->SetMinimumValue(10); | |||
| term_width->SetMaximumValue(1024); | |||
There was a problem hiding this comment.
I removed the maximum value here because I didn't want to duplicate it for the terminal height. If this bothers folks I can do that in a separate commit. I don't think it makes sense to arbitrarily limit the upper bound in the age of ultra-wide monitors.
JDevlieghere
force-pushed
the
editline-pagination
branch
from
a4d5118 to
2745e5c
Compare
labath
reviewed
Dec 16, 2024
| @@ -1000,14 +1010,17 @@ PrintCompletion(FILE *output_file, | |||
| if (position + description_length < max_length) { | |||
| fprintf(output_file, "%.*s\n", static_cast<int>(description_length), | |||
| line.data()); | |||
| lines_printed++; | |||
There was a problem hiding this comment.
It seems like this could be a problem if we cross the page boundary in the middle of a multi-line description. The worst part is that given that the code above checks for strict equality, this situation might end up cause us to dump everything in a single go.
There was a problem hiding this comment.
Ah good point. I considered the situation where we would potentially exceed the page size if the last entry was a multiline one (and deemed that acceptable) but I didn't account for it in the check. We can make that a >=.
There was a problem hiding this comment.
I suppose its not the end of the world, though it that case, it might be better to bail out here (and skip printing the rest of the description) instead of pushing the first completion off the screen (and if it fit on one line, the user wouldn't even know that it was there).
If this was c++20 I guess we could make this a coroutine which continues where it left off for the next page.
Up to you, I guess...
labath
approved these changes
Dec 16, 2024
lldb/source/Host/common/Editline.cpp
Outdated
Comment on lines
1050
to
1051
| size_t remaining = results.size() - cur_pos; | ||
| size_t next_size = all ? remaining : std::min(page_size, remaining); |
There was a problem hiding this comment.
I think you don't need this now, and could always just pass the remainder of the list.
| @@ -1000,14 +1010,17 @@ PrintCompletion(FILE *output_file, | |||
| if (position + description_length < max_length) { | |||
| fprintf(output_file, "%.*s\n", static_cast<int>(description_length), | |||
| line.data()); | |||
| lines_printed++; | |||
There was a problem hiding this comment.
I suppose its not the end of the world, though it that case, it might be better to bail out here (and skip printing the rest of the description) instead of pushing the first completion off the screen (and if it fit on one line, the user wouldn't even know that it was there).
If this was c++20 I guess we could make this a coroutine which continues where it left off for the next page.
Up to you, I guess...
- Always pass the whole list. - Truncate multi-line descriptions if they're about to overflow.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
JDevlieghere
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Dec 16, 2024
…119914) Currently, we arbitrarily paginate editline completions to 40 elements. On large terminals, that leaves some real-estate unused. On small terminals, it's pretty annoying to not see the first completions. We can address both issues by using the terminal height for pagination. This builds on the improvements of llvm#116456. (cherry picked from commit 3dfc1d9)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, we arbitrarily paginate editline completions to 40 elements. On large terminals, that leaves some real-estate unused. On small terminals, it's pretty annoying to not see the first completions. We can address both issues by using the terminal height for pagination.
This builds on the improvements of #116456.