Skip to content
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

Add support for OSC 22 control sequence to change mouse cursor shape #6292

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

reykjalin
Copy link

@reykjalin reykjalin commented Oct 18, 2024

Description

  • Adds handling for the OSC 22 control sequence.
  • Follows the example set by Kitty to support CSS cursor values for the OSC 22 parameter.
  • Currently only supports pointer, text, row-resize, ns-resize, col-resize, and ew-resize, based on the currently available MouseCursor options in the codebase. Any other value defaults to MouseCursor::Arrow.

Videos

Running comlink (terminal IRC client) before and after:

Before (9ddca7b) After
Screen.Recording.2024-10-17.at.11.39.06.PM.mov
Screen.Recording.2024-10-22.at.2.31.45.PM.mov

Testing

This requires a terminal program that makes use of OSC 22 codes to change the mouse cursor. The only one I know of and actively use is comlink, but I'm sure there are other TUIs out there that do use the codes, I'm just not aware of them.

Run a terminal program that uses OSC 22 codes and check if they work.

I'll try to make a terminal program to demonstrate at least some of them as I work on this PR. It should make testing easier.

@reykjalin reykjalin changed the title Add support for OSC 22 control sequence to change mouse shape Add support for OSC 22 control sequence to change mouse cursor shape Oct 18, 2024
term/src/terminalstate/mod.rs Outdated Show resolved Hide resolved
term/src/terminalstate/mod.rs Outdated Show resolved Hide resolved
wezterm-client/src/pane/clientpane.rs Outdated Show resolved Hide resolved
wezterm-gui/src/overlay/copy.rs Outdated Show resolved Hide resolved
wezterm-gui/src/overlay/quickselect.rs Outdated Show resolved Hide resolved
Comment on lines +1529 to +1534
MuxNotification::Alert {
alert: Alert::MouseCursorShapeChanged { .. },
..
} => {
// fall through
}
Copy link
Author

Choose a reason for hiding this comment

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

I copied what's done for Alert::PaletteChanged here, not sure if that's correct.

Copy link
Owner

@wez wez left a comment

Choose a reason for hiding this comment

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

I'm surprised that you got to this so soon!
A few suggestions below!

mux/src/pane.rs Outdated Show resolved Hide resolved
term/src/terminalstate/mod.rs Outdated Show resolved Hide resolved
term/src/terminalstate/mod.rs Outdated Show resolved Hide resolved
term/src/terminalstate/mod.rs Outdated Show resolved Hide resolved
term/src/terminalstate/mod.rs Outdated Show resolved Hide resolved
term/src/terminalstate/performer.rs Outdated Show resolved Hide resolved
alert: Alert::MouseCursorShapeChanged,
pane_id,
} => {
// FIXME: Is there a cache that needs to be invalidated like for PaletteChanged?
Copy link
Owner

Choose a reason for hiding this comment

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

There isn't an explicit cache, but you will need to cause the logic in the normal mouse event processing flow to trigger and assess what the cursor should be here, based on its position.

I don't have a great suggestion for this off the top of my head. Most likely will need to add a helper function to figure out if the mouse cursor is over the terminal area and if so, get the shape from the pane identified by pane_id and apply it to the window.

wezterm-gui/src/termwindow/mouseevent.rs Outdated Show resolved Hide resolved
window/Cargo.toml Outdated Show resolved Hide resolved
@reykjalin
Copy link
Author

I'm surprised that you got to this so soon!

You and me both 😅

I think I've addressed most of the feedback, but I'm still seeing the issue where it's like the mouse shape is cached. I'll dig into that some more next week :)

wezterm-gui/src/termwindow/mod.rs Outdated Show resolved Hide resolved
wezterm-gui/src/termwindow/mouseevent.rs Outdated Show resolved Hide resolved
@reykjalin
Copy link
Author

reykjalin commented Oct 21, 2024

I think I figured out the reason the mouse cursor shape gets "stuck" on whatever was last set through the OSC 22 sequence. It's because the TUI doesn't set the OSC22 code back to the default, or doesn't clear it I guess? So once the TUI is closed whichever pointer was active is still stored in mouse_cursor_shape in the termstate, so it keeps getting applied over everything else.

I'm not sure if that's "correct" behavior or not. I also don't know what the right time to clear the OSC 22 setting would be, if that even is the correct approach here. The xterm docs don't seem to specify that, at least not in relation to what the control sequence does.

I'll keep looking into this over the next couple of days.

Comment on lines +843 to +851
} else if let Some(shape) = pane.get_mouse_cursor_shape() {
if pane.is_mouse_grabbed() {
parse_mouse_cursor_shape(shape.as_str())
} else {
// If the mouse is no longer grabbed by a TUI, reset the cursor shape.
pane.clear_mouse_cursor_shape();
MouseCursor::Arrow
}
} else if pane.is_mouse_grabbed() {
Copy link
Author

@reykjalin reykjalin Oct 22, 2024

Choose a reason for hiding this comment

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

As it turns out, this fixed the issue of the mouse cursor shape being wrong. I have absolutely no idea if this is the right way to approach it though.

But basically; if the mouse is grabbed by whichever application is running, we defer to whatever mouse shape is set by any OSC 22 escape sequences.

If we find that a shape has been set by an OSC 22 escape sequence but the mouse isn't grabbed we clear it and just use the default pointer shape instead.

Some thoughts:

  1. I'm realizing that using mouse_cursor_shape for the names of this might be confusing? Should we specify that this is a shape set through an OSC 22 escape sequence somehow?
    • It's more confusing in the GUI layer, less so in the terminal state itself.
    • But it may be useful to use something like osc_22_mouse_cursor_shape instead of just mouse_cursor_shape?
  2. I'm not sure this is the best place to clear the mouse cursor shape. Maybe there's an event sent when the mouse is grabbed and released that would be more appropriate?
  3. As stated earlier; I have no clue if this is the right way to manage OSC 22 control sequences, but at least it behaves the way I expect.
Before this change After this change
Screen.Recording.2024-10-22.at.2.36.17.PM.mov
Screen.Recording.2024-10-22.at.2.31.45.PM.mov

Happy to get some guidance on any improvements here!

@reykjalin reykjalin marked this pull request as ready for review October 22, 2024 18:39
@reykjalin reykjalin requested a review from wez November 1, 2024 03:04
@reykjalin
Copy link
Author

Hi @wez 👋 Just wanted to ask if there's anything I can do to make moving this along easier?

I think the changes here are at least functionally good, so I'd love to help move this along if there's anything I can do to help with that :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants