awesome-gocui / gocui

Minimalist Go package aimed at creating Console User Interfaces.
BSD 3-Clause "New" or "Revised" License
350 stars 39 forks source link

Implemented mouse release handling #110

Closed Kavantix closed 2 years ago

Kavantix commented 2 years ago

This PR adds/fixes a few things:

Kavantix commented 2 years ago

@mjarkk I was thinking about this feature a bit more today and I am not sure anymore whether this is the best solution. Currently the release event is triggered on the view the mouse is at when it is released, however it would be better if we always trigger the release event on the view that triggered the initial down event, this way it will be a lot easier to add keybindings for users.

Also, currently a mouse press can only be detected on a view, it would make more sense if a global mouse key binding would also trigger if no view is under the mouse on the down event.

dankox commented 2 years ago

I'm sorry for the delay, was a bit off during the holidays so didn't have time to check what's going on.

This looks good.
@Kavantix regarding your concern. I was thinking about it and it kinda makes sense, but on the other hand, it could make sense to send that event to the view which the mouse is over (like a drag where the dragged view is not moving, but would be moved after mouse release) for some cases.
However, both case could be implemented with the current implementation, so I guess it's more up to what is more convenient.

Not really sure though. Maybe there could be an option where the user could choose where the MouseRelease event is sent to (the originate view or the target view like gui.MouseReleaseOrigin bool?).

Kavantix commented 2 years ago

@dankox I agree on having the option. The only question then is what should be the default? I still think that having the view from the down event as default makes the most sense

dankox commented 2 years ago

The release on origin view as default make sense. But I'm not sure from "backward compatibility". I'm not even sure if anybody was using it before the tcell version (because in tcell version the MouseRelease probably never worked as intended), but in the original termbox version the MouseRelease was there and was triggered on the view where it was released (assuming from the code).

Anyway, this is kinda tricky question and not sure. @mjarkk what do you think about it?

Kavantix commented 2 years ago

Hmm good point, from that standpoint it should by default use the old behaviour

mjarkk commented 2 years ago

Not really sure though. Maybe there could be an option where the user could choose where the MouseRelease event is sent to (the originate view or the target view like gui.MouseReleaseOrigin bool?).

I don't think we should add such an option.

How about this.. The mouse down and up are triggered on the views they happen on but the mouse event data add an extra property like MouseOriginView with some good documentation describing the property?

Kavantix commented 2 years ago

Hmm that sounds confusing. Let’s move this discussion to a new issue though since either way the behavior that is currently here is backwards compatible with the termbox version. So I think we can merge this.

mjarkk commented 2 years ago

Good point, lets merge this first.