Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enhanced surround_replace to provide visual feedback #7588

Merged
merged 7 commits into from
Jul 13, 2023

Conversation

alevinval
Copy link
Contributor

When the target character is pressed, the selection is temporarily updated to display the affected positions. It restores the original selection after the operation is cancelled or completed.

@alevinval alevinval marked this pull request as draft July 10, 2023 07:39
@alevinval alevinval changed the title WIP: enhance(sourround_replace): provide visual feedback during the operation enhance(sourround_replace): provide visual feedback during the operation Jul 10, 2023
When the target character is pressed, the selection is temporarily updated
to display the affected positions. It restores the original selection after
the operation is cancelled or completed.
helix-term/src/commands.rs Outdated Show resolved Hide resolved
helix-term/src/commands.rs Outdated Show resolved Hide resolved
@alevinval alevinval changed the title enhance(sourround_replace): provide visual feedback during the operation enhanced surround_replace to provide visual feedback Jul 10, 2023
@alevinval alevinval marked this pull request as ready for review July 10, 2023 10:22
@@ -5115,11 +5115,15 @@ fn surround_replace(cx: &mut Context) {
}
};

let selection = selection.clone();
let ranges: SmallVec<_> = change_pos.iter().map(|&pos| Range::point(pos)).collect();
doc.set_selection(view.id, Selection::new(ranges, 0));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, one last thing: you need to compute the correct primary_idx here or the view will be moved if you have a lot of selections: if you selected all the lines in a very big file and were viewing somewhere further down, an index of 0 would move the view all the way to the top

Something like

let primary = selection.primary();
let (i, _) = ranges.iter().enumerate().find(|_i, range| range.overlaps(primary))...

Selection::new(ranges, i)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I've pushed this change and so far so good during smoke testing, but I wonder if there's a better way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a better way. There should be twice as many matches (surrounding positions) as original selections, so the primary index of this selection is twice the original selection primary index.

@archseer archseer added this to the next milestone Jul 12, 2023
Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good to me, nice visual improvement with very little additional complexity added. Great job!

@pascalkuthe pascalkuthe added E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-review Status: Awaiting review from a maintainer. A-command Area: Commands labels Jul 13, 2023
@archseer archseer merged commit 843ae97 into helix-editor:master Jul 13, 2023
mtoohey31 pushed a commit to mtoohey31/helix that referenced this pull request Jun 2, 2024
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-command Area: Commands E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants