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.

Sign up for GitHub

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

Jump to bottom

Expand env. vars in backgroundImage #3204

Merged
merged 12 commits into from
Nov 16, 2019
Merged

Expand env. vars in backgroundImage #3204

merged 12 commits into from
Nov 16, 2019

Conversation

bitcrazed
Copy link
Contributor

Summary of the Pull Request

  • Adds HasBackgroundImage() and GetExpandedBackgroundImagePath() to
    Profiles.cpp/h
  • Fills Terminal Settings with expanded path, rather than path value
    from profiles.json
  • Adds simple regression tests to detect and fail if this fix is
    circumvented in the future

PR Checklist

Validation Steps Performed

  • Comprehensive debugging with expected, unexpected and empty path strings
  • Tests executed & validated

@bitcrazed
Expands env-vars in background image paths
9677cd7 
* Fixes #2922
* Adds HasBackgroundImage() and GetExpandedBackgroundImagePath() to
Profiles.cpp/h
* Fills Terminal Settings with expanded path, rather than path value
from profiles.json
* Adds simple regression tests to detect and fail if this fix is
circumvented in the future
@DHowett-MSFT
Copy link
Contributor

DHowett-MSFT commented Oct 15, 2019 •
edited
Loading

😿 😢 why'd you make a new PR and not just force push the old one

DHowett-MSFT
DHowett-MSFT approved these changes Oct 15, 2019
View reviewed changes
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

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

"approve with comments" -- i think the tests, if you add them, should actually validate that the env var got expanded. otherwise, i'm glad you added them. otherwise, I like this PR.

src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp Show resolved Hide resolved
@DHowett-MSFT
Copy link
Contributor

I don't think these tests are going to pass. We expand the env vars on read, not on storage. This is by design so that if the env vars change at runtime they'll be reflected.

@DHowett-MSFT
Copy link
Contributor

I have no idea why these tests pass.

@bitcrazed
Copy link
Contributor Author

Interestingly, these tests should NOT be passing - they're failing on my box.

If they are passing, there's a good chance our tests aren't running correctly.

@DHowett-MSFT
Copy link
Contributor

This latest commit says "ignore don't look at", but I can't tell why we shouldn't -- it looks like they answer the creative brief?
Do the tests pass on your machine?

carlos-zamora
carlos-zamora approved these changes Oct 16, 2019
View reviewed changes
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

The tests look like they're passing now and the PR looks good to me. I'll let you two decide if you want to merge it in as-is though.

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 16, 2019
@bitcrazed
Copy link
Contributor Author

"Do the tests pass on your machine?"

Nope, hence "In-progress. Incomplete. Ignore" ;)

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 16, 2019
zadjii-msft
zadjii-msft approved these changes Oct 24, 2019
View reviewed changes
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.

I think that this is fine

src/cascadia/LocalTests_TerminalApp/precomp.h Outdated Show resolved Hide resolved
@bitcrazed
Copy link
Contributor Author

Okay y'all. The tests now run too - had to layer JSON into userSettings. Please final-review and approve if no more changes needed. TIA.

carlos-zamora
carlos-zamora approved these changes Oct 25, 2019
View reviewed changes
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Just a small nit but approving with suggestions. Thanks Rich!

src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp Outdated Show resolved Hide resolved
@carlos-zamora
Corrected variable init
873c90b 
Co-Authored-By: Carlos Zamora <[email protected]>
@zadjii-msft zadjii-msft added the Area-Settings Issues related to settings and customizability, for console or terminal label Nov 5, 2019
@zadjii-msft zadjii-msft added Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Nov 5, 2019
@zadjii-msft
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

miniksa
miniksa reviewed Nov 6, 2019
View reviewed changes
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

Looking for header explanation, but this is otherwise fine with me.

@carlos-zamora
Copy link
Member

@DHowett-MSFT putting this on your radar 😊

DHowett-MSFT
DHowett-MSFT suggested changes Nov 15, 2019
View reviewed changes
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

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

I've taken a look as requested and still am not satisfied.

@ghost 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 Nov 15, 2019
@DHowett-MSFT DHowett-MSFT changed the title Expand env-vars in background image paths Expand env. vars in backgroundImage Nov 16, 2019
@DHowett-MSFT DHowett-MSFT merged commit efa68ab into master Nov 16, 2019
@DHowett-MSFT DHowett-MSFT deleted the bitcrazed/Expand-Background-Image-Path branch November 16, 2019 00:58
@DHowett-MSFT
Copy link
Contributor

Congrats!

@ghost
Copy link

ghost commented Nov 26, 2019

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

Handy links:

@ghost ghost mentioned this pull request Nov 26, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miniksa miniksa miniksa left review comments

@zadjii-msft zadjii-msft zadjii-msft approved these changes

@DHowett-MSFT DHowett-MSFT DHowett-MSFT approved these changes

@carlos-zamora carlos-zamora carlos-zamora approved these changes

Assignees
No one assigned
Labels
Area-Settings Issues related to settings and customizability, for console or terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Terminal takes an unexpected dirt nap when backgroundImage contains environment variables
5 participants
@bitcrazed @DHowett-MSFT @zadjii-msft @carlos-zamora @miniksa