fix scroll being off in branchlist after tab

refactored scroll stuff into new abstraction for reuse

Author: Stephan Dilly

Cargo.lock

@@ -55,6 +55,9 @@ which = "4.1"
 [target.'cfg(not(windows))'.dependencies]
 pprof = { version = "0.4", features = ["flamegraph"], optional = true }
 
+[dev-dependencies]
+pretty_assertions = "0.7"
+
 [badges]
 maintenance = { status = "actively-developed" }
 
@@ -81,4 +84,4 @@ codegen-units = 1
 opt-level = 3
 
 [profile.dev]
-split-debuginfo = "unpacked"
+split-debuginfo = "unpacked"

Cargo.toml

@@ -1,13 +1,14 @@
 use super::{
-    visibility_blocking, CommandBlocking, CommandInfo, Component,
-    DrawableComponent, EventState,
+    utils::scroll_vertical::VerticalScroll, visibility_blocking,
+    CommandBlocking, CommandInfo, Component, DrawableComponent,
+    EventState,
 };
 use crate::{
     components::ScrollType,
     keys::SharedKeyConfig,
     queue::{Action, InternalEvent, NeedsUpdate, Queue},
     strings, try_or_popup,
-    ui::{self, calc_scroll_top, Size},
+    ui::{self, Size},
 };
 use anyhow::Result;
 use asyncgit::{
@@ -37,7 +38,7 @@ pub struct BranchListComponent {
     local: bool,
     visible: bool,
     selection: u16,
-    scroll_top: Cell<usize>,
+    scroll: VerticalScroll,
     current_height: Cell<u16>,
     queue: Queue,
     theme: SharedTheme,
@@ -87,7 +88,6 @@ impl DrawableComponent for BranchListComponent {
                 .split(area);
 
             self.draw_tabs(f, chunks[0]);
-
             self.draw_list(f, chunks[1])?;
         }
 
@@ -280,7 +280,7 @@ impl BranchListComponent {
             local: true,
             visible: false,
             selection: 0,
-            scroll_top: Cell::new(0),
+            scroll: VerticalScroll::new(),
             queue,
             theme,
             key_config,
@@ -381,9 +381,13 @@ impl BranchListComponent {
         width_available: u16,
         height: usize,
     ) -> Text {
+        const UPSTREAM_SYMBOL: char = '\u{2191}';
+        const HEAD_SYMBOL: char = '*';
+        const EMPTY_SYMBOL: char = ' ';
+        const THREE_DOTS: &str = "...";
         const COMMIT_HASH_LENGTH: usize = 8;
         const IS_HEAD_STAR_LENGTH: usize = 3; // "*  "
-        const THREE_DOTS_LENGTH: usize = 3; // "..."
+        const THREE_DOTS_LENGTH: usize = THREE_DOTS.len(); // "..."
 
         let branch_name_length: usize =
             width_available as usize * 40 / 100;
@@ -398,7 +402,7 @@ impl BranchListComponent {
         for (i, displaybranch) in self
             .branches
             .iter()
-            .skip(self.scroll_top.get())
+            .skip(self.scroll.get())
             .take(height)
             .enumerate()
         {
@@ -409,7 +413,7 @@ impl BranchListComponent {
                     commit_message_length
                         .saturating_sub(THREE_DOTS_LENGTH),
                 );
-                commit_message += "...";
+                commit_message += THREE_DOTS;
             }
 
             let mut branch_name = displaybranch.name.clone();
@@ -423,25 +427,26 @@ impl BranchListComponent {
                     )
                     .0
                     .to_string();
-                branch_name += "...";
+                branch_name += THREE_DOTS;
             }
 
             let selected =
-                self.selection as usize - self.scroll_top.get() == i;
+                (self.selection as usize - self.scroll.get()) == i;
 
             let is_head = displaybranch
                 .local_details()
                 .map(|details| details.is_head)
                 .unwrap_or_default();
-            let is_head_str = if is_head { "*" } else { " " };
+            let is_head_str =
+                if is_head { HEAD_SYMBOL } else { EMPTY_SYMBOL };
             let has_upstream_str = if displaybranch
                 .local_details()
                 .map(|details| details.has_upstream)
                 .unwrap_or_default()
             {
-                "\u{2191}"
+                UPSTREAM_SYMBOL
             } else {
-                " "
+                EMPTY_SYMBOL
             };
 
             let span_prefix = Span::styled(
@@ -537,11 +542,11 @@ impl BranchListComponent {
         let height_in_lines = r.height as usize;
         self.current_height.set(height_in_lines.try_into()?);
 
-        self.scroll_top.set(calc_scroll_top(
-            self.scroll_top.get(),
-            height_in_lines,
+        self.scroll.update(
             self.selection as usize,
-        ));
+            self.branches.len(),
+            height_in_lines,
+        );
 
         f.render_widget(
             Paragraph::new(self.get_text(
@@ -558,13 +563,7 @@ impl BranchListComponent {
         r.height += 2;
         r.y = r.y.saturating_sub(1);
 
-        ui::draw_scrollbar(
-            f,
-            r,
-            &self.theme,
-            self.branches.len().saturating_sub(height_in_lines),
-            self.scroll_top.get(),
-        );
+        self.scroll.draw(f, r, &self.theme);
 
         Ok(())
     }

src/components/branchlist.rs

@@ -3,6 +3,7 @@ use unicode_width::UnicodeWidthStr;
 
 pub mod filetree;
 pub mod logitems;
+pub mod scroll_vertical;
 pub mod statustree;
 
 /// macro to simplify running code that might return Err.

src/components/utils/mod.rs

@@ -0,0 +1,88 @@
+use std::cell::Cell;
+
+use tui::{backend::Backend, layout::Rect, Frame};
+
+use crate::ui::{draw_scrollbar, style::SharedTheme};
+
+pub struct VerticalScroll {
+    top: Cell<usize>,
+    max_top: Cell<usize>,
+}
+
+impl VerticalScroll {
+    pub const fn new() -> Self {
+        Self {
+            top: Cell::new(0),
+            max_top: Cell::new(0),
+        }
+    }
+
+    pub fn get(&self) -> usize {
+        self.top.get()
+    }
+
+    pub fn update(
+        &self,
+        selection: usize,
+        selection_max: usize,
+        visual_height: usize,
+    ) -> usize {
+        let new_top = calc_scroll_top(
+            self.get(),
+            visual_height,
+            selection,
+            selection_max,
+        );
+        self.top.set(new_top);
+
+        let new_max = selection_max.saturating_sub(visual_height);
+        self.max_top.set(new_max);
+
+        new_top
+    }
+
+    pub fn draw<B: Backend>(
+        &self,
+        f: &mut Frame<B>,
+        r: Rect,
+        theme: &SharedTheme,
+    ) {
+        draw_scrollbar(
+            f,
+            r,
+            theme,
+            self.max_top.get(),
+            self.top.get(),
+        );
+    }
+}
+
+const fn calc_scroll_top(
+    current_top: usize,
+    height_in_lines: usize,
+    selection: usize,
+    selection_max: usize,
+) -> usize {
+    if selection_max < height_in_lines {
+        return 0;
+    }
+
+    if current_top + height_in_lines <= selection {
+        selection.saturating_sub(height_in_lines) + 1
+    } else if current_top > selection {
+        selection
+    } else {
+        current_top
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use pretty_assertions::assert_eq;
+
+    #[test]
+    fn test_scroll_no_scroll_to_top() {
+        assert_eq!(calc_scroll_top(1, 10, 4, 4), 0);
+    }
+}