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

Request: mouse mode 1016 (SGR-Pixels) #1457

Closed
ghost opened this issue Dec 24, 2021 · 25 comments
Closed

Request: mouse mode 1016 (SGR-Pixels) #1457

ghost opened this issue Dec 24, 2021 · 25 comments
Labels
enhancement New feature or request

Comments

@ghost
Copy link

ghost commented Dec 24, 2021

Is your feature request related to a problem? Please describe.

I would like to support pixel-based mouse position in my application.

Describe the solution you'd like

I would like SGR-Pixels (mouse mode 1016) to be supported. The difference between this and 1006 mode which is already supported are:

  • Mouse movement by pixel, not cell, triggers an event.
  • The rows/cols reported are in pixels, not cells.
  • DECRQM (CSI ? 1016 $ p) produces a corresponding DECRPM so that applications can detect it.

Describe alternatives you've considered

Two methods to do this are DEC Locator Input (DECLRP) which according to that doc is only supported for REGIS and Tektronix and SGR-Pixels which is supported by xterm and works in normal cases.

Additional context

Other applications desire this too, example: dankamongmen/notcurses#2326

Other features use DECRQM/DECRPM: https://gist.github.com/christianparpart/d8a62cc1ab659194337d73e399004036#feature-detection

I just checked xterm via:

echo -ne '\033[?1003;1016h' ; cat

...and verified that click-drag outside the windows does continue to produce mouse events, i.e. xterm continues reporting for the entire mouse grab.

@ghost ghost added the enhancement New feature or request label Dec 24, 2021
@wez
Copy link
Owner

wez commented Dec 24, 2021

I'd like to see this also.

term/src/input.rs's MouseEvent would need to be extended to support pixel_x and pixel_y coordinates so that that information could be passed through to the terminal, and then I think the compiler will probably tell you where those fields need to be initialized and thus where we need to pass the coordinates down from mouse_event_impl

The DECRQM bits you'd need could be cribbed from similar code:
https://github.com/wez/wezterm/blob/main/term/src/terminalstate/mod.rs#L1174-L1184

Would you like to take a crack at a PR for this?

@ghost
Copy link
Author

ghost commented Dec 24, 2021

I just might, if you haven't gotten to it in the next couple weeks. :-)

I just got it running on my Swing backend, and oooooohhhhh: https://jexer.sourceforge.io/screenshots/cute_mouse.gif

EDIT: ...aaand now for the Xterm backend too: https://gitlab.com/klamonte/jexer/-/raw/master/screenshots/xterm_pixel_mouse.gif?raw=true ;-)

@wez
Copy link
Owner

wez commented Dec 24, 2021

Very cool!

@ghost
Copy link
Author

ghost commented Dec 28, 2021

SGR-Pixels in wezterm:

wezterm_sgrpixels

wez pushed a commit that referenced this issue Jan 5, 2022
wez pushed a commit that referenced this issue Jan 5, 2022
wez added a commit that referenced this issue Jan 5, 2022
wez added a commit that referenced this issue Jan 5, 2022
Multiplexer would error out when mousing over the window!

refs: #1457
@dankamongmen
Copy link
Contributor

wezterm_sgrpixels

nice! @klamonte leads the way once again

@ghost
Copy link
Author

ghost commented Jan 8, 2022

@dankamongmen From your comments in dankamongmen/notcurses#2326, I might have implemented it differently/incorrectly here. Internally for wezterm I put in pixelmouse as an offset_x/offset_y which is recomputed back to a pixel position for 1016 mouse reports. Here is why I did it that way (though it took me a few days to figure it out): #1467 (comment) -- which is also how jexer does it.

It will work fine for x/y >= 0, but not for negative which I did not know that xterm does. (Honestly I didn't expect negative numbers to be in CSI parameters anyway. I've got to fix my keyboard/mouse parser now.) The good news is that 1006 mode is still reported as >= 1 even when the mouse is grabbed by xterm.

@wez Around here

we force the x/y cell position to be >= 0 always ({stuff}.max(0)), which means the x/y_offset won't work as it is now. I think it would be easiest to change ClickPosition to x_column_pixels / y_row_pixels always.

@ghost
Copy link
Author

ghost commented Jan 8, 2022

Gaaa which might mean another codec version change.

Alternative: keep the offset, but set to 0 if actual pixel position is negative. So it would be perhaps incomplete compared to xterm, but not seriously different.

@dankamongmen
Copy link
Contributor

@dankamongmen From your comments in dankamongmen/notcurses#2326, I might have implemented it differently/incorrectly here.

relative to xterm, you mean?

xterm and mlterm seem to be using completely different methods, but wezterm seems to be similar to mlterm. is that expected? note that i'm not yet too sure of these results, so take them with a grain of salt.

@ghost
Copy link
Author

ghost commented Jan 8, 2022

Yup, I meant different from xterm.

@dankamongmen
Copy link
Contributor

so were you consciously emulating MLterm with your protocol (which i dig)? or are the two only superficially similar?

@dankamongmen
Copy link
Contributor

i very much hope we don't have three protocols all calling themselves 1016 out there

@ghost
Copy link
Author

ghost commented Jan 8, 2022

No, it was a mistake on my part. We should match xterm since this feature originated there.

@wez
Copy link
Owner

wez commented Jan 10, 2022

Gaaa which might mean another codec version change.

Don't sweat it: if we need to change it, we need to change it :)

@klamonte I'm not sure I'm following re: negative numbers.

The section of code you highlighted clamps to >= 0 because something elsewhere was doing abs() and we had some weird effects when hovering off the left side of the window at the same height as the tab bar. I'm open to doing this differently, and I'm also fine with switching to straight pixels (even signed pixel values) to feed down in the mouse report!

@ghost
Copy link
Author

ghost commented Jan 10, 2022

@wez (Pssst: --> https://www.youtube.com/watch?v=WElNELJedug ;-) This is with sixel, it goes even prettier/smoother with iTerm2.)

So it seems that we may be A-OK here, and xterm might be buggy. According to @j4james over here CSI parameters are not supposed to be negative.

@wez
Copy link
Owner

wez commented Jan 11, 2022

I think you're saying that wezterm's sixel implementation needs work? :-)

@ghost
Copy link
Author

ghost commented Jan 11, 2022

More like my encoder should get faster, and better. :-) BTW @dankamongmen has been doing some serious cooking, his new sixel encoder is smokin' hot.

@wez
Copy link
Owner

wez commented Jan 11, 2022

I know @dankamongmen has said that wezterm's sixel implementaiton needs work :-p

@dankamongmen
Copy link
Contributor

I know @dankamongmen has said that wezterm's sixel implementaiton needs work :-p

fwiw, i have not been testing wezterm's sixel implementation since it went to supporting kitty by default, as i much prefer that protocol (both in spirit and in code). afaik, the problems seen in #1270 are still present, but i haven't checked in at least a month.

@dankamongmen
Copy link
Contributor

fwiw, i have not been testing wezterm's sixel implementation since it went to supporting kitty by default, as i much prefer that protocol (both in spirit and in code). afaik, the problems seen in #1270 are still present, but i haven't checked in at least a month.

nvm, #1270 is long-fixed. i might have another one open? i might not.

wez added a commit that referenced this issue Jan 12, 2022
@ghost
Copy link
Author

ghost commented Jan 13, 2022

@wez I am seeing 1006 mode reporting for every pixel motion, rather than only motion that crosses a full text cell. Is that something that I broke in the PR?

@wez
Copy link
Owner

wez commented Jan 13, 2022

Ah, probably! There was some logic somewhere in the event path that would avoid sending the event if the cell coordinates didn't change. What is probably best to do is centralize that in the terminal state (eg: store optional last mouse movement coordinates), then use that to avoid sending a duplicate of the last move event based on the granularity of the reporting mode. We'd want to clear that state when the reporting mode changes and on soft reset.

@ghost
Copy link
Author

ghost commented Jan 14, 2022

So you already have last_mouse_move in terminalstate, and I think it has what we need. I'm adding checks in terminalstate/mouse.rs referencing it and running into this. So my dumb I-don't-know-Rust question:

error[E0609]: no field `x` on type `std::option::Option<input::MouseEvent>`
   --> term/src/terminalstate/mouse.rs:198:61
    |
198 |                         && (event.x != self.last_mouse_move.x)

How do I get at self.last_mouse_move?

@wez
Copy link
Owner

wez commented Jan 15, 2022

It depends on what you want to do if there was no previous value.

Option<T> forces you to consider the None case; you have a couple of ways of going about it:

match

match self.last_mouse_move.as_ref() {
Some(last) if *last == event => {
return Ok(());
}
_ => {}
}

uses the match statement to see inside and return if the event was the same in the current code in main. It uses .as_ref() to operate on a reference to the contained data. Omitting as_ref() will cause the match to try to consume the value and the compiler won't like that.

.unwrap_or(value) or .unwrap_or_else(func)

These combinator methods of Option will extract the contained value, but return an alternate value if it isn't set. It could perhaps be made to work on the little snippet from your error message like this, if say it made sense to assume that no recorded mouse coordinate was equivalent to a 0:

 && (event.x != self.last_mouse_move.x.unwrap_or(0))

@ghost
Copy link
Author

ghost commented Jan 15, 2022

@wez Thank you for the pointer, I think this fixes it.

wez pushed a commit that referenced this issue Jan 17, 2022
* #1457 Initial commit compiles but doesn't yet work

* #1457 Fix cell offsets

* #1457 refactor, ClickPosition

* fix nits

* #1457 filter per-pixel motion when not in SGR-Pixels mode

* Much cleaner match condition for filtering mouse events based on encoding protocol
@wez wez closed this as completed Feb 18, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 4, 2023

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants