Skip to content

Remove the fallback to 50% opacity when only useAcrylic is set#11363

Merged
1 commit merged intomainfrom
dev/migrie/b/11355-oops-opacity
Sep 29, 2021
Merged

Remove the fallback to 50% opacity when only useAcrylic is set#11363
1 commit merged intomainfrom
dev/migrie/b/11355-oops-opacity

Conversation

@zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Sep 28, 2021

This logic was seemingly redundant. There's two cases I'm looking at here:

Case 1

    "defaults":
    {
        "opacity": 35
    },
    "list":
    [
        {
            "commandline": "cmd.exe",
            "name": "Command Prompt"
        },

In this case, we wouldn't set the TerminalSettings Opacity to .35, we'd set it to 1.0, because the profile didn't have an opactity.

Case 2

    "defaults":
    {
        "useAcrylic": true
    },
    "list":
    [
        {
            "commandline": "cmd.exe",
            "name": "Command Prompt"
        },

In this case we still want to have an acrylic effect. Previously, we'd default this effect to 50% opaque. I'm not sure that we can actually get that anymore. BUT it turns out, we can have 100% opacity and HostBackdropAcrylic. It is very subtle, but is maybe something we should be allowing anyways. It kinda looks like:
image

    This logic was seemingly redundant. There's two cases I'm looking at here:

    #### Case 1
    ```jsonc
        "defaults":
        {
            "opacity": 35
        },
        "list":
        [
            {
                "commandline": "cmd.exe",
                "name": "Command Prompt"
            },
    ```

    In this case, we wouldn't set the `TerminalSettings` Opacity to .35, we'd set it to 1.0, because the profile didn't have an `opactity`.

    #### Case 2
    ```jsonc
        "defaults":
        {
            "useAcrylic": true
        },
        "list":
        [
            {
                "commandline": "cmd.exe",
                "name": "Command Prompt"
            },
    ```

    In this case we still want to have an acrylic effect. Previously, we'd default this effect to 50% opaque. I'm not sure that we can actually get that anymore. BUT it turns out, we _can_ have 100% opacity and HostBackdropAcrylic. It is very subtle, but is maybe something we should be allowing anyways.
@ghost ghost added Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Area-Settings Issues related to settings and customizability, for console or terminal Priority-1 A description (P1) labels Sep 28, 2021
@zadjii-msft zadjii-msft removed Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Priority-3 A description (P3) labels Sep 28, 2021
@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 29, 2021
@ghost
Copy link

ghost commented Sep 29, 2021

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 c0574f5 into main Sep 29, 2021
@ghost ghost deleted the dev/migrie/b/11355-oops-opacity branch September 29, 2021 10:26
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[1.12] Setting opacity as profiles.defaults doesn't work

3 participants