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

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

Merged
26 commits merged into from
Oct 6, 2020

Conversation

leonMSFT
Copy link
Contributor

@leonMSFT leonMSFT commented Oct 1, 2020

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

I've also put some TODOs that I wanted to get some thoughts on - I'll make a follow up PR but perhaps they can be revisited when we flesh out the Settings UI more.

  • Does a Settings UI tab shutdown need to do anything special for cleanup?
  • Does a Settings UI tab need to have GetActiveTitle? Maybe depending on which page in the UI is open?
  • Technically, I can't focus a Grid control, so I'll need to figure out what to Focus when the tab is selected.
  • The Settings UI tab doesn't have a TermControl, so once focus is moved to that tab, users won't be able to nextTab/prevTab out of it (along with all other keybindings).

References #1564, #5915

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.

note to self: review SettingsTab, TerminalPage::_OpenSettingsUI

src/cascadia/TerminalSettingsEditor/MainPage.idl Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/ITab.idl Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/ITab.idl Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/ITab.idl Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/ITab.idl Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/ITab.idl Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/Utils.h Show resolved Hide resolved
@leonMSFT leonMSFT marked this pull request as ready for review October 1, 2020 21:29
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.

This looks great! I want to download this branch and play around with it for a bit. I also have a few questions:

  • Is the "Settings" button in the dropdown hookup up? If not, that should be an easy one we can cross of our list.

@zadjii-msft (for when you come back) I'd feel more comfortable if you took a closer look at this PR since you seem pretty familiar with this layer.

NOTE: this targets the feature branch so we're on a "1 approval policy". But we'll do one big review as a team when the Settings UI is ready and the feature branch gets merged into master.

src/cascadia/TerminalApp/Utils.h Show resolved Hide resolved
src/cascadia/TerminalApp/AppLogic.h Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalAppLib.vcxproj Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Show resolved Hide resolved
src/cascadia/TerminalApp/SettingsTab.h Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/SettingsTab.cpp Outdated Show resolved Hide resolved
Comment on lines +111 to +113
// TODO: Does/Will the settings UI need some shutdown procedures?
Content(nullptr);
_ClosedHandlers(nullptr, nullptr);
Copy link
Member

Choose a reason for hiding this comment

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

My train of thought 🚂🧠 When the Settings UI is opened, we to create a clone of CascadiaSettings. As the user makes changes, we directly modify that clone. When the user saves the changes, that clone gets promoted to be the real CascadiaSettings. When we close the Settings UI, the Settings UI will own that clone. So the clone dies. All of the Settings UI is also destroyed, so I think that'll be fine.
I think we'll be fine. Would definitely like a second opinion from @DHowett if possible.

// - <none>
// Return Value:
// - <none>
void SettingsTab::_CreateContextMenu()
Copy link
Member

Choose a reason for hiding this comment

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

Hmm... Maybe this context menu should be abstracted differently. I'm looking at #7728 and we would have to duplicate that code here and in TerminalTab. Here's a few ideas:

  1. Could we make ITab an abstract class instead of an interface? That way the context menu gets inherited by both (and all future tabs)
  2. Should we move the responsibility of populating the context menu over to TerminalPage? That way it checks the type of tab we have, and populates it accordingly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I def agree with wanting to clean up the context menu creation in light of incoming additions in that PR and in the future. Here's another idea - I kinda want to make our own custom control named something like "TabContextFlyout".

  • Flyout menu items become xaml code - we won't need to have cpp code that creates our menu items.
  • Each tab type owns one of these controls.
  • Each tab can hook itself up to the Click events of each of the control's menu items that they wish to use. Then we can turn each menu item's visibility off if the click handler isn't registered.

Copy link
Member

Choose a reason for hiding this comment

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

You got me at "become xaml code" haha.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright this might take a little longer than I thought. I'm gonna file a follow up issue for this so that I can get this PR in, work on the issue on the side and unblock myself for some data binding work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

issue filed here: #7841

