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

Add support for multiple panes in the same window #825

Merged
merged 41 commits into from Jun 7, 2019

Conversation

@zadjii-msft
Copy link
Member

commented May 15, 2019

image

This is probably good enough to PR, but might not be polished enough to ship quite yet. There's a LOT of work to still do one panes, even after this is checked in. See #1000 for the megathread of all panes related issues.

Things that might need todo's, and what issue they're linked to:

  • Clicking on a pane focuses it, but also starts a selection. This is probably #670
  • You can tab between panes. That's obviously unwanted. This is also related to #744
  • If you click on the separator's between panes, then on panes will be focused. Related to #528 but definitely different. #999
  • You can't resize panes. They're always 50% of their parent. #991 #992
  • You have to exit the shell to close the pane, there's no shortcut for "ClosePane" #993
  • You can only open a pane with the default profile #998
    • I left space for adding support for opening a specific pane, but it didn't really seem like there was a good keychord for doing this currently. Maybe if we added support for multi-key keychords, then it'd make more sense.
  • There's no real UI to indicate that a particular pane is focused, other than the blinking cursor. #994
  • You can't navigate the focus of panes with the keyboard currently. #995
  • I'm not sure Ctrl+Shift+plus and Ctrl+Shift+- are the right combo for splitting. Maybe we move them to Alt+Shift+plus/-
    • This would nicely fit with Alt+Shift+Arrows to navigate focus.
    • I'm removing the default keybinding entirely. If people want to start using this experimental feature, they can add the keybinding manually. The fundamentals of this PR should be in sooner than later, but the feature probably isn't ready for most people yet.

I'm pretty sure most of these don't need to be addressed in the initial PR, and can have follow-up tasks created, but they're all important to note.

This pretty aggressively touches Tab.cpp and App.cpp. App.cpp was doing a lot of work that assumed there was one control per tab. I figured that getting the structure in now would be beneficial, before we add any more code that assumes that.

Closes #532

zadjii-msft added some commits May 8, 2019

Add basic pane splitting
  * adds two keybindings, Ctrl+Shift++ and Ctrl+Shift+- for horizontal and
    vertical split, respectively.
  * Splits only open the default profile - This should probably be a setting,
    whether to use the default or a new copy of the current pane. There should
    also be a keystroke for opening a pane with a specific profile.
  * Only supports one pane ATM.
  * need to figure out which control is actively focused. Having an IsFocused
    method on the TermControl seems wrong.
This works for nesting panes
  Moving focus between panes is still a janky - selection isn't dismissed when
  the TermControl loses focus, and the cursor remains visible.

  Switching to a tab with panes doesn't focus _any_ control, so that's bad.
Reload settings for the panes.
  On reload of settings, change the icon of the tab to that of the last focused
  pane
Merge remote-tracking branch 'origin/master' into dev/migrie/f/panes
# Conflicts:
#	src/cascadia/TerminalApp/App.cpp
#	src/cascadia/TerminalApp/AppKeyBindings.cpp
#	src/cascadia/TerminalApp/AppKeyBindings.idl
Hook up the terminal's closing to the pane's to the tab's
  But something seems fucky with the closing of the tabs. I'm getting crashes,
  when I close with `exit` from the commandline and `closeOnExit:true`, but I
  don't think it has anything to with my change. Gonna mess with master and see
  if it's busted.
Revert "This is me trying to mess with the tab closing, but I think i…
…t's probably _more_ wrong."

This reverts commit 23930b5.
@mdtauk

This comment has been minimized.

Copy link

commented May 15, 2019

Will those panes need their own TabView controls?

I am assuming these panes are live consoles, and not just an area to pin/copy output for easy reference, as per #644 which I submitted?

@zadjii-msft

This comment has been minimized.

Copy link
Member Author

commented May 15, 2019

@mdtauk That's correct, these are full terminal instances.

@DHowett-MSFT and I previously discussed the merits of having top-level tabs and then nested panes vs toplevel panes with nested tabs, and felt that toplevel tabs (similar to tmux) would be the better UX. If each pane had it's own tabview, that would result in a good amount of window real estate being dedicated to just displaying tabs.

This could in the future be used to support pinning content in a new pane. Theoretically you don't even need to limit it to another terminal control in the pane (though this PR is definitely limited to a terminal in the pane). I'll leave that discussion for #644.

@DHowett-MSFT

This comment has been minimized.

Copy link
Member

commented May 15, 2019

Wow! Phone thoughts before reviewing:

Ctrl+Shift+_ looks like a divider... and so does |!

Neither of those characters have a direct VT representation, I believe?

@DHowett-MSFT
Copy link
Member

left a comment

  • I'd love to see a design doc on this go into docs/specs; it'll be good to get a quick overview of why it works the way it does.. and also what "the way it does" is.
  • Should SplitVertical/Horizontal be AddSplit(const Pane::SplitState)?
Show resolved Hide resolved src/cascadia/TerminalApp/App.cpp Outdated
Show resolved Hide resolved src/cascadia/TerminalApp/App.cpp Outdated
Show resolved Hide resolved src/cascadia/TerminalApp/App.cpp Outdated
Show resolved Hide resolved src/cascadia/TerminalApp/CascadiaSettings.cpp Outdated
Show resolved Hide resolved src/cascadia/TerminalApp/Pane.cpp Outdated
Show resolved Hide resolved src/cascadia/TerminalApp/Tab.cpp Outdated
Show resolved Hide resolved src/cascadia/TerminalApp/Tab.cpp Outdated
Show resolved Hide resolved src/cascadia/TerminalApp/Tab.cpp Outdated
Show resolved Hide resolved src/cascadia/TerminalApp/Tab.cpp Outdated
Show resolved Hide resolved src/cascadia/TerminalControl/TermControl.cpp Outdated
@miniksa

This comment has been minimized.

Copy link
Member

commented May 16, 2019

Wow! Phone thoughts before reviewing:

Ctrl+Shift+_ looks like a divider... and so does |!

Neither of those characters have a direct VT representation, I believe?

That's super creative and I love it if it doesn't conflict with a VT sequence.

@mdtauk

This comment has been minimized.

Copy link

commented May 16, 2019

I am assuming there will be keyboard shortcuts for creating these panes. Might I suggest they be added to the Context Menu also.
------------
Split Right
Split Below
------------

Merge remote-tracking branch 'origin/master' into dev/migrie/f/panes
# Conflicts:
#	src/cascadia/TerminalApp/App.cpp
#	src/cascadia/TerminalApp/AppKeyBindings.cpp
@zadjii-msft

This comment has been minimized.

Copy link
Member Author

commented May 17, 2019

My only qualm with | and - as the shortcuts for new panes is that the key for | is a layout-dependent vkey:

https://docs.microsoft.com/en-us/windows/desktop/inputdev/virtual-key-codes

Constant/value Description
VK_OEM_5
0xDC
Used for miscellaneous characters; it can vary by keyboard. For the US standard keyboard, the '|' key

So it'll work great on EN-US, but I don't know about other locales. + didn't have that problem.

I really like that Ctrl+| isn't otherwise a VT sequence though. (Technically ctrl+\ is, but ctrl+shift+\ == ctrl+|). ctrl+- is not a sequence, while ctrl+shift+- == ctrl+_ is.

My thinking with moving to Alt+Shift+plus and Alt+Shift+- (^[+ and ^[_) is to put moving between panes on alt+arrow, and generally putting pane-like operations on alt.

Actually, playing with this more, Ctrl+= and Ctrl+- are both not VT sequences...

zadjii-msft and others added some commits Jun 7, 2019

Merge remote-tracking branch 'origin/master' into dev/migrie/f/panes
# Conflicts:
#	src/cascadia/TerminalApp/AppKeyBindings.cpp
Carlos Zamora
Show resolved Hide resolved doc/cascadia/Panes.md Outdated

#pragma once
#include <winrt/Microsoft.Terminal.TerminalControl.h>
#include "../../cascadia/inc/cppwinrt_utils.h"

This comment has been minimized.

Copy link
@DHowett-MSFT

DHowett-MSFT Jun 7, 2019

Member

alarming

Show resolved Hide resolved src/cascadia/TerminalApp/Tab.cpp Outdated
Apply suggestions from code review
Don't capture a dead reference

Co-Authored-By: Dustin L. Howett (MSFT) <duhowett@microsoft.com>
@miniksa

miniksa approved these changes Jun 7, 2019

Copy link
Member

left a comment

:shipit:

Show resolved Hide resolved src/cascadia/TerminalApp/Tab.cpp Outdated
Show resolved Hide resolved src/cascadia/TerminalApp/Tab.cpp Outdated
Apply suggestions from code review
Let's try and write code in github

@zadjii-msft zadjii-msft merged commit 2da5b0b into master Jun 7, 2019

6 checks passed

Terminal CI Build #0.0.1906.0715 succeeded
Details
Terminal CI (Build ARM64 Release) Build ARM64 Release succeeded
Details
Terminal CI (Build x64 Release) Build x64 Release succeeded
Details
Terminal CI (Build x86 Release) Build x86 Release succeeded
Details
Terminal CI (Static Analysis Build x64) Static Analysis Build x64 succeeded
Details
license/cla All CLA requirements met.
Details

Specification Tracker automation moved this from Spec In Review ⏰ to Spec Accepted! 🎉 Jun 7, 2019

@zadjii-msft zadjii-msft deleted the dev/migrie/f/panes branch Jun 7, 2019

@zadjii-msft zadjii-msft referenced this pull request Jun 7, 2019

Open

Deliverable: Add support for panes #1000

4 of 13 tasks complete
@Summon528

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2019

You might also want to close #745 and #541

@whitecolor

This comment has been minimized.

Copy link

commented Jul 8, 2019

When this can be expected on Insiders update?

@zadjii-msft

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2019

@whitecolor this is in the version that's currently available on the Store. You need to add splitHorizontal and splitVertical keybindings manually. Note though that panes are super beta and kinda buggy and unpolished ATM.

@whitecolor

This comment has been minimized.

Copy link

commented Jul 8, 2019

@zadjii-msft I tried it so a little buggy but actually may work)

  1. Is it possible to close exiting split pane, is there a command? Ctrl+W closes tab would be better to close the current split.

  2. Is there commands to jump between splits? (Alt +arrow) I think is used often for this.

@zadjii-msft

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

@whitecolor Not yet, but most of these are being tracked by #1000

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.