-
-
Notifications
You must be signed in to change notification settings - Fork 679
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.
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
Mouse AnyEvent tracking (1003) -- work in progress #3538
Conversation
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:
|
Hey @AutumnMeowMeow ! The direction looks very good. We do not enforce clippy anywhere, so all good.
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.
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. |
zellij-utils/src/input/mouse.rs
Outdated
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@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.)
Thank you for your help! :-) |
zellij-server/src/panes/grid.rs
Outdated
self.mouse_buttons_value_sgr(event), | ||
// AZL: Why is event.position not staying 0-based on | ||
// both axes? | ||
event.position.column(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that I missed the relative coordinate conversion in handle_mouse_event(). I'll fix that.
} | ||
Ok(false) // we shouldn't even get here, but might as well not needlessly render if we do | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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().
pub shift: bool, | ||
pub alt: bool, | ||
pub ctrl: bool, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, even Alt
+ mouse_click
would be amazing. I'm thinking of using it for multiple selection of panes.
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.
Brief explanation: this is happening because the Suggested solution: I think the cleanest way to get around this is to remove the 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
I think we should be able to use these functions as is (maybe even removing the whole
I think you found this in your other comment, right? |
Refactoring termwiz_mouse_convert and from_termwiz got the builds A few more glitchy things fixed, and now viola, actual proof of concept: simplescreenrecorder-2024-08-29_15.46.26.mp4 |
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. |
Hey @AutumnMeowMeow - I hope you are well and don't mind the mention. I want to get this merged and released in the next version, as it will complement some of the planned new features very nicely. Is this something you'd like to keep working on? (no pressure - there is no release date for now) |
Very cool stuff here! Looking forward to this 🙂 |
* fix(plugins): various cwd fixes * fix tests
* feat(plugins): rerun_command_pane API * fix tests
* separate saved/runtime structure, kind of working * serializing config * work * work * save config through the configuration screen * work * startup wizard * style(code): cleanups * fix(session): reload config from disk when switching sessions * style(fmt): rustfmt * fix(config): propagate cli config options to screen * style(fmt): rustfmt
* feat(ux): reload config at runtime * style(fmt): rustfmt
* initial draft * working with floating panes as well * use the same method for applying an initial layout to tiled panes * some refactoring * all code paths working with logical positioning fallback! * get tests to compile * get e2e tests to pass * fix e2e remote runner * breadth-first layout sorting * fix some bugs * style(fmt): rustfmt * style(fmt): remove comments
The response to the 2026 mode query was missing a `?` character. The response should be of the format `CSI ? 2026 ; N $ y` where N can be any value in the range 0-4 inclusive. References: https://gist.github.com/christianparpart/d8a62cc1ab659194337d73e399004036 https://vt100.net/docs/vt510-rm/DECRPM.html
After receiving permission from @AutumnMeowMeow - I'm continuing the work on this. Just rebased from |
I spent some time with this and at this point almost everything is working:
Left to do functionality-wise (mostly minor quirks):
Otherwise I'd like to get the tests/formatting working, write some more tests for this, do more robust testing for forwarding these events to the underlying panes (maybe vttest as @AutumnMeowMeow suggested) since I might have inadvertently broken something, and then we can merge so we can start adding really cool UI/UX stuff with these crazy good bits of functionality. |
Right, so - worked on this some more. Everything is mostly fixed and working! More things to do: All in all - not a lot more to do. I'm hoping to have this merged soon. |
Everything is done and working great! 🎉 Once the CI passes I'm going to merge this. If @AutumnMeowMeow has the time and feels like testing this, it would be most welcome (even after the merge - three's still some time before the release). Thanks again for all your hard work! |
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: