Skip to content

Add a key binding Toggle Acrylic #15717

Open
Jaswir wants to merge 20 commits intomicrosoft:mainfrom
Jaswir:jaswir/2531
Open

Add a key binding Toggle Acrylic #15717
Jaswir wants to merge 20 commits intomicrosoft:mainfrom
Jaswir:jaswir/2531

Conversation

@Jaswir
Copy link
Contributor

@Jaswir Jaswir commented Jul 15, 2023

Summary of the Pull Request

Added a key binding Toggle Acrylic

References and Relevant Issues

#2531

Detailed Description of the Pull Request / Additional comments

toggle acrylic v1

Validation Steps Performed

Tested whether things are still working along with:

  • Mouse scroll wheel
  • Adjust opacity: image
  • Acrylic Off when they turn off transparency.

In both situations with
image

"useAcrylic": true 
"useAcrylic": false

PR Checklist

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Task It's a feature request, but it doesn't really need a major design. Area-Settings Issues related to settings and customizability, for console or terminal Product-Terminal The new Windows Terminal. labels Jul 15, 2023
@Jaswir

This comment was marked as resolved.

Sign in to view
zadjii-msft
zadjii-msft reviewed Jul 17, 2023
View reviewed changes
@Jaswir Jaswir force-pushed the jaswir/2531 branch 4 times, most recently from e2f9c15 to 717f86d Compare July 20, 2023 09:00
@Jaswir
Copy link
Contributor Author

Jaswir commented Jul 21, 2023 •
edited
Loading

@zadjii-msft @DHowett Solved the issues. It's ready to be reviewed. Let me know if any changes need to be made.

@Jaswir
Copy link
Contributor Author

Jaswir commented Aug 4, 2023

@zadjii-msft @DHowett-MSFT @DHowett

Hi Dustin, Hi Mike,

I was wondering if you got a chance yet to review the code?

@zadjii-msft
Or would it stop being able to talk with you in a more comfortable manner about the other issue inside 2531, because you wouldn't get notifications anymore once it's merged or something?

zadjii-msft
zadjii-msft reviewed Aug 7, 2023
View reviewed changes
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea I suppose I'm mostly just curious about the runtime setting thing. That seems like it would get us a lot of this for free.

Sorry for the delays! Been a busy summer 😅

src/cascadia/TerminalControl/ControlCore.cpp Outdated
// Don't Toggle Acrylic if they have transparency turned off
if (Opacity() < 1.0)
{
UseAcrylic(!_acrylicToggle);
Copy link
Member

@zadjii-msft zadjii-msft Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this line will set the _runtimeUseAcrylic value to !_acrylicToggle. Presumably, the first time, this always sets it to true (since _acrylicToggle will always be initialized to false).

What happens if the user has acrylic enabled in their settings originally? I don't believe this would toggle it off the first time.

Ignore me, I saw the _acrylicToggle = _settings->UseAcrylic(); line in ControlCore::UpdateSettings just now

src/cascadia/TerminalControl/ControlCore.h Outdated
std::shared_ptr<ThrottledFuncTrailing<Control::ScrollPositionChangedArgs>> updateScrollBar;
};

bool _acrylicToggle;
Copy link
Member

@zadjii-msft zadjii-msft Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why add another bool here, rather than leverage the existing UseAcrylic RUNTIME_SETTING?

Copy link
Contributor Author

@Jaswir Jaswir Aug 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zadjii-msft Initially I used the UseAcrylic RUNTIME_SETTING and it seemed to work, but after testing Mouse Scroll Wheel to change Opacity and Adjust Opacity to change opacity, I stumbled upon some problems with this method.

Problems:
runtime_acrylic_breaks_adjustopac
runtime_acrylic_breaks_scrollwheel

The actual Terminal Preview behaviour with UseAcrylic TRUE in settings.json in these scenarios:
runtime_acrylic_correct_adjustop

runtime_acrylic_correct_scrollwheel

