Box.SetMouseCapture does not capture/consume events properly [+PR]

[GiGurra]

At present, it is not possible to consume events in the mouse event capture function. Even if you return nil, the event will not be marked as consumed. This causes (among other things) Application.draw() to not fire properly, and essentially, means that any gui state changes you make inside the capture function, isn't rendered.

The following code (from the Application event loop) shows where the draw call would be skipped when the wrong consume status is returned

case *tcell.EventMouse:
	consumed, isMouseDownAction := a.fireMouseActions(event)
	if consumed {
		a.draw()
	}
	a.lastMouseButtons = event.Buttons()
	if isMouseDownAction {
		a.mouseDownX, a.mouseDownY = event.Position()
	}

The problem/bug exists in Box.WrapMouseHandler, and I have created a PR with suggestion on how to fix this here: #967

As I understand the tview maintainers prefer an issue created together with a PR. Is this correct?

[digitallyserviced]

@GiGurra
While you're use case makes sense for your application, it may not be for others. I would consider this a feature request rather than a bug. The API exposes the needed way to handle draw updates from event handlers.

Also your change is altering the mouseCapture handler's response to consume an event which is not actually consuming the event, it's removing it, and technically not "consumed".

The fact that it does not force a redraw is by design as handlers and other event handling functions may not happen on the main UI thread, or even warrant a redraw when an event is "consumed" or nil'd.

Also your change is altering the mouseCapture handler's response to consume an event which is not actually consuming the event, it's removing it, and technically not "consumed".

While the mouseHandler (which is also definable) supports consuming events using return values.

I believe that what you're trying to implement may be due to how you're handling events whereas it may not be in line with how overriding handlers should be done.

This is the reasoning for the app.QueueUpdateDraw(func(){}) type of functions to inject into the normal update queue UI updates to ensure they occur on the main UI thread.

Main reasoning I believe is essentially a handled mouse event, may not warrant a re-draw. So why force it if there may have been nothing changing? Since you can control the draw, you should do so when it is neccessary.

This is a reason I like tview since it does not take control of event flows opionatedly, but gives you the API to implement what is needed.

[rivo]

In a sense, @digitallyserviced is right that we don't want to assume that an event that is intercepted and not passed on needs to result in the screen being redrawn. Let's say a primitive handles mouse movements and you want to disable them. You wouldn't want the screen to be redrawn every time the user moves the mouse in that case. So this is why I've closed your PR.

To be honest, this is something I simply forgot to implement. The mouse capture function should also return a consumed flag (and a capture primitive). But I cannot simply add this now. Instead, my latest commit introduces a MouseConsumed action which you can return, to indicate that you've consumed an event and that you want the screen to be redrawn. Sure, you could also call Application.Draw() yourself but whenever possible, I want to allow access to this functionality without having to keep a global reference to Application. (It's not always possible but it should be possible in the most common cases.)

As for opening an issue alongside a PR, I would even suggest only opening an issue first, without a PR. (This is mentioned in the contributing guide.) Most PR's I get ignore important details, don't consider negative side effects, introduce unnecessary complexity, or simply lack the elegance I'd like to see in this project. Unfortunately, I have to reject most of them. So it's better to simply open an issue and we'll take it from there.