From 0d67ece1f32c0b536a494c9c5f812404d1fa58ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20R=C3=BC=C3=9Fler?= Date: Wed, 28 Jun 2023 09:35:32 +0200 Subject: [PATCH 1/5] Fix file history for all sizes - Replace temporary fix by proper fix - Re-organize code related to setting selection and fetching data to make sure `self.items` is updated whenever necessary --- src/components/file_revlog.rs | 134 +++++++++++++++++----------------- 1 file changed, 66 insertions(+), 68 deletions(-) diff --git a/src/components/file_revlog.rs b/src/components/file_revlog.rs index 6f62a9a192..451a763275 100644 --- a/src/components/file_revlog.rs +++ b/src/components/file_revlog.rs @@ -19,7 +19,6 @@ use asyncgit::{ diff_contains_file, get_commits_info, CommitId, RepoPathRef, }, AsyncDiff, AsyncGitNotification, AsyncLog, DiffParams, DiffType, - FetchStatus, }; use chrono::{DateTime, Local}; use crossbeam_channel::Sender; @@ -123,7 +122,10 @@ impl FileRevlogComponent { &self.sender, Some(filter), )); - self.table_state.get_mut().select(Some(0)); + + self.items.clear(); + self.set_selection(open_request.selection.unwrap_or(0)); + self.show()?; self.diff.focus(false); @@ -146,20 +148,9 @@ impl FileRevlogComponent { /// pub fn update(&mut self) -> Result<()> { if let Some(ref mut git_log) = self.git_log { - let log_changed = - git_log.fetch()? == FetchStatus::Started; - - let table_state = self.table_state.take(); - let start = table_state.selected().unwrap_or(0); - self.table_state.set(table_state); - - if self.items.needs_data(start, git_log.count()?) - || log_changed - { - self.fetch_commits()?; - self.set_open_selection(); - } + git_log.fetch()?; + self.fetch_commits_if_needed()?; self.update_diff()?; } @@ -222,45 +213,18 @@ impl FileRevlogComponent { fn fetch_commits(&mut self) -> Result<()> { if let Some(git_log) = &mut self.git_log { - let table_state = self.table_state.take(); + let offset = self.table_state.get_mut().offset(); let commits = get_commits_info( &self.repo_path.borrow(), - &git_log.get_slice(0, SLICE_SIZE)?, + &git_log.get_slice(offset, SLICE_SIZE)?, self.current_width.get(), ); if let Ok(commits) = commits { - // 2023-04-12 - // - // There is an issue with how windowing works in `self.items` and - // `self.table_state`. Because of that issue, we currently have to pass - // `0` as the first argument to `set_items`. If we did not do that, the - // offset that is kept separately in `self.items` and `self.table_state` - // would get out of sync, resulting in the table showing the wrong rows. - // - // The offset determines the number of rows `render_stateful_widget` - // skips when rendering a table. When `set_items` is called, it clears - // its internal `Vec` of items and sets `index_offset` based on the - // parameter passed. Unfortunately, there is no way for us to pass this - // information, `index_offset`, to `render_stateful_widget`. Because of - // that, `render_stateful_widget` assumes that the rows provided by - // `Table` are 0-indexed while in reality they are - // `index_offset`-indexed. - // - // This fix makes the `FileRevlog` unable to show histories that have - // more than `SLICE_SIZE` items, but since it is broken for larger - // histories anyway, this seems acceptable for the time being. - // - // This issue can only properly be fixed upstream, in `tui-rs`. See - // [tui-issue]. - // - // [gitui-issue]: https://github.com/extrawurst/gitui/issues/1560 - // [tui-issue]: https://github.com/fdehau/tui-rs/issues/626 - self.items.set_items(0, commits, &None); + self.items.set_items(offset, commits, &None); } - self.table_state.set(table_state); self.count_total = git_log.count()?; } @@ -273,7 +237,7 @@ impl FileRevlogComponent { let commit_id = table_state.selected().and_then(|selected| { self.items .iter() - .nth(selected) + .nth(selected.saturating_sub(table_state.offset())) .as_ref() .map(|entry| entry.id) }); @@ -345,10 +309,12 @@ impl FileRevlogComponent { }) } - fn move_selection(&mut self, scroll_type: ScrollType) -> bool { - let mut table_state = self.table_state.take(); - - let old_selection = table_state.selected().unwrap_or(0); + fn move_selection( + &mut self, + scroll_type: ScrollType, + ) -> Result<()> { + let old_selection = + self.table_state.get_mut().selected().unwrap_or(0); let max_selection = self.get_max_selection(); let height_in_items = self.current_height.get() / 2; @@ -372,20 +338,34 @@ impl FileRevlogComponent { self.queue.push(InternalEvent::Update(NeedsUpdate::DIFF)); } - table_state.select(Some(new_selection)); - self.table_state.set(table_state); + self.set_selection(new_selection); + self.fetch_commits_if_needed()?; + + Ok(()) + } + + fn set_selection(&mut self, selection: usize) { + let height_in_items = self.current_height.get() / 2; + let new_offset = selection.saturating_sub(height_in_items); - needs_update + *self.table_state.get_mut().offset_mut() = new_offset; + self.table_state.get_mut().select(Some(selection)); } - fn set_open_selection(&mut self) { - if let Some(selection) = - self.open_request.as_ref().and_then(|req| req.selection) - { - let mut table_state = self.table_state.take(); - table_state.select(Some(selection)); - self.table_state.set(table_state); + fn fetch_commits_if_needed(&mut self) -> Result<()> { + let selection = + self.table_state.get_mut().selected().unwrap_or(0); + let height_in_items = self.current_height.get() / 2; + let new_offset = selection.saturating_sub(height_in_items); + + if self.items.needs_data( + new_offset, + selection.saturating_add(height_in_items), + ) { + self.fetch_commits()?; } + + Ok(()) } fn get_selection(&self) -> Option { @@ -423,10 +403,28 @@ impl FileRevlogComponent { .border_style(self.theme.block(true)), ); - let mut table_state = self.table_state.take(); + let table_state = self.table_state.take(); + // We have to adjust the table state for drawing to account for the fact + // that `self.items` not necessarily starts at index 0. + // + // When a user scrolls down, items outside of the current view are removed + // when new data is fetched. Let’s have a look at an example: if the item at + // index 50 is the first item in the current view and `self.items` has been + // freshly fetched, the current offset is 50 and `self.items[0]` is the item + // at index 50. Subtracting the current offset from the selected index + // yields the correct index in `self.items`, in this case 0. + let mut adjusted_table_state = TableState::default() + .with_selected(table_state.selected().map(|selected| { + selected.saturating_sub(table_state.offset()) + })) + .with_offset(0); f.render_widget(Clear, area); - f.render_stateful_widget(table, area, &mut table_state); + f.render_stateful_widget( + table, + area, + &mut adjusted_table_state, + ); draw_scrollbar( f, @@ -545,12 +543,12 @@ impl Component for FileRevlogComponent { } } else if key_match(key, self.key_config.keys.move_up) { - self.move_selection(ScrollType::Up); + self.move_selection(ScrollType::Up)?; } else if key_match( key, self.key_config.keys.move_down, ) { - self.move_selection(ScrollType::Down); + self.move_selection(ScrollType::Down)?; } else if key_match( key, self.key_config.keys.shift_up, @@ -558,7 +556,7 @@ impl Component for FileRevlogComponent { key, self.key_config.keys.home, ) { - self.move_selection(ScrollType::Home); + self.move_selection(ScrollType::Home)?; } else if key_match( key, self.key_config.keys.shift_down, @@ -566,15 +564,15 @@ impl Component for FileRevlogComponent { key, self.key_config.keys.end, ) { - self.move_selection(ScrollType::End); + self.move_selection(ScrollType::End)?; } else if key_match(key, self.key_config.keys.page_up) { - self.move_selection(ScrollType::PageUp); + self.move_selection(ScrollType::PageUp)?; } else if key_match( key, self.key_config.keys.page_down, ) { - self.move_selection(ScrollType::PageDown); + self.move_selection(ScrollType::PageDown)?; } } From 2f1dcea9044043ae3eb294f022faaf884795c201 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20R=C3=BC=C3=9Fler?= Date: Thu, 10 Aug 2023 16:19:50 +0200 Subject: [PATCH 2/5] Subtract correct offset --- src/components/file_revlog.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/file_revlog.rs b/src/components/file_revlog.rs index 451a763275..b5f38e76e4 100644 --- a/src/components/file_revlog.rs +++ b/src/components/file_revlog.rs @@ -415,7 +415,7 @@ impl FileRevlogComponent { // yields the correct index in `self.items`, in this case 0. let mut adjusted_table_state = TableState::default() .with_selected(table_state.selected().map(|selected| { - selected.saturating_sub(table_state.offset()) + selected.saturating_sub(self.items.index_offset()) })) .with_offset(0); From 21d2eff1265dc5f31d1c443f4eb837f6d48a6e72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20R=C3=BC=C3=9Fler?= Date: Thu, 10 Aug 2023 20:47:13 +0200 Subject: [PATCH 3/5] Subtract correct offset --- src/components/file_revlog.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/components/file_revlog.rs b/src/components/file_revlog.rs index b5f38e76e4..c1dbe1267b 100644 --- a/src/components/file_revlog.rs +++ b/src/components/file_revlog.rs @@ -237,7 +237,10 @@ impl FileRevlogComponent { let commit_id = table_state.selected().and_then(|selected| { self.items .iter() - .nth(selected.saturating_sub(table_state.offset())) + .nth( + selected + .saturating_sub(self.items.index_offset()), + ) .as_ref() .map(|entry| entry.id) }); From 8004749c7c6584a543f149a2ee11b3ca87027c4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20R=C3=BC=C3=9Fler?= Date: Fri, 8 Sep 2023 16:56:17 +0200 Subject: [PATCH 4/5] Sync fetch_commits with needs_data --- src/components/file_revlog.rs | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/src/components/file_revlog.rs b/src/components/file_revlog.rs index c1dbe1267b..ea50145279 100644 --- a/src/components/file_revlog.rs +++ b/src/components/file_revlog.rs @@ -211,18 +211,24 @@ impl FileRevlogComponent { Ok(()) } - fn fetch_commits(&mut self) -> Result<()> { + fn fetch_commits( + &mut self, + new_offset: usize, + new_max_offset: usize, + ) -> Result<()> { if let Some(git_log) = &mut self.git_log { - let offset = self.table_state.get_mut().offset(); + let amount = new_max_offset + .saturating_sub(new_offset) + .max(SLICE_SIZE); let commits = get_commits_info( &self.repo_path.borrow(), - &git_log.get_slice(offset, SLICE_SIZE)?, + &git_log.get_slice(new_offset, amount)?, self.current_width.get(), ); if let Ok(commits) = commits { - self.items.set_items(offset, commits, &None); + self.items.set_items(new_offset, commits, &None); } self.count_total = git_log.count()?; @@ -360,12 +366,11 @@ impl FileRevlogComponent { self.table_state.get_mut().selected().unwrap_or(0); let height_in_items = self.current_height.get() / 2; let new_offset = selection.saturating_sub(height_in_items); + let new_max_offset = + selection.saturating_add(height_in_items); - if self.items.needs_data( - new_offset, - selection.saturating_add(height_in_items), - ) { - self.fetch_commits()?; + if self.items.needs_data(new_offset, new_max_offset) { + self.fetch_commits(new_offset, new_max_offset)?; } Ok(()) From cf1be61ff7e8c90a8fc6ea9eaaf96c5268576bda Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20R=C3=BC=C3=9Fler?= Date: Fri, 8 Sep 2023 20:00:35 +0200 Subject: [PATCH 5/5] Only move viewport if necessary --- src/components/file_revlog.rs | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/src/components/file_revlog.rs b/src/components/file_revlog.rs index ea50145279..6ce4eeace1 100644 --- a/src/components/file_revlog.rs +++ b/src/components/file_revlog.rs @@ -354,8 +354,14 @@ impl FileRevlogComponent { } fn set_selection(&mut self, selection: usize) { - let height_in_items = self.current_height.get() / 2; - let new_offset = selection.saturating_sub(height_in_items); + let height_in_items = + (self.current_height.get().saturating_sub(2)) / 2; + + let offset = *self.table_state.get_mut().offset_mut(); + let min_offset = selection + .saturating_sub(height_in_items.saturating_sub(1)); + + let new_offset = offset.clamp(min_offset, selection); *self.table_state.get_mut().offset_mut() = new_offset; self.table_state.get_mut().select(Some(selection)); @@ -364,13 +370,14 @@ impl FileRevlogComponent { fn fetch_commits_if_needed(&mut self) -> Result<()> { let selection = self.table_state.get_mut().selected().unwrap_or(0); - let height_in_items = self.current_height.get() / 2; - let new_offset = selection.saturating_sub(height_in_items); + let offset = *self.table_state.get_mut().offset_mut(); + let height_in_items = + (self.current_height.get().saturating_sub(2)) / 2; let new_max_offset = selection.saturating_add(height_in_items); - if self.items.needs_data(new_offset, new_max_offset) { - self.fetch_commits(new_offset, new_max_offset)?; + if self.items.needs_data(offset, new_max_offset) { + self.fetch_commits(offset, new_max_offset)?; } Ok(()) @@ -425,7 +432,11 @@ impl FileRevlogComponent { .with_selected(table_state.selected().map(|selected| { selected.saturating_sub(self.items.index_offset()) })) - .with_offset(0); + .with_offset( + table_state + .offset() + .saturating_sub(self.items.index_offset()), + ); f.render_widget(Clear, area); f.render_stateful_widget(