[wicket] add page up/page down/home/end to the keymap (#3692)

Model this with a new `PendingScroll` action stored on the view. This
action is resolved at draw time.

This ended up bundling several more changes (sorry!):

* Fix an off-by-one issue in the inventory view -- the bottom row of text was getting cut
  off previously, whoops :(
* Fix update pane popup scroll state to be more like the better model
  @jgallagher created while writing the rack setup screen. Previously,
  we were storing a single scroll state at the top. This was actually
  incorrect because when we moved from "waiting" to "failed", we weren't
  setting the scroll state to Some. The new model stores scroll state
  inline, and is correct as a result.
* Put in a compile-time separation between scrollable and non-scrollable
  popups -- I made a couple of mistakes with unwrapping scroll popup
  state which this caught.

I tested all the screens which support scrolling, and it worked great
on all of them.

Closes #3653.

Author: sunshowers

wicket/src/keymap.rs

@@ -83,6 +83,12 @@ pub enum Cmd {
     /// Begin rack setup.
     StartRackSetup,
 
+    /// Page up.
+    PageUp,
+
+    /// Page down.
+    PageDown,
+
     /// Goto top of list/screen/etc...
     GotoTop,
 
@@ -232,6 +238,10 @@ impl KeyHandler {
             KeyCode::Down => Cmd::Down,
             KeyCode::Right => Cmd::Right,
             KeyCode::Left => Cmd::Left,
+            KeyCode::PageUp => Cmd::PageUp,
+            KeyCode::PageDown => Cmd::PageDown,
+            KeyCode::Home => Cmd::GotoTop,
+            KeyCode::End => Cmd::GotoBottom,
             KeyCode::Char('y') => Cmd::Yes,
             KeyCode::Char('u') if event.modifiers == KeyModifiers::CONTROL => {
                 Cmd::StartUpdate

wicket/src/ui/panes/mod.rs

@@ -8,6 +8,7 @@ mod update;
 
 pub use super::Control;
 use crate::ui::defaults::style;
+use crate::Cmd;
 pub use overview::OverviewPane;
 pub use rack_setup::RackSetupPane;
 use tui::layout::{Constraint, Direction, Layout, Rect};
@@ -75,6 +76,40 @@ pub fn align_by(
     Text::from(Spans::from(text))
 }
 
+/// A pending scroll command.
+///
+/// This is used to communicate between the `on` and `draw` functions.
+///
+/// **NOTE:** `PendingScroll` deliberately does not implement `Copy` so that
+/// users are forced to reset pending scrolls with `Option::take`, or at least
+/// that there's some friction.
+#[derive(Clone, Copy, Debug, Eq, PartialEq)]
+pub enum PendingScroll {
+    Up,
+    Down,
+    PageUp,
+    PageDown,
+    GotoTop,
+    GotoBottom,
+}
+
+impl PendingScroll {
+    /// Maps a [`Cmd`] to a `PendingScroll`, if possible.
+    ///
+    /// Use in [`Control::on`] to handle scroll events.
+    pub fn from_cmd(cmd: &Cmd) -> Option<Self> {
+        match cmd {
+            Cmd::Up => Some(Self::Up),
+            Cmd::Down => Some(Self::Down),
+            Cmd::PageUp => Some(Self::PageUp),
+            Cmd::PageDown => Some(Self::PageDown),
+            Cmd::GotoTop => Some(Self::GotoTop),
+            Cmd::GotoBottom => Some(Self::GotoBottom),
+            _ => None,
+        }
+    }
+}
+
 /// A computed scroll offset.
 ///
 /// This scroll offset is computed by [`Self::new`], and is capped so that we
@@ -107,8 +142,25 @@ impl ComputedScrollOffset {
         current_offset: usize,
         text_height: usize,
         num_lines: usize,
+        pending_scroll: Option<PendingScroll>,
     ) -> Self {
-        let mut offset: usize = current_offset;
+        let mut offset = if let Some(pending_scroll) = pending_scroll {
+            // For page up and down, scroll by num_lines - 1 so at least one line is shared.
+            let page_lines = num_lines.saturating_sub(1);
+            match pending_scroll {
+                PendingScroll::Up => current_offset.saturating_sub(1),
+                PendingScroll::Down => current_offset + 1,
+                PendingScroll::PageUp => {
+                    current_offset.saturating_sub(page_lines)
+                }
+                PendingScroll::PageDown => current_offset + page_lines,
+                PendingScroll::GotoTop => 0,
+                // text.height() will get capped below.
+                PendingScroll::GotoBottom => text_height,
+            }
+        } else {
+            current_offset
+        };
 
         if offset > text_height {
             offset = text_height;

wicket/src/ui/panes/overview.rs

@@ -8,12 +8,12 @@ use std::collections::BTreeMap;
 use super::help_text;
 use super::ComputedScrollOffset;
 use super::Control;
+use super::PendingScroll;
 use crate::state::Component;
 use crate::state::{ComponentId, ALL_COMPONENT_IDS};
 use crate::ui::defaults::colors::*;
 use crate::ui::defaults::style;
 use crate::ui::widgets::IgnitionPopup;
-use crate::ui::widgets::PopupScrollKind;
 use crate::ui::widgets::{BoxConnector, BoxConnectorKind, Rack};
 use crate::ui::wrap::wrap_text;
 use crate::{Action, Cmd, Frame, State};
@@ -220,6 +220,7 @@ pub struct InventoryView {
     help: Vec<(&'static str, &'static str)>,
     // Vertical offset used for scrolling
     scroll_offsets: BTreeMap<ComponentId, usize>,
+    pending_scroll: Option<PendingScroll>,
     ignition: IgnitionPopup,
     popup: Option<PopupKind>,
 }
@@ -237,6 +238,7 @@ impl InventoryView {
                 .iter()
                 .map(|id| (*id, 0))
                 .collect(),
+            pending_scroll: None,
             ignition: IgnitionPopup::default(),
             popup: None,
         }
@@ -267,8 +269,7 @@ impl InventoryView {
 
         let popup_builder =
             self.ignition.to_popup_builder(state.rack_state.selected);
-        // Scrolling in the ignition popup is always disabled.
-        let popup = popup_builder.build(full_screen, PopupScrollKind::Disabled);
+        let popup = popup_builder.build(full_screen);
         frame.render_widget(popup, full_screen);
     }
 
@@ -319,6 +320,7 @@ impl Control for InventoryView {
             .constraints(
                 [
                     Constraint::Length(3),
+                    // This includes a top border but not the bottom border.
                     Constraint::Min(0),
                     Constraint::Length(3),
                 ]
@@ -366,11 +368,17 @@ impl Control for InventoryView {
             ),
         );
 
+        // chunks[1].height includes the top border, which means that the number
+        // of lines displayed is height - 1.
+        let num_lines = (chunks[1].height as usize).saturating_sub(1);
+
         let scroll_offset = self.scroll_offsets.get_mut(&component_id).unwrap();
+
         let y_offset = ComputedScrollOffset::new(
             *scroll_offset,
             text.height(),
-            chunks[1].height as usize,
+            num_lines,
+            self.pending_scroll.take(),
         )
         .into_offset();
         *scroll_offset = y_offset as usize;
@@ -400,6 +408,23 @@ impl Control for InventoryView {
         if self.popup.is_some() {
             return self.handle_cmd_in_popup(state, cmd);
         }
+
+        // For Up, Down, PageUp, PageDown, GotoTop and GotoBottom, a
+        // previous version of this code set the scroll offset directly.
+        // Sadly that doesn't work with page up/page down because we don't
+        // know the height of the viewport here.
+        //
+        // Instead, we now set a pending_scroll variable that is operated on
+        // while the view is drawn.
+        //
+        // The old scheme does work with the other commands (up, down, goto
+        // top and goto bottom), but use pending_scroll for all of these
+        // commands for uniformity.
+        if let Some(pending_scroll) = PendingScroll::from_cmd(&cmd) {
+            self.pending_scroll = Some(pending_scroll);
+            return Some(Action::Redraw);
+        }
+
         match cmd {
             Cmd::Left => {
                 state.rack_state.prev();
@@ -409,42 +434,6 @@ impl Control for InventoryView {
                 state.rack_state.next();
                 Some(Action::Redraw)
             }
-            Cmd::Down => {
-                let component_id = state.rack_state.selected;
-
-                // We currently debug print inventory on each call to
-                // `draw`, so we don't know  how many lines the total text is.
-                // We also don't know the Rect containing this Control, since
-                // we don't keep track of them anymore, and only know during
-                // rendering. Therefore, we just increment and correct
-                // for more incrments than lines exist during render, since
-                // `self` is passed mutably.
-                //
-                // It's also worth noting that the inventory may update
-                // before rendering, and so this is a somewhat sensible
-                // strategy.
-                *self.scroll_offsets.get_mut(&component_id).unwrap() += 1;
-                Some(Action::Redraw)
-            }
-            Cmd::Up => {
-                let component_id = state.rack_state.selected;
-                let offset =
-                    self.scroll_offsets.get_mut(&component_id).unwrap();
-                *offset = offset.saturating_sub(1);
-                Some(Action::Redraw)
-            }
-            Cmd::GotoTop => {
-                let component_id = state.rack_state.selected;
-                *self.scroll_offsets.get_mut(&component_id).unwrap() = 0;
-                Some(Action::Redraw)
-            }
-            Cmd::GotoBottom => {
-                let component_id = state.rack_state.selected;
-                // This will get corrected to be the bottom line during `draw`
-                *self.scroll_offsets.get_mut(&component_id).unwrap() =
-                    usize::MAX;
-                Some(Action::Redraw)
-            }
             Cmd::Tick => {
                 // TODO: This only animates when the pane is active. Should we move the
                 // tick into the [`Runner`] instead?