Add Close... option to Tab context menu #7728

mpela81 · 2020-09-24T17:29:10Z

Summary of the Pull Request

Add a "Close..." option to the tab context menu, with nested entries to close tabs to the right and close other tabs (actions already available)

References

#1912

PR Checklist

Closes Feature request: close tabs to the right #5524
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: #xxx
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

Detailed Description of the Pull Request / Additional comments

First contribution 🙂
Tried to follow some suggestions from #1912 (comment)

Validation Steps Performed

skyline75489 · 2020-09-25T03:05:38Z

Hi! Thanks for your contribution! The engineer who knows the most about this (@zadjii-msft ) is currently on paternity leave. And with the ongoing pandemic and everybody working from home, the team is just even more short-handed. Just want to let you know the feedback from the team may be slow. But I'm sure they will be more than willing to review your PR when they are available.

Why do I use they, you ask? Well I'm not part of the internal engineering team. I'm just a MS employee who wrote some code for this project. Feel free to contact me. Thanks again 😃

mpela81 · 2020-09-30T12:43:06Z

@skyline75489, thanks for your comment, needless to say the team can review this at their convenience.

Well, by the way, I may need some expert hand as I'm getting random crashes 😞
I don't have exact repro steps (play with tabs and their menus for a while). The call stack is all about external code, no clue at the moment on how to debug it 🤔

Windows.UI.Xaml.dll!GetProgrammaticHitTestingParams(CCoreServices * coreServices, CUIElement * uiElement, ITransformer * * transformer, CUIElement * * ppThisNoRef, CPopupRoot * * popupRootNoRef, CUIElement * * popupHitTestSubtreeRootNoRef, bool * isRootNull) Line 5395    C++
    Windows.UI.Xaml.dll!UIElement_HitTestPoint(CUIElement * uiElement, CCoreServices * coreServices, XPOINTF ptHit, bool canHitDisabledElements, bool canHitInvisibleElements, int * pnCount, CUIElement * * * ppElements) Line 5513    C++
    Windows.UI.Xaml.dll!DirectUI::VisualTreeHelper::FindElementsInHostCoordinatesPointStatic(Windows::Foundation::Point intersectingPoint, Windows::UI::Xaml::IUIElement * pSubTree, unsigned char ppElements, unsigned char) Line 250  C++
    Windows.UI.Xaml.dll!DirectUI::VisualTreeHelper::FindAllElementsInHostCoordinatesPointStatic(Windows::Foundation::Point intersectingPoint, Windows::UI::Xaml::IUIElement * pSubTree, unsigned char includeAllElements, Windows::Foundation::Collections::IIterable<Windows::UI::Xaml::UIElement *> * * ppElements) Line 329  C++
    Windows.UI.Xaml.dll!DirectUI::CascadingMenuHelper::OnPointerExited(Windows::UI::Xaml::Input::IPointerRoutedEventArgs * args, bool parentIsSubMenu) Line 284 C++
    Windows.UI.Xaml.dll!DirectUI::MenuFlyoutSubItem::OnPointerExited(Windows::UI::Xaml::Input::IPointerRoutedEventArgs * args) Line 140 C++
    Windows.UI.Xaml.dll!DirectUI::ControlGenerated::OnPointerExitedProtected(Windows::UI::Xaml::Input::IPointerRoutedEventArgs * pE) Line 1404  C++
    Windows.UI.Xaml.dll!DirectUI::Control::FireEvent(KnownEventIndex nDelegate, DirectUI::DependencyObject * pSender, IInspectable * pArgs) Line 267    C++
...

Edit: also occurs with one tab only, just open/close the menu a few times in a row.

Thanks again!

zadjii-msft

This is awesome, thanks for doing this!

zadjii-msft · 2020-10-07T12:44:14Z

As far as the crash is concerned - I'm not seeing that locally when playing around with this. I vaguely recall seeing a similar crash when debugging a MenuFlyout like, maybe 14 months ago, but the crash ended up being a transient thing, or was related to XAML debugging or something. I think this is probably safe to go in now, and we can keep an eye on it

