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

Initial mouse support #448

Merged
merged 48 commits into from
Jul 2, 2021
Merged

Initial mouse support #448

merged 48 commits into from
Jul 2, 2021

Conversation

tlinford
Copy link
Contributor

@tlinford tlinford commented May 3, 2021

  • enable mouse support (termion MouseTerminal)
  • handle scroll up and down events

This is a very initial WIP for #175 now, with quite a few things that need discussing in more detail, but I figure it's a start:

  • have mouse support be an option?
  • once mouse reporting is turned on it left clicks are captured, but shift+left click allows text selection; right clicks seem to be forwarded normally, not sure how/if mouse input can be passed through

@imsnif
Copy link
Member

imsnif commented May 6, 2021

Hey @tlinford - happy to see you taking this up!
I haven't yet tried this out or looked at the code, but to some of your concerns:
We have to have the ability to let users mark text with left-click. Ideally the marking should stop at the boundaries of the pane, but this is very much an advanced feature. :)
Do you think it would be possible to still leave text-marking, even if we implement it ourselves?

@tlinford
Copy link
Contributor Author

tlinford commented May 6, 2021

Hi @imsnif ,
After some tinkering I've gotten changing the focused pane on left click working decently, but the code is a bit of a mess.

We can implement text-marking ourselves for sure, but as you say it's going to be an advanced thing with quite a lot of work to get it going
My idea at this point is that one needs to send clicks to the correct pane and the pane can figure out what to do in regards to text selection.
Also have to figure out how to have click events sent to plugins, that way we could change tabs in the tab-bar, or turn modes on/off etc.

Also, are you thinking of having mouse support always enabled, or should it be a toggle?

@imsnif
Copy link
Member

imsnif commented May 7, 2021

We can implement text-marking ourselves for sure, but as you say it's going to be an advanced thing with quite a lot of work to get it going

Right. The issue is that we can't publish a version that doesn't allow text-marking. I think it will kind of render the app unusable (not being able to copy/paste from the terminal). Maybe there's some sort of workaround, or?

Also have to figure out how to have click events sent to plugins, that way we could change tabs in the tab-bar, or turn modes on/off etc.

That would be very cool!

Also, are you thinking of having mouse support always enabled, or should it be a toggle?

IMO it should be always enabled.

@tlinford
Copy link
Contributor Author

tlinford commented May 7, 2021

Right. The issue is that we can't publish a version that doesn't allow text-marking. I think it will kind of render the app unusable (not being able to copy/paste from the terminal). Maybe there's some sort of workaround, or?

Agreed, the only thing that I've found for now is that shift click bypasses mouse capture so that can be used. Haven't found a way to capture the events and let them pass through yet.

@imsnif
Copy link
Member

imsnif commented May 7, 2021

If it's a termion issue, I am very open to looking at other solutions (eg. crossterm) if it helps...

@tlinford
Copy link
Contributor Author

tlinford commented May 8, 2021

I could easily be wrong about this, as I don't really know much about terminal escapes codes, but from what I found it's not really a termion thing. Once the csi sequence escape codes (termion uses csi!("?1000h\x1b[?1002h\x1b[?1015h\x1b[?1006h")) to enable mouse reporting are written, shift click is the only way to inhibit that behaviour.

@imsnif
Copy link
Member

imsnif commented May 10, 2021

I could easily be wrong about this, as I don't really know much about terminal escapes codes, but from what I found it's not really a termion thing. Once the csi sequence escape codes (termion uses csi!("?1000h\x1b[?1002h\x1b[?1015h\x1b[?1006h")) to enable mouse reporting are written, shift click is the only way to inhibit that behaviour.

