Allow empty strings, %Environment-Variable% references in profile "icon" settings - fixes #1468, #1867 #2050

metathinker · 2019-07-20T20:24:07Z

Summary of the pull request

Previously, trying to set the "icon" property of a settings profile to either the empty string or a string containing %environment variable names% would cause Terminal to crash when loading that profile into the profile selection menu. Now, empty strings for "icon" are treated the same as JSON null or not setting the property at all, and environment variable references in the "icon" value are expanded.

Detailed description of the pull request / Additional comments

To expand environment variables, I first tried reusing the existing ExpandEnvironmentVariableStrings() helper in TerminalApp/CascadiaSettings.cpp, but then I remembered that the Windows Implementation Libraries already provide their own wrapper for ExpandEnvironmentStrings(), so instead I deleted ExpandEnvironmentVariableStrings() and replaced its existing usages with wil::ExpandEnvironmentStringsW(). I then used wil::ExpandEnvironmentStringsW() when resolving the icon path as well.

Validation steps performed

Manual testing by taking a default profiles.json file and changing the '"icon" value for one profile, with empty string and with strings containing %ProgramFiles% at the start.

PR Checklist

Closes Bug Report: Invalid string for Icon in profile causes a crash #1468, Closes Feature Request: Support environment variables for icon #1867
CLA signed - N/A; I work for Microsoft
Tests added/passed
I wasn't sure where to add these. It seems like the best place would be in ut_app/SettingsTests.cpp but that file has a comment that due to TerminalSettings and other WinRT types, it doesn't work right in Azure DevOps CI?
Requires documentation to be updated - N/A, no requirements changes
I've discussed this with core contributors already

…on" settings - fixes bugs 1468, 1867 First, I tried reusing the existing ExpandEnvironmentVariableStrings() helper in TerminalApp/CascadiaSettings.cpp, but then I realized that WIL already provides its own wrapper for ExpandEnvironmentStrings(), so instead I deleted ExpandEnvironmentVariableStrings() and replaced its usages with wil::ExpandEnvironmentStringsW(). I then used wil::ExpandEnvironmentStringsW() when resolving the icon path as well. In addition, to allow empty strings, I made changes to treat empty strings for "icon" the same as JSON `null` or not setting the property at all.

DHowett-MSFT

This is awesome. Just a couple minor questions and suggestions.

As for [1]: should we just knock it out for background image as well? That suggests that functions that return paths out of Profile should just pre-expand them (but never store them expanded, because reserialization would be depressing), except with the notable (and commentable!) exception of command line, which we can just leave up to CreateProcess

src/cascadia/TerminalApp/App.cpp

DHowett-MSFT · 2019-07-20T21:58:23Z

src/cascadia/TerminalApp/App.cpp

-            auto path = profile.GetIconPath();
-            winrt::hstring iconPath{ path };
+            std::wstring path{ profile.GetIconPath() };
+            std::wstring envExpandedPath{ wil::ExpandEnvironmentStringsW<std::wstring>(path.data()) };


should we expand these here or in GetIconPath? Either way, we'll need to issue a settings reload when we get the window message indicating that the environment changed (#1125), but perhaps it'll make easier the comment I'm about to leave at the end of the review [1]

I could go either way on this. Initially I had the expansion done in Profile.GetIconPath(); I changed it because I felt it would be inconsistent with other code in Profile. But see my reply to your point [1].

src/cascadia/TerminalApp/App.cpp

src/cascadia/TerminalApp/CascadiaSettings.cpp

DHowett-MSFT

(i meant to mark "request changes")

metathinker

As for [1]: should we just knock it out for background image as well? That suggests that functions that return paths out of Profile should just pre-expand them (but never store them expanded, because reserialization would be depressing), except with the notable (and commentable!) exception of command line, which we can just leave up to CreateProcess

If we change to expand variables in all of Profile's file path strings other than commandLine, we'd also have to change some of the special handling for startingDirectory, which already expands variables as well. That might be a bigger change than would reasonably fit in a single PR. What do you think?

metathinker · 2019-07-21T02:03:21Z

src/cascadia/TerminalApp/App.cpp

-            auto path = profile.GetIconPath();
-            winrt::hstring iconPath{ path };
+            std::wstring path{ profile.GetIconPath() };
+            std::wstring envExpandedPath{ wil::ExpandEnvironmentStringsW<std::wstring>(path.data()) };


I could go either way on this. Initially I had the expansion done in Profile.GetIconPath(); I changed it because I felt it would be inconsistent with other code in Profile. But see my reply to your point [1].

src/cascadia/TerminalApp/CascadiaSettings.cpp

metathinker · 2019-07-24T05:54:22Z

src/cascadia/ut_app/TerminalApp.Unit.Tests.manifest

@@ -20,122 +20,4 @@
      <longPathAware xmlns="http://schemas.microsoft.com/SMI/2016/WindowsSettings">true</longPathAware>
    </windowsSettings>
  </application>
-  <!--


I'm not sure why, but the bcz build itself seems to keep changing this file. Should I leave the changes? They seem similar to changes that were manually done in PR #2043 so I hope they're not bad.

Yeah, I think it's fine. And that one is already merged. @DHowett-MSFT would be able to confirm.

Oh shiz, that's actually a really good catch. I'll file a task to fix this the right way. Basically #2043 needs to get applied to these tests too.

(for know this is fine)

miniksa

src/cascadia/TerminalApp/App.cpp

src/cascadia/TerminalApp/CascadiaSettings.cpp

miniksa · 2019-07-24T16:19:25Z

src/cascadia/ut_app/TerminalApp.Unit.Tests.manifest

@@ -20,122 +20,4 @@
      <longPathAware xmlns="http://schemas.microsoft.com/SMI/2016/WindowsSettings">true</longPathAware>
    </windowsSettings>
  </application>
-  <!--


Yeah, I think it's fine. And that one is already merged. @DHowett-MSFT would be able to confirm.

miniksa

I guess I left comments and should request changes, not approve.

miniksa · 2019-07-24T16:21:10Z

I very much want to start using this wil::ExpandEnvironmentStrings thing elsewhere like in Entrypoints.cpp as well, but that's probably outside the scope of this particular fix.

Co-Authored-By: Michael Niksa <[email protected]>

zadjii-msft

Looks great to me!

metathinker · 2019-07-25T16:43:38Z

@DHowett-MSFT, please take another look.

DHowett-MSFT

Thanks!

…icrosoft#1468, microsoft#1867 (microsoft#2050) First, I tried reusing the existing ExpandEnvironmentVariableStrings() helper in TerminalApp/CascadiaSettings.cpp, but then I realized that WIL already provides its own wrapper for ExpandEnvironmentStrings(), so instead I deleted ExpandEnvironmentVariableStrings() and replaced its usages with wil::ExpandEnvironmentStringsW(). I then used wil::ExpandEnvironmentStringsW() when resolving the icon path as well. In addition, to allow empty strings, I made changes to treat empty strings for "icon" the same as JSON `null` or not setting the property at all. Co-Authored-By: Michael Niksa <[email protected]>

ghost · 2019-08-03T01:48:22Z

🎉Windows Terminal Preview v0.3.2142.0 has been released which incorporates this pull request.:tada:

Handy links:

DHowett-MSFT reviewed Jul 20, 2019

View reviewed changes

DHowett-MSFT suggested changes Jul 20, 2019

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 Jul 20, 2019

metathinker commented Jul 21, 2019

View reviewed changes

metathinker added 3 commits July 23, 2019 21:54

Merge branch 'master' (69c67f8) into icon-paths-1468-1867 (1eb2811)

c8e8063

respond to CR feedback

5e85bab

why does the build keep changing this file?

3fdec0d

metathinker commented Jul 24, 2019

View reviewed changes

miniksa approved these changes Jul 24, 2019

View reviewed changes

miniksa requested changes Jul 24, 2019

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 Jul 24, 2019

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 24, 2019

metathinker and others added 2 commits July 24, 2019 09:29

Apply suggestions from code review (commit from github.com)

6ae172b

Co-Authored-By: Michael Niksa <[email protected]>

fix build error due to last change

ee8fc6c

miniksa approved these changes Jul 24, 2019

View reviewed changes

zadjii-msft approved these changes Jul 25, 2019

View reviewed changes

DHowett-MSFT approved these changes Jul 25, 2019

View reviewed changes

miniksa mentioned this pull request Jul 25, 2019

use wil::ExpandEnvironmentStrings where possible #2097

Closed

DHowett-MSFT merged commit 66d46ed into microsoft:master Jul 25, 2019

This was referenced Aug 3, 2019

Feature Request: Support environment variables for icon #1867

Closed

Bug Report: Invalid string for Icon in profile causes a crash #1468

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow empty strings, %Environment-Variable% references in profile "icon" settings - fixes #1468, #1867 #2050

Allow empty strings, %Environment-Variable% references in profile "icon" settings - fixes #1468, #1867 #2050

metathinker commented Jul 20, 2019 •

edited

Loading

DHowett-MSFT left a comment

DHowett-MSFT Jul 20, 2019

metathinker Jul 21, 2019

DHowett-MSFT left a comment

metathinker left a comment

metathinker Jul 21, 2019

metathinker Jul 24, 2019

miniksa Jul 24, 2019

zadjii-msft Jul 25, 2019

zadjii-msft Jul 25, 2019

miniksa left a comment

miniksa Jul 24, 2019

miniksa left a comment

miniksa commented Jul 24, 2019

zadjii-msft left a comment

metathinker commented Jul 25, 2019

DHowett-MSFT left a comment

ghost commented Aug 3, 2019

Allow empty strings, %Environment-Variable% references in profile "icon" settings - fixes #1468, #1867 #2050

Allow empty strings, %Environment-Variable% references in profile "icon" settings - fixes #1468, #1867 #2050

Conversation

metathinker commented Jul 20, 2019 • edited Loading

Summary of the pull request

Detailed description of the pull request / Additional comments

Validation steps performed

PR Checklist

DHowett-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

DHowett-MSFT left a comment

Choose a reason for hiding this comment

metathinker 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

miniksa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miniksa left a comment

Choose a reason for hiding this comment

miniksa commented Jul 24, 2019

zadjii-msft left a comment

Choose a reason for hiding this comment

metathinker commented Jul 25, 2019

DHowett-MSFT left a comment

Choose a reason for hiding this comment

ghost commented Aug 3, 2019

metathinker commented Jul 20, 2019 •

edited

Loading