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

Add experimental setting to make bg images fit the whole tab #12893

Merged
4 commits merged into from
Apr 27, 2022
Merged

Add experimental setting to make bg images fit the whole tab #12893

4 commits merged into from
Apr 27, 2022

Conversation

nico-abram
Copy link
Contributor

Summary of the Pull Request

Fixes #6028
Setting is "experimental.useBackgroundImageForWindow"

References

#6028

PR Checklist

Validation Steps Performed

Set "experimental.useBackgroundImageForWindow": true and a bg image for one profile, then make splits and tabs and make sure the bg updates accordingly:
xqMUWpo1JK
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.

@nico-abram
Add experimental setting to make bg images fit the whole tab
be1cacf 
Fixes microsoft#6028
Setting is "experimental.useBackgroundImageForWindow"
@ghost ghost added Area-Extensibility A feature that would ideally be fulfilled by us having an extension model. Area-Settings Issues related to settings and customizability, for console or terminal Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. labels Apr 12, 2022
@nico-abram nico-abram marked this pull request as draft April 12, 2022 15:28
@nico-abram nico-abram mentioned this pull request Apr 12, 2022
Closed
@nico-abram
Copy link
Contributor Author

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.

@nico-abram nico-abram marked this pull request as ready for review April 12, 2022 21:42
zadjii-msft
zadjii-msft requested changes Apr 14, 2022
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.

Okay this is really cool. I have some minor stuff - honestly, probably not worth blocking over, but I'd love to know for sure.

src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TabManagement.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/TermControl.cpp Outdated
@@ -452,10 +459,16 @@ namespace winrt::Microsoft::Terminal::Control::implementation
{
auto settings{ _core.Settings() };
auto bgColor = til::color{ _core.FocusedAppearance().DefaultBackground() };

auto transparent_bg = settings.BgImageForWindow();
Copy link
Member

Choose a reason for hiding this comment

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

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 😉

Copy link
Member

Choose a reason for hiding this comment

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

There's another transparent_bg left in the PR. But that's a "super nit" indeed. 😅

src/cascadia/TerminalApp/TerminalPage.cpp Outdated
// - <none>
void TerminalPage::_SetBackgroundImage(const winrt::Microsoft::Terminal::Control::IControlAppearance& newAppearance)
{
if (newAppearance.BackgroundImage().empty())
Copy link
Member

Choose a reason for hiding this comment

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

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 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 Apr 14, 2022
@nico-abram
Copy link
Contributor Author

Applied requested changes

lhecker
lhecker approved these changes Apr 19, 2022
View reviewed changes
Copy link
Member

@lhecker lhecker left a comment

Choose a reason for hiding this comment

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

The only nit I have worth mentioning is that I'd prefer if BgImageForWindow was called UseBackgroundImageForWindow because that's more descriptive.

@nico-abram
Copy link
Contributor Author

Renamed to UseBackgroundImageForWindow

zadjii-msft
zadjii-msft approved these changes Apr 27, 2022
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.

Yea I dig this. Thanks for throwing this all together! It's a fun feature, looking forward to shipping it ☺️

@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 27, 2022
@ghost
Copy link

ghost commented Apr 27, 2022

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit d8379ff into microsoft:main Apr 27, 2022
This was referenced May 16, 2022
Merged
Merged
DHowett pushed a commit that referenced this pull request May 17, 2022
@carlos-zamora
Add experimental.useBackgroundImageForWindow to schema (#13114)
b699f92 
Adds the `experimental.useBackgroundImageForWindow` global setting introduced in #12893 to the schema.
carlos-zamora added a commit that referenced this pull request May 17, 2022
@carlos-zamora
Add experimental.useBackgroundImageForWindow to schema (#13114)
5fbd539 
Adds the `experimental.useBackgroundImageForWindow` global setting introduced in #12893 to the schema.

(cherry picked from commit b699f92)
Service-Card-Id: 82002294
Service-Version: 1.14
@ghost
Copy link

ghost commented May 24, 2022

🎉Windows Terminal Preview v1.14.143 has been released which incorporates this pull request.:tada:

Handy links:

@j4james j4james mentioned this pull request Oct 20, 2022
Open
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lhecker lhecker lhecker approved these changes

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

Assignees
No one assigned
Labels
Area-Extensibility A feature that would ideally be fulfilled by us having an extension model. Area-Settings Issues related to settings and customizability, for console or terminal AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature request: Background image for tab, not per split pane
3 participants
@nico-abram @lhecker @zadjii-msft