Add settings UI string localization #7833

cinnamon-msft · 2020-10-05T23:15:15Z

Summary of the Pull Request

Moved all strings into resources file for localization.

References

#1564 - Epic

PR Checklist

Closes #xxx
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
Schema updated.
I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

src/cascadia/TerminalSettingsEditor/GlobalAppearance.xaml

carlos-zamora · 2020-10-05T23:17:56Z

NOTE: the merge conflict is trivial to resolve. But we'll resolve that after the PR gets some feedback. That way, we can still test this out as if the settings editor was its own app.

src/cascadia/TerminalSettingsEditor/GlobalAppearance.xaml

carlos-zamora

There's a few places the uid wasn't named in order (largest to smallest).

For the ones that mention a comment, I recommend looking at some of the other resw files for what that should look like.

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

carlos-zamora · 2020-10-06T18:00:55Z

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

+  <data name="Globals_CopyFormatting.[using:Windows.UI.Xaml.Controls]ToolTipService.ToolTip" xml:space="preserve">
+    <value>When checked, the color and font formatting of selected text is also copied to your clipboard. When unchecked, only plain text is copied to your clipboard.</value>
+  </data>


(note to self) Grr. We need to update the entire control to support the copy formatting array (i.e. [ "html" ]). This is minor though, so we'll push this work off until like next month.

(same dealio with the dynamic profiles - but lets just turn those into follow-up tasks)

carlos-zamora · 2020-10-06T18:16:20Z

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

+    <value>Automatically create new profiles when new shells are installed</value>
+  </data>
+  <data name="Globals_DisableDynamicProfiles.[using:Windows.UI.Xaml.Controls]ToolTipService.ToolTip" xml:space="preserve">
+    <value>Enables all of the dynamic profile generators, adding their profiles to the list of profiles on startup.</value>


Suggested change

<value>Enables all of the dynamic profile generators, adding their profiles to the list of profiles on startup.</value>

<value>Disables all of the dynamic profile generators, adding their profiles to the list of profiles on startup.</value>

?

It felt weird to have a "negative effect" checkbox. I think this was mentioned in my spec review for the settings UI. I was thinking we would flip the checkbox logic for this one.

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

zadjii-msft

These are all kinda just nits, but I'm blocking so GitHub will let me actually see the changes since my last review.

zadjii-msft · 2020-10-07T14:46:12Z

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

+    <value>Equal</value>
+  </data>
+  <data name="Globals_ForceFullRepaint.Content" xml:space="preserve">
+    <value>Screen redrawing</value>


Suggested change

<value>Screen redrawing</value>

<value>Full screen redrawing</value>

maybe?

This feels like it only takes effect when in full screen mode?

Good lord.

Can we call it "Disable differential drawing" or "use differential drawing" or something and not avoid negatives? The tooltip can explain what it is.

I may unsubscribe if we're already getting into individual word-by-word bikeshedding.

If now's not the time for nitpicking the text, when is? When we're reviewing a 200 file mega merge of all the settings UI code? I'd rather not, and be able to mentally check "yea, we already reviewed the resources".

I think that "Disable differential drawing" is the most accurate to what this does.

I think we should avoid using negatives for checkboxes. I can change it to "Enable differential drawing" and flip the logic of the tooltip.

But this is a troubleshooting option. It's only to be used when there's trouble to be shot. If the user opens this up and sees one of the troubleshooting options turned on, what are they going to think?

As a user, I would think "that's guarded with an electric fence, I should not turn it off" or "that's weird. I'll see how it works without this option that is to be used in case of trouble turned on."

I have not seen a clear articulation on why exactly we want to avoid using negatives?

I have some guidelines here that say we should use positive messaging:

In general, prefer positive phrasing. For example, use do instead of do not, and notify instead of do not notify. Exception: The check box label, "Don't show this message again," is widely used.

On the flip side, we could rename it to something close to its actual name like "Force full repaint" or "Redraw entire screen when display updates".

Does "disable" count as negative phrasing though?

I'm partial to "Redraw entire screen when display updates" with a subtitle indicating that it may be helpful when troubleshooting an issue or that it will reduce performance. Every option on this page, I think, either needs that subtitle or needs the whole page to have a disclaimer about it.

We can't be surfacing these options as just normal every-day run-of-the-mill things users should be toggling. This is a product made for power users, we should treat them as though they can make decisions and the best way to enable them to do so is to inform them.

I would argue having a negative effect on a checkbox may be confusing. I'm happy to change it to "Redraw entire screen when display updates" and add a disclaimer to the top of the page.

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

zadjii-msft · 2020-10-07T14:48:43Z

src/cascadia/TerminalSettingsEditor/MainPage.xaml


                    <muxc:NavigationViewItem.MenuItems>
-                        <muxc:NavigationViewItem Tag="GlobalProfile_Nav" >
+                        <muxc:NavigationViewItem x:Uid="Nav_GlobalProfile"
+                                                 Tag="GlobalProfile_Nav">
                            <!-- Couldn't find spaceship -->


Aside: is this  comment just a copy-paste artifact, or is it actually meaningful?

This was from #7370 (and the hackathon). We have a few missing icons that may need to be designed. "Spaceship" is one of them. Currently tracked in the epic.

zadjii-msft · 2020-10-07T14:51:01Z

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

+    <value>Theme</value>
+  </data>
+  <data name="Globals_Theme.[using:Windows.UI.Xaml.Controls]ToolTipService.ToolTip" xml:space="preserve">
+    <value>Sets the theme of the application. The value 'system' refers to the active Windows system theme.</value>


Do we need to do the

<comment>{Locked="\"system\""}</comment>

thing here for "system" to prevent it from being localized?

Actually, if you can, just remove the 'system' sentence and put that on the 'system' value.

Couple things:

I changed this to 'default' since 'system' isn't an option on the UI

You can't add tooltips to specific radio button items, so I'll add a comment similar to the previous ones about replacing the text with the Uid value

src/cascadia/TerminalSettingsEditor/Launch.xaml

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

src/cascadia/TerminalSettingsEditor/Profiles.xaml

carlos-zamora · 2020-10-07T16:45:20Z

Easiest way to make the name fixes is probably with a find&replace. Idk if VS lets you find and replace across the xaml and resw files, but if it doesn't, VS Code definitely does. Hope this helps.

carlos-zamora

Looks great!

zadjii-msft

Yea this is way better, thanks!

carlos-zamora · 2020-10-09T16:21:05Z

I'll handle merging this into the feature branch (just need to make sure the merge conflict in the vcxproj is resolved properly). Thanks a ton @cinnamon-msft!

Just for transparency, there's still one last thing that needs to be localized: the Save button. But we don't have a save button yet. We'll throw that together soon then get localization locked and ship all of this out. 😊

carlos-zamora · 2020-10-09T16:34:54Z

@msftbot merge this in 5 minutes

ghost · 2020-10-09T16:34:58Z

Hello @carlos-zamora!

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 Fri, 09 Oct 2020 16:39:56 GMT, which is in 5 minutes

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".

Moved all strings into resources file for localization.   * [ ] Closes #xxx * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [x] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

carlos-zamora and others added 2 commits September 30, 2020 00:08

Localize Settings UI Text

445d1e9

add resources for localization

03a0110

cinnamon-msft commented Oct 5, 2020

View reviewed changes

src/cascadia/TerminalSettingsEditor/GlobalAppearance.xaml Outdated Show resolved Hide resolved

Update src/cascadia/TerminalSettingsEditor/GlobalAppearance.xaml

e0e366e

cinnamon-msft commented Oct 5, 2020

View reviewed changes

src/cascadia/TerminalSettingsEditor/GlobalAppearance.xaml Outdated Show resolved Hide resolved

Update src/cascadia/TerminalSettingsEditor/GlobalAppearance.xaml

9c6c71a

carlos-zamora mentioned this pull request Oct 6, 2020

Feature Request - Implement Settings UI #1564

Closed

34 tasks

carlos-zamora requested changes Oct 6, 2020

View reviewed changes

ghost 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 6, 2020

zadjii-msft requested changes Oct 7, 2020

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 7, 2020

zadjii-msft added Area-Localization Area-Settings Issues related to settings and customizability, for console or terminal Product-Terminal The new Windows Terminal. labels Oct 7, 2020

zadjii-msft assigned cinnamon-msft Oct 7, 2020

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 7, 2020

cinnamon-msft added 2 commits October 7, 2020 11:55

fix largest to smallest

0372a84

address feedback

a3eaf7b

cinnamon-msft requested review from carlos-zamora and zadjii-msft October 7, 2020 20:13

add rendering disclaimer

9b11805

carlos-zamora approved these changes Oct 7, 2020

View reviewed changes

zadjii-msft approved these changes Oct 9, 2020

View reviewed changes

carlos-zamora assigned carlos-zamora and unassigned cinnamon-msft Oct 9, 2020

Merge branch 'feature/settings-ui' into dev/cazamor/sui/localization

bc18dd8

code format

868a297

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

ghost merged commit 29578d3 into feature/settings-ui Oct 9, 2020

ghost deleted the dev/cazamor/sui/localization branch October 9, 2020 16:40

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add settings UI string localization #7833

Add settings UI string localization #7833

cinnamon-msft commented Oct 5, 2020 •

edited by carlos-zamora

Loading

carlos-zamora commented Oct 5, 2020

carlos-zamora left a comment

carlos-zamora Oct 6, 2020

zadjii-msft Oct 7, 2020

carlos-zamora Oct 6, 2020

cinnamon-msft Oct 7, 2020

zadjii-msft left a comment

zadjii-msft Oct 7, 2020

cinnamon-msft Oct 7, 2020

DHowett Oct 7, 2020

zadjii-msft Oct 7, 2020

cinnamon-msft Oct 7, 2020

DHowett Oct 7, 2020

cinnamon-msft Oct 7, 2020

zadjii-msft Oct 7, 2020

DHowett Oct 7, 2020 •

edited

Loading

cinnamon-msft Oct 7, 2020

zadjii-msft Oct 7, 2020

carlos-zamora Oct 7, 2020

zadjii-msft Oct 7, 2020

carlos-zamora Oct 7, 2020

cinnamon-msft Oct 7, 2020

carlos-zamora commented Oct 7, 2020

carlos-zamora left a comment

zadjii-msft left a comment

carlos-zamora commented Oct 9, 2020

carlos-zamora commented Oct 9, 2020

ghost commented Oct 9, 2020

	<value>Enables all of the dynamic profile generators, adding their profiles to the list of profiles on startup.</value>
	<value>Disables all of the dynamic profile generators, adding their profiles to the list of profiles on startup.</value>

	<value>Screen redrawing</value>
	<value>Full screen redrawing</value>

Add settings UI string localization #7833

Add settings UI string localization #7833

Conversation

cinnamon-msft commented Oct 5, 2020 • edited by carlos-zamora Loading

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

carlos-zamora commented Oct 5, 2020

carlos-zamora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett Oct 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlos-zamora commented Oct 7, 2020

carlos-zamora left a comment

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

carlos-zamora commented Oct 9, 2020

carlos-zamora commented Oct 9, 2020

ghost commented Oct 9, 2020

cinnamon-msft commented Oct 5, 2020 •

edited by carlos-zamora

Loading

DHowett Oct 7, 2020 •

edited

Loading