src/cascadia/TerminalApp/SettingsTab.cpp Outdated Show resolved Hide resolved
@carlos-zamora
Copy link
Member

/azp run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

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.

Approving w/ suggestions

src/cascadia/TerminalApp/TerminalPage.cpp Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Show resolved Hide resolved
src/cascadia/TerminalApp/Utils.h Show resolved Hide resolved
@leonMSFT leonMSFT added the AutoMerge Marked for automatic merge by the bot when requirements are met label Oct 6, 2020
@ghost
Copy link

ghost commented Oct 6, 2020

Hello @leonMSFT!

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.

@ghost ghost merged commit bde5d5e into feature/settings-ui Oct 6, 2020
@ghost ghost deleted the dev/lelian/tse-keybinding branch October 6, 2020 22:22
@german-one
Copy link
Contributor

Ignore me if this was on purpose … The PR has not been merged into master.

@zadjii-msft
Copy link
Member

zadjii-msft commented Oct 7, 2020

Hey uh, was this supposed to get merged? since it wasn't going into master, the bot wasn't going to enforce the two signoff requirement...

EDIT: @german-one ninja'd me

@leonMSFT
Copy link
Contributor Author

leonMSFT commented Oct 7, 2020

Oh yeah this was 100% intended to go into the feature branch for the SUI, not master. I got so scared waking up to these two comments, I thought it accidentally went into master instead 😆

@german-one
Copy link
Contributor

To ruin your morning was 100% unintended 😦

@carlos-zamora
Copy link
Member

We're just doing one sign-off needed for feature/settings-ui. We'll do a full review when that merges into master in preparation for release. So occasionally we'll tag team members for a closer look or some immediate feedback. We were all surprised that AutoMerge worked hahaha.

@zadjii-msft
Copy link
Member

Ah okay. I was planning on giving feedback on this part, but nbd. It would have probably boiled down to "I have concerns, but we need to ship the SUI so :shipit:". The concerns aren't that important now TBH

@DHowett
Copy link
Member

DHowett commented Oct 7, 2020

@zadjii-msft they’re still worth tracking, so do write them up! Since this will merge to main eventually

DHowett pushed a commit that referenced this pull request Nov 3, 2020
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 pushed a commit that referenced this pull request Nov 3, 2020
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 pushed a commit that referenced this pull request Nov 3, 2020
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 added a commit that referenced this pull request Nov 4, 2020
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>
@zadjii-msft zadjii-msft mentioned this pull request Feb 18, 2021
zadjii-msft added a commit that referenced this pull request Aug 24, 2023
I originally just wanted to close #1104, but then discovered that hey,
this event wasn't even used anymore. Excerpts of Teams convo:

* [Snap to character grid when resizing window by mcpiroman · Pull
Request #3181 · microsoft/terminal
(github.com)](https://github.com/microsoft/terminal/pull/3181/files#diff-d7ca72e0d5652fee837c06532efa614191bd5c41b18aa4d3ee6711f40138f04c)
added it to Tab.cpp
  * where it was added 
  * which called `pane->Relayout` which I don't even REMEMBER
* By [Add functionality to open the Settings UI tab through openSettings
by leonMSFT · Pull Request #7802 · microsoft/terminal
(github.com)](https://github.com/microsoft/terminal/pull/7802/files#diff-83d260047bed34d3d9d5a12ac62008b65bd6dc5f3b9642905a007c3efce27efd),
there was seemingly no FontSizeChanged in Tab.cpp (when it got moved to
terminaltab.cpp)
 

> `Pane::Relayout` functionally did nothing because sizing was switched
 to `star` sizing at some point in the past, so it was just deleted.

From [Misc pane refactoring by Rosefield · Pull Request #11373 ·
microsoft/terminal](https://github.com/microsoft/terminal/pull/11373/files#r736900998)

So, great. We can kill part of it, and convert the rest to a
`TypedEvent`, and get rid of `DECLARE_` / `DEFINE_`.

`ScrollPositionChangedEventArgs` was ALSO apparently already promoted to
a typed event, so kill that too.
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants