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

Add a setting to flash the pane when BEL is emitted #9270

Merged
32 commits merged into from
May 24, 2021
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
370e0c0
initial
PankajBhojwani Feb 18, 2021
3d7c9db
optional timer
PankajBhojwani Feb 18, 2021
54a683d
add setting
PankajBhojwani Feb 19, 2021
b4ae30c
Merge branch 'main' of https://github.com/microsoft/terminal into dev…
PankajBhojwani Feb 19, 2021
d59a432
change timer
PankajBhojwani Feb 24, 2021
c1172cd
format
PankajBhojwani Mar 1, 2021
e0c9f89
stash, lock
PankajBhojwani Mar 19, 2021
e89f273
weak this
PankajBhojwani Mar 19, 2021
f93c9e8
Merge branch 'main' of https://github.com/microsoft/terminal into dev…
PankajBhojwani Mar 24, 2021
4c125c3
resw, schema
PankajBhojwani Mar 24, 2021
4b19b57
merge main, fix conflicts
PankajBhojwani May 7, 2021
76c182a
use xaml light
PankajBhojwani May 7, 2021
5ed7e93
spell
PankajBhojwani May 7, 2021
678cfb5
er what
PankajBhojwani May 7, 2021
cecc6d8
Merge branch 'main' of https://github.com/microsoft/terminal into dev…
PankajBhojwani May 7, 2021
23dc1fd
move light to own file
PankajBhojwani May 10, 2021
9282a16
nits
PankajBhojwani May 10, 2021
eedb317
lazy load
PankajBhojwani May 10, 2021
5b72726
some cleanup
PankajBhojwani May 10, 2021
692ad14
animation
PankajBhojwani May 12, 2021
8a42804
Format
PankajBhojwani May 12, 2021
b3c50f8
small changes
PankajBhojwani May 13, 2021
ab0ce5f
ahhh
PankajBhojwani May 17, 2021
8026698
deprecate visual
PankajBhojwani May 19, 2021
fb27787
here and there
PankajBhojwani May 19, 2021
2f9bc61
spell
PankajBhojwani May 19, 2021
084ff79
fix crash, two-way binding
PankajBhojwani May 20, 2021
f1237d9
remove flag map
PankajBhojwani May 20, 2021
b675b2a
Cleanup
PankajBhojwani May 21, 2021
6c9d8ef
== flag
PankajBhojwani May 21, 2021
730cf09
Ternary
PankajBhojwani May 24, 2021
f5bb54e
Apply suggestions from code review
DHowett May 24, 2021
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
8 changes: 5 additions & 3 deletions doc/cascadia/profiles.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,17 @@
"type": "string",
"enum": [
"audible",
"visual"
"window",
"taskbar"
]
}
},
{
"type": "string",
"enum": [
"audible",
"visual",
"taskbar",
"window",
"all",
"none"
]
Expand Down Expand Up @@ -1171,7 +1173,7 @@
},
"bellStyle": {
"default": "audible",
"description": "Controls what happens when the application emits a BEL character. When set to \"all\", the Terminal will play a sound and flash the taskbar icon. An array of specific behaviors can also be used. Supported array values include `audible` and `visual`. When set to \"none\", nothing will happen.",
"description": "Controls what happens when the application emits a BEL character. When set to \"all\", the Terminal will play a sound, flash the taskbar icon (if the terminal window is not in focus) and flash the window. An array of specific behaviors can also be used. Supported array values include `audible`, `window` and `taskbar`. When set to \"none\", nothing will happen.",
"$ref": "#/definitions/BellStyle"
},
"closeOnExit": {
Expand Down
9 changes: 7 additions & 2 deletions src/cascadia/TerminalApp/Pane.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -378,8 +378,13 @@ void Pane::_ControlWarningBellHandler(const winrt::Windows::Foundation::IInspect
PlaySound(soundAlias, NULL, SND_ALIAS_ID | SND_ASYNC | SND_SENTRY);
}

// raise the event with the bool value corresponding to the visual flag
_PaneRaiseBellHandlers(nullptr, WI_IsFlagSet(paneProfile.BellStyle(), winrt::Microsoft::Terminal::Settings::Model::BellStyle::Visual));
if (WI_IsFlagSet(paneProfile.BellStyle(), winrt::Microsoft::Terminal::Settings::Model::BellStyle::Window))
{
_control.BellLightOn();
Copy link
Member

Choose a reason for hiding this comment

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

this seems like the wrong abstraction-- the app should not tell the control to blink after the control tells the app that there was a bell.

Copy link
Member

Choose a reason for hiding this comment

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

Your comment about the visual bell moving up into the application resonates with me. Thank you.

}

// raise the event with the bool value corresponding to the taskbar flag
_PaneRaiseBellHandlers(nullptr, WI_IsFlagSet(paneProfile.BellStyle(), winrt::Microsoft::Terminal::Settings::Model::BellStyle::Taskbar));
}
}
}
Expand Down
44 changes: 43 additions & 1 deletion src/cascadia/TerminalControl/TermControl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
_lastAutoScrollUpdateTime{ std::nullopt },
_cursorTimer{},
_blinkTimer{},
_searchBox{ nullptr }
_searchBox{ nullptr },
_bellLightAnimation{ Window::Current().Compositor().CreateScalarKeyFrameAnimation() }
{
InitializeComponent();

Expand Down Expand Up @@ -167,6 +168,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation
_autoScrollTimer.Interval(AutoScrollUpdateInterval);
_autoScrollTimer.Tick({ this, &TermControl::_UpdateAutoScroll });

// Add key frames and a duration to our bell light animation
_bellLightAnimation.InsertKeyFrame(0.0, 2.0);
_bellLightAnimation.InsertKeyFrame(1.0, 1.0);
_bellLightAnimation.Duration(winrt::Windows::Foundation::TimeSpan(std::chrono::milliseconds(TerminalWarningBellInterval)));

_ApplyUISettings(_settings);
}

