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 miscellaneous simple settings to the settings UI #17923

Merged
merged 3 commits into from
Oct 10, 2024

Conversation

carlos-zamora
Copy link
Member

@carlos-zamora carlos-zamora commented Sep 12, 2024

Summary of the Pull Request

Adds the following settings to the settings UI:

  • $profile.RainbowSuggestions
  • $profile.CellWidth
  • $global.SearchWebDefaultQueryUrl
  • $global.EnableColorSelection
  • $global.ShowAdminShield
  • $global.EnableUnfocusedAcrylic

Additionally, the following settings have graduated from experimental 🎓:

  • $profile.rightClickContextMenu
  • $profile.RainbowSuggestions

Part of #10000

@@ -1839,4 +1871,20 @@
<value>Non-monospace fonts:</value>
<comment>This is a label that is followed by a list of proportional fonts.</comment>
</data>
<data name="Profile_RainbowSuggestions.Header" xml:space="preserve">
<value>Display suggestions UI preview text with rainbow formatting</value>
<comment>This is a label for a setting that, when enabled, applies a rainbow coloring to the previewable text from the suggestions UI.</comment>

Check failure

Code scanning / check-spelling

Unrecognized Spelling

[previewable](#security-tab) is not a recognized word. \(unrecognized-spelling\)
@carlos-zamora carlos-zamora force-pushed the dev/cazamor/sui/misc-simple-settings branch from 0c7979c to c895a73 Compare September 12, 2024 21:37
@carlos-zamora

This comment was marked as resolved.

Comment on lines +139 to +129
<!-- Enable Unfocused Acrylic -->
<local:SettingContainer x:Uid="Globals_EnableUnfocusedAcrylic">
<ToggleSwitch IsOn="{x:Bind ViewModel.EnableUnfocusedAcrylic, Mode=TwoWay}"
Style="{StaticResource ToggleSwitchInExpanderStyle}" />
</local:SettingContainer>
Copy link
Member Author

Choose a reason for hiding this comment

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

Noticed that this setting is compatibility.enableUnfocusedAcrylic. Any ideas why it's in the compatibility namespace? To me, it makes sense to have in the appearance page, but the compatibility namespace is making me second guess that it should be in the new compatibility page. Should we remove the compatibility prefix?

Copy link
Member

Choose a reason for hiding this comment

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

I think it was more "OS compatibility" - unfocused acrylic is actually a different implementation of acrylic than usual. It has caused some issues in the past.

@carlos-zamora carlos-zamora force-pushed the dev/cazamor/sui/misc-simple-settings branch from c895a73 to 76b17f9 Compare September 25, 2024 17:55
@carlos-zamora
Copy link
Member Author

Demo

Interaction page

  • $global.SearchWebDefaultQueryUrl
  • $global.EnableColorSelection

{F6CA19D3-04AC-4939-8D76-67BE6AE34EEA}

Appearance page

  • $global.ShowAdminShield
  • $global.EnableUnfocusedAcrylic

{85C2B229-E1B2-4449-B46F-6EA461273963}

Profile > Advanced

  • $profile.RainbowSuggestions

{7589FF4B-2914-47D2-BEFB-457CEB7CFC56}

Profile > Appearance

  • $profile.CellWidth

{AF44E188-910F-48D8-BB55-7CDEBB6AD994}

<comment>This is a label for a setting that, when enabled, applies a rainbow coloring to the preview text from the suggestions UI.</comment>
</data>
<data name="Globals_EnableColorSelection.Header" xml:space="preserve">
<value>Experimental: Add key bindings to color selected text</value>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<value>Experimental: Add key bindings to color selected text</value>
<value>Experimental: Add key bindings to highlight selected text and to search for and highlight all instances selected text</value>

wording could use work, but more accurate

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to "Experimental: Add key bindings to highlight selected text and to search for and highlight all instances of selected text" (there was a missing "of")

I think that works. We can iterate on it if we get feedback.

Copy link
Member Author

@carlos-zamora carlos-zamora Oct 10, 2024

Choose a reason for hiding this comment

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

Urgh, we have a problem:
{E943A5D4-6A79-4161-BB3A-279F41755537}

It's because of the SettingContainer styling. See inline comments below

<Style TargetType="local:SettingContainer">
    <Setter Property="Margin" Value="0,4,0,0" />
    <Setter Property="IsTabStop" Value="False" />
    <Setter Property="MaxWidth" Value="1000" />
    <Setter Property="Template">
        <Setter.Value>
            <ControlTemplate TargetType="local:SettingContainer">
                <Grid AutomationProperties.Name="{TemplateBinding Header}"
                      Style="{StaticResource NonExpanderGrid}">
                    <Grid.ColumnDefinitions>
                        <ColumnDefinition Width="*" />
                        <ColumnDefinition Width="Auto" />
                    </Grid.ColumnDefinitions>
                    <!--  This StackPanel is the problem! I'm guessing the StackPanel makes it so that wrapping property
                        isn't applied because we _technically_ never wrap (even though wrap is set on the text block).
                        This doesn't occur with the HelpTextBlock below. Surprised this issue never came up for
                        localization!  -->
                    <StackPanel Style="{StaticResource StackPanelInExpanderStyle}">
                        <StackPanel Orientation="Horizontal">
                            <TextBlock Style="{StaticResource SettingsPageItemHeaderStyle}"
                                       Text="{TemplateBinding Header}" />
                            <Button x:Name="ResetButton"
                                    Style="{StaticResource SettingContainerResetButtonStyle}">
                                <FontIcon Glyph="&#xE845;"
                                          Style="{StaticResource SettingContainerFontIconStyle}" />
                            </Button>
                        </StackPanel>
                        <TextBlock x:Name="HelpTextBlock"
                                   Style="{StaticResource SettingsPageItemDescriptionStyle}"
                                   Text="{TemplateBinding HelpText}" />
                    </StackPanel>
                    <ContentPresenter Grid.Column="1"
                                      HorizontalAlignment="Right"
                                      VerticalAlignment="Center"
                                      Content="{TemplateBinding Content}" />
                </Grid>
            </ControlTemplate>
        </Setter.Value>
    </Setter>
</Style>

Copy link
Member

Choose a reason for hiding this comment

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

let's put experimental on the next line. or since it's a subtitle, Experimental Setting

Copy link
Member

Choose a reason for hiding this comment

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

(or explain the whole setting in there. Experimental: conhost-like Color Selection Adds blah blah blah to blah blah and also blah blah

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, I've reworded such that the header is shorter and I added additional details as the help text. Here's the result:
{38337514-0674-4A00-BB59-07CE121CD142}

\
if (value >= 0.1 && value <= 10.0) \
{ \
str = fmt::format(FMT_STRING(L"{:.6g}"), value); \
Copy link
Member

Choose a reason for hiding this comment

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

hey @lhecker why is this STRING and not COMPILE?

Copy link
Member Author

Choose a reason for hiding this comment

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

I blame Copilot haha. Changed to COMPILE

Copy link
Member

Choose a reason for hiding this comment

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

ಠ_ಠ

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, didn't see this. FMT_STRING used to be for doing compile-time checks but runtime formatting (compact assembly). Nowadays compile-time checks are always done.

src/cascadia/TerminalSettingsEditor/Appearances.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsEditor/Appearances.cpp Outdated Show resolved Hide resolved
@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Oct 3, 2024
@DHowett DHowett merged commit 0b4d3d5 into main Oct 10, 2024
20 checks passed
@DHowett DHowett deleted the dev/cazamor/sui/misc-simple-settings branch October 10, 2024 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants