Gocui mouse event fixes (#5854)

Some fixes to gocui mouse event handling; these don't fix current bugs
in lazygit, but they are needed for upcoming work involving drag
gestures.

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
 

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)
+}

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,

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)
+}

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

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
+}