Skip to content

Add onMouseEvent callback #3774

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
cutelisp wants to merge 1 commit into from
Closed

Add onMouseEvent callback #3774

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions internal/action/tab.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,15 @@ func (t *TabList) HandleEvent(event tcell.Event) {
t.Resize()
case *tcell.EventMouse:
mx, my := e.Position()
err := config.RunPluginFn("onMouseEvent",
luar.New(ulua.L, mx),
luar.New(ulua.L, my),
luar.New(ulua.L, e.Buttons()),
luar.New(ulua.L, e.Modifiers()),
Comment on lines +111 to +112
Copy link
Member

@JoeKar JoeKar Jun 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that it is a good idea to export values/definitions from tcell's interface.
Shouldn't we wrap it?

Copy link
Contributor Author

@cutelisp cutelisp Jun 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by wrap it, could you give an example?

Copy link
Member

@JoeKar JoeKar Jun 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By exporting the content of e.Buttons() and e.Modifiers() we implicit export a dependency to the interface of tcell.
If this is OK, then we can go that way, but I've some doubts right now.
The plugins should only have a dependency to micro, but not its dependencies. To solve this we would need to define our own MouseEvent, which we already have (with tcell dependencies too)...
So a new struct could fit the needs and it should include the coordinates, buttons and modifiers. On the other hand this means, that we duplicate this kind of event.

@dmaluka:
What is your opinion about it?

Copy link
Collaborator

@dmaluka dmaluka Jun 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually we already export *tcell.EventMouse to lua in some cases, namely, we pass it to lua functions bound to mouse events (MouseLeft, MouseRight, MouseRightDrag etc), since #2605. Yeah, I'm guilty of that. (Also I'm guilty of not documenting that.)

So, we may debate whether or not it is/was a good idea to directly expose tcell stuff to lua... But since we are already doing that anyway, I think, at least, that simply passing the entire *tcell.EventMouse is in all respects better than passing its components as individual arguments.

...But first of all, I'm not sure we want this new callback in the first place. It seems what we actually want is to pass the mouse info (for example, again, just pass *tcell.EventMouse) to the already existing callbacks of "mouse actions", i.e. to onMousePress, onMouseDrag, onMouseRelease, onMouseMultiCursor, preMousePress, preMouseDrag, preMouseRelease, preMouseMultiCursor.

And that seems easy to implement: #3779

Copy link
Member

@JoeKar JoeKar Jun 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But since we are already doing that anyway, I think, at least, that simply passing the entire *tcell.EventMouse is in all respects better than passing its components as individual arguments.

Agree.

Copy link
Collaborator

@dmaluka dmaluka Jun 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#3779 merged, can we close this PR as unneeded?

Copy link
Contributor Author

@cutelisp cutelisp Jun 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#3779 merged, can we close this PR as unneeded?

Absolutely, thanks for your PR.

)
if err != nil {
screen.TermMessage(err)
}
switch e.Buttons() {
case tcell.Button1:
if my == t.Y && len(t.List) > 1 {
Expand Down