When the user uses Mouse Scroll Wheel to change Opacity and Adjust Opacity to change opacity through the command palette the ControlCore::_setOpacity function is triggered. This function manually turns off the _runtimeUseAcrylic when opacity becomes 100.

When the user scroll to opacity 100 and then scroll down again, the runtime acrylic goes false at 100 and would stay false when you go back under 100. Also when adjusting the opacity through the command palette similar thing happens when going to set background opacity to 100% option it stays false when going to 25%, 50%, etc.

Basically we can’t use the UseAcrylic RUNTIME_SETTING, because it becomes False when the user turns off transparency(opacity becomes 100) by using Mouse Scroll Wheel to change Opacity and Adjust Opacity to change opacity and we can’t use UseAcrylic RUNTIME_SETTING to turn _runtimeUseAcrylic back on since it’s false. So I figured we need to use an external boolean besides the UseAcrylic () RUNTIME_SETTING.

Using an external boolean solves these issues.

Copy link
Contributor Author

@Jaswir Jaswir Aug 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zadjii-msft How would you have solved this problem?

Copy link
Member

@zadjii-msft zadjii-msft Aug 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so I tried something in ffc7862.

Instead of setting _runtimeUseAcrylic based on the opacity, when the opacity changes, just do the opacity < 1.0 when we actually need it.

That seems to work... I'll play with it more but that's a start

Copy link
Contributor Author

@Jaswir Jaswir Aug 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

took 🗡️

Copy link
Contributor Author

@Jaswir Jaswir Aug 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zadjii-msft Are you a DnD player?

Copy link
Member

@zadjii-msft zadjii-msft Sep 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am, but that was more of a reference to
image

Copy link
Contributor Author

@Jaswir Jaswir Sep 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I didn't play much Zelda to be honest ..

zadjii-msft
zadjii-msft requested changes Aug 7, 2023
View reviewed changes
@microsoft-github-policy-service 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 Aug 7, 2023
@Jaswir
Copy link
Contributor Author

Jaswir commented Aug 10, 2023 •
edited
Loading

Yea I suppose I'm mostly just curious about the runtime setting thing. That seems like it would get us a lot of this for free.

Sorry for the delays! Been a busy summer 😅

@zadjii-msft
I am worried that when this issue gets merged you won't get notification anymore for things said in 2531 chat. And I can't use it anymore to talk in a comfortable manner about the other issue. And feel like your response doesn't adress my concern.

So I would like to know if you still get notifications when something is said inside 2531 once it's merged.

@microsoft-github-policy-service 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 Aug 10, 2023
@zadjii-msft
Copy link
Member

zadjii-msft commented Aug 10, 2023

So I would like to know if you still get notifications when something is said inside #2531 once it's merged

I get emails for every comment and on every issue, PR and discussion, and I try best to keep on top of them, no worries ☺️

Jaswir and others added 3 commits August 15, 2023 20:17
lhecker
lhecker previously approved these changes Sep 5, 2023
View reviewed changes
@Jaswir Jaswir mentioned this pull request Oct 12, 2023
Merged
4 tasks
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@Jaswir
Instead of setting _runtimeUseAcrylic based on the opacity, when the …
eed9cad 
…opacity changes, just do the opacity < 1.0 when we actually need it.
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@zadjii-msft zadjii-msft added this to the Terminal v1.21 milestone Feb 27, 2024
@lhecker lhecker removed the Needs-Attention The core contributors need to come back around and look at this ASAP. label Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DHowett DHowett DHowett left review comments

@zadjii-msft zadjii-msft zadjii-msft requested changes

@lhecker lhecker lhecker left review comments

Requested changes must be addressed to merge this pull request.

Assignees

@DHowett DHowett

@zadjii-msft zadjii-msft

Labels

Area-Settings Issues related to settings and customizability, for console or terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.

Projects

None yet

Milestone

Terminal v1.21

Development

Successfully merging this pull request may close these issues.

Add a key binding to set or toggle acrylic

4 participants

@Jaswir @zadjii-msft @DHowett @lhecker