Good find! I played a little bit with this sequence and it seems that the one part of it that actually stops mouse reporting tot he terminal is [?1002h. The most readable reference I could find to it is in the alacritty source code: https://github.com/alacritty/alacritty/blob/master/alacritty_terminal/src/ansi.rs#L572 which refers to it as ReportCellMouseMotion. Whereas ReportMouseClicks, which is what we want, is [?1000h. So I think this might be a sort of catchall by termion and we could actually do well with just part of it.

So if this is not something that's possible with termion, maybe it's possible with crossterm? Or something else? Or maybe we can somehow hack around this by immediately cancelling the ReportCellMouseLocation to the user's terminal by sending a [?1002l signal back? What say you?

@tlinford tlinford force-pushed the mouse-support branch 2 times, most recently from ba66136 to f030bf6 Compare May 19, 2021 19:20
@tlinford tlinford force-pushed the mouse-support branch 5 times, most recently from 54fabf5 to 3fb73fb Compare May 26, 2021 19:07
@tlinford
Copy link
Contributor Author

Hey @imsnif,
things are still a bit rough, but for now we have:

  • select active pane with mouse
  • scroll pane content with mouse wheel
  • select text within a pane, with initial copy to clipboard, for now on wayland only, since that's what I'm using (https://github.com/alacritty/copypasta won't work on wayland as it needs a surface)

The text selection is still buggy when selecting across multiple panes, but I'd appreciate some feedback on the direction this is going

@imsnif
Copy link
Member

imsnif commented May 27, 2021

Hey @tlinford - I didn't have a whole lot of time today to take a deep look at this. I plan on doing so tomorrow. Meanwhile my thoughts on copy-paste: I would recommend going in a different direction. The problem with using something that interacts directly with the clipboard is that aside from having to support many different platforms, we would encounter problems when Zellij is run on a remote server.

There is a terminal signal that tells the terminal emulator to place something in the clipboard (OSC 52). Meaning we'd be sending it to stdout and the terminal emulator would do the rest. I only did some brief searching for it, and found this generally related article: https://jdhao.github.io/2021/01/05/nvim_copy_from_remote_via_osc52/

So if you're having trouble finding the exact syntax of the signal let me know and I'll do some more thorough research.

@imsnif
Copy link
Member

imsnif commented May 28, 2021

@tlinford - I took a deeper look and also tried this. It looks really good. As you said, still a little rough but I'm very happy to see where this is going.

A behaviour I noticed that needs changing: it's possible to select text in more than one pane at the same time (not sure if this is what you meant with the multiple pane bugginess?)

Other than that, let me know if you want a hand with the OSC stuff. Let me know when you're done and I'll do a thorough go-over of the code (though at a glance things seem fine).

@imsnif
Copy link
Member

imsnif commented May 28, 2021

Oh! One more thing: I'm doing some work on the render function to improve its performance. Considering that, it might actually be better to handle the character selection in the grid and not in the render function. I know I said otherwise on discord - sorry about that :)

The reason for this is that the optimization I'm doing would mean that only changed characters will be sent to the terminal render function from the grid - so if you do it that way, the merge will be less painful I think. I hope that doesn't throw a wrench in your plans?

@tlinford
Copy link
Contributor Author

Glad to hear that you are happy with things so far :)

  • Regarding OSC52, absolutely, in the rush to get the selection copied to the clipboard I kind of forgot some important details, will give that a try as soon as possible
  • Multiple panes and selection: yes, that's what I meant with bugginess, there's quite a bit of work missing there, mainly a way to handle selecting text in one pane at a time, and resetting (or maybe it would be cool to recalculate) the selection on pane resizes.
  • Rendering: after you suggested doing it in render, I realized that it's much simpler that way, because modifying the character styles in the grid makes it necessary to also be able to undo the changes when the selection changes or is reset. But it totally makes sense from a performance perspective to redraw only changed characters, I'll see what I can come up with.

@imsnif
Copy link
Member

imsnif commented May 29, 2021

Rendering: after you suggested doing it in render, I realized that it's much simpler that way, because modifying the character styles in the grid makes it necessary to also be able to undo the changes when the selection changes or is reset. But it totally makes sense from a performance perspective to redraw only changed characters, I'll see what I can come up with.

Don't work too hard on this (or maybe leave it to the end?) - I'm in very initial stages of the performance improvements and things might change.

@tlinford tlinford force-pushed the mouse-support branch 2 times, most recently from fc5767f to 4c81bad Compare May 30, 2021 09:55
@tlinford
Copy link
Contributor Author

@imsnif Quick update:

  • osc52 seems to be working fine
  • selecting with multiple panes should be improved, once a selection is started by clicking in a pane, drag and release events are considered only for that pane. For now non empty selections in other panes are not modified, I think we could go both ways on that.

I realized that the current approach is a bit limited, as the selection is relative to the viewport, and not the whole terminal's text buffer, so for example holding left mouse button, and scrolling up/down won't behave as one would expect at the moment.

@imsnif
Copy link
Member

imsnif commented Jun 1, 2021

Sounds good! Sorry, I just realized maybe you're stuck because of me?

@tlinford
Copy link
Contributor Author

tlinford commented Jun 2, 2021

Not stuck, no worries! Will try and start moving the selection to Grid in the next days and see how things go.

@tlinford
Copy link
Contributor Author

tlinford commented Jul 1, 2021

Hi @imsnif, I migrated the tests to the new e2e setup (very cool!), the action repeater is now in os_input_output, and the problem with the disable command line flag should be fixed. Scrolling speed has been updated to 10ms.

@imsnif
Copy link
Member

imsnif commented Jul 2, 2021

Hey @tlinford - this looks great. Thanks for all the changes. I think I'm ready to merge this into main - unless there's something I'm forgetting? Some issue still open or that we still need to decide about?

@tlinford
Copy link
Contributor Author

tlinford commented Jul 2, 2021

Hey @tlinford - this looks great. Thanks for all the changes. I think I'm ready to merge this into main - unless there's something I'm forgetting? Some issue still open or that we still need to decide about?

Hey @imsnif, awesome, I think we covered everything we've discussed so far.

@imsnif
Copy link
Member

imsnif commented Jul 2, 2021

Amazing :)

So thanks very much for your incredible hard work and perseverance in implementing this feature. You've done a really great job!

@imsnif imsnif merged commit f93308f into zellij-org:main Jul 2, 2021
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.

None yet

3 participants