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

Add support for hlsearch variable #1040

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jphalip
Copy link
Contributor

@jphalip jphalip commented Nov 14, 2024

No description provided.

}
return false
}

Copy link
Member

Choose a reason for hiding this comment

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

This is fine, but personally, I'd prefer an implementation like I outlined in this discussion comment - implement v:hlsearch as a variable with state in VimVariableServiceBase and move state from VimSearchGroup to the variable service.

The v:hlsearch variable would then be the source of truth of whether or not highlights were active, and there wouldn't be a need to check with all editors or introduce a new API.

typeText("n")
enterCommand("echo v:hlsearch")
assertExOutput("1")
}
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned in the other PRs, extracting to a new file and splitting into separate functions would be good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. I suggest we wait until the other PR for the v:register (#1046) variable is available so we can access the new Variable interface.

@AlexPl292 AlexPl292 self-requested a review November 22, 2024 14:44
Copy link
Member

@AlexPl292 AlexPl292 left a comment

Choose a reason for hiding this comment

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

Thank you for your PR!. What do you think about the @ citizenmatt suggestion?

@jphalip
Copy link
Contributor Author

jphalip commented Nov 22, 2024

@AlexPl292 I think I understand overall @citizenmatt's suggestion: Set the highlight status into a boolean value anytime a new highlighting occurs or anytime the highlighting is cleared, so that consulting the value would not require any processing. However, I'm not totally clear on the exact way to implement this. @citizenmatt Could you share a few more details on what you have in mind?

@citizenmatt
Copy link
Member

Sure. It's not a value that we update when a highlight is shown or hidden, it's a value we update in order to show or hide highlights. It's the flag that controls whether the highlights are shown. It defaults to true and we set it to false when we call :nohlsearch.

So, I would replace this read/write property in IjVimSearchGroup:

private var showSearchHighlight: Boolean = injector.globalOptions().hlsearch

with a read-only property, something like:

private val showSearchHighlight: Boolean
  get() = injector.globalOptions().hlsearch && injector.variableService.getNonNullVariableValue(Variable("v", "hlsearch")).asBoolean()

And then whenever we currently set showSearchHighlight, we set the v:hlsearch, either to true or false so this:

override fun clearSearchHighlight() {
showSearchHighlight = false
updateSearchHighlights(false)
}

would be something like:

override fun clearSearchHighlight() {
  injector.variableService.storeVariable(Variable("v", "hlsearch"), false.asVimInt(), ???, ???, ???)
}

And storeVariable would update the flag and then call injector.searchGroup.updateSearchHighlights. This would be a new method added to the VimSearchGroup interface. It's already defined in VimSearchGroupBase, but not the interface.

The only tricky bit is what else to pass to storeVariable. It requires an editor, an execution context and a vim script context, and we don't have those or even need them for Vim variables. I'd be inclined to make them nullable.

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.

4 participants