From b9c763756a54ea40b7ad975d09f48b7bfa2e4a7e Mon Sep 17 00:00:00 2001 From: Stephan Dilly Date: Sat, 5 Jun 2021 22:52:46 +0200 Subject: [PATCH] fix scroll being off in branchlist after tab refactored scroll stuff into new abstraction for reuse --- Cargo.lock | 1 + Cargo.toml | 5 +- src/components/branchlist.rs | 49 +++++++------- src/components/utils/mod.rs | 1 + src/components/utils/scroll_vertical.rs | 88 +++++++++++++++++++++++++ 5 files changed, 118 insertions(+), 26 deletions(-) create mode 100644 src/components/utils/scroll_vertical.rs diff --git a/Cargo.lock b/Cargo.lock index 42dc679a..8c8969c7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -484,6 +484,7 @@ dependencies = [ "lazy_static", "log", "pprof", + "pretty_assertions", "rayon-core", "ron", "scopeguard", diff --git a/Cargo.toml b/Cargo.toml index 8b809f47..07737944 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -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" \ No newline at end of file diff --git a/src/components/branchlist.rs b/src/components/branchlist.rs index 56c7cf98..ee4a78ab 100644 --- a/src/components/branchlist.rs +++ b/src/components/branchlist.rs @@ -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, + scroll: VerticalScroll, current_height: Cell, 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(()) } diff --git a/src/components/utils/mod.rs b/src/components/utils/mod.rs index c322035f..20c2ad80 100644 --- a/src/components/utils/mod.rs +++ b/src/components/utils/mod.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. diff --git a/src/components/utils/scroll_vertical.rs b/src/components/utils/scroll_vertical.rs new file mode 100644 index 00000000..73105513 --- /dev/null +++ b/src/components/utils/scroll_vertical.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, + max_top: Cell, +} + +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( + &self, + f: &mut Frame, + 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); + } +}