Skip to content

[lldb] Use the terminal height for paging editline completions #119914

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

Merged
merged 5 commits into from
Dec 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lldb/include/lldb/API/SBDebugger.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 4 additions & 0 deletions lldb/include/lldb/Core/Debugger.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
3 changes: 3 additions & 0 deletions lldb/include/lldb/Host/Editline.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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;
Expand Down
13 changes: 13 additions & 0 deletions lldb/source/API/SBDebugger.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
4 changes: 4 additions & 0 deletions lldb/source/Core/CoreProperties.td
Original file line number Diff line number Diff line change
Expand Up @@ -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">,
Expand Down
28 changes: 25 additions & 3 deletions lldb/source/Core/Debugger.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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");
Expand Down
49 changes: 33 additions & 16 deletions lldb/source/Host/common/Editline.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -924,18 +924,26 @@ 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;

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;

Expand All @@ -956,6 +964,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;
}

Expand All @@ -964,6 +973,7 @@ PrintCompletion(FILE *output_file,
if (c.GetDescription().empty() ||
description_col + separator_length + ellipsis_length >= max_length) {
fprintf(output_file, "\n");
lines_printed++;
continue;
}

Expand All @@ -990,6 +1000,8 @@ PrintCompletion(FILE *output_file,
for (llvm::StringRef line : llvm::split(c.GetDescription(), '\n')) {
if (line.empty())
break;
if (max_height && lines_printed >= *max_height)
break;
if (!first)
fprintf(output_file, "%*s",
static_cast<int>(description_col + separator_length), "");
Expand All @@ -1000,14 +1012,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++;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} 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(
Expand All @@ -1016,7 +1031,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 =
Expand All @@ -1026,21 +1045,12 @@ 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), max_len,
editline.GetTerminalWidth(),
all ? std::nullopt : std::optional<size_t>(page_size));

if (cur_pos >= results.size())
break;
Expand Down Expand Up @@ -1525,6 +1535,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(); }
Expand Down
15 changes: 12 additions & 3 deletions lldb/test/API/functionalities/completion/TestCompletion.py
Original file line number Diff line number Diff line change
Expand Up @@ -335,13 +335,22 @@ def test_settings_replace_target_ru(self):
)

def test_settings_show_term(self):
self.complete_from_to("settings show term-", "settings show term-width")
self.complete_from_to("settings show term-w", "settings show term-width")

def test_settings_list_term(self):
self.complete_from_to("settings list term-", "settings list term-width")
self.complete_from_to("settings list term-w", "settings list term-width")

def test_settings_show_term(self):
self.complete_from_to("settings show term-h", "settings show term-height")

def test_settings_list_term(self):
self.complete_from_to("settings list term-h", "settings list term-height")

def test_settings_remove_term(self):
self.complete_from_to("settings remove term-w", "settings remove term-width")

def test_settings_remove_term(self):
self.complete_from_to("settings remove term-", "settings remove term-width")
self.complete_from_to("settings remove term-h", "settings remove term-height")

def test_settings_s(self):
"""Test that 'settings s' completes to ['set', 'show']."""
Expand Down
34 changes: 34 additions & 0 deletions lldb/test/API/terminal/TestEditlineCompletions.py
Original file line number Diff line number Diff line change
Expand Up @@ -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")
7 changes: 5 additions & 2 deletions lldb/tools/driver/Driver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -627,16 +629,17 @@ 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) {
struct winsize window_size;
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);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion lldb/tools/driver/Driver.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Loading