Allow closing tabs by index

[ianjoneill]

Summary of the Pull Request

Updates the closeTab action to optionally take an index.

PR Checklist

• Closes closeTab should accept an index param #7180
• CLA signed. If not, go over here and sign the CLA
• Tests added/passed
• Documentation updated. If checked, please file a pull request on our docs repo and link it here: Add documentation for closing tabs by index MicrosoftDocs/terminal#347
• Schema updated.
• I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Validation Steps Performed

Added the following configuration to settings.json and validated both key combinations behaved as expected. Also opened the command palette and ensured that the actions were displayed.

{ "command": "closeTab", "keys": "ctrl+shift+delete" },
{ "command": { "action": "closeTab", "index": 0 }, "keys": "ctrl+shift+end" }

[ianjoneill]

When running the settings model tests in PowerShell, this is the output I got:

PS F:\windows-terminal> Invoke-OpenConsoleTests -Test localSettingsModel
Test Authoring and Execution Framework v10.58k for x64

Warning: TAEF: The test file "F:/windows-terminal\x64\Debug\TestHostApp\SettingsModel.LocalTests.dll" does not exist.
Error: TAEF: None of the specified test files were found.

Summary of TAEF Warnings:
    Warning: TAEF: The test file "F:/windows-terminal\x64\Debug\TestHostApp\SettingsModel.LocalTests.dll" does not exist.

Summary of Errors Outside of Tests:
    Error: TAEF: None of the specified test files were found.

Test Authoring and Execution Framework v10.58k for x64

Warning: TAEF: The test file "F:/windows-terminal\bin\x64\Debug\SettingsModel.LocalTests.dll" does not exist.
Error: TAEF: None of the specified test files were found.

Summary of TAEF Warnings:
    Warning: TAEF: The test file "F:/windows-terminal\bin\x64\Debug\SettingsModel.LocalTests.dll" does not exist.

Summary of Errors Outside of Tests:
    Error: TAEF: None of the specified test files were found.

Those paths don't look right to me - I can find the dll in a different location:

PS F:\windows-terminal> ls F:\windows-terminal\bin\x64\Debug\TestHostApp\SettingsModel.LocalTests.dll

    Directory: F:\windows-terminal\bin\x64\Debug\TestHostApp

Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
-a---          17/06/2021    08:47       11624960 SettingsModel.LocalTests.dll

Also path related - when running Invoke-CodeFormat I got errors too:

PS F:\windows-terminal> Invoke-CodeFormat
All packages listed in packages.config are already installed.
Tool 'xamlstyler.console' (version '3.2008.4') was restored. Available commands: xstyler

Restore was successful.
...
Processed 31 of 31 files.
MethodInvocationException: F:\windows-terminal\tools\OpenConsole.psm1:399
Line |
 399 |          [IO.File]::WriteAllLines("$file", $content)
     |          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     | Exception calling "WriteAllLines" with "2" argument(s): "Could not find a part of the path
     | 'C:\Users\Ian\samples\ConPTY\GUIConsole\GUIConsole.WPF\App.xaml'."
... lots of these ...

Changing

terminal/tools/OpenConsole.psm1

Line 399 in b3b6484

[IO.File]::WriteAllLines("$file", $content)

to [IO.File]::WriteAllLines("$root/$file", $content) seemed to fix that, though I assumed you wouldn't want that change in the same PR.

[ianjoneill]

Only the x86 build failed on a seemingly unrelated test:

2021-06-17T19:57:54.2230324Z StartGroup: ApiRoutinesTests::ApiGetConsoleOriginalTitleA
2021-06-17T19:57:54.2235988Z Verify: IsNotNull(pszExpected)
2021-06-17T19:57:54.2238351Z Verify: Win32BoolSucceeded(WideCharToMultiByte(gci.OutputCP, 0, originalTitle.data(), gsl::narrow_cast<int>(originalTitle.size()), pszExpected.get(), iBytesNeeded, nullptr, nullptr))
2021-06-17T19:57:54.2240676Z Verify: SUCCEEDED(_pApiRoutines->GetConsoleOriginalTitleAImpl(gsl::span<char>(pszTitle, ARRAYSIZE(pszTitle)), cchWritten, cchNeeded))
2021-06-17T19:57:54.2243069Z Verify: AreNotEqual(0u, cchWritten)
2021-06-17T19:57:54.2245602Z Verify: AreEqual(gci.GetOriginalTitle().length() + 1, cchWritten)
2021-06-17T19:57:54.2248016Z Verify: AreEqual(gci.GetOriginalTitle().length(), cchNeeded)
2021-06-17T19:57:54.2251111Z Error: Verify: AreEqual(WEX::Common::String(pszExpected.get()), WEX::Common::String(pszTitle)) - Values (Test original window title.�, Test original window title.) [File: C:\a\1\s\src\host\ut_host\ApiRoutinesTests.cpp, Function: ApiRoutinesTests::ApiGetConsoleOriginalTitleA, Line: 302]
2021-06-17T19:57:54.2254898Z EndGroup: ApiRoutinesTests::ApiGetConsoleOriginalTitleA [Failed]
``

[miniksa]

Changing

terminal/tools/OpenConsole.psm1

Line 399 in b3b6484

[IO.File]::WriteAllLines("$file", $content)

to [IO.File]::WriteAllLines("$root/$file", $content) seemed to fix that, though I assumed you wouldn't want that change in the same PR.

I thought #10268 fixed this...

Only the x86 build failed on a seemingly unrelated test:

2021-06-17T19:57:54.2230324Z StartGroup: ApiRoutinesTests::ApiGetConsoleOriginalTitleA
2021-06-17T19:57:54.2235988Z Verify: IsNotNull(pszExpected)
2021-06-17T19:57:54.2238351Z Verify: Win32BoolSucceeded(WideCharToMultiByte(gci.OutputCP, 0, originalTitle.data(), gsl::narrow_cast<int>(originalTitle.size()), pszExpected.get(), iBytesNeeded, nullptr, nullptr))
2021-06-17T19:57:54.2240676Z Verify: SUCCEEDED(_pApiRoutines->GetConsoleOriginalTitleAImpl(gsl::span<char>(pszTitle, ARRAYSIZE(pszTitle)), cchWritten, cchNeeded))
2021-06-17T19:57:54.2243069Z Verify: AreNotEqual(0u, cchWritten)
2021-06-17T19:57:54.2245602Z Verify: AreEqual(gci.GetOriginalTitle().length() + 1, cchWritten)
2021-06-17T19:57:54.2248016Z Verify: AreEqual(gci.GetOriginalTitle().length(), cchNeeded)
2021-06-17T19:57:54.2251111Z Error: Verify: AreEqual(WEX::Common::String(pszExpected.get()), WEX::Common::String(pszTitle)) - Values (Test original window title.�, Test original window title.) [File: C:\a\1\s\src\host\ut_host\ApiRoutinesTests.cpp, Function: ApiRoutinesTests::ApiGetConsoleOriginalTitleA, Line: 302]
2021-06-17T19:57:54.2254898Z EndGroup: ApiRoutinesTests::ApiGetConsoleOriginalTitleA [Failed]
``

Looks like it passed on rerun or push so nevermind.

[miniksa]

Code looks reasonable to me.

[ianjoneill]

I thought #10268 fixed this...

This SO answer would seem to backup the behaviour I'm seeing - apparently .NET objects don't get the cwd of the shell, so I'm not sure how/why it works for others.

/cc @j4james

[j4james]

Changing

terminal/tools/OpenConsole.psm1

Line 399 in b3b6484

[IO.File]::WriteAllLines("$file", $content)

to [IO.File]::WriteAllLines("$root/$file", $content) seemed to fix that, though I assumed you wouldn't want that change in the same PR.

I don't know the first thing about PowerShell, so I wouldn't be hugely surprised to find out that there was something wrong with my patch, but the above change causes the script to fail for me. I'm running it via the runxamlformat.cmd batch file if that makes any difference.

[ianjoneill]

I'm running it via the runxamlformat.cmd batch file if that makes any difference.

Apparently it does...

PS F:\windows-terminal> $Env:OPENCON_TOOLS = "F:\windows-terminal\tools\"
PS F:\windows-terminal> .\tools\runformat.cmd
All packages listed in packages.config are already installed.
Tool 'xamlstyler.console' (version '3.2008.4') was restored. Available commands: xstyler

Restore was successful.
...
Processed 31 of 31 files.

I think this works because runformat.cmd invokes a new instance of powershell, so .NET uses whatever directory you're in as it's cwd, so [IO.File]::WriteAllLines("$file", $content) works.

Switching to this seems to make it work in any directory, using runformat.cmd or Invoke-CodeFormat - @j4james if you can confirm it works for you, I can put up a separate PR.

    $xamls = (git ls-files --full-name "$root/**/*.xaml")
    foreach ($file in $xamls) {
        $content = Get-Content "$root/$file"
        [IO.File]::WriteAllLines("$root/$file", $content)
    }

[j4james]

if you can confirm it works for you, I can put up a separate PR.

@ianjoneill Yep. That works for me.

[ianjoneill]

Gentle bump as this has been open a week with 1 out of 2 approvals.

[lhecker]

Looks good to me personally. All comments are absolutely minor.

[lhecker]

I'll leave this PR open for a bit in case someone more knowledgeable in these parts of the code has any objections.

@msftbot please wait 1 more day before merging this

[ghost]

Hello @carlos-zamora!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[DHowett]

Thanks for the contribution, and sorry for the delay 😄

[ghost]

🎉Windows Terminal Preview v1.10.1933.0 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes
• Store Download