Polish the Background Image settings #8778

zadjii-msft · 2021-01-13T18:18:58Z

Summary of the Pull Request

Two parts:

Hide the BG image settings when no image is specified
Add a checkbox for "Use desktop wallpaper". When that's checked, the BG image path input is hidden. Unchecking that box restores the path to what it was before.

PR Checklist

Closes Polish Settings UI: Profile > Background Image #8763
I work here

Validation Steps Performed

Tested manually

…roll-to-opacity

This doesn't work as I'd hope though. If you manually type in `desktopWallpaper`, then the text box is hidden permanently. This is because the StringIsNotDesktopConverter imediately can tell that the string is desktopWallpaper, and hides the text box, but it _doesn't_ update the checkbox. We're going to have to get creative.

…image-polish

carlos-zamora

Mind adding some pics or a gif to see what it looks like? I think I can visualize it but it'd be nice to have in the PR body.

Love the changes to converters!
Mostly nits here.

carlos-zamora · 2021-01-13T18:35:44Z

src/cascadia/inc/cppwinrt_utils.h

    return result;
 }
+
+#define DECLARE_CONVERTER(nameSpace, className)                                                                   \


Might as well use DECLARE_CONVERTER on all the other converter header files. For consistency.

src/cascadia/TerminalSettingsEditor/Profiles.cpp

carlos-zamora · 2021-01-13T18:41:54Z

src/cascadia/TerminalSettingsEditor/Profiles.cpp

+        // Cache the original BG image path. If the user clicks "Use desktop
+        // wallpaper", then un-checks it, this is the string we'll restore to
+        // them.
+        if (BackgroundImagePath() != L"desktopWallpaper")


nit: should L"desktopWallpaper" be a static constexpr std::wstring_view?

That way you just reference it below in UseDesktopBGImage() too

src/cascadia/TerminalSettingsEditor/Profiles.idl

zadjii-msft · 2021-01-13T18:56:22Z

carlos-zamora

Realized something. Make sure to disable the "hide background image settings" behavior in the "Base Layer" page. There's a good use case for customizing those settings in "Base Layer" even though you don't have a background image set.

Shouldn't be too difficult. There's an IsBaseLayer bool exposed off of the ProfileViewModel that you can use to check this.

…image-polish

PankajBhojwani · 2021-01-14T20:04:43Z

I think I would prefer if the checkbox was immediately below the textbox for the path, and that the textbox and button are disabled when the checkbox is set (rather than hidden), this gets us

consistency with Add missing functionality to SUI #8786
no need to have two 'background image' headers like there currently is, we could just have one
imo its more intuitive - currently, if the user has 'desktop wallpaper' set and wants to change it, its not immediately obvious that they need to uncheck desktop wallpaper before the textbox will show. But if the textbox is greyed out when the checkbox right underneath it is checked, it becomes more obvious

Thoughts?

zadjii-msft · 2021-01-14T21:24:33Z

zadjii-msft · 2021-01-14T22:02:40Z

Realized something. Make sure to disable the "hide background image settings" behavior in the "Base Layer" page. There's a good use case for customizing those settings in "Base Layer" even though you don't have a background image set.

Shouldn't be too difficult. There's an IsBaseLayer bool exposed off of the ProfileViewModel that you can use to check this.

carlos-zamora

Thanks!

…image-polish

ghost · 2021-01-18T22:33:29Z

Hello @DHowett!

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.

## Summary of the Pull Request Two parts: * Hide the BG image settings when no image is specified * Add a checkbox for "Use desktop wallpaper". When that's checked, the BG image path input is hidden. Unchecking that box restores the path to what it was before. ## PR Checklist * [x] Closes microsoft#8763 * [x] I work here ## Validation Steps Performed Tested manually

zadjii-msft added 11 commits January 13, 2021 04:17

50 ways to _not do this correctly_

b60245f

Merge remote-tracking branch 'origin/main' into dev/migrie/b/8764b-sc…

