Allow Opacity to be set differently in both focused and unfocused terminals #15974

Jaswir · 2023-09-16T16:59:06Z

Summary of the Pull Request

Closes #11092

Allowing opacity to be set differently in both focused and unfocused terminals

References and Relevant Issues

#11092 , references: #7158

Detailed Description of the Pull Request / Additional comments

Allowing Opacity to be set differently in both focused and unfocused terminals:

`_runtimeFocusedOpacity`

Mike also had to say something about this: #2531 (comment)

Initially I had something like _setOpacity(newAppearance->Opacity());

But with the introduction of unfocused opacity we encounter new challenges:
When Adjusting the Opacity with CTRL+SHIFT+Mouse Scroll Wheel or Set background opacity in command pallette, the Runtime opacity changes, but when we go to unfocused and back to focused the opacity changes back to focused opacity in Settings.

Also when adjusting opacity through the command palette the window becomes unfocused and then focused again after setting background opacity hence the _setOpacity(newAppearance->Opacity()); would override the changes made through command palette

With the introduction of unfocused opacity we encounter new challenges. The runtime opacity stores both the unfocused opacity and focused opacity from settings at different moments. This all works well until we combine this with Adjusting the Opacity with CTRL+SHIFT+Mouse Scroll Wheel or Set background opacity in command pallette. This brings the need for a separate Focused Opacity. When we change the runtime opacity with scroll wheel or through command pallette this value needs to be stored separately from the one in settings. So we can change back to it when going to unfocused mode and back to focused instead of the focused opacity defined in settings.

`skipUnfocusedOpacity` solves Opacity going from solid to unfocused to focused bug:

Validation Steps Performed

Checked if unfocused Opacity works well when adjusting opacity through Mouse Scroll Wheel or Command Palette and in combination with Acrylic as mentioned in "Detailed Description of the Pull Request / Additional comments"

PR Checklist

Closes Add support for Acrylic and Opacity in "Unfocused Appearance" #11092
Tests added/passed
Documentation updated
- If checked, please file a pull request on our docs repo and link it here:(Update Doc for Unfocused Acrylic and Opacity MicrosoftDocs/terminal#714)
Schema updated (if necessary)

Jaswir · 2023-09-16T17:07:34Z

Would be handy to have tests, so many different permutations to check with Acrylic + Opacity + Mouse Scroll Wheel + Adjusting Opacity through Command Palette

Jaswir · 2023-09-16T17:08:49Z

@lhecker

Made a new PR!

Jaswir · 2023-09-16T17:24:57Z

Ugh there's still some bugs was sure I removed them, one sec ...
Something must have broken when reducing the function calls from 2 to 1

carlos-zamora

Looks and feels great! Tested it out on my machine even :)

Here's some other feedback:

Please update the docs repo

I'll approve since the comments are pretty small, but please make sure you get to them. Thanks and great work!

src/cascadia/TerminalControl/ControlCore.cpp

carlos-zamora · 2023-09-18T22:07:26Z

src/cascadia/TerminalControl/ControlCore.cpp

+            // Skip Unfocused Opacity when EnableUnfocusedAcrylic is false
+            // and Acrylic is true as this results into seeing the opacity going
+            // from solid to unfocused to focused bug.
+            bool skipUnfocusedOpacity = !focused && !(!_settings->EnableUnfocusedAcrylic() && UseAcrylic());


Suggested change

bool skipUnfocusedOpacity = !focused && !(!_settings->EnableUnfocusedAcrylic() && UseAcrylic());

bool skipUnfocusedOpacity = !focused && (_settings->EnableUnfocusedAcrylic() || !UseAcrylic());

Personally, I'd prefer that you De Morgan's law it haha

Yea there's definitely something real weird with the way the conditionals are worded here that confuses me.

hell it might be easier to say useUnfocusedOpacity = focused && _settings->EnableUnfocusedAcrylic()... and flip the next statement around...

@zadjii-msft Should be good now

zadjii-msft

The wording of these conditionals has me scratching my head. They might need a bit of re-wording just to make the logic a bit easier to follow, but if the code works, I'm happy to ship it

zadjii-msft · 2023-09-19T16:49:28Z

src/cascadia/TerminalControl/ControlCore.cpp

+            // Skip Unfocused Opacity when EnableUnfocusedAcrylic is false
+            // and Acrylic is true as this results into seeing the opacity going
+            // from solid to unfocused to focused bug.
+            bool skipUnfocusedOpacity = !focused && !(!_settings->EnableUnfocusedAcrylic() && UseAcrylic());


Yea there's definitely something real weird with the way the conditionals are worded here that confuses me.

hell it might be easier to say useUnfocusedOpacity = focused && _settings->EnableUnfocusedAcrylic()... and flip the next statement around...

zadjii-msft · 2023-09-19T16:51:01Z

src/cascadia/TerminalControl/ControlCore.cpp

+            // and Acrylic is true as this results into seeing the opacity going
+            // from solid to unfocused to focused bug.
+            bool skipUnfocusedOpacity = !focused && !(!_settings->EnableUnfocusedAcrylic() && UseAcrylic());
+            double newOpacity = skipUnfocusedOpacity ? newAppearance->Opacity() : FocusedOpacity();


Actually what?

If skipUnfocusedOpacity: use newAppearance->Opacity.
else, we shouldn't skip unfocused opacity, and we should use FocusedOpacity

?

That doesn't really seem to make sense, at least not literally.

@zadjii-msft Should be good now

Jaswir · 2023-09-21T17:03:37Z

@zadjii-msft

Hey Mike,
How was your Hackathon week? Hope it was nice.

I was wondering if you got a chance yet to answer my question:
#2531 (comment)

But to be honest I'd really like you to answer this question:
It keeps me kinda distracted from focusing on this to not have it answered yet.
#2531 (comment)

Jaswir · 2023-09-21T18:22:54Z

Looks and feels great! Tested it out on my machine even :)

Here's some other feedback:

Please update the docs repo

I'll approve since the comments are pretty small, but please make sure you get to them. Thanks and great work!

Thanks Carlos.

"Looks and feels great!"

xD, I am glad you say that. I actually used to be one of the best (1000) game designers in The Netherlands if I say so myself in 2018, 2019 and there's this concept called Game Feel which is a lot about how things feel. Usually needs lots of player testing before it's good. Glad you like it! Hope the users will like it as well. If you like Game Feel and want to see a fun presentation about it see this: https://www.youtube.com/watch?v=AJdEqssNZ-U
Obviously this concept is not limited to just games and very much also applicable for the windows terminal.

I'll look into the docs and comments. Not sure what to add to the docs, let me try something..

Jaswir · 2023-09-21T18:36:27Z

@zadjii-msft @carlos-zamora

I think my expectations of you guys may be wrong. I expected Microsoft Engineers would like go over all the possible scenario's when reviewing. in this case try all the various permutations of UseAcrylic , Opacity in Unfocused and Focused mode + all other combinations with Adjust Opacity in Command Palette and Scroll Wheel, etc. Maybe come up with things I missed, like edgecases about the feature (not code stuff). But didn't notice this in 7158 or this one.

It seems like your reviewing process is mostly aimed at the code quality?
What is the reviewing process of code like at Microsoft? I guess this differs per team?

Jaswir · 2023-09-22T14:31:50Z

@carlos-zamora @zadjii-msft
Having some trouble making the code clear to you / other developers for this project.
I reworded the code this should be good. Let me know if it's not!

      bool useFocusedRuntimeOpacity = focused || (!_settings->EnableUnfocusedAcrylic() && UseAcrylic());
      double newOpacity = useFocusedRuntimeOpacity ?
                              FocusedOpacity() :
                              newAppearance->Opacity();
      _setOpacity(newOpacity, focused);

But not sure how to make this clear in a concise way with comments.
@zadjii-msft @carlos-zamora Let me know what is clear to you.

Essentially there are 2 special things going on that I want to communicate somehow with the comments to make it more clear.

Incase the window goes from unfocused to focused it should always use the runtime focused opacity and not the focused opacity in settings because of what I explain in the PR Description in _runtimeFocusedOpacity
Incase EnableUnfocusedAcrylic is disabled and Focused Acrylic is set to true, the terminal should not use the unfocused opacity, but ignore it and use focused runtime opacity. as described in PR description in skipUnfocusedOpacity

So now here's where it get's unclear for me
I could comment at 3 locations:

About the first one

I think it woud be good to mention something like that this prevents a bug from happening / covers an edge case.
That focused runtime Opacity should be used when EnableUnfocusedAcrylic is false and Acrylic is true because otherwise there's a bug where you see the opacity going from solid to unfocused to focused when switching from unfocused to focused.

Second, Third:

This could also be mentioned here:

Also I could tell about how EnableUnfocusedAcrylic and Focused and Unfocused Acrylic and Opacity in the docs and provide more clearance there?

… properly

…o jaswir/11092

…escribe what the new parameter does also provided clearer comments at other places

Jaswir · 2023-09-22T16:16:36Z

@carlos-zamora @zadjii-msft
Ok I reworded the conditionals and added a function description like with some of the other functions to describe what the new parameter does and adapted my comments to make things more clear for other developers hopefully.

Let me know what you think! I'd be happy to adapt it if needed. Still working on the docs

…and unfocused terminals

Jaswir · 2023-09-25T17:24:34Z

@zadjii-msft

Hi Mike,

This might be a silly way to do this.
I'll just come up straight with it.
I am looking for a referral for Microsoft.
Can you give me a referral?

Solving 7158 boosted my confidence to start seriously applying to Big Tech Jobs. Can you give me a referral? I rather have a referral from you then someone that doesn’t know me.

Also, are there any relevant opportunities at Microsoft that would let me work on this project or similar ones to it?

PS: I'll be continuing with the docs part and making a PR for that probably later this week

@DHowett @lhecker @carlos-zamora @j4james @PankajBhojwani
I am not sure how to approach this situation hence I put this in the group with you more Senior Experienced Engineers.

DHowett · 2023-09-26T05:49:34Z

@Jaswir sorry for the silence! We are all heads down preparing for our release on Tuesday. We'll have some more mental bandwidth as a team for discussions and PR reviews after that 🙂

Jaswir · 2023-09-26T17:00:19Z

@DHowett

@Jaswir sorry for the silence! We are all heads down preparing for our release on Tuesday. We'll have some more mental bandwidth as a team for discussions and PR reviews after that 🙂

Thanks for letting me know! Will unfocused Acrylic be in the release ??? Lots of users want that and would be happy with that, including me.

Jaswir · 2023-09-26T19:57:18Z

@carlos-zamora Updated the docs

Jaswir · 2023-10-12T11:09:35Z

@zadjii-msft Did you get a chance to rereview it yet? It's blocking #15717

Jaswir · 2023-10-25T15:39:24Z

@DHowett

Hey Dustin, when it's convenient for you. Could you please add this to the next release? I want this in my default windows preview terminal. It would make me very happy :D If it wouldn't be too much effort for you I'd really appreciate it.

Thanks in advance.

