Add experimental setting to make bg images fit the whole tab#12893
Add experimental setting to make bg images fit the whole tab#128934 commits merged intomicrosoft:mainfrom
Conversation
Fixes microsoft#6028 Setting is "experimental.useBackgroundImageForWindow"
ghost
added
Area-Extensibility
|
I found https://github.com/microsoft/terminal/blob/2c2f4f9be230e0be015f9bf799f58ef4bea807eb/doc/cascadia/AddASetting.md but that does not mention a schema. I found https://github.com/microsoft/terminal/blob/2c2f4f9be230e0be015f9bf799f58ef4bea807eb/doc/cascadia/profiles.schema.json but that seems to be for profiles, and what I added is a global setting. |
zadjii-msft
left a comment
zadjii-msft
left a comment
There was a problem hiding this comment.
Okay this is really cool. I have some minor stuff - honestly, probably not worth blocking over, but I'd love to know for sure.
| auto settings{ _core.Settings() }; | ||
| auto bgColor = til::color{ _core.FocusedAppearance().DefaultBackground() }; | ||
|
|
||
| auto transparent_bg = settings.BgImageForWindow(); |
There was a problem hiding this comment.
super nit: stylistically we usually use camelCase for variable names (e.g. transparentBg), but I wouldn't have even bothered with this if there weren't other comments on the PR. We also don't have that codified anywhere so that's on me 😉
There was a problem hiding this comment.
There's another transparent_bg left in the PR. But that's a "super nit" indeed. 😅
| // - <none> | ||
| void TerminalPage::_SetBackgroundImage(const winrt::Microsoft::Terminal::Control::IControlAppearance& newAppearance) | ||
| { | ||
| if (newAppearance.BackgroundImage().empty()) |
There was a problem hiding this comment.
note: this is quite nearly copy-pasted from TermControl::_SetBackgroundImage, so we know that's already good. We must have already evaluated env vars for the settings, so don't need to worry about that here.
ghost
added
Needs-Author-Feedback
|
Applied requested changes |
lhecker
left a comment
lhecker
left a comment
There was a problem hiding this comment.
The only nit I have worth mentioning is that I'd prefer if BgImageForWindow was called UseBackgroundImageForWindow because that's more descriptive.
|
Renamed to |
zadjii-msft
left a comment
zadjii-msft
left a comment
There was a problem hiding this comment.
Yea I dig this. Thanks for throwing this all together! It's a fun feature, looking forward to shipping it
zadjii-msft
added
the
AutoMerge
|
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 (
|
ghost
merged commit 
Adds the `experimental.useBackgroundImageForWindow` global setting introduced in #12893 to the schema.
|
🎉 Handy links: |
3 participants
Summary of the Pull Request
Fixes #6028
Setting is "experimental.useBackgroundImageForWindow"
References
#6028
PR Checklist
Validation Steps Performed
Set
"experimental.useBackgroundImageForWindow": trueand a bg image for one profile, then make splits and tabs and make sure the bg updates accordingly:I also did the same with the setting off to make sure it still works correctly and didn't break. And I made sure opening the settings tab does not crash or show the bg image.