Add support for tmux control mode (#3656) - #18928
Joe Xue (joexue) wants to merge 15 commits into
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
@microsoft-github-policy-service agree |
|
whoa |
|
This is a long-awaited feature! Thanks Joe Xue (@joexue)! |
|
FYI: I am working with reduced staffing at the moment, so reviews may be slow to come. I apologize for that. |
|
Is there a nightly build with this PR to test? |
Your best bet is to build this locally. Once it merges, it will be available on the Canary channel though. |
I tried to find the build guide, but couldn't find one. Do you have the link handy? |
README.md ->Developer Guidance Darshan Prajapati (@iDarshan) |
|
Hi Joe Xue (@joexue)! Thanks for doing this! I published a build for the team to test internally and we've got a few notes we want to share with you 😊
Overall, this is a really exciting feature and we're excited to see it land! Let us know if you need any guidance or additional comments on any of the thoughts above. 😊 CC Dustin L. Howett (@DHowett) Leonard Hecker (@lhecker) Mike Griese (@zadjii-msft) |
Scrollbars are gone is by design, I put the comment in the code, since we want to make local panes size match remote(tmux) panes size, but each split, tmux just lost 1 character to use it as separator, if we have side scrollbars, we cannot make two side's size match. To meet this, the padding size is changed too. As the menu, it is because I tried to touch as less code as I can to do the job. So give the tmux a separate UI system. I described in the PR, this could be improved and should integrated into the present UI/menu system.
I did not see this bug, to help me to reproduce it here, could you describe your env? how do you connect to tmux server? what is the tmux version?
Sure, let me fix it.
Sure, let me look it.
This is by design, remote pane should close by quit it's app. but this is arguable, if we think close local pane should close remote pane too, it is easy and feasible.
Sure. Fair enough.
Please provide the idea, I can try to implement it.
Thanks! Since this PR is a bit big, another option is we split it into two or three parts to make it easy to review and integrate. Such as What do you think, split it or just keep as it is? |
fed711a to
2bf4d08
Compare
|
Carlos Zamora (@carlos-zamora) Dustin L. Howett (@DHowett) Leonard Hecker (@lhecker) Mike Griese (@zadjii-msft) Thanks |
This comment has been minimized.
This comment has been minimized.
2bf4d08 to
b70dad2
Compare
This comment has been minimized.
This comment has been minimized.
54c2bac to
4817698
Compare
|
I tried to test this and seems like its not working Anything special need to be done to try this? |
You probably need to turn it on in Settings. |
Which settings? in Terminal or System settings? Do you have path to setting? |
|
Which version of Terminal are you using? Did you compile it yourself? Since this pull request is still open and has not been accepted, you would need to have compiled it yourself. |
Compiled myself. It took hell of a time to figure out compilation steps. it always fails due to vcpkg. Readme needs to include steps around vcpkg failure. |
|
Oh dang, that's a new failure state we haven't heard of. If you remember what you had to do, would you mind sharing? It's supposed to use the vcpkg install that comes with VS, but if that's broken it will try a couple other things. I think this PR puts the control mode toggle into Terminal's in-app settings, profile (or defaults), advanced. |
|
Enable this setting to allow Tmux Control mode: |
|
I have tested this briefly and here are the basic bugs:
|
original-cooling-space (zhangweize9-cyber)
left a comment
There was a problem hiding this comment.
It is recommended that the author optimize the memory safety aspect to make the overall logic more robust.
| std::shared_ptr<Pane> AttachPane(std::shared_ptr<Pane> pane, | ||
| winrt::Microsoft::Terminal::Settings::Model::SplitDirection splitType); | ||
| winrt::Microsoft::Terminal::Settings::Model::SplitDirection splitType, | ||
| const float splitSize = .5); |
There was a problem hiding this comment.
const float splitSize = .5The correct way to write it is 0.5f. While using .5 might not show any obvious errors at runtime, it introduces numerous type conversions. It's also recommended to add a type specification to this variable.
- const float splitSize = .5
+ const float splitSize = 0.5f|
|
||
| namespace winrt::TerminalApp::implementation | ||
| { | ||
| void TmuxConnection::Initialize(const Windows::Foundation::Collections::ValueSet&) const noexcept |
There was a problem hiding this comment.
| void TmuxConnection::Initialize(const Windows::Foundation::Collections::ValueSet&) const noexcept | |
| // WIP: `TmuxConnection::Initialize()` is working a progress...... | |
| void TmuxConnection::Initialize(const Windows::Foundation::Collections::ValueSet&) const noexcept {} |
I suggest you add a comment declaring the purpose of this function. Or you can write a // WIP: comment to just to clarify, this feature is currently under development.
| DiscoverPanes, | ||
| }; | ||
|
|
||
| struct ResponseInfo |
There was a problem hiding this comment.
I suggest you use some more compatible functions to implement this functionality, such as std::variant.
| void _handleDetach(); | ||
| void _handleSessionChanged(int64_t sessionId); | ||
| void _handleWindowAdd(int64_t windowId); | ||
| void _handleWindowRenamed(int64_t windowId, winrt::hstring name); |
There was a problem hiding this comment.
| void _handleWindowRenamed(int64_t windowId, winrt::hstring name); | |
| void _handleWindowRenamed(int64_t windowId, const winrt::hstring name); |
It is recommended to define this as a constant, that is, a read-only variable, for runtime safety reasons.
| if (auto inputCallback = args->InputCallback()) | ||
| { | ||
| return [inputCallback = std::move(inputCallback)](wchar_t ch) -> bool { | ||
| const auto c16 = static_cast<char16_t>(ch); |
There was a problem hiding this comment.
| const auto c16 = static_cast<char16_t>(ch); | |
| const auto c16 = static_cast<char16_t>(static_cast<unsigned char>(ch)); |
I mentioned that this syntax might cause garbled characters in multilingual support or emojis......
| else | ||
| { | ||
| // Do we ever get here (= uninitialized terminal)? If so: How? | ||
| assert(false); |
There was a problem hiding this comment.
Deleting this assertion is like burying your head in the sand, it may seem like the problem is solved, but it will actually trigger a series of chain reactions.
|
|
||
| std::function<bool(wchar_t)> Terminal::EnterTmuxControl() | ||
| { | ||
| return _pfnEnterTmuxControl ? _pfnEnterTmuxControl() : nullptr; |
There was a problem hiding this comment.
This line is written quite meticulously.
| // Not implemented for conhost. | ||
| } | ||
|
|
||
| std::function<bool(wchar_t)> ConhostInternalGetSet::EnterTmuxControl() |
There was a problem hiding this comment.
I suggest you add a comment to this section, indicating that the logic of this function needs further verification.
| std::function<bool(wchar_t)> EnterTmuxControl() override | ||
| { | ||
| Log::Comment(L"EnterTmuxControl MOCK called..."); | ||
| return nullptr; |
There was a problem hiding this comment.
| return nullptr; | |
| return 1; |
If you need to return an error message, you can define the return value as 1 instead of a null pointer.
|
|
||
| ITermDispatch::StringHandler AdaptDispatch::EnterTmuxControl(const VTParameters parameters) | ||
| { | ||
| if (parameters.size() != 1 || parameters.at(0).value() != 1000) |
There was a problem hiding this comment.
| if (parameters.size() != 1 || parameters.at(0).value() != 1000) | |
| const int TmuxControlModeId = 1000; | |
| if (parameters.size() != 1 || parameters.at(0).value() != TmuxControlModeld) |
To improve the maintainability of the source code, it is recommended to store 1000 as a variable.
It works for me to split both vertically and horizontally, at least before Leonard Hecker (@lhecker) change. It calculates automatically, try split several times. Thank you for your testing. |
|
Hi team, |
Joe Xue (@joexue) How do you split horizontally? What is the step? I have attached the screenshot for splitting from + button. Also, how do you resize the pan? |
Hi Darshan Prajapati (@iDarshan) |
|
One more thing Joe Xue (@joexue). I see that once I am in tmux control mode, Terminal is not showing space key as input. So if I type |
|
It's been a year since this PR was opened. When will this get merged? |
|
Why is it still on indefinite hold? What stage is it stuck at, exactly? |
|
Joe Xue (@joexue) mind merging main into this? looks like the branch went stale |
Done, thanks |
|
/azp run |
|
Azure Pipelines: Successfully started running 2 pipeline(s). |
|
Apologies for the delays. The PR doesn't integrate well into our existing architecture and we technically really need to rework how panes are represented. (They shouldn't own the UI content, but rather represent the layout only. This way, the tmux extension could re-layout the entire pane tree cheaply, and we only re-layout the XAML UI lazily later.) |



Summary of the Pull Request
Let WT supports tmux control mode
References and Relevant Issues
#3656
Detailed Description of the Pull Request / Additional comments
Support:
Create/attach tmux session
Split pane vertical/horizontal
Window/panes size change
Remove pane/tab if remote pane exit or window exit
Improvements may do:
Tested by using tmux 3.4:
ssh to a machine has tmux or use wsl, then run
"tmux -CC" or "tmux -CC a"
Validation Steps Performed
PR Checklist
tmuxControl Mode #3656