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

Extract SearchHighlightsHelper from SearchGroup #257

Merged
merged 6 commits into from
Dec 4, 2020

Conversation

citizenmatt
Copy link
Member

I want to refactor state management in SearchGroup to allow search state to be per-project, which will give us a chance to correctly handle the h in 'vminfo' (see VIM-2173).

However, SearchGroup is a large god class, which makes it hard to reason about. This PR is the first step, extracting most of the search highlight code out of SearchGroup and introducing SearchHighlightsHelper as a Kotlin helper. It pulls out static methods for adding/removing/updating the search highlights for hlsearch and incsearch. The methods that require current state are still in SearchGroup but defer to SearchHighlightsHelper.

It also introduces a cleaner API for setting the search and command registers which does not require a non-null Editor that doesn't get used.

A further PR(s) will build on this to continue refactoring and extract state management.

@AlexPl292
Copy link
Member

Nice work, thank you!

@AlexPl292 AlexPl292 merged commit 4ea7c42 into JetBrains:master Dec 4, 2020
@@ -115,15 +110,13 @@ public int getLastDir() {
public void resetState() {
lastSearch = lastPattern = lastSubstitute = lastReplace = lastOffset = null;
lastIgnoreSmartCase = false;
lastDir = 0;
lastDir = Direction.UNSET;
Copy link
Member

Choose a reason for hiding this comment

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

@citizenmatt Don't you think we can remove UNSET value? It's used only here (I've checked getLastDir and "This method is used in AceJump integration plugin", it's okay to remove it). Wouldn't it make usage of this enum more convenient (no need to process UNSET value)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good. I'll remove UNSET and getLastDir as part of the next round of changes.

Copy link
Member

Choose a reason for hiding this comment

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

Nono! Don't remove getLastDir! Sorry, I've written it incorrectly, I meant that it's okay to remove UNSET because getLastDir usage in AceJump integration plugin doesn't use 0 value. It would be safe just to have 1 and -1.
But getLastDiris still needed :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ha ha! I read that wrong! I'll make sure to keep it. 😁

@citizenmatt citizenmatt deleted the refactor-searchgroup branch December 4, 2020 10:51
@citizenmatt citizenmatt mentioned this pull request Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants