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

Feature Request: Background image alignment #1949

Closed
trigger-segfault opened this issue Jul 12, 2019 · 7 comments · Fixed by #1959
Closed

Feature Request: Background image alignment #1949

trigger-segfault opened this issue Jul 12, 2019 · 7 comments · Fixed by #1959
Assignees
Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@trigger-segfault
Copy link
Contributor

trigger-segfault commented Jul 12, 2019

Summary of the new feature/enhancement

When using the backgroundImage profile setting, the image is anchored to the center of the TermControl. Background images that have a primary point of interest that is not the center are not aesthetically pleasing when either... a large part of this focus is cut off via uniformToFill, or the image is centered via uniform or none and there is a abrupt end to the image before the border of the terminal control.

Image Example

Here is a background designed for anchoring to the bottom right. If we rely on uniformToFill, then we run the risk of the image's focus getting completely cut off. If we use none or uniform, then the image will display in the middle and abruptly end on the right while the console display has more room. Alignment anchoring would allow us to display the background image as it is intended.

Preview Figure A (Left) and Figure B (Right)

Figure A (Left), shows an image making use of alignment. Figure B (Right), shows the outcome of using the same background without alignment.

Proposed technical implementation details (optional)

Changes would require two additional profile settings shown below, (with center being the default for both).

"backgroundImageHorizontalAlignment": "center" | "left" | "right",
"backgroundImageVerticalAlignment":   "center" | "top"  | "bottom",

These settings will be needed only once, in this location:

Media::Imaging::BitmapImage image(imageUri);
_bgImageLayer.Source(image);
_bgImageLayer.HorizontalAlignment(HorizontalAlignment::Center);
_bgImageLayer.VerticalAlignment(VerticalAlignment::Center);

Here, HorizontalAlignment::Center and VerticalAlignment::Center will be replaced with their respective parsed settings of backgroundImageHorizontalAlignment and backgroundImageVerticalAlignment.

These two settings will need constant definitions for of their JSON/serialized values, and Parse/Serialize methods for the enums (in Profile.cpp).

Alternative Setting

One enum setting that combines the flags of HorizontalAlignment and VerticalAlignment could be used. The setting and values would be as follows:

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

A value of none, or missing axis alignment, implies that the default action of centering is used. Alternatively none could be labeled center to make this clearer.

There are pros and cons to this alternative. The pros being the setting name is shorter, and can be fully modified from a single line in profiles.json. The longer enum value names are still relatively simple and easy to remember. The cons being we are not referring to a real existing enum anymore, but a combination of enums. We also require more enum definitions and handling cases than if we made each setting separate. Lastly the implied centering with the values of none, left, top, right, and bottom are not as user-friendly.

@trigger-segfault trigger-segfault added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Jul 12, 2019
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Jul 12, 2019
@trigger-segfault trigger-segfault changed the title Feature Request: Background image alignment setting Feature Request: Background image alignment Jul 12, 2019
@trigger-segfault
Copy link
Contributor Author

This feature also has minor relations to #1936 and the ability to more-thoroughly customize background layout.

@trigger-segfault
Copy link
Contributor Author

I will take a go at writing a PR to implement this as my first contribution. Implementation seems very straight forwards, especially when looking at the structure of the PR that first introduced background images.

@ghost ghost added the In-PR This issue has a related PR label Jul 13, 2019
@DHowett-MSFT
Copy link
Contributor

This seems totally reasonable to me; I'm going to triage it as an accepted feature request. 😄

I'm not averse to the single setting that you've got in PR in #1959; in general I'm worried that we have too many settings and even though this is technically two enums it's one setting. For discussions about schema I'll defer to @zadjii-msft, however, since settings is his area.

Thanks for designing this!

@DHowett-MSFT DHowett-MSFT added Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. and removed Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. labels Jul 14, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jul 14, 2019
@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jul 14, 2019
@DHowett-MSFT
Copy link
Contributor

I've gone ahead and assigned you, as well, so people know this one is claimed!

@trigger-segfault
Copy link
Contributor Author

Thanks! I’m definitely happy about the preference towards a single setting. This was a very simple and fun first contribution.

Shall I hold off on undrafting PR #1959 so that the specifics of the JSON setting can be discussed with zadjii-msft here?

@zadjii-msft
Copy link
Member

Ah see, I think I am slightly in favor of the two settings, which is a little closer to the raw XAML properties. That being said, I'm not about to start a crusade over this. This seems fine the way it is, so I'll try and take a look at the PR.

@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Jul 25, 2019
DHowett-MSFT pushed a commit that referenced this issue Jul 25, 2019
* Implement base background image alignment settings

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 profile setting

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.

* Fix capitalization typo for BackgroundImageStretchModeKey

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.

* Document Background Image settings

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

* Fix setting capitalization error in UsingJsonSettings.md

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


Fixes #1949.
@ghost
Copy link

ghost commented Aug 3, 2019

🎉This issue was addressed in #1959, which has now been successfully released as Windows Terminal Preview v0.3.2142.0.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants