-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Fix for configuring starting directory in SUI when defaults sets it to null #9862
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
I'd love to understand why these checks were added -- @zadjii-msft any idea? I think your Background Image code was the first to have it |
Oh, @carlos-zamora added it in #8919... |
@PankajBhojwani Just to be safe, mind testing this with the inheritance system? Like, check how it works when a fragment-extension is setting the background image and starting directory to null. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, yeah, always calling the setter can be a problem when we start thinking about inheritance. I think we may still need to keep into account whether the user explicitly set the value or not still.
Best way to test this out is playing with base layer and fragment extensions and making sure the UI is correct when the value is inherited.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PankajBhojwani verified that this now works as intended. Thank you!
Tested scenarios...
- fragment extension set starting directory to null
- fragment extension set starting directory to an actual value
- override the inherited values from above
The reset arrow appears when appropriate, and the checkbox works as intended.
Hello @zadjii-msft! Because this pull request has the 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 (
|
…o null (#9862) ## Summary of the Pull Request Remove an unnecessary check in `Profiles.cpp` that was preventing us from enabling the text box and browse button when the user unchecks 'use parent process directory' ## PR Checklist * [x] Closes #9847 * [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 work here ## Validation Steps Performed Played around with it and it works. (cherry picked from commit ad34291)
…o null (#9862) ## Summary of the Pull Request Remove an unnecessary check in `Profiles.cpp` that was preventing us from enabling the text box and browse button when the user unchecks 'use parent process directory' ## PR Checklist * [x] Closes #9847 * [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 work here ## Validation Steps Performed Played around with it and it works. (cherry picked from commit ad34291)
🎉 Handy links: |
Summary of the Pull Request
Remove an unnecessary check in
Profiles.cpp
that was preventing us from enabling the text box and browse button when the user unchecks 'use parent process directory'PR Checklist
Validation Steps Performed
Played around with it and it works.