feat: page up/down navigation keys for skctl xray

Cargo.toml

@@ -22,6 +22,12 @@ rust-version = "1.79"
 [profile.dev.package."*"]
 debug = false
 
+[profile.dev.package.insta]
+opt-level = 3
+
+[profile.dev.package.similar]
+opt-level = 3
+
 [workspace.dependencies]
 sk-api = { version = "2.0.0", path = "sk-api" }
 sk-core = { version = "2.0.0", path = "sk-core" }

sk-cli/src/xray/app.rs

@@ -15,11 +15,19 @@ pub(super) enum Mode {
 
 pub(super) enum Message {
     Deselect,
-    Down(u16, bool),
+    Down,
+    PageDown,
+    PageUp,
     Quit,
     Select,
     Unknown,
-    Up(u16, bool),
+    Up,
+}
+
+#[derive(Copy, Clone, Eq, PartialEq)]
+pub(super) enum JumpDir {
+    Down,
+    Up,
 }
 
 #[derive(Default)]
@@ -34,7 +42,7 @@ pub(super) struct App {
     pub(super) object_contents_list_state: ListState,
 
     pub(super) focused_frame_height: u16,
-    pub(super) jump: bool,
+    pub(super) jump: Option<JumpDir>,
 }
 
 impl App {
@@ -55,21 +63,17 @@ impl App {
     }
 
     pub(super) fn update_state(&mut self, msg: Message) -> bool {
-        self.jump = false;
+        self.jump = None;
         let focused_list_state = match self.mode {
             Mode::ObjectSelected => &mut self.object_contents_list_state,
             Mode::EventSelected => &mut self.object_list_state,
             Mode::RootView => &mut self.event_list_state,
         };
         match msg {
-            Message::Down(i, jump) => {
-                focused_list_state.scroll_down_by(i);
-                self.jump = jump;
-            },
-            Message::Up(i, jump) => {
-                focused_list_state.scroll_up_by(i);
-                self.jump = jump;
-            },
+            Message::Down => focused_list_state.select_next(),
+            Message::Up => focused_list_state.select_previous(),
+            Message::PageDown => self.jump = Some(JumpDir::Down),
+            Message::PageUp => self.jump = Some(JumpDir::Up),
 
             Message::Deselect => match self.mode {
                 Mode::ObjectSelected => {
@@ -81,15 +85,17 @@ impl App {
             },
             Message::Select => match self.mode {
                 Mode::EventSelected => {
-                    let i = self.selected_event_index();
+                    let i = self.highlighted_event_index();
                     if !self.annotated_trace.is_empty_at(i) {
                         self.mode = Mode::ObjectSelected;
                         self.object_contents_list_state.select(Some(0));
+                        *self.object_contents_list_state.offset_mut() = 0;
                     }
                 },
                 Mode::RootView => {
                     self.mode = Mode::EventSelected;
                     self.object_list_state.select(Some(0));
+                    *self.object_list_state.offset_mut() = 0;
                 },
                 _ => (),
             },
@@ -102,7 +108,8 @@ impl App {
         false
     }
 
-    pub(super) fn selected_event_index(&self) -> usize {
+    pub(super) fn highlighted_event_index(&self) -> usize {
+        // "selected" in this context means "highlighted" in the xray context
         self.event_list_state.selected().unwrap() // there should always be a selected event
     }
 }

sk-cli/src/xray/event.rs

@@ -6,26 +6,19 @@ use ratatui::crossterm::event::{
     KeyModifiers,
 };
 
-use super::{
-    App,
-    Message,
-};
+use super::Message;
 
 const NO_MOD: KeyModifiers = KeyModifiers::empty();
 
-pub(super) fn handle_event(app: &App) -> anyhow::Result<Message> {
+pub(super) fn handle_event() -> anyhow::Result<Message> {
     if let Event::Key(key) = read()? {
         if key.kind == KeyEventKind::Press {
             return Ok(match (key.code, key.modifiers) {
                 // navigation
-                (KeyCode::Up, NO_MOD) | (KeyCode::Char('k'), NO_MOD) => Message::Up(1, false),
-                (KeyCode::Down, NO_MOD) | (KeyCode::Char('j'), NO_MOD) => Message::Down(1, false),
-                (KeyCode::PageUp, NO_MOD) | (KeyCode::Char('b'), KeyModifiers::CONTROL) => {
-                    Message::Up(app.focused_frame_height - 1, true)
-                },
-                (KeyCode::PageDown, NO_MOD) | (KeyCode::Char('f'), KeyModifiers::CONTROL) => {
-                    Message::Down(app.focused_frame_height - 1, true)
-                },
+                (KeyCode::Up, NO_MOD) | (KeyCode::Char('k'), NO_MOD) => Message::Up,
+                (KeyCode::Down, NO_MOD) | (KeyCode::Char('j'), NO_MOD) => Message::Down,
+                (KeyCode::PageUp, NO_MOD) | (KeyCode::Char('b'), KeyModifiers::CONTROL) => Message::PageUp,
+                (KeyCode::PageDown, NO_MOD) | (KeyCode::Char('f'), KeyModifiers::CONTROL) => Message::PageDown,
 
                 // selection
                 (KeyCode::Char(' '), NO_MOD) => Message::Select,

sk-cli/src/xray/mod.rs

@@ -34,7 +34,7 @@ fn run_loop<B: Backend>(mut term: Terminal<B>, mut app: App) -> EmptyResult {
             app.rebuild_annotated_trace();
         }
         term.draw(|frame| view(&mut app, frame))?;
-        let msg = handle_event(&app)?;
+        let msg = handle_event()?;
         trace_changed = app.update_state(msg);
     }
     Ok(())

sk-cli/src/xray/tests/mod.rs

@@ -1,9 +1,6 @@
 mod app_test;
-mod testutils;
-mod view_test;
 
 use rstest::*;
 
 use super::app::*;
 use super::*;
-use crate::set_snapshot_suffix;

sk-cli/src/xray/tests/view_test.rs

@@ -12,6 +12,7 @@ use crate::validation::{
     AnnotatedTrace,
     ValidationStore,
 };
+use crate::xray::view::jump_list_state;
 
 #[fixture]
 fn test_app(test_validation_store: ValidationStore, mut annotated_trace: AnnotatedTrace) -> App {
@@ -23,6 +24,13 @@ fn test_app(test_validation_store: ValidationStore, mut annotated_trace: Annotat
     }
 }
 
+#[rstest]
+fn test_jump_list_state_down() {
+    let mut list_state = ListState::default();
+    jump_list_state(&mut list_state, JumpDir::Down, 42, 20, false);
+    assert_eq!(list_state.offset(), 20);
+}
+
 #[rstest]
 #[case::first(0)]
 #[case::last(3)]

sk-cli/src/xray/view/mod.rs

@@ -1,5 +1,9 @@
 mod helpers;
 
+use std::cmp::{
+    max,
+    min,
+};
 use std::iter::{
     once,
     repeat,
@@ -11,13 +15,15 @@ use ratatui::widgets::{
     Borders,
     Clear,
     List,
+    ListState,
     Padding,
     Paragraph,
 };
 
 use self::helpers::*;
 use super::app::{
     App,
+    JumpDir,
     Mode,
 };
 
@@ -93,18 +99,17 @@ fn render_event_list(app: &mut App, frame: &mut Frame, layout: Rect) {
     let start_ts = app.annotated_trace.start_ts().unwrap_or(0);
 
     // Add one so the selected event is included on top
+    let hi_index = app.highlighted_event_index();
     let (sel_index_inclusive, sel_event) = match app.mode {
-        Mode::EventSelected | Mode::ObjectSelected => {
-            let sel_index = app.selected_event_index();
-            (sel_index + 1, app.annotated_trace.get_event(sel_index))
-        },
+        Mode::EventSelected | Mode::ObjectSelected => (hi_index + 1, app.annotated_trace.get_event(hi_index)),
         _ => (num_events, None),
     };
 
     let event_spans = app.annotated_trace.iter().map(|event| make_event_spans(event, start_ts));
     let mut top_entries = format_list_entries(event_spans, layout.width as usize);
     let bottom_entries = top_entries.split_off(sel_index_inclusive);
 
+    // chain together the applied and deleted objects with either a "+" or "-" prefix, respectively
     let obj_spans = sel_event.map_or(vec![], |evt| {
         let mut sublist_items = evt
             .data
@@ -116,21 +121,53 @@ fn render_event_list(app: &mut App, frame: &mut Frame, layout: Rect) {
             .map(|(i, (obj, op))| make_object_spans(i, obj, op, evt))
             .peekable();
         if sublist_items.peek().is_none() {
+            // if there are no objects associated with this event, we display an empty span
             format_list_entries(once((Span::default(), Span::default())), layout.width as usize)
         } else {
             format_list_entries(sublist_items, layout.width as usize)
         }
     });
 
+    // Compute the constraint values for the first part of the events list and the objects list
+    // AND ALSO update the selected and offset pointers if we've jumped; we have to interweave
+    // these because we base the jump distance on the currently viewed events and objects
+    let (top_height, mid_height) = if app.mode == Mode::RootView {
+        if let Some(dir) = app.jump {
+            jump_list_state(&mut app.event_list_state, dir, top_entries.len(), layout.height, false);
+        }
+        // If we're in the root view, the number of entries to display is just the total number of
+        // events minus the current view offset.  Since there is no selected event, the number of
+        // middle entries is 0.
+        (top_entries.len().saturating_sub(app.event_list_state.offset()) as u16, 0)
+    } else {
+        // If we've selected an event to view, we have to first compute the height ot the top list
+        // as above; the length of the middle list is either:
+        //   - 1 if the selected event is at the very bottom of the display (which will push the offset of
+        //     the top list up by one)
+        //   - the number of rows between the selected event and the bottom of the display, OR
+        //   - the total number of objects belonging to this event (if they all fit)
+        let th = top_entries.len().saturating_sub(app.event_list_state.offset()) as u16;
+        let mh = min(
+            max(1, layout.height.saturating_sub(th)),
+            obj_spans.len().saturating_sub(app.object_list_state.offset()) as u16,
+        );
+
+        // Once we know how many objects we can display for this event, we can compute the amount
+        // to jump (if any); the key observation that makes this all work is that if we've selected
+        // an event, the length of the top entries is fixed.
+        if app.mode == Mode::EventSelected {
+            if let Some(dir) = app.jump {
+                jump_list_state(&mut app.object_list_state, dir, obj_spans.len(), mh, true);
+            };
+        }
+        (th, mh)
+    };
+
+    // We know how many lines we have; use max constraints here so the lists are next to
+    // each other.  The last one can be min(0) and take up the rest of the space
     let nested_layout = Layout::default()
         .direction(Direction::Vertical)
-        .constraints(vec![
-            // We know how many lines we have; use max constraints here so the lists are next to
-            // each other.  The last one can be min(0) and take up the rest of the space
-            Constraint::Max(top_entries.len() as u16),
-            Constraint::Max(obj_spans.len() as u16),
-            Constraint::Min(0),
-        ])
+        .constraints(vec![Constraint::Max(top_height), Constraint::Max(mid_height), Constraint::Min(0)])
         .split(layout);
 
     let list_part_one = List::new(top_entries)
@@ -141,10 +178,6 @@ fn render_event_list(app: &mut App, frame: &mut Frame, layout: Rect) {
         .highlight_symbol("++ ");
     let list_part_two = List::new(bottom_entries).block(Block::new().padding(Padding::left(LIST_PADDING as u16)));
 
-    if app.jump && sel_index_inclusive - 1 < list_part_one.len() {
-        *app.event_list_state.offset_mut() = sel_index_inclusive - 1;
-    }
-
     frame.render_stateful_widget(list_part_one, nested_layout[0], &mut app.event_list_state);
     frame.render_stateful_widget(sublist, nested_layout[1], &mut app.object_list_state);
     frame.render_widget(list_part_two, nested_layout[2]);
@@ -153,16 +186,61 @@ fn render_event_list(app: &mut App, frame: &mut Frame, layout: Rect) {
 fn render_object(app: &mut App, frame: &mut Frame, layout: Rect) {
     let event_idx = app.event_list_state.selected().unwrap();
     let obj_idx = app.object_list_state.selected().unwrap();
-    let obj_contents_idx = app.object_contents_list_state.selected().unwrap();
 
     let Some(obj) = app.annotated_trace.get_object(event_idx, obj_idx) else {
         return;
     };
     let obj_str = serde_json::to_string_pretty(obj).unwrap();
     let contents = List::new(obj_str.split('\n')).highlight_style(Style::new().bg(Color::Blue));
 
-    if app.jump && obj_contents_idx < contents.len() {
-        *app.object_contents_list_state.offset_mut() = obj_contents_idx;
+    if app.mode == Mode::ObjectSelected {
+        if let Some(dir) = app.jump {
+            jump_list_state(&mut app.object_contents_list_state, dir, contents.len(), layout.height, false);
+        }
     }
+
     frame.render_stateful_widget(contents, layout, &mut app.object_contents_list_state);
 }
+
+fn jump_list_state(list_state: &mut ListState, jump_dir: JumpDir, list_len: usize, view_height: u16, pin_bottom: bool) {
+    // compute how far to jump in the specified direction, given the number of items in the
+    // list that we're trying to display and how much room we have to display them
+    let offset = list_state.offset();
+    let selected = list_state.selected().unwrap();
+
+    let new_pos = match jump_dir {
+        // If we jump down, we increase the selection by the view height - 1, so that the
+        // last-visible item before the jump becomes the first-visible item after the jump
+        JumpDir::Down => (offset + view_height as usize).saturating_sub(1),
+
+        // If we jump up, and we aren't on the first item, just jump to the top of the current
+        // page; otherwise, page up, but subtract one so that the top item before the jump
+        // becomes the bottom item after the jump
+        JumpDir::Up => {
+            if selected != offset {
+                offset
+            } else {
+                // make sure the -1 is _inside_ the saturating_sub, instead of adding 1 _outside_;
+                // otherwise you get weird behaviours when selected == offset == 0.
+                offset.saturating_sub(view_height as usize - 1)
+            }
+        },
+    };
+
+    // we're relying on the ratatui behaviour of adjusting the selected index to be "within bounds"
+    // if we computed something out of bounds above.
+    list_state.select(Some(new_pos));
+
+    // pin_bottom means that we don't want to have empty space at the bottom if we jumped "too
+    // far", so we make a special (smaller) offset computation in this case
+    if pin_bottom && list_len.saturating_sub(new_pos) < view_height as usize {
+        *list_state.offset_mut() = list_len.saturating_sub(view_height as usize);
+    } else if new_pos < list_len {
+        // otherwise, we just set the new offset to the selected item, so that it appears at the
+        // top of the page
+        *list_state.offset_mut() = new_pos;
+    }
+}
+
+#[cfg(test)]
+mod tests;

sk-cli/src/xray/view/tests/mod.rs

@@ -0,0 +1,8 @@
+mod testutils;
+mod view_test;
+
+use rstest::*;
+
+use super::*;
+use crate::set_snapshot_suffix;
+use crate::xray::app::*;