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

Support multiple selections when "Only In Selection" option is enabled #1088

Merged
merged 1 commit into from Jun 5, 2019

Conversation

Projects
None yet
1 participant
@nathansobo
Copy link
Contributor

commented Jun 4, 2019

Fixes #1010

Description of the Change

Previously, the "Only In Selection" option only supported the most recently created selection rather than all selections. This led to particularly bad results when clicking "Find All" and then "Replace All", because "Find All" would select all of the results, creating multiple selections. Then the search markers would be re-created, but only for the last of the selections. When you then clicked "Replace All", only the last of the selected original results would be replaced due to this being the only marker. In this PR, we now loop over all selections when populating the search markers.

Alternate Designs

In this PR, I'm looping over the selected ranges within the find and replace package and calling findAndMarkAllInRangeSync for each range. This will generate a call into the C++ buffer implementation once for each range, which is probably less optimal than changing the search API in superstring to accept multiple ranges. That would be a much more extensive change, however, and I'm expecting that toggling "Only In Selection" with a huge number of selections is exceedingly rare.

To make sure performance was at least reasonable, I recorded a profile of changing the search pattern with 7849 selections. We only spend about 26ms calling into findAndMarkAllInRangeSync.

Screen Shot 2019-06-04 at 2 02 55 PM

That seems like acceptable performance, especially for a rather rare case.

Possible Drawbacks

None that I can think of.

Other notes

In testing this, I also experimented with the "Find Next" and "Find Previous" commands, which select the next and previous result respectively. When "Only In Selection" is enabled, these commands become pretty much worthless, because obviously only one result is selected. Maybe it would make more sense to ignore the "Only In Selection" option for the purpose of these commands, but since doing so would involve a big change and we haven't gotten complaints about that use case, I'm going to avoid it for now.

@nathansobo nathansobo requested a review from rafeca Jun 4, 2019

@nathansobo nathansobo merged commit 3aa884f into master Jun 5, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nathansobo nathansobo deleted the ns/multiple-selections branch Jun 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.