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

closeTab should accept an index param #7180

Closed
zadjii-msft opened this issue Aug 4, 2020 · 3 comments · Fixed by #10447
Closed

closeTab should accept an index param #7180

zadjii-msft opened this issue Aug 4, 2020 · 3 comments · Fixed by #10447
Labels
Area-Settings Issues related to settings and customizability, for console or terminal good first issue This is a fix that might be easier for someone to do as a first contribution Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@zadjii-msft
Copy link
Member

Similar to the other closetab actions being introduced in #7176.

This is being tracked with the other support for customizing the new-tab dropdown, which will need the ability to specify and action for the "close tab" menu item

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Aug 4, 2020
@zadjii-msft zadjii-msft added Area-Settings Issues related to settings and customizability, for console or terminal good first issue This is a fix that might be easier for someone to do as a first contribution Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Aug 4, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Aug 4, 2020
@zadjii-msft zadjii-msft added this to the Terminal Backlog milestone Aug 4, 2020
@DHowett DHowett removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Aug 6, 2020
@KimKiHyuk
Copy link

KimKiHyuk commented Aug 21, 2020

Could you elaborate on it?

I didn't catch why closetab should accept index parameter.

you want to modify CloseTabsAction and _HandleCloseTab like #7176 ?

then, _HandleCloseTab method will be modified like under code

void TerminalPage::_HandleCloseTab(const IInspectable& /*sender*/,
                                       const TerminalApp::ActionEventArgs& args)
{
     if (const auto& realArgs = actionArgs.ActionArgs().try_as<TerminalApp::CloseTabArgs>())
     {
            uint32_t index = realArgs.Index(); // do you want to get it?

            _CloseFocusedTab();
             args.Handled(true);
      }
}

@zadjii-msft
Copy link
Member Author

@KimKiHyuk Pretty much yea - except, it'll be more like how it's implemented in #7390. So the implementation would be more like:

void TerminalPage::_HandleCloseTab(const IInspectable& /*sender*/,
                                       const TerminalApp::ActionEventArgs& args)
{
    if (const auto& realArgs = actionArgs.ActionArgs().try_as<TerminalApp::CloseTabArgs>())
    {
        if (realArgs.Index()) {
            // The user provided an `index`

            const auto index = realArgs.Index().Value();
            const bool handled = _CloseTheNthTab(index); // TODO: This function would need to be implemented of course
            args.Handled(handled); 
        }
        else
        {
            // No index was provided - close the currently active tab.
            _CloseFocusedTab();
             args.Handled(true);
        }
    }
}

@ghost ghost added the In-PR This issue has a related PR label Jun 17, 2021
@ghost ghost closed this as completed in #10447 Jun 25, 2021
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Jun 25, 2021
ghost pushed a commit that referenced this issue Jun 25, 2021
## Summary of the Pull Request
Updates the `closeTab` action to optionally take an index.

## PR Checklist
* [x] Closes #7180
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [x] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: MicrosoftDocs/terminal#347
* [x] 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.

```json
{ "command": "closeTab", "keys": "ctrl+shift+delete" },
{ "command": { "action": "closeTab", "index": 0 }, "keys": "ctrl+shift+end" }
```
@ghost
Copy link

ghost commented Jul 14, 2021

🎉This issue was addressed in #10447, which has now been successfully released as Windows Terminal Preview v1.10.1933.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Settings Issues related to settings and customizability, for console or terminal good first issue This is a fix that might be easier for someone to do as a first contribution Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants