Skip to content

Commit 608edd2

Browse files
Copilotcosmincatalin
Copilot
and
cosmincatalin
authored
Fix cursor position jumping to top after update/delete operations (#5)
## Problem When updating or deleting a package, the cursor selection would jump back to the topmost element in the list, making it frustrating to perform multiple operations in succession. This was especially noticeable when working through a list of packages that need updating or when cleaning up multiple packages. ## Root Cause The `apply_filter()` method was unconditionally resetting the list selection to index 0 after filtering. This method was called by `refresh_packages()`, which is invoked after both update and delete operations to ensure the package list is up-to-date. ## Solution This PR implements cursor position preservation for both update and delete operations: ### UPDATE Operation - **Before**: Cursor jumps to the first package in the list - **After**: Cursor stays on the updated package The implementation saves the current selection index before refreshing the package list, then restores it after the refresh is complete. ### DELETE Operation - **Before**: Cursor jumps to the first package in the list - **After**: Cursor moves to the package above the deleted one (or stays at position 0 if deleting the first item) The implementation calculates the appropriate new position (`index - 1` for items after the first, `0` for the first item) before refreshing the package list. ## Implementation Details The changes introduce two new internal methods that allow optional selection preservation: - `apply_filter_with_selection(preserve_selection: Option<usize>)` - Applies filtering while optionally preserving a specific cursor position - `refresh_packages_with_selection(preserve_selection: Option<usize>)` - Refreshes packages while optionally preserving a specific cursor position The original public methods (`apply_filter()` and `refresh_packages()`) remain unchanged and delegate to these new methods, ensuring backward compatibility. ## Testing - All existing tests pass - Code compiles without warnings - Clippy produces no issues - Changes are backward compatible with no breaking changes to the API Fixes #[issue_number] <!-- START COPILOT CODING AGENT SUFFIX --> <details> <summary>Original prompt</summary> > > ---- > > *This section details on the original issue you should resolve* > > <issue_title>Potential improvement</issue_title> > <issue_description>## Issue > When deleting or updating a package, the cursor selection jumps back to the topmost element. > > ## Potential Improvement > UPDATE -> Keep the position on the updated package. > DELETE -> Move the cursor position to the element above the deleted element. > </issue_description> > > ## Comments on the Issue (you are @copilot in this section) > > <comments> > <comment_new><author>@cosmincatalin</author><body> > Sounds reasonable, let me look into it.</body></comment_new> > </comments> > </details> Fixes #3 <!-- START COPILOT CODING AGENT TIPS --> --- ✨ Let Copilot coding agent [set things up for you](https://github.com/cosmincatalin/brew-explorer/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: cosmincatalin <525590+cosmincatalin@users.noreply.github.com> Co-authored-by: Cosmin Catalin SANDA <me@cosminsanda.com>
1 parent 0381592 commit 608edd2

1 file changed

Lines changed: 55 additions & 22 deletions

File tree

‎src/app.rs‎

Lines changed: 55 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -92,12 +92,17 @@ impl App {
9292

9393
/// Refreshes the package list from the repository
9494
pub fn refresh_packages(&mut self) -> Result<()> {
95+
self.refresh_packages_with_selection(None)
96+
}
97+
98+
/// Refreshes the package list from the repository, optionally preserving selection
99+
fn refresh_packages_with_selection(&mut self, preserve_selection: Option<usize>) -> Result<()> {
95100
// Use the new repository method to refresh all packages
96101
self.repository.refresh_all_packages()?;
97102

98103
// Get the refreshed packages from the repository
99104
self.items = self.repository.get_all_packages()?;
100-
self.apply_filter();
105+
self.apply_filter_with_selection(preserve_selection);
101106
self.reset_column_scroll(); // Reset horizontal scrolling on refresh
102107

103108
Ok(())
@@ -440,6 +445,10 @@ impl App {
440445

441446
/// Applies the current search filter
442447
fn apply_filter(&mut self) {
448+
self.apply_filter_with_selection(None);
449+
}
450+
451+
fn apply_filter_with_selection(&mut self, preserve_selection: Option<usize>) {
443452
if self.search_query.is_empty() {
444453
self.filtered_items = self.items.clone();
445454
} else {
@@ -455,11 +464,29 @@ impl App {
455464
.collect();
456465
}
457466

458-
// Reset selection to first item after filtering
459-
if !self.filtered_items.is_empty() {
460-
self.list_state.select(Some(0));
467+
// Apply selection based on preservation request
468+
if let Some(target_index) = preserve_selection {
469+
// Preserve selection at the given index
470+
let max_index = if self.is_searching {
471+
self.filtered_items.len()
472+
} else {
473+
self.items.len()
474+
};
475+
476+
if max_index > 0 {
477+
// Ensure index is within bounds
478+
let clamped_index = target_index.min(max_index - 1);
479+
self.list_state.select(Some(clamped_index));
480+
} else {
481+
self.list_state.select(None);
482+
}
461483
} else {
462-
self.list_state.select(None);
484+
// Default behavior: Reset selection to first item after filtering
485+
if !self.filtered_items.is_empty() {
486+
self.list_state.select(Some(0));
487+
} else {
488+
self.list_state.select(None);
489+
}
463490
}
464491
self.reset_scroll();
465492
}
@@ -608,6 +635,9 @@ impl App {
608635
fn finish_mock_uninstall(&mut self) {
609636
let package_name = self.update_package_name.clone();
610637

638+
// Save current selection before making changes
639+
let current_selection = self.list_state.selected();
640+
611641
self.is_updating = false;
612642
self.is_uninstalling = false;
613643
self.real_update_called = false;
@@ -627,24 +657,23 @@ impl App {
627657
self.filtered_items.retain(|p| p.name != name);
628658
}
629659

630-
// Refresh the entire package list to ensure consistency
631-
if let Err(e) = self.refresh_packages() {
632-
self.add_status_message(format!("⚠️ Failed to refresh package list: {}", e));
633-
}
634-
635-
// Adjust selection if needed
636-
let max_index = if self.is_searching {
637-
self.filtered_items.len()
660+
// Calculate new selection position: move to the item above the deleted one
661+
// If the deleted item was at index 0, stay at 0
662+
// Otherwise, move to index - 1
663+
let new_selection = if let Some(selected) = current_selection {
664+
if selected > 0 {
665+
Some(selected - 1)
666+
} else {
667+
Some(0)
668+
}
638669
} else {
639-
self.items.len()
670+
None
640671
};
641672

642-
if max_index == 0 {
643-
self.list_state.select(None);
644-
} else if let Some(selected) = self.list_state.selected()
645-
&& selected >= max_index
646-
{
647-
self.list_state.select(Some(max_index - 1));
673+
// Refresh the entire package list to ensure consistency
674+
// and apply the new selection
675+
if let Err(e) = self.refresh_packages_with_selection(new_selection) {
676+
self.add_status_message(format!("⚠️ Failed to refresh package list: {}", e));
648677
}
649678

650679
self.add_status_message(format!("✅ Successfully uninstalled {}", name));
@@ -655,6 +684,9 @@ impl App {
655684
fn finish_mock_update(&mut self) {
656685
let package_name = self.update_package_name.clone();
657686

687+
// Save current selection before refreshing
688+
let current_selection = self.list_state.selected();
689+
658690
self.is_updating = false;
659691
self.is_uninstalling = false;
660692
self.real_update_called = false;
@@ -673,8 +705,9 @@ impl App {
673705
self.add_status_message(format!("⚠️ Failed to refresh {}: {}", name, e));
674706
}
675707

676-
// Also refresh the entire package list to ensure consistency
677-
if let Err(e) = self.refresh_packages() {
708+
// Also refresh the entire package list to ensure consistency,
709+
// preserving the cursor position on the updated package
710+
if let Err(e) = self.refresh_packages_with_selection(current_selection) {
678711
self.add_status_message(format!("⚠️ Failed to refresh package list: {}", e));
679712
}
680713
}

0 commit comments

Comments
 (0)