Mouse AnyEvent tracking (1003) -- work in progress #3538

AutumnMeowMeow · 2024-08-03T15:41:25Z

PR for more detailed discussion, for #1679

Current bit compiles, passes tests, fails clippy (not my fault, other parts of zellij-utils doing that).

Things to do:

Fix broken mouse Release events
Decide how Motion events will be exposed to zellij code, i.e. which Action:: items will it have?
Implement Motion events
Fix broken mouse text selection / copy-paste
Add CSI ? 1003 support inside the terminal pane
Test Normal tracking in vttest
Test ButtonEvent tracking in vttest
Test AnyEvent tracking in vttest
Fixup other code style issues (I still suck at Rust)

…se button state

AutumnMeowMeow · 2024-08-03T15:52:16Z

@imsnif

You can see my TODOs on top. I will try to minimize commits to this PR so it doesn't constantly spin up the tests.

Initial questions:

Does it make sense what I have here so far? Moving away from a "single button down" --> "held" --> "release" model, instead to "button(s) down" --> "button(s) maybe released or dragged, no buttons, etc." I would like to carry this simpler model down to the terminal/grid layer, replacing the six {left/right/middle}_button_{signal/release_signal} functions with three functions {press/release/motion}. On the plugins layer all the existing Actions could stay as-is so as not to break existing code.
What Action's would you like to see for Motion events? (My vote would be a Action::MouseMotion(point, buttons).)

imsnif · 2024-08-05T15:59:45Z

Hey @AutumnMeowMeow !

The direction looks very good. We do not enforce clippy anywhere, so all good.

* Does it make sense what I have here so far?  Moving away from a "single button down" --> "held" --> "release" model, instead to "button(s) down" --> "button(s) maybe released or dragged, no buttons, etc."  I would like to carry this simpler model down to the terminal/grid layer, replacing the six {left/right/middle}_button_{signal/release_signal} functions with three functions {press/release/motion}.  On the plugins layer all the existing Actions could stay as-is so as not to break existing code.

Sounds perfect. From my side, as long as the plugin API (and especially the protobuf layer) doesn't change we're not breaking anything, so should be good. It sounds like this structure is much more in-tune with the way these events are encoded as seen in your changes, so let's go with it.

* What Action's would you like to see for Motion events?  (My vote would be a Action::MouseMotion(point, buttons).)

Sounds good to me! Note that when you get to serializing these actions to protobuffs, you can just ignore them as unsupported actions here: https://github.com/zellij-org/zellij/blob/main/zellij-utils/src/plugin_api/action.rs#L1295

I'm excited about this feature! Already thinking of how to use it in the UI.

imsnif · 2024-08-05T15:34:58Z

zellij-utils/src/input/mouse.rs

+        // unfortunate side effect of the pre-SGR-encoded X10 mouse
+        // protocol design in which release events don't carry
+        // information about WHICH button(s) were released, so we have
+        // to maintain a wee bit of state in between events.


Honestly, even this is much better than what we've been doing up until now with the HeldMouse stuff.

side terminal(s) in, including protobuf. Removed "held" mouse actions. Currently commented out calls to left/middle/right-click/release -- need to fix this though, as selection/copy-paste are broken too. cargo build/test/run works OK. cargo xtask build/test/run fails, unable to find crate input::mouse.

AutumnMeowMeow · 2024-08-06T20:28:13Z

@imsnif OK most of the core new function is here. :) Some things are broken though, and I could use your pointers. (Also converted this to draft in the futile hope it would not spin up the whole CI stuff on every commit...sigh.)

cargo build/run/test work fine. However cargo xtask build/run/test do not. They cannot see the input::mouse crate, no idea why. Any ideas on how to fix?
I temporarily commented out the other left/right/middle-click functions, which now breaks text selection of course. But along the way I also removed the MouseHold stuff, and see a few decently large functions that got axed in zellij-server-src/tab/mod.rs. Do we actually need those? Or would those functions still work via only LeftClick / LeftRelease / etc.?
Somewhere along the way the mouse position is getting a couple off-by-ones, i.e. not 0-based. I suspect that is an artifact of the find_active_tab stuff I cargo culted from the left_click code, e.g. failing to properly handle the conversion from absolute coordinates returned by termwiz to the relative-pane coordinates needed before sending to the terminal.

Thank you for your help! :-)

AutumnMeowMeow · 2024-08-06T20:40:51Z

zellij-server/src/panes/grid.rs

+                self.mouse_buttons_value_sgr(event),
+                // AZL: Why is event.position not staying 0-based on
+                // both axes?
+                event.position.column(),


I see that I missed the relative coordinate conversion in handle_mouse_event(). I'll fix that.

AutumnMeowMeow · 2024-08-06T20:43:57Z

zellij-server/src/tab/mod.rs

-        }
-        Ok(false) // we shouldn't even get here, but might as well not needlessly render if we do
-    }
-


The three functions above are decently large. Is there logic anywhere else to handle changing the selection buffer size as the mouse is dragged around? If not, then we will need to put that in handle_mouse_event().

AutumnMeowMeow · 2024-08-06T20:46:01Z

zellij-utils/src/input/mouse.rs

+    pub shift: bool,
+    pub alt: bool,
+    pub ctrl: bool,
+


Also FYI that Swing and WIN32 both expose keyboard modifiers with mouse events. Soooo when zellij is running natively on Windows we'll have the infrastructure to do things like Ctrl-Click / Alt-Drag / etc. :-)

