refactor(picker): refactor picker logic and add tests to picker, cli, and events (#57)

* refactor(picker): refactor picker logic and add tests for picker, cli and events

* Update changelog

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

CHANGELOG.md

@@ -8,9 +8,13 @@ All notable changes to this project will be documented in this file.
 
 - Quote file names that contain spaces when printing them to stdout (#51)
 
+### 🚜 Refactor
+
+- *(picker)* Refactor picker logic and add tests for picker, cli and events
+
 ### 📚 Documentation
 
-- Terminal emulators compatibility and good first issues
+- Terminal emulators compatibility and good first issues (#56)
 
 ### ⚙️ Miscellaneous Tasks
 

Makefile

@@ -36,7 +36,7 @@ lint:
 
 fix: format
 	@echo "Fixing $(NAME)"
-	@cargo fix --allow-staged
+	@cargo fix --allow-staged --allow-dirty
 	@make lint
 
 run:

crates/television/cli.rs

@@ -91,3 +91,28 @@ Config directory: {config_dir_path}
 Data directory: {data_dir_path}"
     )
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_from_cli() {
+        let cli = Cli {
+            channel: CliTvChannel::Files,
+            tick_rate: 50.0,
+            frame_rate: 60.0,
+            passthrough_keybindings: Some("q,ctrl-w,ctrl-t".to_string()),
+        };
+
+        let post_processed_cli: PostProcessedCli = cli.into();
+
+        assert_eq!(post_processed_cli.channel, CliTvChannel::Files);
+        assert_eq!(post_processed_cli.tick_rate, 50.0);
+        assert_eq!(post_processed_cli.frame_rate, 60.0);
+        assert_eq!(
+            post_processed_cli.passthrough_keybindings,
+            vec!["q".to_string(), "ctrl-w".to_string(), "ctrl-t".to_string()]
+        );
+    }
+}

crates/television/event.rs

@@ -277,3 +277,194 @@ pub fn convert_raw_event_to_key(event: KeyEvent) -> Key {
         _ => Key::Null,
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crossterm::event::{
+        KeyCode, KeyEvent, KeyEventKind, KeyEventState, KeyModifiers,
+    };
+
+    #[test]
+    fn test_convert_raw_event_to_key() {
+        // character keys
+        let event = KeyEvent {
+            code: KeyCode::Char('a'),
+            modifiers: KeyModifiers::NONE,
+            kind: KeyEventKind::Press,
+            state: KeyEventState::NONE,
+        };
+        assert_eq!(convert_raw_event_to_key(event), Key::Char('a'));
+
+        let event = KeyEvent {
+            code: KeyCode::Char('a'),
+            modifiers: KeyModifiers::CONTROL,
+            kind: KeyEventKind::Press,
+            state: KeyEventState::NONE,
+        };
+        assert_eq!(convert_raw_event_to_key(event), Key::Ctrl('a'));
+
+        let event = KeyEvent {
+            code: KeyCode::Char('a'),
+            modifiers: KeyModifiers::ALT,
+            kind: KeyEventKind::Press,
+            state: KeyEventState::NONE,
+        };
+        assert_eq!(convert_raw_event_to_key(event), Key::Alt('a'));
+
+        let event = KeyEvent {
+            code: KeyCode::Char('a'),
+            modifiers: KeyModifiers::SHIFT,
+            kind: KeyEventKind::Press,
+            state: KeyEventState::NONE,
+        };
+        assert_eq!(convert_raw_event_to_key(event), Key::Char('a'));
+
+        let event = KeyEvent {
+            code: KeyCode::Char(' '),
+            modifiers: KeyModifiers::NONE,
+            kind: KeyEventKind::Press,
+            state: KeyEventState::NONE,
+        };
+        assert_eq!(convert_raw_event_to_key(event), Key::Char(' '));
+
+        let event = KeyEvent {
+            code: KeyCode::Char(' '),
+            modifiers: KeyModifiers::CONTROL,
+            kind: KeyEventKind::Press,
+            state: KeyEventState::NONE,
+        };
+        assert_eq!(convert_raw_event_to_key(event), Key::CtrlSpace);
+
+        let event = KeyEvent {
+            code: KeyCode::Char(' '),
+            modifiers: KeyModifiers::ALT,
+            kind: KeyEventKind::Press,
+            state: KeyEventState::NONE,
+        };
+        assert_eq!(convert_raw_event_to_key(event), Key::AltSpace);
+
+        let event = KeyEvent {
+            code: KeyCode::Char(' '),
+            modifiers: KeyModifiers::SHIFT,
+            kind: KeyEventKind::Press,
+            state: KeyEventState::NONE,
+        };
+        assert_eq!(convert_raw_event_to_key(event), Key::Char(' '));
+
+        let event = KeyEvent {
+            code: KeyCode::Backspace,
+            modifiers: KeyModifiers::NONE,
+            kind: KeyEventKind::Press,
+            state: KeyEventState::NONE,
+        };
+        assert_eq!(convert_raw_event_to_key(event), Key::Backspace);
+
+        let event = KeyEvent {
+            code: KeyCode::Backspace,
+            modifiers: KeyModifiers::CONTROL,
+            kind: KeyEventKind::Press,
+            state: KeyEventState::NONE,
+        };
+        assert_eq!(convert_raw_event_to_key(event), Key::CtrlBackspace);
+
+        let event = KeyEvent {
+            code: KeyCode::Backspace,
+            modifiers: KeyModifiers::ALT,
+            kind: KeyEventKind::Press,
+            state: KeyEventState::NONE,
+        };
+
+        assert_eq!(convert_raw_event_to_key(event), Key::AltBackspace);
+
+        let event = KeyEvent {
+            code: KeyCode::Backspace,
+            modifiers: KeyModifiers::SHIFT,
+            kind: KeyEventKind::Press,
+            state: KeyEventState::NONE,
+        };
+
+        assert_eq!(convert_raw_event_to_key(event), Key::Backspace);
+
+        let event = KeyEvent {
+            code: KeyCode::Delete,
+            modifiers: KeyModifiers::NONE,
+            kind: KeyEventKind::Press,
+            state: KeyEventState::NONE,
+        };
+
+        assert_eq!(convert_raw_event_to_key(event), Key::Delete);
+
+        let event = KeyEvent {
+            code: KeyCode::Delete,
+            modifiers: KeyModifiers::CONTROL,
+            kind: KeyEventKind::Press,
+            state: KeyEventState::NONE,
+        };
+
+        assert_eq!(convert_raw_event_to_key(event), Key::CtrlDelete);
+
+        let event = KeyEvent {
+            code: KeyCode::Delete,
+            modifiers: KeyModifiers::ALT,
+            kind: KeyEventKind::Press,
+            state: KeyEventState::NONE,
+        };
+
+        assert_eq!(convert_raw_event_to_key(event), Key::AltDelete);
+
+        let event = KeyEvent {
+            code: KeyCode::Delete,
+            modifiers: KeyModifiers::SHIFT,
+            kind: KeyEventKind::Press,
+            state: KeyEventState::NONE,
+        };
+
+        assert_eq!(convert_raw_event_to_key(event), Key::Delete);
+
+        let event = KeyEvent {
+            code: KeyCode::Enter,
+            modifiers: KeyModifiers::NONE,
+            kind: KeyEventKind::Press,
+            state: KeyEventState::NONE,
+        };
+
+        assert_eq!(convert_raw_event_to_key(event), Key::Enter);
+
+        let event = KeyEvent {
+            code: KeyCode::Enter,
+            modifiers: KeyModifiers::CONTROL,
+            kind: KeyEventKind::Press,
+            state: KeyEventState::NONE,
+        };
+
+        assert_eq!(convert_raw_event_to_key(event), Key::CtrlEnter);
+
+        let event = KeyEvent {
+            code: KeyCode::Enter,
+            modifiers: KeyModifiers::ALT,
+            kind: KeyEventKind::Press,
+            state: KeyEventState::NONE,
+        };
+
+        assert_eq!(convert_raw_event_to_key(event), Key::AltEnter);
+
+        let event = KeyEvent {
+            code: KeyCode::Enter,
+            modifiers: KeyModifiers::SHIFT,
+            kind: KeyEventKind::Press,
+            state: KeyEventState::NONE,
+        };
+
+        assert_eq!(convert_raw_event_to_key(event), Key::Enter);
+
+        let event = KeyEvent {
+            code: KeyCode::Up,
+            modifiers: KeyModifiers::NONE,
+            kind: KeyEventKind::Press,
+            state: KeyEventState::NONE,
+        };
+
+        assert_eq!(convert_raw_event_to_key(event), Key::Up);
+    }
+}

crates/television/picker.rs

@@ -6,8 +6,7 @@ use television_utils::strings::EMPTY_STRING;
 pub struct Picker {
     pub(crate) state: ListState,
     pub(crate) relative_state: ListState,
-    pub(crate) view_offset: usize,
-    _inverted: bool,
+    inverted: bool,
     pub(crate) input: Input,
 }
 
@@ -22,21 +21,25 @@ impl Picker {
         Self {
             state: ListState::default(),
             relative_state: ListState::default(),
-            view_offset: 0,
-            _inverted: false,
+            inverted: false,
             input: Input::new(EMPTY_STRING.to_string()),
         }
     }
 
+    pub(crate) fn offset(&self) -> usize {
+        self.selected()
+            .unwrap_or(0)
+            .saturating_sub(self.relative_selected().unwrap_or(0))
+    }
+
     pub(crate) fn inverted(mut self) -> Self {
-        self._inverted = !self._inverted;
+        self.inverted = !self.inverted;
         self
     }
 
     pub(crate) fn reset_selection(&mut self) {
         self.state.select(Some(0));
         self.relative_state.select(Some(0));
-        self.view_offset = 0;
     }
 
     pub(crate) fn reset_input(&mut self) {
@@ -60,7 +63,7 @@ impl Picker {
     }
 
     pub(crate) fn select_next(&mut self, total_items: usize, height: usize) {
-        if self._inverted {
+        if self.inverted {
             self._select_prev(total_items, height);
         } else {
             self._select_next(total_items, height);
@@ -68,7 +71,7 @@ impl Picker {
     }
 
     pub(crate) fn select_prev(&mut self, total_items: usize, height: usize) {
-        if self._inverted {
+        if self.inverted {
             self._select_next(total_items, height);
         } else {
             self._select_prev(total_items, height);
@@ -78,38 +81,186 @@ impl Picker {
     fn _select_next(&mut self, total_items: usize, height: usize) {
         let selected = self.selected().unwrap_or(0);
         let relative_selected = self.relative_selected().unwrap_or(0);
-        if selected > 0 {
-            self.select(Some(selected - 1));
-            self.relative_select(Some(relative_selected.saturating_sub(1)));
-            if relative_selected == 0 {
-                self.view_offset = self.view_offset.saturating_sub(1);
-            }
-        } else {
-            self.view_offset =
-                total_items.saturating_sub(height.saturating_sub(2));
-            self.select(Some(total_items.saturating_sub(1)));
-            self.relative_select(Some(height.saturating_sub(3)));
+        self.select(Some(selected.saturating_add(1) % total_items));
+        self.relative_select(Some((relative_selected + 1).min(height)));
+        if self.selected().unwrap() == 0 {
+            self.relative_select(Some(0));
         }
     }
 
     fn _select_prev(&mut self, total_items: usize, height: usize) {
-        let new_index = (self.selected().unwrap_or(0) + 1) % total_items;
-        self.select(Some(new_index));
-        if new_index == 0 {
-            self.view_offset = 0;
-            self.relative_select(Some(0));
-            return;
-        }
-        if self.relative_selected().unwrap_or(0) == height.saturating_sub(3) {
-            self.view_offset += 1;
-            self.relative_select(Some(
-                self.selected().unwrap_or(0).min(height.saturating_sub(3)),
-            ));
-        } else {
-            self.relative_select(Some(
-                (self.relative_selected().unwrap_or(0) + 1)
-                    .min(self.selected().unwrap_or(0)),
-            ));
+        let selected = self.selected().unwrap_or(0);
+        let relative_selected = self.relative_selected().unwrap_or(0);
+        self.select(Some((selected + (total_items - 1)) % total_items));
+        self.relative_select(Some(relative_selected.saturating_sub(1)));
+        if self.selected().unwrap() == total_items - 1 {
+            self.relative_select(Some(height));
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    /// - item 0 S     R *
+    /// - item 1 next    *
+    /// - item 2         * height
+    /// - item 3
+    #[test]
+    fn test_picker_select_next_default() {
+        let mut picker = Picker::default();
+        picker.select(Some(0));
+        picker.relative_select(Some(0));
+        picker.select_next(4, 2);
+        assert_eq!(picker.selected(), Some(1), "selected");
+        assert_eq!(picker.relative_selected(), Some(1), "relative_selected");
+    }
+
+    /// - item 0         *
+    /// - item 1 S     R *
+    /// - item 2 next    * height
+    /// - item 3
+    #[test]
+    fn test_picker_select_next_before_relative_last() {
+        let mut picker = Picker::default();
+        picker.select(Some(1));
+        picker.relative_select(Some(1));
+        picker.select_next(4, 2);
+        assert_eq!(picker.selected(), Some(2), "selected");
+        assert_eq!(picker.relative_selected(), Some(2), "relative_selected");
+    }
+
+    /// - item 0         *
+    /// - item 1         *
+    /// - item 2 S     R * height
+    /// - item 3 next
+    #[test]
+    fn test_picker_select_next_relative_last() {
+        let mut picker = Picker::default();
+        picker.select(Some(2));
+        picker.relative_select(Some(2));
+        picker.select_next(4, 2);
+        assert_eq!(picker.selected(), Some(3), "selected");
+        assert_eq!(picker.relative_selected(), Some(2), "relative_selected");
+    }
+
+    /// - item 0 next    *
+    /// - item 1         *
+    /// - item 2       R * height
+    /// - item 3 S
+    #[test]
+    fn test_picker_select_next_last() {
+        let mut picker = Picker::default();
+        picker.select(Some(3));
+        picker.relative_select(Some(2));
+        picker.select_next(4, 2);
+        assert_eq!(picker.selected(), Some(0), "selected");
+        assert_eq!(picker.relative_selected(), Some(0), "relative_selected");
+    }
+
+    /// - item 0 next   *
+    /// - item 1        *
+    /// - item 2 S    R *
+    ///                 * height
+    #[test]
+    fn test_picker_select_next_less_items_than_height_last() {
+        let mut picker = Picker::default();
+        picker.select(Some(2));
+        picker.relative_select(Some(2));
+        picker.select_next(3, 2);
+        assert_eq!(picker.selected(), Some(0), "selected");
+        assert_eq!(picker.relative_selected(), Some(0), "relative_selected");
+    }
+
+    /// - item 0 prev    *
+    /// - item 1 S     R *
+    /// - item 2         * height
+    /// - item 3
+    #[test]
+    fn test_picker_select_prev_default() {
+        let mut picker = Picker::default();
+        picker.select(Some(1));
+        picker.relative_select(Some(1));
+        picker.select_prev(4, 2);
+        assert_eq!(picker.selected(), Some(0), "selected");
+        assert_eq!(picker.relative_selected(), Some(0), "relative_selected");
+    }
+
+    /// - item 0 S     R *
+    /// - item 1         *        *
+    /// - item 2         * height *
+    /// - item 3 prev             *
+    #[test]
+    fn test_picker_select_prev_first() {
+        let mut picker = Picker::default();
+        picker.select(Some(0));
+        picker.relative_select(Some(0));
+        picker.select_prev(4, 2);
+        assert_eq!(picker.selected(), Some(3), "selected");
+        assert_eq!(picker.relative_selected(), Some(2), "relative_selected");
+    }
+
+    /// - item 0         *
+    /// - item 1         *
+    /// - item 2 prev  R * height
+    /// - item 3 S
+    #[test]
+    fn test_picker_select_prev_relative_trailing() {
+        let mut picker = Picker::default();
+        picker.select(Some(3));
+        picker.relative_select(Some(2));
+        picker.select_prev(4, 2);
+        assert_eq!(picker.selected(), Some(2), "selected");
+        assert_eq!(picker.relative_selected(), Some(1), "relative_selected");
+    }
+
+    /// - item 0         *
+    /// - item 1 prev    *
+    /// - item 2 S     R * height
+    /// - item 3
+    #[test]
+    fn test_picker_select_prev_relative_sync() {
+        let mut picker = Picker::default();
+        picker.select(Some(2));
+        picker.relative_select(Some(2));
+        picker.select_prev(4, 2);
+        assert_eq!(picker.selected(), Some(1), "selected");
+        assert_eq!(picker.relative_selected(), Some(1), "relative_selected");
+    }
+
+    #[test]
+    fn test_picker_offset_default() {
+        let picker = Picker::default();
+        assert_eq!(picker.offset(), 0, "offset");
+    }
+
+    #[test]
+    fn test_picker_offset_none() {
+        let mut picker = Picker::default();
+        picker.select(None);
+        picker.relative_select(None);
+        assert_eq!(picker.offset(), 0, "offset");
+    }
+
+    #[test]
+    fn test_picker_offset() {
+        let mut picker = Picker::default();
+        picker.select(Some(1));
+        picker.relative_select(Some(2));
+        assert_eq!(picker.offset(), 0, "offset");
+    }
+
+    #[test]
+    fn test_picker_inverted() {
+        let mut picker = Picker::default();
+        picker.select(Some(0));
+        picker.relative_select(Some(0));
+        picker.select_next(4, 2);
+        picker = picker.inverted();
+        picker.select_next(4, 2);
+        assert!(picker.inverted, "inverted");
+        assert_eq!(picker.selected(), Some(0), "selected");
+        assert_eq!(picker.relative_selected(), Some(0), "relative_selected");
+    }
+}

crates/television/television.rs

@@ -67,8 +67,8 @@ impl Television {
             ),
             mode: Mode::Channel,
             current_pattern: EMPTY_STRING.to_string(),
-            results_picker: Picker::default(),
-            rc_picker: Picker::default().inverted(),
+            results_picker: Picker::default().inverted(),
+            rc_picker: Picker::default(),
             results_area_height: 0,
             previewer: Previewer::default(),
             preview_scroll: None,
@@ -383,7 +383,7 @@ impl Television {
         // help bar (metadata, keymaps, logo)
         self.draw_help_bar(f, &layout)?;
 
-        self.results_area_height = u32::from(layout.results.height);
+        self.results_area_height = u32::from(layout.results.height - 2); // 2 for the borders
         self.preview_pane_height = layout.preview_window.height;
 
         // top left block: results

crates/television/ui/remote_control.rs

@@ -53,7 +53,7 @@ impl Television {
 
         let entries = self.remote_control.results(
             area.height.saturating_sub(2).into(),
-            u32::try_from(self.rc_picker.view_offset)?,
+            u32::try_from(self.rc_picker.offset())?,
         );
 
         let channel_list = build_results_list(

crates/television/ui/results.rs

@@ -196,7 +196,7 @@ impl Television {
 
         let entries = self.channel.results(
             layout.results.height.saturating_sub(2).into(),
-            u32::try_from(self.results_picker.view_offset)?,
+            u32::try_from(self.results_picker.offset())?,
         );
 
         let results_list = build_results_list(