diff --git a/AGENTS.md b/AGENTS.md index 2cafd9d50..96c5ba758 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -213,6 +213,16 @@ that changes the relevant test(s) or adds new ones to demonstrate the bug, then fix the bug in a follow-up commit. This gives reviewers (and `git bisect`) a clear before/after and proves the test actually exercises the broken code path. +This applies only to defects that existed before the entire branch or branch +stack. Never use the bug-demonstration pattern for a regression introduced by +an earlier commit in the current stack. Fix or rewrite the commit that +introduced the regression so that no commit in the final history contains it. +Put the regression test in a preparatory commit before the introducing commit, +so it guards that commit in the final history. If the test cannot pass before +the feature exists, restructure the implementation or test seam until it can; +if that would require a design tradeoff, stop and discuss it rather than adding +a later demonstration/fix pair. + Use the `EXPECTED` / `ACTUAL` pattern in the bug-demonstrating commit. The test asserts the current (wrong) behavior so it passes on the broken code, with the correct expectation preserved inline as a comment. The fix commit then swaps @@ -255,7 +265,11 @@ If you find yourself reaching for a local variable so that both forms can be expressed against the same receiver, the structure isn't right yet — go back and fix it instead of papering over it with a binding. -Use this pattern only where it makes sense; don't apply it by default. +Use this pattern only where it makes sense; don't apply it by default. Only +ever use it for bugs, never for added features or behavior changes that aren't +bugfixes; it is useful to demonstrate how a bug existed before fixing it, but +it is never useful to demonstrate how a feature didn't exist before implementing +it. ## Unify duplicated logic before you change it diff --git a/pkg/gocui/double_click_test.go b/pkg/gocui/double_click_test.go new file mode 100644 index 000000000..b8d9f5f9c --- /dev/null +++ b/pkg/gocui/double_click_test.go @@ -0,0 +1,34 @@ +package gocui + +import ( + "testing" + + "github.com/gdamore/tcell/v3" + "github.com/stretchr/testify/assert" +) + +func TestMouseReleaseDoesNotBreakDoubleClickDetection(t *testing.T) { + t.Cleanup(resetMouseState) + resetMouseState() + g := newTestGui(t) + view, _ := g.SetView("list", 0, 0, 20, 10, 0) + doubleClicks := []bool{} + assert.NoError(t, g.SetViewClickBinding(&ViewMouseBinding{ + ViewName: "list", + Key: MouseLeft, + Handler: func(opts ViewMouseBindingOpts) error { + doubleClicks = append(doubleClicks, opts.IsDoubleClick) + return nil + }, + })) + + for _, event := range []GocuiEvent{ + gocuiEventFromTcellEvent(tcell.NewEventMouse(view.x0+1, view.y0+1, tcell.ButtonPrimary, tcell.ModNone)), + gocuiEventFromTcellEvent(tcell.NewEventMouse(view.x0+1, view.y0+1, tcell.ButtonNone, tcell.ModNone)), + gocuiEventFromTcellEvent(tcell.NewEventMouse(view.x0+1, view.y0+1, tcell.ButtonPrimary, tcell.ModNone)), + } { + assert.NoError(t, g.onKey(&event)) + } + + assert.Equal(t, []bool{false, true}, doubleClicks) +} diff --git a/pkg/gocui/gui.go b/pkg/gocui/gui.go index 57818960c..be52c8584 100644 --- a/pkg/gocui/gui.go +++ b/pkg/gocui/gui.go @@ -208,7 +208,9 @@ type Gui struct { // busy?" doesn't count itself. currentTask Task - lastHoverView *View + lastHoverView *View + mouseCapture *View + mouseGestureCanceled bool // uiThreadID is the goroutine id of the main event loop, recorded when // MainLoop starts. IsUIThread compares against it. Written once, read from @@ -597,6 +599,12 @@ func (g *Gui) DeleteView(name string) error { for i, v := range g.views { if v.name == name { + if g.mouseCapture == v { + g.CancelMouseCapture() + } + if g.lastHoverView == v { + g.lastHoverView = nil + } g.views = append(g.views[:i], g.views[i+1:]...) return nil } @@ -666,6 +674,24 @@ func (g *Gui) SetViewClickBinding(binding *ViewMouseBinding) error { return nil } +// captureMouse routes subsequent mouse events to view until the mouse button is +// released or CancelMouseCapture is called. +func (g *Gui) captureMouse(view *View) { + g.mouseCapture = view + g.mouseGestureCanceled = false +} + +func (g *Gui) releaseMouseCapture() { + g.mouseCapture = nil +} + +// CancelMouseCapture releases capture and ignores the rest of the physical +// gesture until the mouse button is released. +func (g *Gui) CancelMouseCapture() { + g.releaseMouseCapture() + g.mouseGestureCanceled = true +} + func (g *Gui) SetFocusHandler(handler func(bool) error) { g.focusHandler = handler } @@ -1658,9 +1684,26 @@ func (g *Gui) onKey(ev *GocuiEvent) error { case eventMouse: mx, my := ev.MouseX, ev.MouseY - v, err := g.VisibleViewByPosition(mx, my) - if err != nil { - break + if g.mouseGestureCanceled { + if ev.Key.KeyName() == MouseRelease { + g.mouseGestureCanceled = false + } + return nil + } + // While the mouse is captured, all mouse events go to the view that + // was under the pointer when the button was pressed, even if the + // pointer has since left it; this is what lets drag gestures keep + // acting on the view they started in. + v := g.mouseCapture + if v == nil { + var err error + v, err = g.VisibleViewByPosition(mx, my) + if err != nil { + break + } + } + if ev.Key.KeyName() == MouseRelease { + g.releaseMouseCapture() } // newCx and newCy are relative to the view port, i.e. to the visible area of the view @@ -1704,9 +1747,20 @@ func (g *Gui) onKey(ev *GocuiEvent) error { break } } + if ev.Key.KeyName() == MouseLeft && ev.Key.Mod()&ModMotion == 0 { + g.captureMouse(v) + } - if !IsMouseScrollKey(ev.Key.KeyName()) { - v.SetCursor(newCx, newCy) + if !IsMouseScrollKey(ev.Key.KeyName()) && ev.Key.KeyName() != MouseRelease { + cursorX, cursorY := newCx, newCy + // A captured drag can report positions outside the view; keep the + // view cursor inside its bounds in that case. Handlers still get + // the unclamped position through the binding opts. + if g.mouseCapture != nil { + cursorX = max(0, min(cursorX, v.InnerWidth()-1)) + cursorY = max(0, min(cursorY, v.InnerHeight()-1)) + } + v.SetCursor(cursorX, cursorY) if v.Editable { v.TextArea.SetCursor2D(newX, newY) @@ -1718,7 +1772,9 @@ func (g *Gui) onKey(ev *GocuiEvent) error { } } - if v.Frame && my == v.y0 { + // Only an actual click may activate tabs; a captured drag that + // crosses the tab row must not switch tabs. + if ev.Key.KeyName() == MouseLeft && ev.Key.Mod()&ModMotion == 0 && v.Frame && my == v.y0 { if len(v.Tabs) > 0 { tabIndex := v.GetClickedTabIndex(mx - v.x0) @@ -1773,6 +1829,12 @@ func (g *Gui) recordClickInfo(x, y int, key KeyName, v *View) bool { g.lastClick = nil return false } + // A release ends a gesture but is not a click of its own; it must leave + // the click info of the press that started it alone, or no double click + // could ever be detected. + if key == MouseRelease { + return false + } clickInfo := &clickInfo{ x: x, diff --git a/pkg/gocui/mouse_capture_test.go b/pkg/gocui/mouse_capture_test.go new file mode 100644 index 000000000..eea1e3f9f --- /dev/null +++ b/pkg/gocui/mouse_capture_test.go @@ -0,0 +1,216 @@ +package gocui + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestMouseCaptureRoutesMotionAndReleaseOutsideView(t *testing.T) { + g := newTestGui(t) + view, err := g.SetView("captured", 10, 5, 30, 15, 0) + if err != nil && !errors.Is(err, ErrUnknownView) { + assert.NoError(t, err) + return + } + + received := []ViewMouseBindingOpts{} + for _, binding := range []*ViewMouseBinding{ + { + ViewName: "captured", + Key: MouseLeft, + Modifier: ModMotion, + Handler: func(opts ViewMouseBindingOpts) error { + received = append(received, opts) + return nil + }, + }, + { + ViewName: "captured", + Key: MouseRelease, + Handler: func(opts ViewMouseBindingOpts) error { + assert.Nil(t, g.mouseCapture) + received = append(received, opts) + return nil + }, + }, + } { + assert.NoError(t, g.SetViewClickBinding(binding)) + } + + g.captureMouse(view) + assert.NoError(t, g.onKey(&GocuiEvent{ + Type: eventMouse, + MouseX: 0, + MouseY: 0, + Key: NewKey(MouseLeft, "", ModMotion), + })) + assert.Equal(t, ViewMouseBindingOpts{X: -11, Y: -6, Key: MouseLeft}, received[0]) + assert.Equal(t, 0, view.CursorX()) + assert.Equal(t, 0, view.CursorY()) + + assert.NoError(t, g.onKey(&GocuiEvent{ + Type: eventMouse, + MouseX: 79, + MouseY: 23, + Key: NewKeyName(MouseRelease), + })) + assert.Equal(t, ViewMouseBindingOpts{X: 68, Y: 17, Key: MouseRelease}, received[1]) + assert.Equal(t, 0, view.CursorX()) + assert.Equal(t, 0, view.CursorY()) + assert.Nil(t, g.mouseCapture) +} + +func TestPrimaryMouseDragStaysWithPressedView(t *testing.T) { + g := newTestGui(t) + left, _ := g.SetView("left", 0, 0, 20, 10, 0) + _, _ = g.SetView("right", 21, 0, 41, 10, 0) + + receivedBy := "" + for _, viewName := range []string{"left", "right"} { + assert.NoError(t, g.SetViewClickBinding(&ViewMouseBinding{ + ViewName: viewName, + Key: MouseLeft, + Modifier: ModMotion, + Handler: func(ViewMouseBindingOpts) error { + receivedBy = viewName + return nil + }, + })) + } + + assert.NoError(t, g.onKey(&GocuiEvent{ + Type: eventMouse, + MouseX: left.x0 + 1, + MouseY: left.y0 + 1, + Key: NewKeyName(MouseLeft), + })) + assert.NoError(t, g.onKey(&GocuiEvent{ + Type: eventMouse, + MouseX: 22, + MouseY: 1, + Key: NewKey(MouseLeft, "", ModMotion), + })) + + assert.Equal(t, "left", receivedBy) +} + +func TestPrimaryMouseDragDoesNotActivateTabs(t *testing.T) { + g := newTestGui(t) + view, _ := g.SetView("tabs", 0, 0, 40, 10, 0) + view.Tabs = []string{"first", "second"} + + clickedTabs := []int{} + assert.NoError(t, g.SetTabClickBinding("tabs", func(tabIndex int) error { + clickedTabs = append(clickedTabs, tabIndex) + return nil + })) + + assert.NoError(t, g.onKey(&GocuiEvent{ + Type: eventMouse, + MouseX: view.x0 + 1, + MouseY: view.y0 + 1, + Key: NewKeyName(MouseLeft), + })) + assert.NoError(t, g.onKey(&GocuiEvent{ + Type: eventMouse, + MouseX: view.x0 + 3, + MouseY: view.y0, + Key: NewKey(MouseLeft, "", ModMotion), + })) + + assert.Empty(t, clickedTabs) + + assert.NoError(t, g.onKey(&GocuiEvent{ + Type: eventMouse, + MouseX: view.x0 + 3, + MouseY: view.y0, + Key: NewKeyName(MouseRelease), + })) + assert.NoError(t, g.onKey(&GocuiEvent{ + Type: eventMouse, + MouseX: view.x0 + 3, + MouseY: view.y0, + Key: NewKeyName(MouseLeft), + })) + assert.Equal(t, []int{0}, clickedTabs) +} + +func TestRejectedMouseReleaseClearsCapture(t *testing.T) { + g := newTestGui(t) + view, _ := g.SetView("captured", 0, 0, 20, 10, 0) + g.captureMouse(view) + g.ShouldHandleMouseEvent = func(*View, KeyName) bool { return false } + + assert.NoError(t, g.onKey(&GocuiEvent{ + Type: eventMouse, + MouseX: view.x0 + 1, + MouseY: view.y0 + 1, + Key: NewKeyName(MouseRelease), + })) + + assert.Nil(t, g.mouseCapture) +} + +func TestDeleteViewClearsMouseState(t *testing.T) { + g := newTestGui(t) + view, _ := g.SetView("temporary", 0, 0, 20, 10, 0) + g.captureMouse(view) + g.lastHoverView = view + + assert.NoError(t, g.DeleteView("temporary")) + + assert.Nil(t, g.mouseCapture) + assert.True(t, g.mouseGestureCanceled) + assert.Nil(t, g.lastHoverView) +} + +func TestCancelMouseCaptureSuppressesRemainingGesture(t *testing.T) { + g := newTestGui(t) + left, _ := g.SetView("left", 0, 0, 20, 10, 0) + _, _ = g.SetView("right", 21, 0, 41, 10, 0) + receivedBy := "" + for _, viewName := range []string{"left", "right"} { + assert.NoError(t, g.SetViewClickBinding(&ViewMouseBinding{ + ViewName: viewName, + Key: MouseLeft, + Modifier: ModMotion, + Handler: func(ViewMouseBindingOpts) error { + receivedBy = viewName + return nil + }, + })) + } + + g.captureMouse(left) + g.CancelMouseCapture() + assert.NoError(t, g.onKey(&GocuiEvent{ + Type: eventMouse, + MouseX: 22, + MouseY: 1, + Key: NewKey(MouseLeft, "", ModMotion), + })) + assert.Empty(t, receivedBy) + + assert.NoError(t, g.onKey(&GocuiEvent{ + Type: eventMouse, + MouseX: 22, + MouseY: 1, + Key: NewKeyName(MouseRelease), + })) + assert.NoError(t, g.onKey(&GocuiEvent{ + Type: eventMouse, + MouseX: 22, + MouseY: 1, + Key: NewKeyName(MouseLeft), + })) + assert.NoError(t, g.onKey(&GocuiEvent{ + Type: eventMouse, + MouseX: 23, + MouseY: 1, + Key: NewKey(MouseLeft, "", ModMotion), + })) + + assert.Equal(t, "right", receivedBy) +} diff --git a/pkg/gocui/tcell_driver.go b/pkg/gocui/tcell_driver.go index 885bcbabb..745725993 100644 --- a/pkg/gocui/tcell_driver.go +++ b/pkg/gocui/tcell_driver.go @@ -202,7 +202,6 @@ const ( var ( lastMouseKey tcell.ButtonMask = tcell.ButtonNone - lastMouseMod tcell.ModMask = tcell.ModNone dragState = NOT_DRAGGING lastX = 0 lastY = 0 @@ -366,9 +365,11 @@ func gocuiEventFromTcellEvent(tev tcell.Event) GocuiEvent { // process button events (not wheel events) button &= tcell.ButtonMask(0xff) + newButtonPress := false + buttonReleased := false if button != tcell.ButtonNone && lastMouseKey == tcell.ButtonNone { + newButtonPress = true lastMouseKey = button - lastMouseMod = tev.Modifiers() switch button { case tcell.ButtonPrimary: mouseKey = MouseLeft @@ -386,6 +387,7 @@ func gocuiEventFromTcellEvent(tev tcell.Event) GocuiEvent { switch tev.Buttons() { case tcell.ButtonNone: if lastMouseKey != tcell.ButtonNone { + buttonReleased = true switch lastMouseKey { case tcell.ButtonPrimary: dragState = NOT_DRAGGING @@ -393,14 +395,13 @@ func gocuiEventFromTcellEvent(tev tcell.Event) GocuiEvent { case tcell.ButtonMiddle: default: } - mouseMod = Modifier(lastMouseMod) - lastMouseMod = tcell.ModNone + mouseMod = ModNone lastMouseKey = tcell.ButtonNone } default: } - if !wheeling { + if !wheeling && !buttonReleased { switch dragState { case NOT_DRAGGING: return GocuiEvent{ @@ -410,9 +411,23 @@ func gocuiEventFromTcellEvent(tev tcell.Event) GocuiEvent { } // if we haven't released the left mouse button and we've moved the cursor then we're dragging case MAYBE_DRAGGING: - if x != lastX || y != lastY { - dragState = DRAGGING + if x == lastX && y == lastY { + // Deliver the button press itself, but swallow held-button + // motion events within the same cell: they carry no new + // information, and if they fell through they would be + // delivered with the default MouseRelease key. + if !newButtonPress { + return GocuiEvent{Type: eventNone} + } + break } + // The first movement is already part of the drag; give it the + // same key and modifier as the DRAGGING events below so it + // reaches drag bindings instead of being delivered with the + // default MouseRelease key. + dragState = DRAGGING + mouseMod = ModMotion + mouseKey = MouseLeft case DRAGGING: mouseMod = ModMotion mouseKey = MouseLeft diff --git a/pkg/gocui/tcell_driver_test.go b/pkg/gocui/tcell_driver_test.go new file mode 100644 index 000000000..9038e73ce --- /dev/null +++ b/pkg/gocui/tcell_driver_test.go @@ -0,0 +1,57 @@ +package gocui + +import ( + "testing" + + "github.com/gdamore/tcell/v3" + "github.com/stretchr/testify/assert" +) + +func TestFirstMouseMovementAfterPressIsDragEvent(t *testing.T) { + t.Cleanup(resetMouseState) + resetMouseState() + + pressEvent := gocuiEventFromTcellEvent(tcell.NewEventMouse(1, 2, tcell.ButtonPrimary, tcell.ModNone)) + unchangedHeldEvent := gocuiEventFromTcellEvent(tcell.NewEventMouse(1, 2, tcell.ButtonPrimary, tcell.ModNone)) + dragEvent := gocuiEventFromTcellEvent(tcell.NewEventMouse(1, 3, tcell.ButtonPrimary, tcell.ModNone)) + + assert.Equal(t, eventMouse, pressEvent.Type) + assert.Equal(t, MouseLeft, pressEvent.Key.KeyName()) + assert.Equal(t, ModNone, pressEvent.Key.Mod()) + assert.Equal(t, eventNone, unchangedHeldEvent.Type) + assert.Equal(t, eventMouse, dragEvent.Type) + assert.Equal(t, MouseLeft, dragEvent.Key.KeyName()) + assert.Equal(t, ModMotion, dragEvent.Key.Mod()) +} + +func TestMouseReleaseAfterDragIsMouseEvent(t *testing.T) { + t.Cleanup(resetMouseState) + resetMouseState() + + gocuiEventFromTcellEvent(tcell.NewEventMouse(1, 2, tcell.ButtonPrimary, tcell.ModNone)) + gocuiEventFromTcellEvent(tcell.NewEventMouse(1, 3, tcell.ButtonPrimary, tcell.ModNone)) + releaseEvent := gocuiEventFromTcellEvent(tcell.NewEventMouse(1, 3, tcell.ButtonNone, tcell.ModNone)) + + assert.Equal(t, eventMouse, releaseEvent.Type) + assert.Equal(t, MouseRelease, releaseEvent.Key.KeyName()) +} + +func TestMouseReleaseDoesNotKeepPressModifiers(t *testing.T) { + t.Cleanup(resetMouseState) + resetMouseState() + + gocuiEventFromTcellEvent(tcell.NewEventMouse(1, 2, tcell.ButtonPrimary, tcell.ModAlt)) + gocuiEventFromTcellEvent(tcell.NewEventMouse(1, 3, tcell.ButtonPrimary, tcell.ModAlt)) + releaseEvent := gocuiEventFromTcellEvent(tcell.NewEventMouse(1, 3, tcell.ButtonNone, tcell.ModAlt)) + + assert.Equal(t, eventMouse, releaseEvent.Type) + assert.Equal(t, MouseRelease, releaseEvent.Key.KeyName()) + assert.Equal(t, ModNone, releaseEvent.Key.Mod()) +} + +func resetMouseState() { + lastMouseKey = tcell.ButtonNone + dragState = NOT_DRAGGING + lastX = 0 + lastY = 0 +}