Expand Down Expand Up @@ -2358,6 +2364,42 @@ namespace winrt::Microsoft::Terminal::Control::implementation
return _core->TaskbarProgress();
}

void TermControl::BellLightOn()
{
Windows::Foundation::Numerics::float2 zeroSize{ 0, 0 };
// If the grid has 0 size or if the bell timer is
// already active, do nothing
if (RootGrid().ActualSize() != zeroSize && !_bellLightTimer)
{
// Start the timer, when the timer ticks we switch off the light
DispatcherTimer invertTimer;
invertTimer.Interval(std::chrono::milliseconds(TerminalWarningBellInterval));
invertTimer.Tick({ get_weak(), &TermControl::_BellLightOff });
invertTimer.Start();
_bellLightTimer.emplace(std::move(invertTimer));
PankajBhojwani marked this conversation as resolved.
Show resolved Hide resolved

// Switch on the light and animate the intensity to fade out
VisualBellLight::SetIsTarget(RootGrid(), true);
BellLight().CompositionLight().StartAnimation(L"Intensity", _bellLightAnimation);
Copy link
Member

Choose a reason for hiding this comment

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

Just to be clear, the animation runs from Intensity=2.0->1.0, then is switched off entirely at the end of the time. That's neat

}
}

void TermControl::_BellLightOff(Windows::Foundation::IInspectable const& /* sender */,
Windows::Foundation::IInspectable const& /* e */)
{
if (_bellLightTimer)
{
// Stop the timer and switch off the light
_bellLightTimer->Stop();
_bellLightTimer.reset();
Copy link
Member

Choose a reason for hiding this comment

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

Theoretically, once the timer's been initialized, we don't need to keep creating and destroying it each time. We could just re-use it, but I'm not gonna block over that.

Copy link
Member

Choose a reason for hiding this comment

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

Good notes for cleanup later. 😄


if (!_closing)
{
VisualBellLight::SetIsTarget(RootGrid(), false);
}
}
}

// Method Description:
// - Checks whether the control is in a read-only mode (in this mode node input is sent to connection).
// Return Value:
Expand Down
7 changes: 7 additions & 0 deletions src/cascadia/TerminalControl/TermControl.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#pragma once

#include "TermControl.g.h"
#include "XamlLights.h"
#include "EventArgs.h"
#include "../../renderer/base/Renderer.hpp"
#include "../../renderer/dx/DxRenderer.hpp"
Expand Down Expand Up @@ -98,6 +99,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
const winrt::hstring& padding,
const uint32_t dpi);

void BellLightOn();

bool ReadOnly() const noexcept;
void ToggleReadOnly();

Expand Down Expand Up @@ -164,8 +167,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation
Windows::UI::Xaml::DispatcherTimer _autoScrollTimer;
std::optional<std::chrono::high_resolution_clock::time_point> _lastAutoScrollUpdateTime;

winrt::Windows::UI::Composition::ScalarKeyFrameAnimation _bellLightAnimation;
Copy link
Contributor Author

@PankajBhojwani PankajBhojwani May 12, 2021

Choose a reason for hiding this comment

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

Not sure about _bellLightAnimation living here - I want it to be somewhere so we don't need to keep recreating the animation every time we want to have a bell flash, but at the same time its wasteful creating this if the bellStyle is audible/taskbar/none

Copy link
Member

Choose a reason for hiding this comment

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

meh that seems fine to me. In the future, we could always init this as

winrt::Windows::UI::Composition::ScalarKeyFrameAnimation _bellLightAnimation{ nullptr };

and only ctor it when actually requested in the settings, but again, meh


std::optional<Windows::UI::Xaml::DispatcherTimer> _cursorTimer;
std::optional<Windows::UI::Xaml::DispatcherTimer> _blinkTimer;
std::optional<Windows::UI::Xaml::DispatcherTimer> _bellLightTimer;

event_token _coreOutputEventToken;

Expand Down Expand Up @@ -202,6 +208,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation

void _CursorTimerTick(Windows::Foundation::IInspectable const& sender, Windows::Foundation::IInspectable const& e);
void _BlinkTimerTick(Windows::Foundation::IInspectable const& sender, Windows::Foundation::IInspectable const& e);
void _BellLightOff(Windows::Foundation::IInspectable const& sender, Windows::Foundation::IInspectable const& e);

void _SetEndSelectionPointAtCursor(Windows::Foundation::Point const& cursorPosition);

Expand Down
2 changes: 2 additions & 0 deletions src/cascadia/TerminalControl/TermControl.idl
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ namespace Microsoft.Terminal.Control
void ToggleShaderEffects();
void SendInput(String input);

void BellLightOn();

Boolean ReadOnly { get; };
void ToggleReadOnly();
}
Expand Down
3 changes: 3 additions & 0 deletions src/cascadia/TerminalControl/TermControl.xaml
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@
-->

<Grid x:Name="RootGrid">
<Grid.Lights>
<local:VisualBellLight x:Name="BellLight" />
</Grid.Lights>
<Image x:Name="BackgroundImage"
AutomationProperties.AccessibilityView="Raw" />
<Grid>
Expand Down
7 changes: 7 additions & 0 deletions src/cascadia/TerminalControl/TerminalControlLib.vcxproj
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@
<ClInclude Include="SearchBoxControl.h">
<DependentUpon>SearchBoxControl.xaml</DependentUpon>
</ClInclude>
<ClInclude Include="XamlLights.h">
<DependentUpon>XamlLights.idl</DependentUpon>
</ClInclude>
<ClInclude Include="TermControl.h">
<DependentUpon>TermControl.xaml</DependentUpon>
</ClInclude>
Expand Down Expand Up @@ -76,6 +79,9 @@
<ClCompile Include="SearchBoxControl.cpp">
<DependentUpon>SearchBoxControl.xaml</DependentUpon>
</ClCompile>
<ClCompile Include="XamlLights.cpp">
<DependentUpon>XamlLights.idl</DependentUpon>
</ClCompile>
<ClCompile Include="TermControl.cpp">
<DependentUpon>TermControl.xaml</DependentUpon>
</ClCompile>
Expand All @@ -102,6 +108,7 @@
<Midl Include="SearchBoxControl.idl">
<DependentUpon>SearchBoxControl.xaml</DependentUpon>
</Midl>
<Midl Include="XamlLights.idl" />
<Midl Include="TermControl.idl">
<DependentUpon>TermControl.xaml</DependentUpon>
</Midl>
Expand Down
87 changes: 87 additions & 0 deletions src/cascadia/TerminalControl/XamlLights.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#include "pch.h"
#include "TermControl.h"
#include "XamlLights.h"
#include "VisualBellLight.g.cpp"

using namespace winrt::Windows::UI::Xaml;
using namespace winrt::Windows::UI::Xaml::Media;