Honestly, even Alt + mouse_click would be amazing. I'm thinking of using it for multiple selection of panes.

imsnif · 2024-08-08T12:32:19Z

Hey @AutumnMeowMeow - sorry for the delay in getting to this - I'd like to give this a priority but I had a bit of a hectic day yesterday.

cargo build/run/test work fine. However cargo xtask build/run/test do not. They cannot see the input::mouse crate, no idea why. Any ideas on how to fix?

Brief explanation: this is happening because the Action struct is shared across the webassembly boundary to plugins. Termwiz has some dependency that doesn't play well with wasm (right now I don't fully remember which one) and since the mouse module depends on termwiz, we stub it out here: https://github.com/zellij-org/zellij/blob/main/zellij-utils/src/input/mod.rs#L12

Suggested solution: I think the cleanest way to get around this is to remove the mouse module's dependency on termwiz. I think we can do that if we make the termwiz_mouse_convert and from_termwiz functions independent of the mouse module. We could for example (just from a brief look, maybe you have an idea that makes more sense!) change their signatures to:

fn termwiz_mouse_convert(original_event: &mut MouseEvent, event: &termwiz::input::MouseEvent)
fn from_termwiz(old_event: &mut MouseEvent, event: termwiz::input::MouseEvent) -> MouseEvent

And then place them in the input_handler.rs file where they are used as bare functions. What do you think?

I temporarily commented out the other left/right/middle-click functions, which now breaks text selection of course. But along the way I also removed the MouseHold stuff, and see a few decently large functions that got axed in zellij-server-src/tab/mod.rs. Do we actually need those? Or would those functions still work via only LeftClick / LeftRelease / etc.?

I think we should be able to use these functions as is (maybe even removing the whole is_repeated and last_position logic) with the new motions. Here we control the movement of floating panes and text selection (basically, they detect whether we're holding the mouse on a the frame of a floating pane or inside a terminal pane). Does that answer your question?

Somewhere along the way the mouse position is getting a couple off-by-ones, i.e. not 0-based. I suspect that is an artifact of the find_active_tab stuff I cargo culted from the left_click code, e.g. failing to properly handle the conversion from absolute coordinates returned by termwiz to the relative-pane coordinates needed before sending to the terminal.

I think you found this in your other comment, right?

AutumnMeowMeow · 2024-08-29T20:59:17Z

Refactoring termwiz_mouse_convert and from_termwiz got the builds ~~all working~~ (oops, test is broken), thank you @imsnif ! :-)

A few more glitchy things fixed, and now viola, actual proof of concept:

simplescreenrecorder-2024-08-29_15.46.26.mp4

imsnif · 2024-08-30T06:46:44Z

Very cool @AutumnMeowMeow !! Just a heads up: I'll be on vacation the next two weeks. Will be sure to answer any questions that come up when I get back.

Switch to multi-valued mouse buttons and stub for remembering old mou…

00bf961

…se button state

AutumnMeowMeow temporarily deployed to cachix August 3, 2024 15:41 — with GitHub Actions Inactive

imsnif reviewed Aug 5, 2024

View reviewed changes

AutumnMeowMeow marked this pull request as draft August 6, 2024 20:11

AutumnMeowMeow had a problem deploying to cachix August 6, 2024 20:11 — with GitHub Actions Failure

AutumnMeowMeow commented Aug 6, 2024

View reviewed changes

'cargo xtask build' working using refactored functions.

774241a

AutumnMeowMeow had a problem deploying to cachix August 29, 2024 20:56 — with GitHub Actions Failure

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mouse AnyEvent tracking (1003) -- work in progress #3538

Mouse AnyEvent tracking (1003) -- work in progress #3538

AutumnMeowMeow commented Aug 3, 2024 •

edited

Loading

AutumnMeowMeow commented Aug 3, 2024 •

edited

Loading

imsnif commented Aug 5, 2024

imsnif Aug 5, 2024

AutumnMeowMeow commented Aug 6, 2024

AutumnMeowMeow Aug 6, 2024

AutumnMeowMeow Aug 6, 2024

AutumnMeowMeow Aug 6, 2024

imsnif Aug 8, 2024

imsnif commented Aug 8, 2024

AutumnMeowMeow commented Aug 29, 2024 •

edited

Loading

imsnif commented Aug 30, 2024

Mouse AnyEvent tracking (1003) -- work in progress #3538

Are you sure you want to change the base?

Mouse AnyEvent tracking (1003) -- work in progress #3538

Conversation

AutumnMeowMeow commented Aug 3, 2024 • edited Loading

AutumnMeowMeow commented Aug 3, 2024 • edited Loading

imsnif commented Aug 5, 2024

imsnif Aug 5, 2024

Choose a reason for hiding this comment

AutumnMeowMeow commented Aug 6, 2024

AutumnMeowMeow Aug 6, 2024

Choose a reason for hiding this comment

AutumnMeowMeow Aug 6, 2024

Choose a reason for hiding this comment

AutumnMeowMeow Aug 6, 2024

Choose a reason for hiding this comment

imsnif Aug 8, 2024

Choose a reason for hiding this comment

imsnif commented Aug 8, 2024

AutumnMeowMeow commented Aug 29, 2024 • edited Loading

imsnif commented Aug 30, 2024

AutumnMeowMeow commented Aug 3, 2024 •

edited

Loading

AutumnMeowMeow commented Aug 3, 2024 •

edited

Loading

AutumnMeowMeow commented Aug 29, 2024 •

edited

Loading