mpela81 · 2020-10-07T16:32:09Z

Noticed that master branch has progressed in the meantime, I guess PR will require some change because of the new TerminalSettingsModel #7667

…bmenu # Conflicts: # src/cascadia/TerminalApp/Tab.cpp

leonMSFT

This is great, I love the detail of disabling the menu items if it doesn't apply to that tab! 😄

mpela81 · 2020-10-13T17:35:43Z

Ah thanks for your reviews so far 🤞

zadjii-msft · 2020-10-14T12:42:28Z

/azp run

azure-pipelines · 2020-10-14T12:42:45Z

Azure Pipelines successfully started running 1 pipeline(s).

ghost · 2020-10-15T07:31:39Z

Hello @zadjii-msft!

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.

zadjii-msft · 2020-10-15T08:10:26Z

This is bizarre, Github can't seem to forget about this one failed CI build from a week ago, despite re-running the CI successfully since then. I think we're going to have to merge this one manually with an override - the CI failure was definitely unrelated to this PR, and the CI has subsequently passed...

Fix for crash occurring when splitting a pane, due to tab context menu created multiple times. ## References #7728 ## PR Checklist * [x] Closes #7941 * [x] CLA signed. ## Detailed Description of the Pull Request / Additional comments When splitting panes the `Tab::Initialize` function is called again. This rebuilt the context menu from scratch and appended the existing Close... sub-menu items to a new parent, thus causing the crash. It is not necessary to re-create the context menu every time you split panes, it can be created only once. ## Validation Steps Performed Manual verification: - Play with the context menu, the Close... submenu is functioning - Split panes (ALT + New tab), no crash occurs and context menu still functioning

`TerminalTab` and `SettingsTab` share some implementation details. The close submenu introduced in #7728 is a good example of functionality that is consistent across all tabs. This PR transforms `ITab` from an interface, into an [unsealed runtime class] to de-duplicate some functionality. Most of the logic from `SettingsTab` was moved there because I expect the default behavior of a tab to resemble the `SettingsTab` over a `TerminalTab`. ## References Verified that Close submenu work was transferred over (#7728, #7961, #8010). ## Validation Steps Performed Check close submenu on first/last tab when multiple tabs are open. Closes #7969 [unsealed runtime class]: https://docs.microsoft.com/en-us/uwp/midl-3/intro#base-classes

@leonMSFT

In preparation for the Settings UI, we needed to make some changes to Tab to abstract out shared, common functionality between different types of tab. This is the result of that work. All code references to the settings have been removed or reverted. Contains changes from #8053, #7802. The messages below only make sense in the context of the Settings UI, which this pull request does not bring in. They do, however, provide valuable information. From #7802 (@leonMSFT): > This PR's goal was to add an option to the `OpenSettings` keybinding to > open the Settings UI in a tab. In order to implement that, a couple of > changes had to be made to `Tab`, specifically: > > - Introduce a tab interface named `ITab` > - Create/Rename two new Tab classes that implement `ITab` called > `SettingsTab` and `TerminalTab` > From #8053: > `TerminalTab` and `SettingsTab` share some implementation details. The > close submenu introduced in #7728 is a good example of functionality > that is consistent across all tabs. This PR transforms `ITab` from an > interface, into an [unsealed runtime class] to de-duplicate some > functionality. Most of the logic from `SettingsTab` was moved there > because I expect the default behavior of a tab to resemble the > `SettingsTab` over a `TerminalTab`. > > ## References > Verified that Close submenu work was transferred over (#7728, #7961, #8010). > > ## Validation Steps Performed > Check close submenu on first/last tab when multiple tabs are open. > > Closes #7969 > > [unsealed runtime class]: https://docs.microsoft.com/en-us/uwp/midl-3/intro#base-classes Co-authored-by: Carlos Zamora <carlos.zamora@microsoft.com>

@leonMSFT

In preparation for the Settings UI, we needed to make some changes to Tab to abstract out shared, common functionality between different types of tab. This is the result of that work. All code references to the settings have been removed or reverted. Contains changes from #8053, #7802. The messages below only make sense in the context of the Settings UI, which this pull request does not bring in. They do, however, provide valuable information. From #7802 (@leonMSFT): > This PR's goal was to add an option to the `OpenSettings` keybinding to > open the Settings UI in a tab. In order to implement that, a couple of > changes had to be made to `Tab`, specifically: > > - Introduce a tab interface named `ITab` > - Create/Rename two new Tab classes that implement `ITab` called > `SettingsTab` and `TerminalTab` > From #8053: > `TerminalTab` and `SettingsTab` share some implementation details. The > close submenu introduced in #7728 is a good example of functionality > that is consistent across all tabs. This PR transforms `ITab` from an > interface, into an [unsealed runtime class] to de-duplicate some > functionality. Most of the logic from `SettingsTab` was moved there > because I expect the default behavior of a tab to resemble the > `SettingsTab` over a `TerminalTab`. > > ## References > Verified that Close submenu work was transferred over (#7728, #7961, #8010). > > ## Validation Steps Performed > Check close submenu on first/last tab when multiple tabs are open. > > Closes #7969 > > [unsealed runtime class]: https://docs.microsoft.com/en-us/uwp/midl-3/intro#base-classes Co-authored-by: Carlos Zamora <carlos.zamora@microsoft.com>

@leonMSFT

In preparation for the Settings UI, we needed to make some changes to Tab to abstract out shared, common functionality between different types of tab. This is the result of that work. All code references to the settings have been removed or reverted. Contains changes from #8053, #7802. The messages below only make sense in the context of the Settings UI, which this pull request does not bring in. They do, however, provide valuable information. From #7802 (@leonMSFT): > This PR's goal was to add an option to the `OpenSettings` keybinding to > open the Settings UI in a tab. In order to implement that, a couple of > changes had to be made to `Tab`, specifically: > > - Introduce a tab interface named `ITab` > - Create/Rename two new Tab classes that implement `ITab` called > `SettingsTab` and `TerminalTab` > From #8053: > `TerminalTab` and `SettingsTab` share some implementation details. The > close submenu introduced in #7728 is a good example of functionality > that is consistent across all tabs. This PR transforms `ITab` from an > interface, into an [unsealed runtime class] to de-duplicate some > functionality. Most of the logic from `SettingsTab` was moved there > because I expect the default behavior of a tab to resemble the > `SettingsTab` over a `TerminalTab`. > > ## References > Verified that Close submenu work was transferred over (#7728, #7961, #8010). > > ## Validation Steps Performed > Check close submenu on first/last tab when multiple tabs are open. > > Closes #7969 > > [unsealed runtime class]: https://docs.microsoft.com/en-us/uwp/midl-3/intro#base-classes Co-authored-by: Carlos Zamora <carlos.zamora@microsoft.com>

@leonMSFT

In preparation for the Settings UI, we needed to make some changes to Tab to abstract out shared, common functionality between different types of tab. This is the result of that work. All code references to the settings have been removed or reverted. Contains changes from #8053, #7802. The messages below only make sense in the context of the Settings UI, which this pull request does not bring in. They do, however, provide valuable information. From #7802 (@leonMSFT): > This PR's goal was to add an option to the `OpenSettings` keybinding to > open the Settings UI in a tab. In order to implement that, a couple of > changes had to be made to `Tab`, specifically: > > - Introduce a tab interface named `ITab` > - Create/Rename two new Tab classes that implement `ITab` called > `SettingsTab` and `TerminalTab` > From #8053: > `TerminalTab` and `SettingsTab` share some implementation details. The > close submenu introduced in #7728 is a good example of functionality > that is consistent across all tabs. This PR transforms `ITab` from an > interface, into an [unsealed runtime class] to de-duplicate some > functionality. Most of the logic from `SettingsTab` was moved there > because I expect the default behavior of a tab to resemble the > `SettingsTab` over a `TerminalTab`. > > ## References > Verified that Close submenu work was transferred over (#7728, #7961, #8010). > > ## Validation Steps Performed > Check close submenu on first/last tab when multiple tabs are open. > > Closes #7969 > > [unsealed runtime class]: https://docs.microsoft.com/en-us/uwp/midl-3/intro#base-classes Co-authored-by: Carlos Zamora <carlos.zamora@microsoft.com> Co-authored-by: Leon Liang <lelian@microsoft.com> Co-authored-by: Carlos Zamora <carlos.zamora@microsoft.com>

ghost · 2020-11-11T18:12:53Z

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

Handy links:

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.

Add Close... sub-menu to Tab

003c59a

ghost added Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Sep 24, 2020

Enable menu items only when applicable

7d9e399

zadjii-msft self-assigned this Oct 1, 2020

carlos-zamora mentioned this pull request Oct 2, 2020

Add functionality to open the Settings UI tab through openSettings #7802

Merged

mpela81 marked this pull request as ready for review October 5, 2020 08:09

zadjii-msft approved these changes Oct 7, 2020

View reviewed changes

zadjii-msft removed their assignment Oct 7, 2020

zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Oct 7, 2020

ghost requested review from miniksa, carlos-zamora, DHowett, leonMSFT and PankajBhojwani October 7, 2020 14:15

mpela81 added 2 commits October 9, 2020 14:11

Merge remote-tracking branch 'upstream/master' into feat/tab_close_su…

115c4a1

…bmenu # Conflicts: # src/cascadia/TerminalApp/Tab.cpp

Align to new TerminalSettingsModel

093617b

leonMSFT approved these changes Oct 9, 2020

View reviewed changes

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Oct 15, 2020

zadjii-msft merged commit 004da88 into microsoft:master Oct 15, 2020

mpela81 deleted the feat/tab_close_submenu branch October 15, 2020 12:32

DHowett mentioned this pull request Oct 16, 2020

Cannot use ALT to split panes after opening Close... tab submenu #7941

Closed

mpela81 mentioned this pull request Oct 19, 2020

Fix crash when creating tab menu multiple times #7961

Merged

2 tasks

carlos-zamora mentioned this pull request Oct 27, 2020

Make ITab an unsealed runtimeclass #8053

Merged

DHowett mentioned this pull request Nov 3, 2020

Make Tab an unsealed runtimeclass (and rename it to TabBase) #8153

Merged

ghost mentioned this pull request Nov 11, 2020

Feature request: close tabs to the right #5524

Closed

mpela81 mentioned this pull request Nov 12, 2020

Crash opening/closing close... submenu multiple times #8238

Closed

zadjii-msft mentioned this pull request Apr 27, 2023

Move the Close... actions to a nested menu on the tab #15250

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Close... option to Tab context menu #7728

Add Close... option to Tab context menu #7728

mpela81 commented Sep 24, 2020

skyline75489 commented Sep 25, 2020

mpela81 commented Sep 30, 2020 •

edited

Loading

zadjii-msft left a comment

zadjii-msft commented Oct 7, 2020

mpela81 commented Oct 7, 2020

leonMSFT left a comment

mpela81 commented Oct 13, 2020

zadjii-msft commented Oct 14, 2020

azure-pipelines bot commented Oct 14, 2020

ghost commented Oct 15, 2020

zadjii-msft commented Oct 15, 2020

ghost commented Nov 11, 2020

Add Close... option to Tab context menu #7728

Add Close... option to Tab context menu #7728

Conversation

mpela81 commented Sep 24, 2020

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

skyline75489 commented Sep 25, 2020

mpela81 commented Sep 30, 2020 • edited Loading

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft commented Oct 7, 2020

mpela81 commented Oct 7, 2020

leonMSFT left a comment

Choose a reason for hiding this comment

mpela81 commented Oct 13, 2020

zadjii-msft commented Oct 14, 2020

azure-pipelines bot commented Oct 14, 2020

ghost commented Oct 15, 2020

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.

zadjii-msft commented Oct 15, 2020

ghost commented Nov 11, 2020

mpela81 commented Sep 30, 2020 •

edited

Loading

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.