…minals (#15974) ## Summary of the Pull Request Closes #11092 Allowing `opacity `to be set differently in both focused and unfocused terminals ## References and Relevant Issues #11092 , references: #7158 ## Detailed Description of the Pull Request / Additional comments ### Allowing Opacity to be set differently in both focused and unfocused terminals: ![unfocused_opacity](https://github.com/microsoft/terminal/assets/15957528/1c38e40b-4678-43ec-b328-ad79d222579f) ![image](https://github.com/microsoft/terminal/assets/15957528/3e3342a8-7908-41db-9c37-26c89f7f2456) ![jolsen](https://github.com/microsoft/terminal/assets/15957528/68553507-d29e-4513-89ce-b1cd305d28b7) ![image](https://github.com/microsoft/terminal/assets/15957528/18864f60-91d0-4159-87da-2b2ee1637a4c) ## `_runtimeFocusedOpacity` Mike also had to say something about this: #2531 (comment) Initially I had something like ` _setOpacity(newAppearance->Opacity());` But with the introduction of unfocused opacity we encounter new challenges: When Adjusting the Opacity with **CTRL+SHIFT+Mouse Scroll Wheel** or **Set background opacity** in command pallette, the Runtime opacity changes, but when we go to unfocused and back to focused the opacity changes back to focused opacity in Settings. Also when adjusting opacity through the command palette the window becomes unfocused and then focused again after setting background opacity hence the ` _setOpacity(newAppearance->Opacity());` would override the changes made through command palette ![runtimeFocusedOpacity](https://github.com/microsoft/terminal/assets/15957528/4de63057-d658-4b5e-99ad-7db050834ade) ![command_pallette_focusswitches](https://github.com/microsoft/terminal/assets/15957528/372526eb-cf0c-40f8-a4e5-a0739f1f0e05) With the introduction of unfocused opacity we encounter new challenges. The runtime opacity stores both the unfocused opacity and focused opacity from settings at different moments. This all works well until we combine this with Adjusting the Opacity with **CTRL+SHIFT+Mouse Scroll Wheel** or **Set background opacity** in command pallette. This brings the need for a separate Focused Opacity. When we change the runtime opacity with scroll wheel or through command pallette this value needs to be stored separately from the one in settings. So we can change back to it when going to unfocused mode and back to focused instead of the focused opacity defined in settings. ## `skipUnfocusedOpacity` solves Opacity going from solid to unfocused to focused bug: ![skipUnfocusedOpacity_bug](https://github.com/microsoft/terminal/assets/15957528/ecc06dcf-fbef-4fef-a40f-68278fdbfb12) ## Validation Steps Performed - Checked if unfocused Opacity works well when adjusting opacity through Mouse Scroll Wheel or Command Palette and in combination with Acrylic as mentioned in "Detailed Description of the Pull Request / Additional comments" ## PR Checklist - [x] Closes #11092 - [ ] Tests added/passed - [x] Documentation updated - If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here:(MicrosoftDocs/terminal#714) - [ ] Schema updated (if necessary) (cherry picked from commit 27e1081) Service-Card-Id: 90949918 Service-Version: 1.19

Added Enable Unfocused Acrylic Global Setting Updated Opacity and Acrylic. No more OS limitations! microsoft/terminal#15974 --------- Co-authored-by: Matt Wojciakowski <mattwoj@microsoft.com>

Jaswir added 4 commits September 16, 2023 11:20

Updated profiles json for unfocused opacity

a114f27

experimenting with setting opacity, but this gives adjust opacity bug

3af12a3

Solved Adjust Opacity + Mouse Scroll Wheel bug

3185b3d

reduced calls from 2 to 1 function call, increasing performance

c3a20df

Run Code Formatting

792c0e0

Fixed bug after reducing function calls from 2 to 1

012e51a

carlos-zamora approved these changes Sep 18, 2023

View reviewed changes

zadjii-msft requested changes Sep 19, 2023

View reviewed changes

microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 19, 2023

microsoft-github-policy-service bot added Needs-Attention The core contributors need to come back around and look at this ASAP. and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Sep 21, 2023

Jaswir added 2 commits September 21, 2023 20:38

Merge branch 'microsoft:main' into jaswir/11092

fe58c9f

Reworded the conditionals, made it more clear

ee139a2

zadjii-msft added the zBugBash-Consider label Sep 21, 2023

Merge branch 'microsoft:main' into jaswir/11092

711e3b1

Jaswir added 3 commits September 22, 2023 16:56

Reworded the conditionals, made it more clear, this time it's working…

979434f

… properly

Merge branch 'jaswir/11092' of https://github.com/Jaswir/terminal int…

f17c224

…o jaswir/11092

added function description like with some of the other functions to d…

abf1889

…escribe what the new parameter does also provided clearer comments at other places

Ran Code formatting

51a676f

DHowett added a commit that referenced this pull request Sep 22, 2023

PRE-MERGE #15974 Allow Opacity to be set differently in both focused …

d125a95

…and unfocused terminals

Jaswir mentioned this pull request Sep 26, 2023

Update Doc for Unfocused Acrylic and Opacity MicrosoftDocs/terminal#714

Merged

Jaswir requested a review from zadjii-msft October 4, 2023 00:19

zadjii-msft approved these changes Oct 13, 2023

View reviewed changes

zadjii-msft merged commit 27e1081 into microsoft:main Oct 13, 2023

adonis-jimenez mentioned this pull request Nov 18, 2023

Windows Terminal Preview v1.19.3172.0 #16337

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Opacity to be set differently in both focused and unfocused terminals #15974

Allow Opacity to be set differently in both focused and unfocused terminals #15974

Jaswir commented Sep 16, 2023 •

edited

Loading

Jaswir commented Sep 16, 2023

Jaswir commented Sep 16, 2023 •

edited

Loading

Jaswir commented Sep 16, 2023 •

edited

Loading

carlos-zamora left a comment

carlos-zamora Sep 18, 2023

zadjii-msft Sep 19, 2023

Jaswir Oct 6, 2023 •

edited

Loading

zadjii-msft left a comment

zadjii-msft Sep 19, 2023

zadjii-msft Sep 19, 2023

Jaswir Oct 6, 2023

Jaswir commented Sep 21, 2023 •

edited

Loading

Jaswir commented Sep 21, 2023 •

edited

Loading

Jaswir commented Sep 21, 2023

Jaswir commented Sep 22, 2023 •

edited

Loading

Jaswir commented Sep 22, 2023 •

edited

Loading

Jaswir commented Sep 25, 2023 •

edited

Loading

DHowett commented Sep 26, 2023

Jaswir commented Sep 26, 2023 •

edited

Loading

Jaswir commented Sep 26, 2023

Jaswir commented Oct 12, 2023

Jaswir commented Oct 25, 2023 •

edited

Loading

	bool skipUnfocusedOpacity = !focused && !(!_settings->EnableUnfocusedAcrylic() && UseAcrylic());
	bool skipUnfocusedOpacity = !focused && (_settings->EnableUnfocusedAcrylic() \|\| !UseAcrylic());

Allow Opacity to be set differently in both focused and unfocused terminals #15974

Allow Opacity to be set differently in both focused and unfocused terminals #15974

Conversation

Jaswir commented Sep 16, 2023 • edited Loading

Summary of the Pull Request

References and Relevant Issues

Detailed Description of the Pull Request / Additional comments

Allowing Opacity to be set differently in both focused and unfocused terminals:

_runtimeFocusedOpacity

skipUnfocusedOpacity solves Opacity going from solid to unfocused to focused bug:

Validation Steps Performed

PR Checklist

Jaswir commented Sep 16, 2023

Jaswir commented Sep 16, 2023 • edited Loading

Jaswir commented Sep 16, 2023 • edited Loading

carlos-zamora left a comment

Choose a reason for hiding this comment

carlos-zamora Sep 18, 2023

Choose a reason for hiding this comment

zadjii-msft Sep 19, 2023

Choose a reason for hiding this comment

Jaswir Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft Sep 19, 2023

Choose a reason for hiding this comment

zadjii-msft Sep 19, 2023

Choose a reason for hiding this comment

Jaswir Oct 6, 2023

Choose a reason for hiding this comment

Jaswir commented Sep 21, 2023 • edited Loading

Jaswir commented Sep 21, 2023 • edited Loading

Jaswir commented Sep 21, 2023

Jaswir commented Sep 22, 2023 • edited Loading

Jaswir commented Sep 22, 2023 • edited Loading

Jaswir commented Sep 25, 2023 • edited Loading

DHowett commented Sep 26, 2023

Jaswir commented Sep 26, 2023 • edited Loading

Jaswir commented Sep 26, 2023

Jaswir commented Oct 12, 2023

Jaswir commented Oct 25, 2023 • edited Loading

Jaswir commented Sep 16, 2023 •

edited

Loading

`_runtimeFocusedOpacity`

`skipUnfocusedOpacity` solves Opacity going from solid to unfocused to focused bug:

Jaswir commented Sep 16, 2023 •

edited

Loading

Jaswir commented Sep 16, 2023 •

edited

Loading

Jaswir Oct 6, 2023 •

edited

Loading

Jaswir commented Sep 21, 2023 •

edited

Loading

Jaswir commented Sep 21, 2023 •

edited

Loading

Jaswir commented Sep 22, 2023 •

edited

Loading

Jaswir commented Sep 22, 2023 •

edited

Loading

Jaswir commented Sep 25, 2023 •

edited

Loading

Jaswir commented Sep 26, 2023 •

edited

Loading

Jaswir commented Oct 25, 2023 •

edited

Loading