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

Represent inheritance in Settings UI #8919

Merged
18 commits merged into from
Feb 8, 2021
Merged

Conversation

carlos-zamora
Copy link
Member

@carlos-zamora carlos-zamora commented Jan 28, 2021

Summary of the Pull Request

Introduces the SettingContainer. SettingContainer is used to wrap a setting in the settings UI and provide the following functionality:

  • a reset button next to the header
  • tooltips and automation properties for the setting being wrapped
  • a comment stating if you are currently overriding a setting

References

Spec - Inheritance in Settings UI
#8804 - removes the ambiguity of leaving a setting blank
#6800 - Settings UI Epic
#8899 - Automation properties for Settings UI
#8768 - Keyboard Navigation

PR Checklist

Detailed Description of the Pull Request / Additional comments

A few highlights in this PR:

  • CommonResources.xaml:
    • we need to merge the SettingContainerStyle.xaml in there. Otherwise, XAML doesn't merge these files properly and can't apply the template.
  • Profiles.cpp:
    • view model checks if the starting directory and background image were reset, to determine which value to show when unchecking the special value
    • Profiles::OnNavigatedTo() needs a property changed handler to update its own "Current" and update the UI properly
  • Profiles.xaml:
    • basically wrapped all of the settings we want to be inheritable in there
    • Binding is used instead of x:Bind in some places because x:Bind can't find the parent SettingContainer and gives you a compiler error.
  • Resources.resw:
    • had to set the "HeaderText" and "HelpText" on each setting container. Does a decent localization burden, unfortunately.
  • SettingContainer files
    • This operates by creating a template and applying that template over other settings. This allows you to inject the existing controls inside of this. This means that we need to provide our UIElements names and access/modify them via OnApplyTemplate
    • We had to remove the header from each individual control, and have SettingContainer be in charge of it. This allows us to add the reset button in there.
    • Due to the problem mentioned earlier about CommonResources.xaml, we can't reference anything from CommonResources.xaml.
    • Using DependencyProperty to let us set a few properties in the XML files. Particularly, Has<Setting> and Clear<Setting> are what do all the heavy lifting of interacting with the inheritance model.

Demo

Inheritance Demo

Validation Steps Performed

  • Verified correct binding behavior with the following generic setting controls:
    • radio buttons
    • toggle switch
    • text block
    • slider
    • settings with browse buttons
    • the background image alignment control
    • controls with special check boxes (starting directory and background image)

Next Steps

@ghost ghost added Area-Settings UI Anything specific to the SUI Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. labels Jan 28, 2021
@carlos-zamora carlos-zamora added this to the Terminal v1.7 milestone Jan 28, 2021
src/cascadia/TerminalSettingsEditor/SettingContainer.cpp Outdated Show resolved Hide resolved
Comment on lines 44 to 53
elem.Visibility(newVal ? Visibility::Visible : Visibility::Collapsed);
}
}

// update visibility for override message
if (auto overrideMsg{ obj.GetTemplateChild(L"OverrideMessage") })
{
if (auto elem{ overrideMsg.try_as<UIElement>() })
{
elem.Visibility(newVal ? Visibility::Visible : 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.

can you not bind these to {TemplateBinding HasSettingValue} inside the template?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is another one that doesn't make sense to me. I tried just that, but the visibility is initialized to Visible regardless of HasSettingValue (even setting HasSettingValue's default value to false results in no different behavior). Figured this might be an OnApplyTemplate thing that's getting in the way of initializing the value properly?

Copy link
Member

Choose a reason for hiding this comment

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

hmm.

Copy link
Member

Choose a reason for hiding this comment

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

Still surprised by this. I'm gonna check out your branch and have a look-see. 😄

src/cascadia/TerminalSettingsEditor/SettingContainer.cpp Outdated Show resolved Hide resolved
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.

did a first pass, will come back again

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jan 28, 2021
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jan 28, 2021
src/cascadia/TerminalSettingsEditor/Profiles.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsEditor/Profiles.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsEditor/Profiles.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsEditor/Profiles.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsEditor/Profiles.xaml Outdated Show resolved Hide resolved
Comment on lines 44 to 53
elem.Visibility(newVal ? Visibility::Visible : Visibility::Collapsed);
}
}

// update visibility for override message
if (auto overrideMsg{ obj.GetTemplateChild(L"OverrideMessage") })
{
if (auto elem{ overrideMsg.try_as<UIElement>() })
{
elem.Visibility(newVal ? Visibility::Visible : 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.

hmm.

@carlos-zamora
Copy link
Member Author

Base layer is still a bit wonky. Polish in that area is coming up next.

@carlos-zamora
Copy link
Member Author

Base layer is still a bit wonky. Polish in that area is coming up next.

Base layer is treated no differently. Long-term, the idea is that "Overrides a setting" will be replaced with a link to the source. Base layer will say something like "Overrides default setting" and we'll link to a view of defaults.json in SUI.

src/cascadia/TerminalSettingsEditor/Profiles.cpp Outdated Show resolved Hide resolved
Comment on lines 44 to 53
elem.Visibility(newVal ? Visibility::Visible : Visibility::Collapsed);
}
}

// update visibility for override message
if (auto overrideMsg{ obj.GetTemplateChild(L"OverrideMessage") })
{
if (auto elem{ overrideMsg.try_as<UIElement>() })
{
elem.Visibility(newVal ? Visibility::Visible : 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.

Still surprised by this. I'm gonna check out your branch and have a look-see. 😄

Comment on lines 105 to 110
const auto& helpText{ HelpText() };
if (!helpText.empty())
{
Controls::ToolTipService::SetToolTip(obj, box_value(helpText));
Automation::AutomationProperties::SetHelpText(obj, helpText);
}
Copy link
Member

Choose a reason for hiding this comment

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

surprised you couldn't do some binding work to make this happen actually. You should be able to {TemplateBinding HelpText} inside the xaml file. Don't block on this. Just curious.

Copy link
Member

Choose a reason for hiding this comment

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

Same for {TemplateBinding Header} on automation property

src/cascadia/TerminalSettingsEditor/Profiles.xaml Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsEditor/Profiles.xaml Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsEditor/Profiles.xaml Outdated Show resolved Hide resolved
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.

Okay, I think I can wrap my head around this. I don't have anything that wasn't already called out, but there's enough other comments that I'll hold off approving till they're fixed.

@carlos-zamora
Copy link
Member Author

A large contribution to #8899 has happened (mostly automatically) in this PR. SettingContainer handles most of the setting of automation properties. I went ahead and added the ones for the background image alignment control and the checkboxes for starting directory and background image. They're a bit out of scope, so I stopped there. But this puts us in a position where all settings now have reasonable automation properties.

#8899 cannot be closed, however, because there's still some work to be done on non-setting ui elements (i.e. discard changes & save buttons, open json button, etc...). I could do that here, but I feel that that's too out of scope for this PR.

@DHowett
Copy link
Member

DHowett commented Feb 5, 2021

Leaving assigned to Carlos because of the discussion we had on Teams.

Summarized here:

  1. I think that the if check for setting the automation help text will ensure that it never gets cleared when override goes from off to on to off again.
  2. use hstrings for GenerateOverrideThing

@carlos-zamora
Copy link
Member Author

  1. I think that the if check for setting the automation help text will ensure that it never gets cleared when override goes from off to on to off again.

This is really annoying. I've tried various methods and the AutomationProperty::HelpText does not update when I set it in _OnHasSettingValueChanged. I've tried...

  • AutomationProperties::SetHelpText to the new value
  • peer.RaisePropertyChangedEvent(Automation::AutomationElementIdentifiers::HelpTextProperty(), box_value(oldVal), box_value(newVal));

The code runs properly in both cases, but the automation client (i.e. screen reader) refuses to acknowledge that the value changed (so frustrating!). I've decided to do the following approach instead:

  • Header --> Content.AP.Name
  • HelpText --> Content.ToolTip & Content.AP.FullDescription
  • OverrideMsg --> ResetButton.AP.HelpText

I've also investigated a bit of what's going to happen when we add a hyperlink in the override message. For whatever reason, hyperlinks do not provide any automation properties automatically. So the user would tab to the hyperlink, and not be read anything. I think the right move there would be to keep the proposed design above, and add the following auto props to the hyperlink:

  • AP.Name --> <the text that is hyperlinked (e.g. "base layer")>
  • AP.HelpText --> "Navigate to <AP.Name (e.g. "base layer")>"

I feel like that'll still be somewhat intuitive. And the reset button is described such that the user is in the context of overriding a setting.

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.

This is an excellent piece of work. Well done. Just the one nit, then we can GO GO GO.

@DHowett DHowett assigned zadjii-msft and unassigned DHowett Feb 5, 2021
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.

Sure lets do it

@zadjii-msft zadjii-msft removed their assignment Feb 8, 2021
@carlos-zamora carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 8, 2021
@ghost
Copy link

ghost commented Feb 8, 2021

Hello @carlos-zamora!

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 3b7b200 into main Feb 8, 2021
@ghost ghost deleted the dev/cazamor/sui/setting-container branch February 8, 2021 18:04
ghost pushed a commit that referenced this pull request Feb 17, 2021
- 0b0dbdf Makes the browse buttons center vertically aligned
  - This is now made possible by #8919. The "center" used to include the height of the header. Now that it's separated, the center is solely calculated to be the text box.
  - Closes #8764 
- 0288f06 Fix keyboard navigation focus for color schemes rename button
  - Enter/Esc when in the scheme renamer now focuses the combo box
  - Keyboard-invoking accept/cancel button focuses the rename button
  - References #8765 and #8768
- d5ef552 Cyclical tab navigation
  - now, if you try to tab past the save button, you cycle back to the beginning of the navigation view
  - this is consistent with the xaml controls gallery
  - References #8768 
- a613b08 AutomationProperties for Save, Reset, and open json buttons
  - References #8899
ghost pushed a commit that referenced this pull request Feb 19, 2021
This PR adds improved override message generation for inheritance in
SUI. The settings model now has an `OriginTag` to be able to denote
where a `Profile` came from. This tag is used in the `SettingContainer`
to generate a more specific override message.

## References
#6800 - SUI Epic
#8919 - SUI Inheritance PR
#8804 - SUI Inheritance (old issue)

## Detailed Description of the Pull Request / Additional comments
- **Terminal Settings Model**
  - Introduced `PROJECTED_SETTING` as a macro to more easily declare the
    functions for each setting
  - Introduced `<setting>OverrideSource` which finds the `Profile` that
    has \<setting\> defined
  - Introduced `OriginTag Profile::Origin {Custom, InBox, Generated}` to
    trace where a profile came from
  - `DefaultProfileUtils` creates profiles for profile generators. So
    that now sets the `Origin` tag to `Generated`
  - `CascadiaSettings::LoadDefaults()` tags all profiles created as
    `InBox`.
  - The view model had to ingest the API change to be able to interact
    with `<setting>OverrideSource`
- **Override Message Generation**
  - The reset button now has a more specific tooltip
  - The reset button now only appears if base layer is being overridden
  - We use the settings model changes to determine the message to
    display for the target

## Validation Steps Performed
Tested the following cases:
- overrides nothing (inherited setting)
- overrides value inherited from...
  - base layer
  - a profile generator
  - in-box profile
- global settings should not have this feature
@ghost
Copy link

ghost commented Mar 1, 2021

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

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Settings UI Anything specific to the SUI AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Represent Inheritance in Settings UI
4 participants