Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix opacity restore for command palette previews #12229

Merged
1 commit merged into from
Jan 24, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 26 additions & 18 deletions src/cascadia/TerminalApp/ActionPreviewHandlers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -70,12 +70,14 @@ namespace winrt::TerminalApp::implementation
// Apply the reverts in reverse order - If we had multiple previews
// stacked on top of each other, then this will ensure the first one in
// is the last one out.
for (auto i{ _restorePreviewFuncs.rbegin() }; i < _restorePreviewFuncs.rend(); i++)
const auto cleanup = wil::scope_exit([this]() {
_restorePreviewFuncs.clear();
});
Comment on lines +73 to +75
Copy link
Member Author

Choose a reason for hiding this comment

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

Just in case a f() call below fails. This ensures the correct behavior for

const auto backup = _restorePreviewFuncs.empty();


for (const auto& f : _restorePreviewFuncs)
{
auto f = *i;
f();
}
_restorePreviewFuncs.clear();
}

// Method Description:
Expand All @@ -94,41 +96,47 @@ namespace winrt::TerminalApp::implementation
{
if (const auto& scheme{ _settings.GlobalSettings().ColorSchemes().TryLookup(args.SchemeName()) })
{
const auto backup = _restorePreviewFuncs.empty();

_ApplyToActiveControls([&](const auto& control) {
// Stash a copy of the current scheme.
auto originalScheme{ control.ColorScheme() };

// Apply the new scheme.
control.ColorScheme(scheme.ToCoreScheme());

// Each control will emplace a revert into the
// _restorePreviewFuncs for itself.
_restorePreviewFuncs.emplace_back([=]() {
// On dismiss, restore the original scheme.
control.ColorScheme(originalScheme);
});
if (backup)
{
// Each control will emplace a revert into the
// _restorePreviewFuncs for itself.
_restorePreviewFuncs.emplace_back([=]() {
// On dismiss, restore the original scheme.
control.ColorScheme(originalScheme);
});
}
});
}
}

void TerminalPage::_PreviewAdjustOpacity(const Settings::Model::AdjustOpacityArgs& args)
{
// Clear the saved preview funcs because we don't need to add a restore each time
// the preview changes, we only need to be able to restore the last one.
_restorePreviewFuncs.clear();
const auto backup = _restorePreviewFuncs.empty();

_ApplyToActiveControls([&](const auto& control) {
// Stash a copy of the original opacity.
auto originalOpacity{ control.BackgroundOpacity() };

// Apply the new opacity
control.AdjustOpacity(args.Opacity(), args.Relative());
control.AdjustOpacity(args.Opacity() / 100.0, args.Relative());

_restorePreviewFuncs.emplace_back([=]() {
// On dismiss:
// Don't adjust relatively, just set outright.
control.AdjustOpacity(::base::saturated_cast<int>(originalOpacity * 100), false);
});
if (backup)
{
_restorePreviewFuncs.emplace_back([=]() {
// On dismiss:
// Don't adjust relatively, just set outright.
control.AdjustOpacity(originalOpacity, false);
});
}
});
}

Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalApp/AppActionHandlers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -970,7 +970,7 @@ namespace winrt::TerminalApp::implementation
if (const auto& realArgs = args.ActionArgs().try_as<AdjustOpacityArgs>())
{
const auto res = _ApplyToActiveControls([&](auto& control) {
control.AdjustOpacity(realArgs.Opacity(), realArgs.Relative());
control.AdjustOpacity(realArgs.Opacity() / 100.0, realArgs.Relative());
});
args.Handled(res);
}
Expand Down
3 changes: 1 addition & 2 deletions src/cascadia/TerminalControl/ControlCore.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1718,9 +1718,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
return _settings->HasUnfocusedAppearance();
}

void ControlCore::AdjustOpacity(const int32_t& opacity, const bool& relative)
void ControlCore::AdjustOpacity(const double opacityAdjust, const bool relative)
Copy link
Member Author

Choose a reason for hiding this comment

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

I wrote the reason for this in the commit description:

It includes an additional change in order to make the AdjustOpacity setter consistent and symmetric with the Opacity getter.

Let me know what you all think. 🙂

{
const double opacityAdjust = static_cast<double>(opacity) / 100.0;
if (relative)
{
AdjustOpacity(opacityAdjust);
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalControl/ControlCore.h
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation

static bool IsVintageOpacityAvailable() noexcept;

void AdjustOpacity(const int32_t& opacity, const bool& relative);
void AdjustOpacity(const double opacity, const bool relative);

RUNTIME_SETTING(double, Opacity, _settings->Opacity());
RUNTIME_SETTING(bool, UseAcrylic, _settings->UseAcrylic());
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalControl/ControlCore.idl
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ namespace Microsoft.Terminal.Control

String ReadEntireBuffer();

void AdjustOpacity(Int32 Opacity, Boolean relative);
void AdjustOpacity(Double Opacity, Boolean relative);

event FontSizeChangedEventArgs FontSizeChanged;

Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalControl/TermControl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2713,7 +2713,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
_core.ColorScheme(scheme);
}

void TermControl::AdjustOpacity(const int32_t& opacity, const bool& relative)
void TermControl::AdjustOpacity(const double opacity, const bool relative)
{
_core.AdjustOpacity(opacity, relative);
}
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalControl/TermControl.h
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
winrt::Microsoft::Terminal::Core::Scheme ColorScheme() const noexcept;
void ColorScheme(const winrt::Microsoft::Terminal::Core::Scheme& scheme) const noexcept;

void AdjustOpacity(const int32_t& opacity, const bool& relative);
void AdjustOpacity(const double opacity, const bool relative);

// -------------------------------- WinRT Events ---------------------------------
// clang-format off
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalControl/TermControl.idl
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ namespace Microsoft.Terminal.Control

String ReadEntireBuffer();

void AdjustOpacity(Int32 Opacity, Boolean relative);
void AdjustOpacity(Double Opacity, Boolean relative);

// You'd think this should just be "Opacity", but UIElement already
// defines an "Opacity", which we're actually not setting at all. We're
Expand Down