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.

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

Background image support #853

Merged
merged 12 commits into from
May 29, 2019
Merged

Background image support #853

merged 12 commits into from
May 29, 2019

Conversation

d-bingham
Copy link
Contributor

Summary of the Pull Request

Initial code check-in for background image support. Handles background images themselves, and adds options to profiles.json to control the feature.

References

#833

PR Checklist

Detailed Description of the Pull Request / Additional comments

This adds three options to the individual profile options in profile.json:

  • backgroundImage: Expects a Uri which locates an image. This property is optional and defaults to null.
  • backgroundImageOpacity: Expects a number, controls opacity of the background image. Optional and defaults to 1.0.
  • backgroundImageStretchMode: expects one of "none", "fill", "uniform", or "uniformToFill", which controls the stretch mode used -- see documentation here. Optional and defaults to uniformToFill.

Background images are only displayed if useAcrylic is turned off.

One known issue:

backgroundImageOpacity currently only works with respect to the application's default black background and does not respect the profile's background color.

One potential issue:

There's no attempt to handle a failed image load from the Uri; I think it is reasonable to expect the underlying classes (specifically BitmapImage) to handle this gracefully. Additionally, it's unclear how such a failure should be reported to the user (beyond their requested image being displayed as a black console background instead)

@zadjii-msft
Copy link
Member

Additionally, it's unclear how such a failure should be reported to the user

I am working on an error dialog in #622, but that's not quite ready yet, so I'm fine with this just doing whatever for now.

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'm a little worried about introducing ImageStretchMode to be the same as windows.ui.xaml.media.stretch. Otherwise no complaints here :)

src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettings/IControlSettings.idl Outdated Show resolved Hide resolved
@miniksa
Copy link
Member

miniksa commented May 21, 2019

I'm concerned as to whether this should be a property of the DxRenderer itself instead of composed on during the XAML process in the control.

But given that I don't currently have the capacity to figure that out, I'm OK with accepting this in the meantime.

@DHowett-MSFT
Copy link
Contributor

I'm concerned as to whether this should be a property of the DxRenderer itself instead of composed on during the XAML process in the control.

But given that I don't currently have the capacity to figure that out, I'm OK with accepting this in the meantime.

Yeah, we can stick with this. Putting it behind the DxRenderer (literally behind) frees us from thinking about image loading, format handling, blitting, invalidation, stretch, interpolation and everything else XAML gives us for ~free.

Given that list, I'm inclined to prefer the XAML solution. 😄

@d-bingham
Copy link
Contributor Author

Given that list, I'm inclined to prefer the XAML solution. 😄

Additionally, as far as I'm aware there's no "official" implementation of the Fluent acrylic brush in Direct2D, so the XAML layer would still be required for that effect.

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.

This will conflict heavily with #1005. This is otherwise a very well-done feature. Let's file a follow-up to support a transparent image over top of acrylic and really set those GPUs on fire

@d-bingham
Copy link
Contributor Author

Working on cleaning up some architectural oddities created by merging this with #891. The current PR build works, but it's inelegant (and the code gets called three times on a settings file change...)

@zadjii-msft
Copy link
Member

@d-bingham you might not need to worry about the settings reloading issue too much. I know that it's definitely called twice per settings change, because for whatever reason, we get two file change notifications.

@d-bingham
Copy link
Contributor Author

@d-bingham you might not need to worry about the settings reloading issue too much. I know that it's definitely called twice per settings change, because for whatever reason, we get two file change notifications.

I'm more concerned about getting a background image refresh (and the resulting flicker of the background image) resulting from terminal input introduced in #891.

…olorChanged and added code to prevent flicker on resetting acrylic or image backgrounds.
@d-bingham
Copy link
Contributor Author

Modified this a bit to make more sense with regard to #891:

  • Split off brush-initialization work from Add support for OSC 10 and 11 to set the default colors #891's TermControl::_BackgroundColorChanged; _BackgroundColorChanged now only, er, changes background color
  • Put the brush initialization into TermControl::_InitializeBackgroundBrush
  • Enhanced TermControl::_InitializeBackgroundBrush so that the acrylic and image brushes avoid redraws (image refresh for the image brush, acrylic redraw and fade for acrylic) when they are reset

@d-bingham
Copy link
Contributor Author

Also, critically, the flicker prevention code maintains animation state of any animated image selected as a background during a settings change that does not change the image source.

@zadjii-msft
Copy link
Member

animation state of any animated image selected as a background

Are you suggesting we can have gifs as a background? Because that would be amazing

@zadjii-msft zadjii-msft merged commit 097f7d3 into microsoft:master May 29, 2019
@d-bingham
Copy link
Contributor Author

animation state of any animated image selected as a background

Are you suggesting we can have gifs as a background? Because that would be amazing

Yeah, just set your backgroundImage to https://lh6.googleusercontent.com/-Kde1FiWiijM/TniZsFpCyqI/AAAAAAAAD7Q/2sZC2Zqx8RM/s500/Contact500_Micael_Reynaud.gif and give it a bit to load, and it works.

miniksa pushed a commit that referenced this pull request Jun 4, 2019
Fixes #1082 -- #853's fix of the acrylic background's flash/fade on any settings change managed to cause a flash/fade on new tab creation. This change removed both flash/fades. #853 split background brush initialization from background color changes; due to the brush being constructed with a default color and then the color being initialized later, new tabs were getting the flash/fade that accompanies a re-focused fluent-style acrylic background. This PR initializes the acrylic color at brush initialization to avoid the problem.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Add support for background images
6 participants