24540c7

…roll-to-opacity

what if we just took Bikini Bottom, and _pushed it somewhere else_

8334a2d

Make the BG image settings appear when then path contains something

9cb1c2e

Some cleanup of the converters

3cc5cb6

This is so very much closer

9e9f0f5

Yea this works nicely

a06c0f3

Code cleanup for review

677b920

spellcheck

cb0db01

Merge remote-tracking branch 'origin/main' into dev/migrie/b/8763-bg-…

15c9a78

…image-polish

ghost added Area-SettingsUI Anything specific to the SUI Issue-Task It's a feature request, but it doesn't really need a major design. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Jan 13, 2021

carlos-zamora approved these changes Jan 13, 2021

View reviewed changes

carlos-zamora requested changes Jan 13, 2021

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 Jan 13, 2021

Merge remote-tracking branch 'origin/main' into dev/migrie/b/8763-bg-…

cade076

…image-polish

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jan 14, 2021

zadjii-msft added 2 commits January 14, 2021 15:30

disable the bg input when it's desktopWallpaper

608ef13

sHoUlDn't bE ToO DiFfIcUlT

bfaafcb

carlos-zamora approved these changes Jan 14, 2021

View reviewed changes

zadjii-msft added 2 commits January 15, 2021 06:44

I'll un-check you bot

a9521b2

Merge remote-tracking branch 'origin/main' into dev/migrie/b/8763-bg-…

dbd2057

…image-polish

zadjii-msft mentioned this pull request Jan 15, 2021

Persist selected color scheme on navigation; Don't gray-out color swatches #8799

Merged

4 tasks

carlos-zamora added the Needs-Second It's a PR that needs another sign-off label Jan 15, 2021

ghost requested review from DHowett, PankajBhojwani, leonMSFT and miniksa January 15, 2021 17:45

DHowett approved these changes Jan 18, 2021

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 18, 2021

ghost merged commit 2b4b8dd into main Jan 18, 2021

ghost deleted the dev/migrie/b/8763-bg-image-polish branch January 18, 2021 22:34

zadjii-msft restored the dev/migrie/b/8763-bg-image-polish branch January 19, 2021 11:13

zadjii-msft deleted the dev/migrie/b/8763-bg-image-polish branch January 19, 2021 11:15

zadjii-msft mentioned this pull request Jan 26, 2021

Starting Directory redesign proposal #8904

Open

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Polish the Background Image settings #8778

Polish the Background Image settings #8778

Uh oh!

zadjii-msft commented Jan 13, 2021

Uh oh!

carlos-zamora left a comment

Uh oh!

carlos-zamora Jan 13, 2021

Uh oh!

Uh oh!

carlos-zamora Jan 13, 2021

Uh oh!

carlos-zamora Jan 13, 2021

Uh oh!

Uh oh!

zadjii-msft commented Jan 13, 2021

Uh oh!

carlos-zamora left a comment

Uh oh!

PankajBhojwani commented Jan 14, 2021

Uh oh!

zadjii-msft commented Jan 14, 2021

Uh oh!

zadjii-msft commented Jan 14, 2021

Uh oh!

carlos-zamora left a comment

Uh oh!

ghost commented Jan 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Polish the Background Image settings #8778

Polish the Background Image settings #8778

Uh oh!

Conversation

zadjii-msft commented Jan 13, 2021

Summary of the Pull Request

PR Checklist

Validation Steps Performed

Uh oh!

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

carlos-zamora Jan 13, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

carlos-zamora Jan 13, 2021

Choose a reason for hiding this comment

Uh oh!

carlos-zamora Jan 13, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zadjii-msft commented Jan 13, 2021

Uh oh!

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

PankajBhojwani commented Jan 14, 2021

Uh oh!

zadjii-msft commented Jan 14, 2021

Uh oh!

zadjii-msft commented Jan 14, 2021

Uh oh!

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Jan 18, 2021

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.

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

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.