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

DHowett · 2020-11-03T23:16:34Z

In preparation for the Settings UI, we needed to make some changes to
Tab to abstract out shared functionality common 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

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>

DHowett · 2020-11-03T23:17:15Z

I was one of the two signoffs on both of these pull requests, so I will merge this if @zadjii-msft, @miniksa or @PankajBhojwani sign off. Highly preferable that it's @zadjii-msft.

@carlos-zamora and @leonMSFT do not count.

zadjii-msft

I'm okay with this.

I think in the future, it'd be better to have it all be a uniform Tab with non-terminal panes, but we can work towards that from this. SUI is way higher priority now anyways.

zadjii-msft · 2020-11-04T14:31:37Z

src/cascadia/TerminalApp/TerminalPage.cpp

        if (auto index{ _GetFocusedTabIndex() })
        {
-            auto focusedTab{ _GetStrongTabImpl(*index) };
-            _UnZoomIfNeeded();
-            focusedTab->ResizePane(direction);
+            if (auto terminalTab = _GetTerminalTabImpl(_tabs.GetAt(*index)))


There's a lot of this if (get focused tab index) { if (get tab from index) { tab.doSomething() } } going on in here. Would it be better to have a helper that just does it all in one go?

I’d rather not add that now to reduce the merge burden when this flows back into SUI (since right now i can use the “take all ours” strategy), but it is worth doing.

zadjii-msft · 2020-11-04T14:35:57Z

src/cascadia/TerminalApp/Tab.idl

-    [default_interface] runtimeclass Tab : Windows.UI.Xaml.Data.INotifyPropertyChanged
-    {
-        String Title { get; };
-        Windows.UI.Xaml.Controls.IconSource IconSource { get; };


So just as a sanity check, IconSource wasn't actually being used anywhere, was it?

On account of it compiles and runs fine without IconSource, I am inclined to say yes.

DHowett · 2020-11-04T17:19:23Z

@msftbot merge this in about 1 minute

ghost · 2020-11-04T17:19:26Z

Hello @DHowett!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I won't merge this pull request until after the UTC date Wed, 04 Nov 2020 17:20:25 GMT, which is in 1 minute

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

DHowett · 2020-11-04T18:14:51Z

Oh right, bot don't merge single signoff PRs

miniksa · 2020-11-04T18:17:21Z

lol I was just about to 2nd this.

A fix for UTs crashing due to bad memory access (accessing page 0). Regression from #8153 Closes #8167

DHowett requested a review from zadjii-msft November 3, 2020 23:16

ghost added Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. labels Nov 3, 2020

zadjii-msft approved these changes Nov 4, 2020

View reviewed changes

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Nov 4, 2020

DHowett merged commit 4eeaddc into main Nov 4, 2020

DHowett deleted the dev/duhowett/itab_interface branch November 4, 2020 18:15

Don-Vito mentioned this pull request Nov 5, 2020

Bold matching text in the command palette #7977

Merged

6 tasks

DHowett mentioned this pull request Nov 5, 2020

8167: Fix TabTests - use tab 0 rather than value 0 #8168

Merged

ghost pushed a commit that referenced this pull request Nov 5, 2020

8167: Fix TabTests - use tab 0 rather than value 0 (#8168)

5de4f98

A fix for UTs crashing due to bad memory access (accessing page 0). Regression from #8153 Closes #8167

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

DHowett commented Nov 3, 2020 •

edited

Loading

DHowett commented Nov 3, 2020

zadjii-msft left a comment

zadjii-msft Nov 4, 2020

DHowett Nov 4, 2020

zadjii-msft Nov 4, 2020

DHowett Nov 4, 2020

DHowett commented Nov 4, 2020

ghost commented Nov 4, 2020

DHowett commented Nov 4, 2020

miniksa commented Nov 4, 2020

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

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

Conversation

DHowett commented Nov 3, 2020 • edited Loading

References

Validation Steps Performed

DHowett commented Nov 3, 2020

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft Nov 4, 2020

Choose a reason for hiding this comment

DHowett Nov 4, 2020

Choose a reason for hiding this comment

zadjii-msft Nov 4, 2020

Choose a reason for hiding this comment

DHowett Nov 4, 2020

Choose a reason for hiding this comment

DHowett commented Nov 4, 2020

ghost commented Nov 4, 2020

DHowett commented Nov 4, 2020

miniksa commented Nov 4, 2020

DHowett commented Nov 3, 2020 •

edited

Loading