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

Added Close Pane to Context Menu #15198

Conversation

joadoumie
Copy link
Contributor

Summary of the Pull Request

Adding a 'Close Pane' menu item in the context menu.

References and Relevant Issues

#13580

Detailed Description of the Pull Request / Additional comments

If a user decides to split a tab to create multiple panes through the context menu, they should be able to then close the pane via the context menu too. This PR introduces a new context menu item, 'Close Pane', that only appears when a user has 2 or more panes in a tab. When a user clicks close pane, the _active_pane will be closed.

Validation Steps Performed

close_pane_terminal

As it's my first PR, I still need to understand how to go through the testing suite.

PR Checklist

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Task It's a feature request, but it doesn't really need a major design. Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Product-Terminal The new Windows Terminal. labels Apr 18, 2023
@zadjii-msft
Copy link
Member

I still need to understand how to go through the testing suite.

You're in luck! For better or worse, this part of the code actually doesn't have any tests! So don't worry too much ☺️

You will likely need to run tools\runformat.cmd though, to get the automatic code-formatter to run 😄

@joadoumie
Copy link
Contributor Author

I get this error when I try to run the code formatter - any suggestions for how to fix?

image

@joadoumie
Copy link
Contributor Author

Also - any suggestions on what the icon should be @zadjii-msft - https://learn.microsoft.com/en-us/windows/apps/design/style/segoe-ui-symbol-font?

@joadoumie
Copy link
Contributor Author

For whatever reason the cmd script wasn't working for me, but is this equivalent?

image

@joadoumie joadoumie marked this pull request as ready for review April 19, 2023 01:54
@zadjii-msft zadjii-msft self-assigned this Apr 20, 2023
@zadjii-msft
Copy link
Member

but is this equivalent

Yep looks like it! I bet the cmd version wasn't robust enough to be run from outside the solution root directory 🤷

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

As far as icons, maybe e89f, "ClosePane"? That's not great, since like, you could have a horizontal split, or be closing the left one... Honestly, I think it's okay without one for now.

src/cascadia/TerminalApp/TerminalTab.cpp Outdated Show resolved Hide resolved
@zadjii-msft
Copy link
Member

Sorry, yea, this is an admittedly crazy obscure feature I don't think most folks have ever used:

15198-parent-pane-thing

@zadjii-msft zadjii-msft removed their assignment Apr 20, 2023
@joadoumie
Copy link
Contributor Author

joadoumie commented Apr 21, 2023

Is there any place where we would know that there's only one pane left?

Update to this comment: The below suggestion I made does not work as ctl + shift + w uses ClosePane --> back to the drawing board :).

Does it make sense to call DetachPane instead of ClosePane in the menu item click?

closePaneMenuItem.Click([weakThis](auto&&, auto&&) { if (auto tab{ weakThis.get() }) { tab->DetachPane(); } });

Then in DetachPane it makes sense to me that we would know that only one pane is left.

std::shared_ptr<Pane>` TerminalTab::DetachPane()
    {
        // if we only have one pane, or the focused pane is the root, remove it
        // entirely and close this tab
        if (_rootPane == _activePane)
        {
            return DetachRoot();
        }
        // Attempt to remove the active pane from the tree
        if (const auto pane = _rootPane->DetachPane(_activePane))
        {
            // Just make sure that the remaining pane is marked active
            _UpdateActivePane(_rootPane->GetActivePane());

            if (_rootPane->GetLeafPaneCount() == 1)
            {
                this->_closePaneMenuItem.Visibility(WUX::Visibility::Collapsed);
            }

            return pane;
        }

        return nullptr;
    }

With these changes it seems like it resolves the edge case you mentioned. I'm very new to the codebase so if this is not quite the way you were thinking this should be implemented let me know :)

Not really sure what DetachPane vs. ClosePane does.

@joadoumie
Copy link
Contributor Author

Is there any place where we would know that there's only one pane?

I'm not sure if this is what you were thinking but seems to work ok if the check is in _UpdateActivePane().

@joadoumie joadoumie requested a review from zadjii-msft April 21, 2023 18:09
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Personally, I think we should...

  1. remove the tooltip (doesn't seem useful)
  2. make the menu item always visible/active (regardless of number of open panes)

Thoughts? Open to discussion/disagreement on this one.

src/cascadia/TerminalApp/TerminalTab.cpp Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalTab.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalTab.cpp Show resolved Hide resolved
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 25, 2023
@carlos-zamora carlos-zamora added Needs-Discussion Something that requires a team discussion before we can proceed and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Apr 25, 2023
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

I love this. This is exactly what I'd've done. Thanks for bearing with us!

Comment on lines +1056 to +1059
if (_rootPane->GetLeafPaneCount() == 1)
{
_closePaneMenuItem.Visibility(WUX::Visibility::Collapsed);
}
Copy link
Member

Choose a reason for hiding this comment

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

this is infinitely better than before ☺️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yay! Completely new to the project and really appreciate the support and guidance from both you and @carlos-zamora. Excited to try out some new issues too.

FYI, I LOVE the walkthroughs. Makes it way easier to get started for people who aren't as experienced in C++ or the Terminal project :)

Co-authored-by: Carlos Zamora <carlos.zamora@microsoft.com>
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Love it. Thanks so much!

@carlos-zamora carlos-zamora added AutoMerge Marked for automatic merge by the bot when requirements are met and removed Needs-Discussion Something that requires a team discussion before we can proceed labels Apr 26, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot merged commit ca5834e into microsoft:main Apr 26, 2023
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

Love it, thanks for the contribution @joadoumie !

DHowett pushed a commit that referenced this pull request Apr 28, 2023
A resurrection of the original nested "Close" menu from #7728. We
discovered that nested flyouts crash in #8238. Those are fixed now
though! So we can bring this back.

This also includes the "Close Pane" item from #15198.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-UserInterface Issues pertaining to the user interface of the Console or Terminal AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add close pane to the tab context menu (when there are multiple panes)
4 participants