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

gui: improve mouse text selection #1805

Merged
merged 4 commits into from
Apr 4, 2022
Merged

Conversation

davidrios
Copy link
Contributor

  • implement mouse press capture between the terminal and UI, so when you
    start selecting text from the terminal the tabs won't activate and
    vice-versa
  • selecting from the top and bottom lines won't scroll the viewport
    anymore, it will only scroll if the mouse is moved out of line bounds
  • change cell selection so that it behaves like text selection usually
    does in other popular software

refs: #1199
refs: #1386
refs: #354
possibly: #1589

* implement mouse press capture between the terminal and UI, so when you
  start selecting text from the terminal the tabs won't activate and
  vice-versa
* selecting from the top and bottom lines won't scroll the viewport
  anymore, it will only scroll if the mouse is moved out of line bounds
* change cell selection so that it behaves like text selection usually
  does in other popular software

refs: wez#1199
refs: wez#1386
refs: wez#354
@davidrios
Copy link
Contributor Author

@wez What do you think of this approach for testing?
davidrios@c8a2764
I moved some things to traits to be able to implement them in testing.

@@ -36,8 +36,8 @@ pub struct MouseEvent {
pub kind: MouseEventKind,
pub x: usize,
pub y: VisibleRowIndex,
pub x_pixel_offset: usize,
pub y_pixel_offset: usize,
pub x_pixel_offset: isize,
Copy link
Owner

Choose a reason for hiding this comment

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

I don't have time for a full review right now, but this caught my eye: why is the type changing? I'm pretty sure that this can never legitimately be negative. @AutumnMeowMeow: can you confirm?

Copy link
Contributor Author

@davidrios davidrios Apr 2, 2022

Choose a reason for hiding this comment

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

The idea is for them to be able to have negative values, to encode information about the mouse leaving the area. For instance, y = 0 means that the cell is the top one, and y pixel offset = -10 means the mouse is 10 pixels above that cell. They'll only have negative values when x/y are = 0.

Choose a reason for hiding this comment

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

(Sorry if incoherent, watching Matrix 4 while drunk... 🍷🔫🔫🔫👭🔫🔫🔫🔫🔫🔫🔫🔫🔫👭🔫🔫🔫🔫🔫🔫🔫🔫🔫💞💞)

The terminal should not be reporting negative values (they are required to be positive in CSI sequences), but at one point xterm itself was reporting negative when the mouse was captured (button down) and dragged above the top-left corner of the text area -- when using SGR-Pixel mode. I didn't report that as a bug, someone else might have. Let's see if I can find that thread.... here: contour-terminal/contour#574 (comment) Of course j4james is the real expert. :-)

Within wezterm, the offset values could work the same whether positive or negative values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that the negative offset is only used internally/where it makes sense, in other places I've clipped it to positive only values, like here:

(event.x * (self.pixel_width / width)) + event.x_pixel_offset.max(0) as usize + 1,

Copy link
Contributor Author

@davidrios davidrios Apr 2, 2022

Choose a reason for hiding this comment

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

On the topic of those offsets, they have an unexpected (to me) behavior, so I'm not sure if it's a bug or could have weird side effects. These are the values reported:

wezterm_cell_reporting

The blue cells are what is reported in the x property, because of the rounding here:

// Round the x coordinate so that we're a bit more forgiving of
// the horizontal position when selecting cells
x.round()
, which is done so mouse selection feel more natural, yet the x_pixel_offset is calculated based on the real cell, so the reported cell, offset are not in sync.

I'm thinking maybe it would be better to always report the real cell and offset, and let the code that's using them do the relevant processing.

Copy link
Owner

Choose a reason for hiding this comment

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

@AutumnMeowMeow: what do you think about these positioning offsets; are they actually what you want to see in the report sent to your app?

@wez
Copy link
Owner

wez commented Apr 2, 2022

What do you think of this approach for testing?

I think it looks good! Just the right amount of trait use IMO.

There are some selection bits in the term subdir that are probably dead code now, but date back to when selection was managed by Terminal rather than the GUI. There might be some stuff there that is worth moving into the gui crate if you'd like to augment selection tests: https://github.com/wez/wezterm/blob/main/term/src/test/selection.rs

@davidrios
Copy link
Contributor Author

davidrios commented Apr 2, 2022

Anyway, I'll create another PR for the tests and some other related changes, so this one is ready for merging (as soon as all discussions are resolved).

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 think this looks good! Thanks!

wezterm-gui/src/termwindow/selection.rs Outdated Show resolved Hide resolved
wezterm-gui/src/termwindow/selection.rs Outdated Show resolved Hide resolved
@davidrios davidrios requested a review from wez April 3, 2022 21:20
@wez wez merged commit c5687ac into wez:main Apr 4, 2022
wez added a commit that referenced this pull request Apr 4, 2022
closes: #1199
closes: #1386
closes: #354
@wez
Copy link
Owner

wez commented Apr 4, 2022

Thanks!

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.

3 participants