Skip to content
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.

Sign up for GitHub

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

Add support for pagination in the CLI wallet #1894

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add support for pagination in the CLI wallet #1894

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 10 additions & 0 deletions node-gui/src/main_window/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -645,6 +645,16 @@ impl MainWindow {
backend_sender,
)
.map(MainWindowMessage::MainWidgetMessage),
ConsoleCommand::PaginatedPrint { header, body } => self
.main_widget
.update(
MainWidgetMessage::TabsMessage(TabsMessage::WalletMessage(
wallet_id,
WalletMessage::ConsoleOutput(header + &body),
)),
backend_sender,
)
.map(MainWindowMessage::MainWidgetMessage),
ConsoleCommand::ClearScreen
| ConsoleCommand::ClearHistory
| ConsoleCommand::PrintHistory
Expand Down
10 changes: 8 additions & 2 deletions wallet/wallet-cli-commands/src/command_handler/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1234,7 +1234,10 @@ where
WalletCommand::ListPendingTransactions => {
let (wallet, selected_account) = wallet_and_selected_acc(&mut self.wallet).await?;
let utxos = wallet.list_pending_transactions(selected_account).await?;
Ok(ConsoleCommand::Print(format!("{utxos:#?}")))
Ok(ConsoleCommand::PaginatedPrint {
header: "Pending Transactions".to_owned(),
body: format!("{utxos:#?}"),
})
}

WalletCommand::ListMainchainTransactions { address, limit } => {
Expand All @@ -1257,7 +1260,10 @@ where
table
};

Ok(ConsoleCommand::Print(table.to_string()))
Ok(ConsoleCommand::PaginatedPrint {
header: String::new(),
body: table.to_string(),
})
}

WalletCommand::GetTransaction { transaction_id } => {
Expand Down
4 changes: 4 additions & 0 deletions wallet/wallet-cli-commands/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -878,6 +878,10 @@ pub enum ManageableWalletCommand {
#[derive(Debug, Clone, serde::Serialize)]
pub enum ConsoleCommand {
Print(String),
PaginatedPrint {
header: String,
body: String,
},
ClearScreen,
PrintHistory,
ClearHistory,
Expand Down
95 changes: 95 additions & 0 deletions wallet/wallet-cli-lib/src/repl/interactive/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,13 @@ mod wallet_prompt;
use std::path::PathBuf;

use clap::Command;
use itertools::{FoldWhile, Itertools};
use reedline::{
default_emacs_keybindings, default_vi_insert_keybindings, default_vi_normal_keybindings,
ColumnarMenu, DefaultValidator, EditMode, Emacs, FileBackedHistory, ListMenu, MenuBuilder,
Reedline, ReedlineMenu, Signal, Vi,
};

use tokio::sync::{mpsc, oneshot};
use wallet_cli_commands::{get_repl_command, parse_input, ConsoleCommand};
use wallet_rpc_lib::types::NodeInterface;
Expand Down Expand Up @@ -197,6 +199,9 @@ fn handle_response<N: NodeInterface>(
Ok(Some(ConsoleCommand::Print(text))) => {
console.print_line(&text);
}
Ok(Some(ConsoleCommand::PaginatedPrint { header, body })) => {
paginate_output(header, body, line_editor, console);
}
Ok(Some(ConsoleCommand::SetStatus {
status,
print_message,
Expand Down Expand Up @@ -226,3 +231,93 @@ fn handle_response<N: NodeInterface>(
}
None
}

fn paginate_output(
header: String,
body: String,
Comment on lines +236 to +237
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO "paginate" should accept a more structured data, where "header" is a list of column names and "body" is a table.

A TODO would be fine too for now.

line_editor: &mut Reedline,
console: &mut impl ConsoleOutput,
) {
let mut current_index = 0;

let (cols, rows) = crossterm::terminal::size().unwrap_or((80, 24));
let cols = cols as usize;
let page_rows = (rows - 4) as usize; // make room for the header and prompt

loop {
line_editor.clear_screen().expect("Should not fail normally");
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make paginate_output return a Result and propagate all errors properly.


let limit = compute_visible_text_limit(
body.get(current_index..).expect("safe point").lines(),
cols,
page_rows,
);
let end_index = std::cmp::min(current_index + limit, body.len());

console.print_line(&header);
console.print_line(body.get(current_index..end_index).expect("safe point"));

let commands = match (current_index, end_index) {
(0, end) if end == body.len() => "Press 'q' to quit",
(0, _) => "Press 'j' for down, 'q' to quit",
Copy link
Contributor

Choose a reason for hiding this comment

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

"down" -> "next"?

(_, end) if end == body.len() => "Press 'k' for previous, 'q' to quit",
(_, _) => "Press 'j' for next, 'k' for previous, 'q' to quit",
};
console.print_line(commands);

// Wait for user input.
crossterm::terminal::enable_raw_mode().expect("Should not fail normally");
let event = crossterm::event::read().expect("Should not fail normally");
crossterm::terminal::disable_raw_mode().expect("Should not fail normally");
Comment on lines +269 to +271
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Probably the more correct way is to switch to the raw mode once at the beginning and switch back via OnceDestructor.
    (and probably we should first check that the terminal is not in the raw mode already).

    The problem is that println! (and therefore ConsoleOutput::print_line) won't work properly, because "\n" won't return the cursor to the beginning of the line ("\r" is needed for that).
    But we can just add a separate function ConsoleOutput::print (so that the line separator can be chosen by the caller) and mention in comments that the existing print_line and print_error are not suitable for a terminal in a raw mode.

    (though I'm not sure if the current implementation has any drawbacks, besides not looking nice).

  2. We probably need a way of pausing logging while in the pagination mode, because when the loop is waiting for a key press, the terminal is in the raw mode either way. So if a logging event happens, it won't be printed correctly (due to "\n" not returning the cursor).

Let's maybe at least add some TODOs for this.


if let crossterm::event::Event::Key(key_event) = event {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to handle the Resize event as well (a TODO is fine for me)

match key_event.code {
reedline::KeyCode::Char('j') | reedline::KeyCode::Down
if end_index < body.len() =>
{
let next_text = body.get(current_index..).expect("safe point").lines();
let limit = compute_visible_text_limit(next_text, cols, 1);
current_index = std::cmp::min(body.len(), current_index + limit);
}
reedline::KeyCode::Char('k') | reedline::KeyCode::Up if current_index > 0 => {
let prev_text = body.get(..current_index).expect("safe point").lines().rev();
let limit = compute_visible_text_limit(prev_text, cols, 1);
current_index = current_index.saturating_sub(limit);
}
reedline::KeyCode::Char('q') | reedline::KeyCode::Esc => {
break; // Exit pagination
}
_ => {} // Ignore other keys
}
}
Comment on lines +273 to +292
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's avoid calling clear_screen if a wrong key was pressed (including "j" and "k" when they are not applicable), to avoid screen flickering.

}

line_editor.clear_screen().expect("Should not fail normally");
}

fn compute_visible_text_limit<'a, I>(mut lines: I, cols: usize, page_rows: usize) -> usize
where
I: Iterator<Item = &'a str>,
{
let (_, end_index) = lines
.fold_while((0, 0), |(current_rows, current_index), line| {
let new_rows = line.len().div_ceil(cols);
if current_rows + new_rows <= page_rows {
let new_total_rows = current_rows + new_rows;
let new_end_index = current_index + line.len() + 1;
FoldWhile::Continue((new_total_rows, new_end_index))
} else {
let rows_available = page_rows - current_rows;
// make sure we cut it on the start of a utf-8 char boundary
Copy link
Contributor

Choose a reason for hiding this comment

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

It's either "on the start of a utf-8 char" or "at a utf-8 char boundary"

let new_end_index = current_index
+ (1..=rows_available * cols)
.rev()
.find(|&i| line.is_char_boundary(i))
// 0 is always a safe char boundary
.unwrap_or(0);
FoldWhile::Done((page_rows, new_end_index))
Comment on lines +312 to +318
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Regaring unicode handling:

    • This won't work correctly in the "k" case, because here you calculate the number of bytes from the beginning of the line to some char boundary and the caller will subtract this value from current_index, treating it as the number of bytes from the end of the line to a char boundary.

    • rows_available * cols should be the number of characters and not bytes.

  2. Even in the ascii case this doesn't work well in the "k" case, because the partial row shown at the top will always have "cols" characters.
    E.g. here is how I see a top row when the size of the terminal is small.
    Initially:

    | db76bb89b8e14daeed2caba4aeb28493c95eef39cd1ea4dc71c53d77c913d0d5 | 297962
 | 1726820532     |

    After presing "j":

    | 1726820532     |

    And after pressing "j" again and then "k":

    2caba4aeb28493c95eef39cd1ea4dc71c53d77c913d0d5 | 297962      | 1726820532     |

I think you should build some kind of "layout" object once (well, maybe rebuilding it on Resize), and the "k"/"j" should only scroll over the lines of that "layout" and not do any text parsing.
(In the current implementation the "layout" can be a Vec<&str>, where each element represents a separate line of the output)

}
})
.into_inner();
end_index
}
1 change: 1 addition & 0 deletions wallet/wallet-cli-lib/src/repl/non_interactive/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ fn to_line_output<N: NodeInterface>(
) -> Result<LineOutput, WalletCliError<N>> {
match command_output {
ConsoleCommand::Print(text) => Ok(LineOutput::Print(text)),
ConsoleCommand::PaginatedPrint { header, body } => Ok(LineOutput::Print(header + &body)),
ConsoleCommand::SetStatus {
status: _,
print_message,
Expand Down