Implement #1949 Background Image Align (as one setting) by trigger-segfault · Pull Request #1959 · microsoft/terminal

trigger-segfault · 2019-07-13T19:40:54Z

Summary of the Pull Request

Implement #1949: profiles.json has been given a setting for background images labeled as shown below, with the following valid values. This allows background images to be anchored to different corners/sides of the console to fit that background image's intended use-case and focus.

"backgroundImageAlignment": "center" | "left" | "top" | "right" | "bottom"
                   | "topLeft" | "topRight" | "bottomLeft" | "bottomRight"

When left, top, right, or bottom is specified, the other axis is implied to be centered, as that is the default behavior. I went in favor of labeling the default value as center instead of none as "none" does not clearly list the alignment behavior.

Alternative Setting

The branch dev/trigger/background-image-align-two-settings implements the exact same functionality, but by splitting the profiles.json setting into separate horizontal and vertical settings. I am happy to submit it as a PR if the two-setting approach is preferred.

References

There seems to be no other issues or pull requests relevant to background image alignment.

PR Checklist

Closes Feature Request: Background image alignment #1949
CLA signed. If not, go over here and sign the CLA
Tests ~~added~~/passed
Requires documentation to be updated ^{(SettingsSchema.md, UsingJsonSettings.md (optional))}
I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: Feature Request: Background image alignment #1949

Detailed Description of the Pull Request / Additional comments

The Profile class defines methods for parsing and serializing the combined HorizontalAlignment and VerticalAlignment enums to and from JSON. An std::tuple<HorizontalAlignment, VerticalAlignment> field _backgroundImageAlignment has been added for storing the profile setting in one place, as it logically makes sense that both alignments to be in use together.

TerminalSettings and IControlSettings have been given BackgroundImageHorizontalAlignment and BackgroundImageVerticalAlignment properties. The combination of std::tuple<HorizontalAlignment, VerticalAlignment> is gone once we leave the Profile class. Combining the two enums is only relevant to how they are stored in profiles.json. TerminalSettings defines the default values of both alignment properties as Center.

The TermControl class changes the image setup in TermControl::_InitializeBackgroundBrush() to assign the _bgImageLayer's (background image control's) alignments to that of the specified alignment settings. The alignments assignments have also been moved from within the image source assignment block, to the bottom of the additional images settings definitions (Stretch and Opacity).

Additional Fixes

The line below for the constant definition has a lowercase i at the beginning of the word "image". The i has been capitalized to follow proper UpperCamelCase. The two references to this BackgroundImageStretchModeKey constant have been updated to reflect the new name.

terminal/src/cascadia/TerminalApp/Profile.cpp

Line 43 in fad7638

    
           static constexpr std::string_view BackgroundimageStretchModeKey{ "backgroundImageStretchMode" };

(This change was performed in its own commit, so it can be reverted if needed.)

Validation Steps Performed

Test and observe all 9 possible alignment values, invalid input, and not specifying the setting at all.
- For each image stretch mode.
- For when a background image is defined or not.
- Confirm alignment names match up.
For each alignment value, confirm they are re-serialized correctly when opening Windows Terminal.
Confirm default setting results in a centered background image.
Test and observe if alignment settings are properly applied when first opening the Windows Terminal or opening a new profile tab.

Preview Example Alignments

Below are examples of the alignments bottom (Left) and bottomRight (Right) in use.

Figure A (Left) shows the use of bottom alignment with uniformToFill stretch mode. The waves will always be at the bottom and stay centered horizontally. Figure B (Right) shows the use of bottomRight alignment with none stretch mode. The image will always remain in the bottom right corner and remain the same size.

TerminalSettings now has two new properties: * BackgroundImageHorizontalAlignment * BackgroundImageVerticalAlignment These properties are used in TermControl::_InitializeBackgroundBrush to specify the alignment for TermControl::_bgImageLayer. This is a base commit that will split into two possible branches: * Use one setting in profiles.json: "backgroundImageAlignment" * Use two settings in profiles.json: "backgroundImageHorizontal/VerticalAlignment"

Implement background image alignment as one profile setting. * This has the benefit of acting as a single setting when the user would likely want to change both horizontal and vertical alignment. * HorizontalAlignment and VerticalAlignment are still stored as a tuple in Profile because they are an optional field. And thus, it would not make sense for one of the alignments to be left unused while the other is not. * Cons are that the tuple signature is quite long, but it is only used in a small number of locations. The Serialize method is also a little mishapen with the nested switch statements. Empty lines have been added between base-level cases to improve readability.

In Profiles.cpp, the key for the image stretch mode json property had a lowercase 'i' in "Backgroundimage", not following proper UpperCamelCase. The "i" has been capitalized and the two usages of the constant have been updated as well.

zadjii-msft

This seems good and reasonable to me. However, would you mind updating SettingsSchema.md as well?

trigger-segfault · 2019-07-16T16:26:37Z

Of course. Wasn't sure if documentation was delegated to other parties or not. I'll also add entries for the original 3 backgroundImage settings.

* Adds entries SettingsSchema.md for the original 3 backgroundImage settings in addition to the new backgroundImageAlignment setting.

* The background image example in UsingJsonSettings.md listing a backgroundImageStretchMode of "Fill" has been corrected to "fill".

trigger-segfault · 2019-07-16T17:44:48Z

4 new entries have been added to the SettingsSchema.md documentation.

In addition, one background image-related setting capitalization typo (which results in an invalid value) has been fixed in the UsingJsonSettings.md file.

Also my apologies, I should have added a [ci skip] to the last two commits.

miniksa · 2019-07-16T18:25:16Z

doc/cascadia/SettingsSchema.md

 | `useAcrylic` | _Required_ | Boolean | `false` | When set to `true`, the window will have an acrylic background. When set to `false`, the window will have a plain, untextured background. |
 | `background` | Optional | String | | Sets the background color of the profile. Overrides `background` set in color scheme if `colorscheme` is set. Uses hex color format: `"#rrggbb"`. |
+| `backgroundImage` | Optional | String | | Sets the file location of the Image to draw over the window background. |
+| `backgroundImageAlignment` | Optional | String | `center` | Sets how the background image aligns to the boundaries of the window. Possible values: `"center"`, `"left"`, `"top"`, `"right"`, `"bottom"`, `"topLeft"`, `"topRight"`, `"bottomLeft"`, `"bottomRight"` |


Can you list out what the default behavior will be if you don't set the alignment/opacity/stretch given they're optional?

I believe that's what column 4 is for, but I'm not sure

Correct, column 4 ( Default ) lists the default settings of center, 1.0, and uniformToFill respectively. Default values are listed for other optional settings in the Globals category as well.

zadjii-msft

Looks good, thanks!

carlos-zamora

I prefer the one setting. Good work!

DHowett-MSFT

Thank you!

ghost · 2019-08-03T01:49:04Z

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

Handy links:

trigger-segfault added 3 commits July 13, 2019 12:27

DHowett-MSFT mentioned this pull request Jul 14, 2019

Feature Request: Background image alignment #1949

Closed

zadjii-msft requested changes Jul 16, 2019

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 16, 2019

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 16, 2019

trigger-segfault marked this pull request as ready for review July 16, 2019 16:26

trigger-segfault added 2 commits July 16, 2019 13:32

Document Background Image settings

28cec5c

* Adds entries SettingsSchema.md for the original 3 backgroundImage settings in addition to the new backgroundImageAlignment setting.

Fix setting capitalization error in UsingJsonSettings.md

7f0ee15

* The background image example in UsingJsonSettings.md listing a backgroundImageStretchMode of "Fill" has been corrected to "fill".

miniksa reviewed Jul 16, 2019

View reviewed changes

zadjii-msft approved these changes Jul 16, 2019

View reviewed changes

carlos-zamora approved these changes Jul 25, 2019

View reviewed changes

DHowett-MSFT approved these changes Jul 25, 2019

View reviewed changes

DHowett-MSFT merged commit 89190c6 into microsoft:master Jul 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement #1949 Background Image Align (as one setting)#1959

Implement #1949 Background Image Align (as one setting)#1959
DHowett-MSFT merged 5 commits intomicrosoft:masterfrom
trigger-segfault:dev/trigger/background-image-align-one-setting

trigger-segfault commented Jul 13, 2019 •

edited

Loading

Uh oh!

zadjii-msft left a comment

Uh oh!

trigger-segfault commented Jul 16, 2019

Uh oh!

trigger-segfault commented Jul 16, 2019 •

edited

Loading

Uh oh!

miniksa Jul 16, 2019

Uh oh!

zadjii-msft Jul 16, 2019

Uh oh!

trigger-segfault Jul 16, 2019

Uh oh!

zadjii-msft left a comment

Uh oh!

carlos-zamora left a comment

Uh oh!

DHowett-MSFT left a comment

Uh oh!

ghost commented Aug 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

trigger-segfault commented Jul 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the Pull Request

Alternative Setting

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Additional Fixes

Validation Steps Performed

Preview Example Alignments

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

trigger-segfault commented Jul 16, 2019

Uh oh!

trigger-segfault commented Jul 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miniksa Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

zadjii-msft Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

trigger-segfault Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

DHowett-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Aug 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

trigger-segfault commented Jul 13, 2019 •

edited

Loading

trigger-segfault commented Jul 16, 2019 •

edited

Loading