namespace winrt::Microsoft::Terminal::Control::implementation
{
DependencyProperty VisualBellLight::_IsTargetProperty{ nullptr };

VisualBellLight::VisualBellLight()
{
_InitializeProperties();
}

void VisualBellLight::_InitializeProperties()
{
// Initialize any dependency properties here.
// This performs a lazy load on these properties, instead of
// initializing them when the DLL loads.
if (!_IsTargetProperty)
{
_IsTargetProperty =
DependencyProperty::RegisterAttached(
L"IsTarget",
winrt::xaml_typename<bool>(),
winrt::xaml_typename<Control::VisualBellLight>(),
PropertyMetadata{ winrt::box_value(false), PropertyChangedCallback{ &VisualBellLight::OnIsTargetChanged } });
}
}

// Method Description:
// - This function is called when the first target UIElement is shown on the screen,
// this enables delaying composition object creation until it's actually necessary.
// Arguments:
// - newElement: unused
void VisualBellLight::OnConnected(UIElement const& /* newElement */)
{
if (!CompositionLight())
{
auto spotLight{ Window::Current().Compositor().CreateAmbientLight() };
spotLight.Color(Windows::UI::Colors::White());
CompositionLight(spotLight);
}
}

// Method Description:
// - This function is called when there are no more target UIElements on the screen
// - Disposes of composition resources when no longer in use
// Arguments:
// - oldElement: unused
void VisualBellLight::OnDisconnected(UIElement const& /* oldElement */)
{
if (CompositionLight())
{
CompositionLight(nullptr);
}
}

winrt::hstring VisualBellLight::GetId()
{
return VisualBellLight::GetIdStatic();
}

void VisualBellLight::OnIsTargetChanged(DependencyObject const& d, DependencyPropertyChangedEventArgs const& e)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think I totally understand what's going on with this method. This is a static function that's called when IsTarget changes on a VisualBellLight? And then d is either a UIElement, or a Brush... When is this ever a brush?

Or is this just forward thinking? All this seems like handy boilerplate around XamlLight, I'm kinda shocked (but also not surprised) that we need this much boilerplate just to add a light.

Copy link
Member

Choose a reason for hiding this comment

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

So yeah I think this is some cool stuff -- he's created an "attached property" (like Grid.Row -- a property that's about one object, stored in global storage, attached to another object). It looks like this one can be attached to either a brush or a UI Element, seamlessly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! I've mostly just adopted the code from the xaml light docs. Lights can only target a Brush or a UIElement, and yeah in our case we never use it for a brush but I feel like its probably a good idea to have that check anyway

{
const auto uielem{ d.try_as<UIElement>() };
Copy link
Member

Choose a reason for hiding this comment

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

kinda surprised that uielem resolved as a word with spellcheck

const auto brush{ d.try_as<Brush>() };

if (!uielem && !brush)
{
// terminate early
return;
}

const auto isAdding = winrt::unbox_value<bool>(e.NewValue());
const auto id = GetIdStatic();

isAdding ? (uielem ? XamlLight::AddTargetElement(id, uielem) : XamlLight::AddTargetBrush(id, brush)) :
PankajBhojwani marked this conversation as resolved.
Show resolved Hide resolved
(uielem ? XamlLight::RemoveTargetElement(id, uielem) : XamlLight::RemoveTargetBrush(id, brush));
}
}
49 changes: 49 additions & 0 deletions src/cascadia/TerminalControl/XamlLights.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#pragma once

#include "cppwinrt_utils.h"
#include "VisualBellLight.g.h"

namespace winrt::Microsoft::Terminal::Control::implementation
{
struct VisualBellLight : VisualBellLightT<VisualBellLight>
{
VisualBellLight();

winrt::hstring GetId();

static Windows::UI::Xaml::DependencyProperty IsTargetProperty() { return _IsTargetProperty; }

static bool GetIsTarget(Windows::UI::Xaml::DependencyObject const& target)
{
return winrt::unbox_value<bool>(target.GetValue(_IsTargetProperty));
}

static void SetIsTarget(Windows::UI::Xaml::DependencyObject const& target, bool value)
{
target.SetValue(_IsTargetProperty, winrt::box_value(value));
}

void OnConnected(Windows::UI::Xaml::UIElement const& newElement);
void OnDisconnected(Windows::UI::Xaml::UIElement const& oldElement);

static void OnIsTargetChanged(Windows::UI::Xaml::DependencyObject const& d, Windows::UI::Xaml::DependencyPropertyChangedEventArgs const& e);

inline static winrt::hstring GetIdStatic()
{
// This specifies the unique name of the light. In most cases you should use the type's full name.
return winrt::xaml_typename<winrt::Microsoft::Terminal::Control::VisualBellLight>().Name;
}

private:
static void _InitializeProperties();
static Windows::UI::Xaml::DependencyProperty _IsTargetProperty;
};
}

