From 30f22929aa0e045e1d5bec8784c24706ba978dbc Mon Sep 17 00:00:00 2001 From: Paul Jolly Date: Thu, 12 Mar 2020 18:55:34 +0000 Subject: [PATCH] cmd/govim: safely handle busy state changes for non-.go buffers (#823) In 8926deca we backed out the changes for tracking all buffer changes. But in so doing we didn't properly handle the situation where the busy state changes and the cursor is in a non-go file. Fix that and add a test to verify we don't regress. Whilst we are at it: * consistently use GOVIM_test_SetUserBusy in our testscript tests * slightly refactor the types/consts used for getting the current cursor position Fixes #821 --- cmd/govim/highlight.go | 32 +++++++++++++------ cmd/govim/main.go | 2 +- .../busy_state_change_non_go_file.txt | 21 ++++++++++++ .../scenario_default/reference_highlight.txt | 10 +++--- cmd/govim/util.go | 20 +++++++----- cmd/govim/vimstate.go | 12 +++---- plugin/govim.vim | 2 +- 7 files changed, 69 insertions(+), 30 deletions(-) create mode 100644 cmd/govim/testdata/scenario_default/busy_state_change_non_go_file.txt diff --git a/cmd/govim/highlight.go b/cmd/govim/highlight.go index ada0dbcf2..15922c305 100644 --- a/cmd/govim/highlight.go +++ b/cmd/govim/highlight.go @@ -121,23 +121,26 @@ func (v *vimstate) redefineHighlights(force bool) error { } func (v *vimstate) referenceHighlight(flags govim.CommandFlags, args ...string) error { - return v.updateReferenceHighlight(true) + return v.updateReferenceHighlight(true, nil) } // removeReferenceHighlight is used to only remove existing reference highlights if the // cursor has moved outside of the range(s) of existing highlights, to avoid flickering -func (v *vimstate) removeReferenceHighlight() error { +func (v *vimstate) removeReferenceHighlight(cursorPos cursorPosition) error { if len(v.currentReferences) == 0 { return nil } - _, pos, err := v.cursorPos() - if err != nil { - return fmt.Errorf("failed to get current position: %v", err) + var pos *types.Point + if b, ok := v.buffers[cursorPos.BufNr]; ok { + point, err := types.PointFromVim(b, cursorPos.Line, cursorPos.Col) + if err == nil { + pos = &point + } } for i := range v.currentReferences { - if !pos.IsWithin(*v.currentReferences[i]) { + if pos == nil || !pos.IsWithin(*v.currentReferences[i]) { v.removeTextProps(types.ReferencesTextPropID) return nil } @@ -145,14 +148,25 @@ func (v *vimstate) removeReferenceHighlight() error { return nil } -func (v *vimstate) updateReferenceHighlight(force bool) error { +// updateReferenceHighlight updates the reference highlighting if the cursor is +// currently in a valid position, i.e. a go file. The cursor position can passed +// in by supplying a non-nil cursorPos. +func (v *vimstate) updateReferenceHighlight(force bool, cursorPos *cursorPosition) error { if !force && (v.config.HighlightReferences == nil || !*v.config.HighlightReferences) { return nil } - b, pos, err := v.cursorPos() + if cursorPos == nil { + v.Parse(v.ChannelExpr(cursorPositionExpr), &cursorPos) + } + b, ok := v.buffers[(*cursorPos).BufNr] + if !ok { + // we are not tracking this buffer... i.e. is not a .go file + return nil + } + pos, err := types.PointFromVim(b, cursorPos.Line, cursorPos.Col) if err != nil { - return fmt.Errorf("failed to get current position: %v", err) + return fmt.Errorf("failed to calculate cursor position in buffer %v from line %v and col %v: %v", b.Num, cursorPos.Line, cursorPos.Col, err) } ctx, cancel := context.WithCancel(context.Background()) diff --git a/cmd/govim/main.go b/cmd/govim/main.go index 7d2b07e50..5f20db5d7 100644 --- a/cmd/govim/main.go +++ b/cmd/govim/main.go @@ -295,7 +295,7 @@ func (g *govimplugin) Init(gg govim.Govim, errCh chan error) error { g.DefineFunction(string(config.FunctionBufChanged), []string{"bufnr", "start", "end", "added", "changes"}, g.vimstate.bufChanged) g.DefineFunction(string(config.FunctionSetConfig), []string{"config"}, g.vimstate.setConfig) g.ChannelExf(`call govim#config#Set("%vFunc", function("%v%v"))`, config.InternalFunctionPrefix, PluginPrefix, config.FunctionSetConfig) - g.DefineFunction(string(config.FunctionSetUserBusy), []string{"isBusy"}, g.vimstate.setUserBusy) + g.DefineFunction(string(config.FunctionSetUserBusy), []string{"isBusy", "cursorPos"}, g.vimstate.setUserBusy) g.DefineFunction(string(config.FunctionPopupSelection), []string{"id", "selected"}, g.vimstate.popupSelection) g.DefineCommand(string(config.CommandReferences), g.vimstate.references) g.DefineCommand(string(config.CommandRename), g.vimstate.rename, govim.NArgsZeroOrOne) diff --git a/cmd/govim/testdata/scenario_default/busy_state_change_non_go_file.txt b/cmd/govim/testdata/scenario_default/busy_state_change_non_go_file.txt new file mode 100644 index 000000000..0542b14d2 --- /dev/null +++ b/cmd/govim/testdata/scenario_default/busy_state_change_non_go_file.txt @@ -0,0 +1,21 @@ +# Test that a busy state change (from busy to not) in a non go file +# succeeds. This ensures we are correctly non trying to handle events +# in non .go files + +vim ex 'call GOVIM_test_SetUserBusy(1)' +vim ex 'e main.go' +vim ex 'e other.txt' +vim ex 'call GOVIM_test_SetUserBusy(0)' + +# Assert that we have received no error (Type: 1) or warning (Type: 2) log messages +# Disabled pending resolution to https://github.com/golang/go/issues/34103 +# errlogmatch -start -count=0 'LogMessage callback: &protocol\.LogMessageParams\{Type:(1|2), Message:".*' + +-- go.mod -- +module mod.com + +go 1.12 +-- main.go -- +package main +-- other.txt -- +hello diff --git a/cmd/govim/testdata/scenario_default/reference_highlight.txt b/cmd/govim/testdata/scenario_default/reference_highlight.txt index b59c0b829..8d6484efd 100644 --- a/cmd/govim/testdata/scenario_default/reference_highlight.txt +++ b/cmd/govim/testdata/scenario_default/reference_highlight.txt @@ -1,20 +1,20 @@ # Tests that references to the identifier at the cursor is being highlighted when the user -# goes idle. Since user idle detection is disabled in tests, GOVIM_internal_SetUserBusy() +# goes idle. Since user idle detection is disabled in tests, GOVIM_test_SetUserBusy() # is invoked directly. vim ex 'e main.go' # Placing the cursor on "foo" should highlight all other occations of foo vim ex 'call cursor(5,5)' -vim ex 'call GOVIM_internal_SetUserBusy(1)' -vim ex 'call GOVIM_internal_SetUserBusy(0)' +vim ex 'call GOVIM_test_SetUserBusy(1)' +vim ex 'call GOVIM_test_SetUserBusy(0)' vimexprwait foo_references.golden 'map(range(1,line(\"$\")), \"prop_list(v:val)\")' # Placing the cursor on "fmt" should highlight all other occations of fmt vim ex 'call cursor(9,2)' -vim ex 'call GOVIM_internal_SetUserBusy(1)' -vim ex 'call GOVIM_internal_SetUserBusy(0)' +vim ex 'call GOVIM_test_SetUserBusy(1)' +vim ex 'call GOVIM_test_SetUserBusy(0)' vim ex 'w' vimexprwait fmt_references.golden 'map(range(1,line(\"$\")), \"prop_list(v:val)\")' diff --git a/cmd/govim/util.go b/cmd/govim/util.go index 735692161..b8a4212ee 100644 --- a/cmd/govim/util.go +++ b/cmd/govim/util.go @@ -24,17 +24,21 @@ func (v *vimstate) currentBufferInfo(expr json.RawMessage) *types.Buffer { return types.NewBuffer(buf.Num, buf.Name, []byte(buf.Contents), buf.Loaded == 1) } +type cursorPosition struct { + BufNr int `json:"bufnr"` + Line int `json:"line"` + Col int `json:"col"` +} + +const cursorPositionExpr = `{"bufnr": bufnr(""), "line": line("."), "col": col(".")}` + func (v *vimstate) cursorPos() (b *types.Buffer, p types.Point, err error) { - var pos struct { - BufNum int `json:"bufnum"` - Line int `json:"line"` - Col int `json:"col"` - } - expr := v.ChannelExpr(`{"bufnum": bufnr(""), "line": line("."), "col": col(".")}`) + var pos cursorPosition + expr := v.ChannelExpr(cursorPositionExpr) v.Parse(expr, &pos) - b, ok := v.buffers[pos.BufNum] + b, ok := v.buffers[pos.BufNr] if !ok { - err = fmt.Errorf("failed to resolve buffer %v", pos.BufNum) + err = fmt.Errorf("failed to resolve buffer %v", pos.BufNr) return } p, err = types.PointFromVim(b, pos.Line, pos.Col) diff --git a/cmd/govim/vimstate.go b/cmd/govim/vimstate.go index 69ed60f89..d6e21bd4a 100644 --- a/cmd/govim/vimstate.go +++ b/cmd/govim/vimstate.go @@ -122,7 +122,7 @@ func (v *vimstate) setConfig(args ...json.RawMessage) (interface{}, error) { // HighlightReferences is now not on - remove existing text properties v.removeTextProps(types.ReferencesTextPropID) } else { - if err := v.updateReferenceHighlight(true); err != nil { + if err := v.updateReferenceHighlight(true, nil); err != nil { return nil, fmt.Errorf("failed to update reference highlight: %v", err) } } @@ -168,15 +168,15 @@ func (v *vimstate) popupSelection(args ...json.RawMessage) (interface{}, error) } func (v *vimstate) setUserBusy(args ...json.RawMessage) (interface{}, error) { - var isBusy int - v.Parse(args[0], &isBusy) - v.userBusy = isBusy != 0 + v.userBusy = v.ParseInt(args[0]) != 0 + var pos cursorPosition + v.Parse(args[1], &pos) if v.userBusy { - return nil, v.removeReferenceHighlight() + return nil, v.removeReferenceHighlight(pos) } - if err := v.updateReferenceHighlight(false); err != nil { + if err := v.updateReferenceHighlight(false, &pos); err != nil { return nil, err } diff --git a/plugin/govim.vim b/plugin/govim.vim index dc03dbbfa..5f8653a5e 100644 --- a/plugin/govim.vim +++ b/plugin/govim.vim @@ -195,7 +195,7 @@ endfunction function s:userBusy(busy) if s:userBusy != a:busy let s:userBusy = a:busy - call GOVIM_internal_SetUserBusy(s:userBusy) + call GOVIM_internal_SetUserBusy(s:userBusy, {"bufnr": bufnr(""), "line": line("."), "col": col(".")}) endif endfunction