Migrate TUI from cursive to ratatui - #3905
Conversation
| @@ -0,0 +1,152 @@ | |||
| // Copyright 2024 The Grin Developers | |||
wiesche89
left a comment
There was a problem hiding this comment.
please rebase (merge conflicts)
Replaces the cursive/pancurses-backed TUI with ratatui + crossterm, which has no external C dependency (no more linking libncurses). This fixes cross-compilation failures, macOS crashes on mouse clicks, and Windows crashes on first run reported in mimblewimble#3841. The cursive retained-mode widget tree is replaced with a single App struct (src/bin/tui/app.rs) that every screen renders from each frame. All previous behavior (status fields, peer/mining table columns and formatting, bottom-anchored log view, keybindings, Controller's public API) is preserved, with a few additions: - Mouse support: click a menu item to switch tabs, scroll wheel moves table selection (safe now that ncurses is gone) - PageUp/PageDown/Home/End navigation in tables, with selection clamped to the table's row count - Redraws are throttled: the UI only repaints on input or new data, with a 250ms cap to keep time-based fields fresh Interactive column sorting (cursive_table_view) is dropped since ratatui's Table has no equivalent; tables render in the order ServerStats provides. Fixes mimblewimble#3841
Removes the now-unused cursive/pancurses dependency tree and resolves the ratatui/crossterm graph against the current staging lockfile.
c08dfce to
0f62d93
Compare
wiesche89
left a comment
There was a problem hiding this comment.
Ratatui looks like the right direction and works well on macOS. I couldn’t test Linux or Windows. I left a few comments that I’d prefer to address in this PR so the migration lands cleanly.
| self.cursive.step(); | ||
| if self.needs_redraw || self.last_draw.elapsed() >= MAX_REDRAW_INTERVAL { | ||
| let app = &mut self.app; | ||
| let _ = self.terminal.draw(|f| draw(f, app)); |
There was a problem hiding this comment.
Could we propagate terminal input and render errors instead of dropping them? A failed poll, read, or draw currently leaves the controller running without a usable UI, and a failed draw is still marked as completed.
There was a problem hiding this comment.
Done in 6f78dcb. UI::step() now returns io::Result<bool> and propagates failures from event::poll, event::read, and terminal.draw instead of swallowing them. On error the controller logs the failure, sets exit code 1, and leaves the loop so we no longer keep running with a dead UI or mark a failed draw as completed.
| if show_dialog_clone.load(Ordering::Relaxed) { | ||
| c.pop_layer(); | ||
| fn handle_key(&mut self, code: KeyCode) { | ||
| if let Some(dialog) = &self.app.dialog { |
There was a problem hiding this comment.
Mouse input is blocked for every dialog, but keyboard input is only blocked for errors. During startup, Tab/j/Enter can still change the view behind the info dialog. Could we make these dialogs modal while still allowing q to shut down?
There was a problem hiding this comment.
Done in 6f78dcb. All dialogs are modal now: keyboard input is blocked while either an info or error dialog is up. Error dialogs still accept Enter / q / Esc to exit; info dialogs (startup status) only allow q so you can shut down without Tab/j/Enter changing the view behind them. Mouse input was already blocked for every dialog.
| for raw_line in text.split('\n') { | ||
| let mut current = String::new(); | ||
| let mut current_len = 0usize; | ||
| for word in raw_line.split(' ') { |
There was a problem hiding this comment.
Could we use Paragraph::wrap() and line_count() here instead of maintaining a separate wrapping implementation? That would preserve terminal display width and whitespace while still allowing the log view to stay bottom aligned.
There was a problem hiding this comment.
Done in 6f78dcb. The custom wrap_text helper is gone. Logs now use Paragraph::wrap(Wrap { trim: false }) plus line_count() (via the unstable-rendered-line-info feature) so wrapping matches terminal display width. Bottom alignment is kept by scrolling when content overflows, or rendering into a bottom sub-area when it is shorter than the pane.
| state.select(None); | ||
| return; | ||
| } | ||
| let current = state.selected().unwrap_or(0) as i64; |
There was a problem hiding this comment.
TableState starts at None, so the first Down/j/scroll jumps straight to row 1. Could we anchor the first move to row 0 and cover it with a small test?
There was a problem hiding this comment.
Done in 6f78dcb. Selection logic is in next_table_selection: when TableState is still None, the first move anchors to row 0 (or the last row for End). Unit tests cover the unselected → 0 case, empty tables, and clamp/move behavior.
| GGGGGGGGGGGGG GGGGGGGG GGGGGGGGGGGGG | ||
| GGGGGGGGGGGGGG GGGGGGGG GGGGGGGGGGGGGG | ||
| GGGGGGGGGGGGGG GGGGGGGGGGGGGGGGGGGGGGGGGGGGG | ||
| pub const _WELCOME_LOGO: &str = " GGGGG GGGGGGG |
There was a problem hiding this comment.
After the migration this module only contains the unused _WELCOME_LOGO. Could we remove the file and mod constant, as part of the cleanup?
There was a problem hiding this comment.
Done in 6f78dcb. Removed src/bin/tui/constants.rs and its mod constants entry; _WELCOME_LOGO was unused after the migration.
| pub fn stop(&mut self) { | ||
| self.cursive.quit(); | ||
| self.app.should_quit = true; | ||
| let _ = disable_raw_mode(); |
There was a problem hiding this comment.
The terminal restore sequence is now repeated in the panic hook, stop(), and Drop. Could we centralize it and make the cleanup so these paths cannot drift?
There was a problem hiding this comment.
Done in 6f78dcb. Terminal teardown is centralized in restore_terminal(), shared by the panic hook, stop(), and Drop. UI also tracks a restored flag so the sequence only runs once and cannot drift between those paths.
| _ => {} | ||
| }, | ||
| match message { | ||
| UIMessage::UpdateStatus(update) => self.app.stats = Some(update), |
There was a problem hiding this comment.
Now that the UI owns App and status updates happen on the same controller thread, could we update app.stats directly and remove the single-message UI channel? It looks like leftover Cursive plumbing.
There was a problem hiding this comment.
Done in 6f78dcb. Removed the UIMessage channel and types.rs. Stats are written straight to app.stats on the controller thread (same thread that owns App), and the redraw flag is set there.
| lines.push(line("Stem Pool Size:", "0 (0)")); | ||
| } | ||
| lines.push(Line::from(SEPARATOR)); | ||
| // These three lines are reserved for mining config/status/network info, |
There was a problem hiding this comment.
These fields were never populated in the old view. Could we drop the reserved blank rows instead of carrying the unused placeholders into the new renderer?
There was a problem hiding this comment.
Done in 6f78dcb. Dropped the three reserved blank rows (and the trailing separator that only framed them) from the status view so we no longer carry unused placeholders forward.
|
|
||
| /// Redraw at least this often even without input or new data, so | ||
| /// time-based fields (peer "last seen" ages, etc.) stay fresh. | ||
| const MAX_REDRAW_INTERVAL: Duration = Duration::from_millis(250); |
There was a problem hiding this comment.
The time based fields only change once per second, while 250 ms still rebuilds the full UI four times per second when idle. Could this use a one-second refresh, or is there a measurement supporting the shorter interval?
There was a problem hiding this comment.
Done in 6f78dcb. Switched MAX_REDRAW_INTERVAL from 250 ms to 1 s so idle redraws match the cadence of the time-based fields (uptime, peer “last seen”, etc.) and the 1 s stats update. No measurement justified the shorter interval.
| let height = area.height as usize; | ||
|
|
||
| LogBufferView { buffer } | ||
| // Walk entries newest-first, wrapping each until we have enough rows to |
There was a problem hiding this comment.
There’s quite a bit of commentary walking through this rendering step by step. Could we simplify it by using Ratatui’s wrapping and line counting support, then keep only the non obvious bottom alignment note?
There was a problem hiding this comment.
Done in 6f78dcb (together with the wrap/line_count change above). Step-by-step wrapping commentary is gone; the log draw path uses ratatui wrapping and only documents the non-obvious bottom-alignment behavior.
Propagate terminal I/O errors, make startup dialogs modal, use Paragraph wrap/line_count for logs, fix first table selection, centralize terminal restore, drop the leftover UI channel and unused constants, and align idle redraw with the 1s stats cadence.
Summary
cursive/cursive_table_viewTUI (which pulls inpancurses/ncurses) withratatui+crossterm, which have no external C dependencyj/k/arrows,Tab,Enter,Esc,q, plusw/dto toggle the Mining sub-view) are preservedPageUp/PageDown/Home/Endnavigation in tables, and throttled redraws (repaint only on input or new data, 250ms cap) to reduce idle CPUcursive_table_view) is dropped, sinceratatui::widgets::Tablehas no equivalent — tables render in the orderServerStatsprovides. Can be revisited as a follow-up if wanted.Controller::new/Controller::run()'s public API (used bysrc/bin/cmd/server.rs) is unchangedKeyEventKind::Press, avoiding the classic doubled-keystroke issue with crossterm on WindowsTest plan
cargo build --bin grin— 0 errorscargo test --bin grin tui::status— bothupdate_sync_statusunit tests pass unmodifiedcargo clippy --bin grin— zero findings insrc/bin/tuiotool -Lthe built binary has noncurses/pancurseslinkageHome/End/PageUp/PageDown, mouse tab-clicks and wheel scrolling (via SGR escape injection) all verified; error-dialog path renders correctly;qexits cleanly with exit code 0 and the terminal fully restored (raw mode off, alternate screen left, mouse capture disabled)Fixes #3841