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.

Sign up for GitHub

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

Jump to bottom

More refactoring of SearchGroup #276

Merged
merged 20 commits into from
Mar 31, 2021
Merged

More refactoring of SearchGroup #276

merged 20 commits into from
Mar 31, 2021

Conversation

citizenmatt
Copy link
Member

@citizenmatt citizenmatt commented Feb 26, 2021 •
edited
Loading

This PR continues the work started in #260. It refactors SearchGroup to provide a more appropriate API that handles user initiated search and substitution. SearchGroup maintains (global) state and highlights results. The SearchHelper class is intended as a lower level API that does not require state, and is an appropriate API to use from extensions.

This PR will:

  • Remove unnecessary APIs from SearchGroup and update usages to use SearchHelper or other appropriate APIs.
  • Remove CommandFlags.FLAG_SEARCH_FWD and FLAG_SEARCH_REV. The commands simply pass direction into the SearchGroup APIs
  • Fixes bugs with last used search or substitution patterns. Various bugs with updating the correct pattern at the correct time, and using the correct pattern. Behaviour matches Vim, with added tests
  • Fixes to the multiple-cursors extension to use the appropriate search APIs that don't interact with state or highlights (VIM-2187). Also fixes a few issues with compatibility with the Vim plugin:
    • Matches no longer update state
    • Search highlighting is not updated for the matched occurrences, and stays at last search/substitute pattern
    • Searches are now always case sensitive, like the plugin
    • Asterisks, etc. in selected text are no longer treated as regex wildcards
    • "Select all occurrences" works correctly with 'nowrapscan', so only finds to the end of the file
    • "Select next occurrence" from an existing selection no longer uses word boundary flags
    • Show "no more matches" at correct time
  • Update tests

@AlexPl292 AlexPl292 force-pushed the refactor-searchgroup branch from fc8420e to 7a2b2c5 Compare March 5, 2021 07:36
citizenmatt and others added 18 commits March 5, 2021 10:38
@citizenmatt @AlexPl292
Remove unused search handling from ProcessExEntry
89913ad
@citizenmatt @AlexPl292
Remove unused code to move caret
c067c67
@citizenmatt @AlexPl292
Remove overload that takes a caret
11da530
@citizenmatt @AlexPl292
Remove overload that assumes primary caret offset
d90db81
@citizenmatt @AlexPl292
Replace search command flags with Direction enum
0b64e26
@citizenmatt @AlexPl292
Remove unused count for main search method
47becb2 
This method is the main implementation for '/' and '?' and does not support a count. It is currently being used incorrectly in places that should be using a simpler find helper.
@citizenmatt @AlexPl292
Rename search to processSearchCommand
284e0b4 
The method is doing more than just searching, such as parsing the search command and state management
@citizenmatt @AlexPl292
Introduce processSearchRange
eccdd72
@citizenmatt @AlexPl292
Introduce test method to set state
e5d086d
@citizenmatt @AlexPl292
Rename searchAndReplace processSubstituteCommand
2d853d8
@citizenmatt @AlexPl292
Simplify writing state
e56e944
@citizenmatt @AlexPl292
Remove duplication in invalid command message
d22d0f6
@citizenmatt @AlexPl292
Fix updating last search/sub patterns
3fe2a2e
@citizenmatt @AlexPl292
Remove unnecessary caret move
eb72def
@citizenmatt @AlexPl292
Remove usage of deprecated API
9a10d6f
@citizenmatt @AlexPl292
Stop multiple-cursors affecting search state
26a3974 
Searching no longer uses the high level search APIs that affect state such as saved searches, history and highlighting. Also conforms better to vim-multiple-cursors behaviour:
* Searches are now case sensitive
* Regular expressions in search text are ignored
* "Select all" works with nowrapscan
* Next occurrence based on existing selection no longer uses word boundary flag
* "No more matches" message shown at more appropriate times
@citizenmatt @AlexPl292
Update formatting
16b901e
@AlexPl292
Update after rebase
78bc406
@AlexPl292 AlexPl292 force-pushed the refactor-searchgroup branch from 7a2b2c5 to 78bc406 Compare March 5, 2021 07:38
@AlexPl292
Copy link
Member

Matt, I've rebased your PR on the latest master and found several things:

  1. CommandParser.java was transferred to kotlin, so it's now CommandParser.kt, so I've updated this class in the latest commit.
  2. One of your previous tests fails with this PR. Exactly because of changes in CommandParser. Could you please check it? test goto line with scrolloff

@citizenmatt
Copy link
Member Author

Oh wow. Saved by another PR! I did merge latest and reapplied the changes to CommandParser.kt, but it was a bad fix even in the Java version! The tests added in #275 found it. At least it means I wasn't guilty of pushing without running tests 😄

@AlexPl292 AlexPl292 merged commit eec23d9 into JetBrains:master Mar 31, 2021
@citizenmatt citizenmatt deleted the refactor-searchgroup branch March 31, 2021 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@citizenmatt @AlexPl292