Add a key binding Toggle Acrylic #15717
Conversation
microsoft-github-policy-service
bot
added
Issue-Task
This comment was marked as resolved.
This comment was marked as resolved.
Jaswir
force-pushed
the
jaswir/2531
branch
4 times, most recently
from
e2f9c15
to
717f86d
Compare
|
@zadjii-msft @DHowett Solved the issues. It's ready to be reviewed. Let me know if any changes need to be made. |
|
@zadjii-msft @DHowett-MSFT @DHowett Hi Dustin, Hi Mike, I was wondering if you got a chance yet to review the code? @zadjii-msft |
src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw
Outdated
Show resolved
Hide resolved
| // Don't Toggle Acrylic if they have transparency turned off | ||
| if (Opacity() < 1.0) | ||
| { | ||
| UseAcrylic(!_acrylicToggle); |
There was a problem hiding this comment.
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
| @@ -272,6 +274,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation | |||
| std::shared_ptr<ThrottledFuncTrailing<Control::ScrollPositionChangedArgs>> updateScrollBar; | |||
| }; | |||
|
|
|||
| bool _acrylicToggle; | |||
There was a problem hiding this comment.
Why add another bool here, rather than leverage the existing UseAcrylic RUNTIME_SETTING?
There was a problem hiding this comment.
@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:
The actual Terminal Preview behaviour with UseAcrylic TRUE in settings.json in these scenarios:
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.
There was a problem hiding this comment.
@zadjii-msft How would you have solved this problem?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@zadjii-msft Are you a DnD player?
There was a problem hiding this comment.
I am, but that was more of a reference to
There was a problem hiding this comment.
I see, I didn't play much Zelda to be honest ..
microsoft-github-policy-service
bot
added
the
Needs-Author-Feedback
@zadjii-msft 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
bot
added
Needs-Attention
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 |
Accepted Mike's suggestion comment for translators + value more consistent with codebase Co-authored-by: Mike Griese <migrie@microsoft.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…opacity changes, just do the opacity < 1.0 when we actually need it.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Summary of the Pull Request
Added a key binding Toggle Acrylic
References and Relevant Issues
#2531
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed
Tested whether things are still working along with:
In both situations with
PR Checklist