cmd/govim: Ensure text prop highlights aren't placed when disabled #730
Conversation
There was a problem hiding this comment.
LGTM with a couple of comments/questions. Also, I think we should have a test for the bug to go into https://github.com/govim/govim/tree/master/cmd/govim/testdata/scenario_bugs
| @@ -51,7 +51,7 @@ func (v *vimstate) textpropDefine() error { | |||
| } | |||
|
|
|||
| func (v *vimstate) redefineHighlights(diags []types.Diagnostic, force bool) error { | |||
| if !force && (v.config.HighlightDiagnostics == nil || !*v.config.HighlightDiagnostics) { | |||
| if v.config.HighlightDiagnostics == nil || !*v.config.HighlightDiagnostics { | |||
There was a problem hiding this comment.
It bothers me that force now means something different for quickfix, signs and highlights. Worth fixing now, or would you like to leave that for a later PR?
There was a problem hiding this comment.
As per our slack conversation, I'll leave it as is for now.
When this is merged we'll have the following meaning of force:
- Highlights: Update even if there are no new diagnostics
- Signs: Update even if there are no new diagnostics
- Quickfix: Update even if there are no new diagnostics AND ignore if auto quickfix is disabled via config
| @@ -113,7 +113,18 @@ func (v *vimstate) setConfig(args ...json.RawMessage) (interface{}, error) { | |||
| if !vimconfig.EqualBool(v.config.HighlightDiagnostics, preConfig.HighlightDiagnostics) { | |||
| if v.config.HighlightDiagnostics == nil || !*v.config.HighlightDiagnostics { | |||
| // HighlightDiagnostics is now not on - remove existing text properties | |||
| v.redefineHighlights([]types.Diagnostic{}, true) | |||
| v.BatchStart() | |||
| for bufnr, buf := range v.buffers { | |||
There was a problem hiding this comment.
It also bothers me that we leak this sort of detail into the handling of config changes. Worth fixing now, or would you like to leave that for a later PR?
There was a problem hiding this comment.
That's a good point. I did consider breaking out the feature to a removeHighlights but went with this since it is how it's done for both signs and quickfix entries above.
The config HighlightDiagnostics can be used to disable in-code
highlights. PR #700 accidently broke that feature so that it was
possible to get highlights even when disabled.
This PR ensures that text prop highlights aren't placed when they
are disabled by the config.
Fixes #724