[WIP] Tmux control mode #1090
[WIP] Tmux control mode #1090
Conversation
when in tmux mode, bypass vtparse
|
Man, I'm excited about this PR. Hope this gets merged eventually 👍 |
|
I think this can be reviewed now since it's been at a basic functional status. All comments are appreciated! |
mux/src/tmux_commands.rs
Outdated
| write!(&mut s, "0x{:X}\r", byte).expect("unable to write key"); | ||
| } | ||
| format!("send-keys -t {} {}", self.pane, s) | ||
| // FIXME: An unexpected duplicated command will prompt next line, why? |
There was a problem hiding this comment.
After some search, I found this really hard to solve. Currently Tmux use ESC k to set the window name, which we don't support yet. It's really annoying to live with that window title echoed after every command..
Relate issue in mosh: mobile-shell/mosh#992
There was a problem hiding this comment.
That ESC k TITLE ST sequence is pretty awkward because it doesn't match the typical OSC style escapes that use ST (string terminator). That makes it difficult to fit into the parser at the right layer :-(
Do you know what is emitting that sequence?
http://manpages.ubuntu.com/manpages/precise/en/man1/tmux.1.html#names%20and%20titles
suggests that automatic-rename may be a factor: can we tell tmux to disable that option when we set up control mode?
There was a problem hiding this comment.
I found that it was my zsh. Set DISABLE_AUTO_TITLE="true" disable this sequence.
|
Build is failing on Windows for some reason. Although tmux itself isn't available natively on Windows, one can still use tmux in WSL and over SSH. |
| @@ -32,6 +32,7 @@ serde = {version="1.0", features = ["rc", "derive"], optional=true} | |||
| sha2 = "0.9" | |||
| terminfo = "0.7" | |||
| thiserror = "1.0" | |||
| tmux-cc = {version = "0.1", path = "../tmux-cc"} | |||
There was a problem hiding this comment.
dependency management thinking: termwiz is its own crate on crates.io.
Adding a dep here means that we'll also either:
- need to publish
tmux-ccto crates.io (and I'm not sure that that makes sense as it doesn't seem obviously reusable) - move tmux-cc to be a module inside termwiz, and potentially make it an optional feature
There was a problem hiding this comment.
Woa, didn't expect that. Move into termwiz seems more reasonable, maybe better to move it after all other issues fixed?
mux/src/tmux_pty.rs
Outdated
| fn read(&mut self, mut buf: &mut [u8]) -> std::io::Result<usize> { | ||
| match self.rx.recv() { | ||
| Ok(str) => { | ||
| return buf.write(str.as_bytes()); |
There was a problem hiding this comment.
This buffer reader has a potential problem: if we recv a String that is larger than buf it won't fit and we'll discard the excess.
I think we can make this safer by using a socketpair from our filedescriptor crate. Take a look at read_from_pane_pty to see how it uses a socketpair as a channel for the bytes.
The tmuxreader could potentially just one of the FileDescriptors from the socket pair and simplify some of the code in here.
There was a problem hiding this comment.
Can't get the point of socketpair here since there is already a channel between tmux's output and TmuxPty reader. But I tried to add a buffer to avoid the "overrange" problem.
Yes, some of the sequences will be filtered due to how ConPTY works. I think this is on the roadmap of the ConPTY project. |
| if !self.head_buffer.is_empty() { | ||
| let mut buffer_cleared = false; | ||
| let bytes = if self.head_cursor + buf.len() >= self.head_buffer.len() { | ||
| buffer_cleared = true; | ||
| &self.head_buffer[self.head_cursor..] | ||
| } else { | ||
| &self.head_buffer[self.head_cursor..(self.head_cursor + buf.len())] | ||
| }; | ||
| return buf.write(bytes.as_bytes()).map(|res| { | ||
| // update buffer if write success | ||
| if buffer_cleared { | ||
| self.head_buffer.clear(); | ||
| self.head_cursor = 0; | ||
| } else { | ||
| self.head_cursor = self.head_cursor + buf.len(); | ||
| } | ||
| res | ||
| }); | ||
| } else { | ||
| match self.rx.recv() { | ||
| Ok(str) => { | ||
| if str.len() > buf.len() { | ||
| self.head_buffer = str; | ||
| self.head_cursor = 0; | ||
| return self.read(buf); | ||
| } else { | ||
| return buf.write(str.as_bytes()); | ||
| } | ||
| } | ||
| Err(_) => { | ||
| return Ok(0); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add a buffer here to prevent write out of range. Will this solve the problem?
There was a problem hiding this comment.
I haven't looked deeply at this change yet, but my knee-jerk reaction is that that looks very complex. I think replacing the channel based reader/writer with a socketpair will make most of the complexity disappear!
|
It's taking me a while to get through and review this: I've been a bit preoccupied with preparation for a long distance house move in real life, so wanted to let you know that that's why I'm going slow |
|
I fully understand that this PR will take some time, I'm thinking of writing a more detailed design description within the next few days. |
What happened with tmux modeTo make tmux working:
1Currently, a This action will cause the current LocalPane switch into tmux mode, handle DCS actions as tmux event. A At the meanwhile, the performer will wire up a For the next time, we will parse tmux content by batch, and dispatch tmux actions using new type 2/3pub(crate) struct TmuxDomainState {
pub pane_id: PaneId, // ID of the original pane
pub domain_id: DomainId, // ID of TmuxDomain
state: RefCell<State>,
pub cmd_queue: Arc<Mutex<TmuxCmdQueue>>,
pub gui_window: RefCell<Option<MuxWindowBuilder>>,
pub gui_tabs: RefCell<Vec<TmuxTab>>,
pub remote_panes: RefCell<HashMap<TmuxPaneId, RefTmuxRemotePane>>,
pub tmux_session: RefCell<Option<TmuxSessionId>>,
}For TmuxDomain, it handle tmux events and send tmux commands. Commands are queued in the All tmux commands can be serialized into string and write into the original pty. There are two special events: Guarded and Output. We expect a Guarded event for each command we sent. Initially there will be a For Output event, the TmuxDomain will pipe its content to the destinate pane by looking into 4If tmux control mode is nicely quitted, there should be a
When an invalid tmux sequence arrived (especially a DeviceControll Exit sequence), tmux_cc::Parser will rise an error containing all bytes cannot be parsed, so the escape::Parser will realized it's time to back to normal. Unparsed bytes will feed as the normal way. For the normal Parser, a DeviceControll Exit sequence will generate a Noted that this procedure isn't perfect because:
|
|
I haven't forgotten this, I'm just in the middle of moving house! |
|
can not wait it. |
|
Kudos to this PR, I'd really like to have it working! |
|
This work looks amazing! |
Needs the % prefix to find the pane by id refs: #1090
|
Thanks for this! I rebased it and fixed up the build for recent changes in The current state is:
There's still a bunch of things to fully reconcile with the items in #336 One of the interesting recent developments in |
|
I'm closing this as this batch of changes is now in |
|
Really glad to hear that! Can't wait to move forward. |
|
whats the current state of this in |
|
It is incomplete as indicated in #1090 (comment) |
A proof of concept mainly inspired by previous tmux-cc related PR and @skyline75489 's tmux_cc branch.
The idea is to place the tmux parser directly into escape::Parser, so we can
Thus, DCS handler will handle the parsed TmuxEvents, things after that will remain trivial.