terminal icon indicating copy to clipboard operation
terminal copied to clipboard

Disable "Always show tabs" when "Hide the title bar" is enabled

Open leejy12 opened this issue 2 years ago • 1 comments

In Global Appearance SUI, the toggle switch for "Always show tabs" is now grayed out when "Hide the title bar" is enabled (the default).

Also, the order of the settingcontainers was changed to help show the cause and effect more. See this comment.

Closes #12873

leejy12 avatar Aug 06 '22 12:08 leejy12

Huh, not sure why my old commits are visible here ...

leejy12 avatar Aug 06 '22 12:08 leejy12

Ok, I'll start working on it!

leejy12 avatar Aug 12 '22 03:08 leejy12

I have some questions about implementing this w/ ViewModels.

Moving ShowTitleBar...Toggled into the Appearance View Model?

I moved the event handler ShowTitlebarToggled to the ViewModel and to bind this to the switch, I think it needs to be a static method. Because of this, editing the ViewModel properties in GlobalAppearanceViewModel::ShowTitlebarToggled is not possible.

I think I should change ShowTitlebarToggled to a non-static member function and bind to the switch, but IDK how to achieve this. Then, the implementation (currently commented out) should work, I think. Is this the right approach @DHowett?

leejy12 avatar Aug 23 '22 09:08 leejy12

@zadjii-msft I rerequested your review so that you could get eyes on the rewrite

DHowett avatar Aug 24 '22 17:08 DHowett

Thanks for the suggestions and approving 😁! But I still have some questions because I'm not seeing the result I want to achieve.

Currently, toggling ShowTabsInTitlebar doesn't immediately affect the AlwaysShowTabs toggle switch. The state of AlwaysShowTabs is changed when the Save button is clicked.

Here's the behavior I was expecting.

  1. User toggles ShowTabsInTitlebar from OFF to ON.
  2. GlobalAppearanceViewModel::ShowTitlebarToggled is called.
  3. ShowTabsInTitlebar() returns true (because it's now on)
  4. AlwaysShowTabs(true) is called.
  5. AlwaysShowTabs toggle switch turns on and becomes disabled, thanks to XAML binding.

However, I'm observing that ShowTabsInTitlebar() returns false, and UI of AlwaysShowTabs switch is unchanged. So the "cause and effect" is not reflected well to the user.

My code avoids the "invalid" setting where ShowTabsInTitlebar=true and AlwaysShowTabs=false because GlobalAppearanceViewModel::ShowTitlebarToggled is called when we navigate to the GlobalAppearance page, which was what the original issue was about.

leejy12 avatar Aug 24 '22 18:08 leejy12

This PR may have pointed us at a incredibly subtle bug in basically all our viewmodel code. We're debugging at the moment ☺️

zadjii-msft avatar Aug 24 '22 20:08 zadjii-msft

Based on the fact that this is actually exposing a bug that we can fix in another PR, I'm inclined to just merge this is, under the knowledge that the PR I'm about to file will fix this

zadjii-msft avatar Aug 24 '22 21:08 zadjii-msft

I added a final commit that should complete this! (after #13869 merges.)

It now works beautifully.

binding

leejy12 avatar Aug 25 '22 01:08 leejy12

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.

ghost avatar Aug 25 '22 13:08 ghost

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

Handy links:

ghost avatar Sep 13 '22 17:09 ghost