namespace winrt::Microsoft::Terminal::Control::factory_implementation
{
BASIC_FACTORY(VisualBellLight);
}
13 changes: 13 additions & 0 deletions src/cascadia/TerminalControl/XamlLights.idl
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

namespace Microsoft.Terminal.Control
{
[default_interface] runtimeclass VisualBellLight : Windows.UI.Xaml.Media.XamlLight
{
VisualBellLight();
static Windows.UI.Xaml.DependencyProperty IsTargetProperty { get; };
static Boolean GetIsTarget(Windows.UI.Xaml.DependencyObject target);
static void SetIsTarget(Windows.UI.Xaml.DependencyObject target, Boolean value);
}
}
29 changes: 27 additions & 2 deletions src/cascadia/TerminalSettingsEditor/Profiles.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,6 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
INITIALIZE_BINDABLE_ENUM_SETTING_REVERSE_ORDER(BackgroundImageStretchMode, BackgroundImageStretchMode, winrt::Windows::UI::Xaml::Media::Stretch, L"Profile_BackgroundImageStretchMode", L"Content");
INITIALIZE_BINDABLE_ENUM_SETTING(AntiAliasingMode, TextAntialiasingMode, winrt::Microsoft::Terminal::Control::TextAntialiasingMode, L"Profile_AntialiasingMode", L"Content");
INITIALIZE_BINDABLE_ENUM_SETTING_REVERSE_ORDER(CloseOnExitMode, CloseOnExitMode, winrt::Microsoft::Terminal::Settings::Model::CloseOnExitMode, L"Profile_CloseOnExit", L"Content");
INITIALIZE_BINDABLE_ENUM_SETTING_REVERSE_ORDER(BellStyle, BellStyle, winrt::Microsoft::Terminal::Settings::Model::BellStyle, L"Profile_BellStyle", L"Content");
INITIALIZE_BINDABLE_ENUM_SETTING(ScrollState, ScrollbarState, winrt::Microsoft::Terminal::Control::ScrollbarState, L"Profile_ScrollbarVisibility", L"Content");

// manually add Custom FontWeight option. Don't add it to the Map
Expand Down Expand Up @@ -573,7 +572,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
}
else if (settingName == L"BellStyle")
{
_PropertyChangedHandlers(*this, PropertyChangedEventArgs{ L"CurrentBellStyle" });
_PropertyChangedHandlers(*this, PropertyChangedEventArgs{ L"IsBellStyleFlagSet" });
}
else if (settingName == L"ScrollState")
{
Expand Down Expand Up @@ -627,6 +626,32 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
_State.Profile().ColorSchemeName(val.Name());
}

bool Profiles::IsBellStyleFlagSet(const uint32_t flag)
{
return (WI_EnumValue(_State.Profile().BellStyle()) & flag) == flag;
Copy link
Member

Choose a reason for hiding this comment

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

Wait uh, what exactly is happening here? Is this just WI_IsFlagSet?

Copy link
Member

Choose a reason for hiding this comment

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

or WI_AreAllFlagsSet?

Copy link
Member

Choose a reason for hiding this comment

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

explained offline -- this is because the WI macros require a compile-time constant. We have to do it manually.

}

void Profiles::SetBellStyleAudible(winrt::Windows::Foundation::IReference<bool> on)
{
auto currentStyle = State().Profile().BellStyle();
WI_UpdateFlag(currentStyle, Model::BellStyle::Audible, winrt::unbox_value<bool>(on));
State().Profile().BellStyle(currentStyle);
}

void Profiles::SetBellStyleWindow(winrt::Windows::Foundation::IReference<bool> on)
{
auto currentStyle = State().Profile().BellStyle();
WI_UpdateFlag(currentStyle, Model::BellStyle::Window, winrt::unbox_value<bool>(on));
State().Profile().BellStyle(currentStyle);
}

void Profiles::SetBellStyleTaskbar(winrt::Windows::Foundation::IReference<bool> on)
{
auto currentStyle = State().Profile().BellStyle();
WI_UpdateFlag(currentStyle, Model::BellStyle::Taskbar, winrt::unbox_value<bool>(on));
State().Profile().BellStyle(currentStyle);
}

void Profiles::DeleteConfirmation_Click(IInspectable const& /*sender*/, RoutedEventArgs const& /*e*/)
{
auto state{ winrt::get_self<ProfilePageNavigationState>(_State) };
Expand Down
Loading