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 spec for adding commandline arguments to wt.exe #3495

Merged
merged 21 commits into from
Jan 15, 2020

Conversation

zadjii-msft
Copy link
Member

Summary of the Pull Request

This is the first draft of a spec for adding commandline arguments to the Windows Terminal. This includes design work for a powerful future version of the commandline args for the Terminal, as well as a way that system could be implemented during 1.0 to provide basic functionality, while creating commadlines that will work without modification in (a future Windows Terminal version).

References

Referenced in the course of this spec:

PR Checklist

Detailed Description of the Pull Request / Additional comments

Read the spec.

This is CURRENTLY a draft. I'm looking for feedback in this step before finishing it. There's not currently any implementation details, because I'd like feedback on the proposal before moving forward.

@zadjii-msft zadjii-msft added Issue-Docs It's a documentation issue that really should be on MicrosoftDocs/Console-Docs Product-Terminal The new Windows Terminal. labels Nov 8, 2019
@DHowett-MSFT
Copy link
Contributor

Should there be a broad unification of parameter names and command names (for bound commands)? Should one be able to pass any argument that a keybinding could take as a --argument?

@zadjii-msft
Copy link
Member Author

Should there be a broad unification of parameter names and command names (for bound commands)? Should one be able to pass any argument that a keybinding could take as a --argument?

@DHowett-MSFT probably. I wasn't sure how far to take that - should someone be able to wt -d c:\foo ; duplicate-tab? What about wt -d c:\foo ; increase-font-size? I figured that we'd have a subset of ShortcutActions available as commands in the commandline, but those that have analogs should probably be an exact match (with a camelCase -> kebab-case translation)

In general though, it kinda makes sense to have the parameters for commands to be exact matches to the arguments to the similar ShortcutAction.

`;` to delimit commands, which might want to also use `;` in the commandline
itself, we'll use `\;` as an escaped `;` within the commandline. This is an area
we've been caught in before, so extensive testing will be necessary to make sure
this works as expected.
Copy link
Member

Choose a reason for hiding this comment

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

I think you should call out that you seem to expressly be avoiding somehow delimiting the arguments on the command line to make life easier for folks who are launching something with arguments. Mismatching 500 escaping mechanisms is the bane of our existence for things like "launching an executable through cmd through powershell through tshell to a device". At least, that was my understanding as to why you didn't have a --arguments "foo arguments" option after the command line for a launch.

Choose a reason for hiding this comment

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

PowerShell uses ; as command separators, so:

wt new-tab cmd.exe ; split-pane -P 30 wsl.exe

Means:

  1. Run wt(.exe) with ['new-tab', 'cmd.exe'] as the arguments.
  2. Run split-pane(.exe) with ['-P', '30', 'wsl.exe'] as the arguments.
    • This will throw an exception, unless the user has a split‑pane command on their PATH or as a PowerShell cmdlet 

Copy link
Member Author

Choose a reason for hiding this comment

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

@ExE-Boss thanks for the breakdown. Clearly, I'm more of a cmd guy 😅.

I've added more details in this section to elaborate upon this point. Let's use this thread to discuss. Technically, we could ask the user to escape our commandline in powershell, like wt new-tab `; new-pane commandline \`; with \`; semicolons or wt new-tab ';' new-pane "commandline \; with \; semicolons". However, if the party line is "use powershell", does it really make sense for us to add a commandline that fundamentally needs to be escaped when running in powershell? What other command separators could we use?

Copy link
Member

Choose a reason for hiding this comment

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

--?

Comment on lines +278 to +280
* `--showGuids,-g`: In addition to showing names, also list each profile's
guid. These GUIDs should probably be listed _first_ on each line, to make
parsing output easier.
Copy link
Member

Choose a reason for hiding this comment

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

I'm all for this, but is there a value to getting the guids?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe? I guess I really just threw this out there as a proposal. GUIDs are going to be more stable than names, at least as far as scripting is concerned. But you're right that this probably doesn't add real value.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the same thing. Not sure if it adds much value.

Comment on lines 324 to 326
* `--percent,-P split-percentage`: Designtates the amount of space that the new
pane should take, as a percentage of the parent's space. If omitted, the pane
will take 50% by default.
Copy link
Member

Choose a reason for hiding this comment

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

will we ever consider this to go beyond % units? Like maybe accept 0.5?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea probably. I figured 50.5 and other decimal values would already be reasonable params here. I was thinking in the future we might also have a --size,-s arg, which specifies the size in number of chars. Though, that one's a little trickier, because it's hard for the shell to know what size in characters the terminal has available to it, you know?




#### `[terminal_parameters]`
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why we're not just adding all of the profile settings here? Like acrylicOpacity, color scheme, etc...

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly there's not a good reason. It probably could have all the settings, I just started somewhere. I'll make a patch with all the settings to see what that looks like.

@zadjii-msft zadjii-msft mentioned this pull request Nov 18, 2019
@qqkookie
Copy link

qqkookie commented Nov 28, 2019

How about option to set tab title label? Currently WT tab with "cmd" and "Power Shell" profile have fixed tab title (from the profile "name" attribute) and taskbar label. It would be nice to have command line option can set the active tab title. This also will set name on the taskbar. For example, --title "This is tab title" will override the default Window name like "cmd". It will help to identify multiple instance of WT windows in the taskbar.

And with same reason, we need new profile attribute in "setting.json" for the tab title ("Title: "This is title" in profile entry) , separate from the profile "name" (default tab title). so that the WT windows tab may have short profile name and longer and more descriptive tab title, different from the profile name. It will be used as default title when no --title option is specified on the command line.

And on --startingDirectory option: how about accepting "~" notation of unix shell for %USERPROFILE%? i.e --startingDirectory "~" will be same as --startingDirectory "%USERPROFILE%".

And if --startingDirectory has no argument, then use current directory inherited form invoking process. i.e. ignoring "startingDirector" attribute in the profile (in setting.json). --startingDirectory alone will be same as --startingDirectory "."

And if --StartingDiretory option is used, then simple lone --title option (with no argument) will set the tab title from the StartingDirectory value. Only last path component name is used to limit length of title and taskbar label. If Both --StartingDiretory and --title option has no argument, then get current directory and use last path component of current directory.

For example, --StartingDirectory "~\Desktop\ProjectA" --title is resolved as --StartingDirectory "C:\Users\JohnDoe\Desktop\ProjectA" --title "ProjectA" , then only "ProjectA" part is used as tab title, not full path.

@miniksa
Copy link
Member

miniksa commented Jan 3, 2020

eh

Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

Gave it another pass. Are we planning on committing this soon? There's not a lot of commentary left. I'm going to approve it as is as it's close enough to a reasonable plan to me.

@zadjii-msft
Copy link
Member Author

@miniksa I'd love to get this merged soon 😄 With the holidays over, I'm hoping it'll be easier to get another 2 signoffs to get this spec merged next week

@DHowett-MSFT
Copy link
Contributor

In favor of "implicit new-tab": wt.exe without any arguments is already an implicit new-window or new-tab; we can't claw back the implicitness and ease of use in that one, so I think in the spirit of keeping that going WT should automatically do anything necessary to service a command (wt split-pane should operate in a new tab or new window, etc.)

@miniksa
Copy link
Member

miniksa commented Jan 9, 2020

In favor of "implicit new-tab": wt.exe without any arguments is already an implicit new-window or new-tab; we can't claw back the implicitness and ease of use in that one, so I think in the spirit of keeping that going WT should automatically do anything necessary to service a command (wt split-pane should operate in a new tab or new window, etc.)

ACK that @DHowett-MSFT won me over in our triage today with this argument and I stand down from my earlier position.

zadjii-msft and others added 3 commits January 9, 2020 16:21
Co-Authored-By: Dustin L. Howett (MSFT) <duhowett@microsoft.com>
functionality that will arrive in 1.0. Even when sessions are supported like
that, I'm not sure that when we're parsing a commandline, we'll be possible to
know what session we're currently running in.That might make it challenging to
dispatch this kind of command to "the current WT window".
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we need a -r option ("remote") to say "control a windows terminal instance?" with autodetection for the one it's running in? hmm

@jantari
Copy link
Contributor

jantari commented Jan 13, 2020

I still don't like the ; syntax for specifying more complex (e.g. multi-pane) start configurations.

What if we could pass JSON as an argument instead?

I don't have a full spec for this but:

  1. It would allow passing arbitrarily complex configurations in a structured manner
  2. It would be trivial to escape/use from PowerShell as JSON does not contain the ' character, which PowerShell uses to denote verbatim/literal strings.
  3. Possibly large chuncks of the existing JSON settings parser could be re-used to parse the command line. Invalid syntax or configurations are easy to detect by the JSON parser.

Examples to illustrate the idea:

wt.exe --profile "Windows Powershell"
# could become:
wt.exe {"profile":"Windows PowerShell"}
# or in PowerShell:
wt.exe '{"profile":"Windows PowerShell"}'
wt.exe cmd.exe ; split-pane --target-pane 0 -v -% 30 wsl.exe
# could become:
wt.exe {"tabs":[{"panes":[{"profile":"cmd","size":"70%","vertical":true},{"executable":"wsl.exe"}]}]}
# or in PowerShell, simply:
wt.exe '{"tabs":[{"panes":[{"profile":"cmd","size":"70%","vertical":true},{"executable":"wsl.exe"}]}]}'

# The JSON expanded is this:
{
  "tabs": [
    {
      "panes": [
        {
          "profile": "cmd",
          "size": "70%",
          "vertical": true
        },
        {
          "executable": "wsl.exe"
        }
      ]
    }
  ]
}

If the existing JSON settings parser is re-used, arbitary settings-values could be overwritten/specified on the command line:

{
  "tabs": [
    {
      "panes": [
        {
          "profile": "cmd",
          "size": "70%",
          "vertical": true,
          "startingDirectory":  "%USERPROFILE%",
          "tabTitle":  "my funny tab 123",
          "useAcrylic":  false
        },
        {
          "executable": "wsl.exe"
        }
      ]
    }
  ]
}

Because command-line commands in CMD.EXE can only be 1024 characters long (forgot source, possibly no longer the case?) this would provide an easy path to specify a JSON-configuration in a file for SUPER DUPER intricate setups, e.g. wt.exe --launchconfig ubersetup.json

In fairness, I want to mention that PowerShell has the --% (stop parsing) operator, which instructs it to stop interpreting anything after it and just pass it on verbatim. So, the semicolon-problem could also be addressed by the following syntax:

# wt.exe still needs to be interpreted by PowerShell as it's a command in PATH, but nothing after it
wt.exe --% cmd.exe ; split-pane --target-pane 0 -v -% 30 wsl.exe

Still, it's a highly ugly and "proprietary" syntax, e.g. yet another thing to learn/go wrong. JSON is well understood both from the user and the dev side.

@DHowett-MSFT
Copy link
Contributor

Still, it's a highly ugly and "proprietary" syntax, e.g. yet another thing to learn/go wrong. JSON is well understood both from the user and the dev side.

I dunno about this one. We're trying to bridge the gap between user-friendliness (which typing a blob of JSON does not have) and machine parseability. It seems like tmux is already using ; as a separator (see the manual, section PARSING SYNTAX), so this is not proprietary.

Ugliness, however, is in the eye of the beholder. Passing a commandline filled with fully syntactically-correct JSON is subjectively uglier to me than "wt action args ; action args ; action args".

@carlos-zamora
Copy link
Member

In fairness, I want to mention that PowerShell has the --% (stop parsing) operator, which instructs it to stop interpreting anything after it and just pass it on verbatim. So, the semicolon-problem could also be addressed by the following syntax:

# wt.exe still needs to be interpreted by PowerShell as it's a command in PATH, but nothing after it
wt.exe --% cmd.exe ; split-pane --target-pane 0 -v -% 30 wsl.exe

@zadjii-msft can we add this to the spec? Specifically where you talk about escaping the semicolons in powershell. This alleviates my main concern.

Copy link

@jawn jawn left a comment

Choose a reason for hiding this comment

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

Add period to end of sentence.

@zadjii-msft zadjii-msft changed the title Spec for adding commandline arguments to wt.exe Add spec for adding commandline arguments to wt.exe Jan 15, 2020
@zadjii-msft zadjii-msft merged commit 1f7578f into master Jan 15, 2020
@zadjii-msft zadjii-msft deleted the dev/migrie/s/607-commandline-args-spec branch January 15, 2020 16:20
ghost pushed a commit that referenced this pull request Jan 27, 2020
## Summary of the Pull Request

Adds support for commandline arguments to the Windows Terminal, in accordance with the spec in #3495

## References

* Original issue: #607
* Original spec: #3495

## PR Checklist
* [x] Closes #607
* [x] I work here
* [x] Tests added/passed
* [ ] We should probably add some docs on these commands
* [x] The spec (#3495) needs to be merged first!

## Detailed Description of the Pull Request / Additional comments

🛑 **STOP** 🛑 - have you read #3495 yet? If you haven't, go do that now.

This PR adds support for three initial sub-commands to the `wt.exe` application:
* `new-tab`: Used to create a new tab.
* `split-pane`: Used to create a new split.
* `focus-tab`: Moves focus to another tab.

These commands are largely POC to prove that the commandlines work. They're not totally finished, but they work well enough. Follow up work items will be filed to track adding support for additional parameters and subcommands

Important scenarios added:
* `wt -d .`: Open a new wt instance in the current working directory #878
* `wt -p <profile name>`: Create a wt instance running the given profile, to unblock  #576, #1357, #2339
* `wt ; new-tab ; split-pane -V`: Launch the terminal with multiple tabs, splits, to unblock #756 

## Validation Steps Performed

* Ran tests
* Played with it a bunch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Docs It's a documentation issue that really should be on MicrosoftDocs/Console-Docs Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.