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

Show indicator of bell in tab #8637

Merged
12 commits merged into from
Jan 22, 2021
Merged
Show file tree
Hide file tree
Changes from 7 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
33 changes: 27 additions & 6 deletions src/cascadia/TerminalApp/Pane.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ Pane::Pane(const GUID& profile, const TermControl& control, const bool lastFocus

// Register an event with the control to have it inform us when it gains focus.
_gotFocusRevoker = control.GotFocus(winrt::auto_revoke, { this, &Pane::_ControlGotFocusHandler });
_lostFocusRevoker = control.LostFocus(winrt::auto_revoke, { this, &Pane::_ControlLostFocusHandler });

// When our border is tapped, make sure to transfer focus to our control.
// LOAD-BEARING: This will NOT work if the border's BorderBrush is set to
Expand Down Expand Up @@ -367,16 +368,23 @@ void Pane::_ControlWarningBellHandler(const winrt::Windows::Foundation::IInspect
auto paneProfile = settings.FindProfile(_profile.value());
if (paneProfile)
{
if (WI_IsFlagSet(paneProfile.BellStyle(), winrt::Microsoft::Terminal::Settings::Model::BellStyle::Visual))
{
// Visual is set, raise the event with the bool set to true
_PaneRaiseBellHandlers(nullptr, true);
PankajBhojwani marked this conversation as resolved.
Show resolved Hide resolved
}
else
{
// Visual is not set, raise the event with the bool set to false
_PaneRaiseBellHandlers(nullptr, false);
}
PankajBhojwani marked this conversation as resolved.
Show resolved Hide resolved

if (WI_IsFlagSet(paneProfile.BellStyle(), winrt::Microsoft::Terminal::Settings::Model::BellStyle::Audible))
{
// Audible is set, play the sound
const auto soundAlias = reinterpret_cast<LPCTSTR>(SND_ALIAS_SYSTEMHAND);
PlaySound(soundAlias, NULL, SND_ALIAS_ID | SND_ASYNC | SND_SENTRY);
}
if (WI_IsFlagSet(paneProfile.BellStyle(), winrt::Microsoft::Terminal::Settings::Model::BellStyle::Visual))
{
// Bubble this event up to app host, starting with bubbling to the hosting tab
_PaneRaiseVisualBellHandlers(nullptr);
}
}
}

Expand All @@ -394,6 +402,16 @@ void Pane::_ControlGotFocusHandler(winrt::Windows::Foundation::IInspectable cons
_GotFocusHandlers(shared_from_this());
}

// Event Description:
// - Called when our control loses focus. We'll use this to trigger our LostFocus
// callback. The tab that's hosting us should have registered a callback which
// can be used to update its own internal focus state
void Pane::_ControlLostFocusHandler(winrt::Windows::Foundation::IInspectable const& /* sender */,
RoutedEventArgs const& /* args */)
{
_LostFocusHandlers(shared_from_this());
}

// Method Description:
// - Fire our Closed event to tell our parent that we should be removed.
// Arguments:
Expand Down Expand Up @@ -703,6 +721,7 @@ void Pane::_CloseChild(const bool closeFirst)

// re-attach our handler for the control's GotFocus event.
_gotFocusRevoker = _control.GotFocus(winrt::auto_revoke, { this, &Pane::_ControlGotFocusHandler });
_lostFocusRevoker = _control.LostFocus(winrt::auto_revoke, { this, &Pane::_ControlLostFocusHandler });

// If we're inheriting the "last active" state from one of our children,
// focus our control now. This should trigger our own GotFocus event.
Expand Down Expand Up @@ -1408,6 +1427,7 @@ std::pair<std::shared_ptr<Pane>, std::shared_ptr<Pane>> Pane::_Split(SplitState
// control telling us that it's now focused, we want it telling its new
// parent.
_gotFocusRevoker.revoke();
_lostFocusRevoker.revoke();

_splitState = actualSplitType;
_desiredSplitPosition = 1.0f - splitSize;
Expand Down Expand Up @@ -2057,4 +2077,5 @@ std::optional<SplitState> Pane::PreCalculateAutoSplit(const std::shared_ptr<Pane
}

DEFINE_EVENT(Pane, GotFocus, _GotFocusHandlers, winrt::delegate<std::shared_ptr<Pane>>);
DEFINE_EVENT(Pane, PaneRaiseVisualBell, _PaneRaiseVisualBellHandlers, winrt::delegate<std::shared_ptr<Pane>>);
DEFINE_EVENT(Pane, LostFocus, _LostFocusHandlers, winrt::delegate<std::shared_ptr<Pane>>);
DEFINE_EVENT(Pane, PaneRaiseBell, _PaneRaiseBellHandlers, winrt::Windows::Foundation::EventHandler<bool>);
6 changes: 5 additions & 1 deletion src/cascadia/TerminalApp/Pane.h
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,8 @@ class Pane : public std::enable_shared_from_this<Pane>

WINRT_CALLBACK(Closed, winrt::Windows::Foundation::EventHandler<winrt::Windows::Foundation::IInspectable>);
DECLARE_EVENT(GotFocus, _GotFocusHandlers, winrt::delegate<std::shared_ptr<Pane>>);
DECLARE_EVENT(PaneRaiseVisualBell, _PaneRaiseVisualBellHandlers, winrt::delegate<std::shared_ptr<Pane>>);
DECLARE_EVENT(LostFocus, _LostFocusHandlers, winrt::delegate<std::shared_ptr<Pane>>);
DECLARE_EVENT(PaneRaiseBell, _PaneRaiseBellHandlers, winrt::Windows::Foundation::EventHandler<bool>);

private:
struct SnapSizeResult;
Expand Down Expand Up @@ -111,6 +112,7 @@ class Pane : public std::enable_shared_from_this<Pane>
winrt::event_token _warningBellToken{ 0 };

winrt::Windows::UI::Xaml::UIElement::GotFocus_revoker _gotFocusRevoker;
winrt::Windows::UI::Xaml::UIElement::LostFocus_revoker _lostFocusRevoker;

std::shared_mutex _createCloseLock{};

Expand Down Expand Up @@ -144,6 +146,8 @@ class Pane : public std::enable_shared_from_this<Pane>
winrt::Windows::Foundation::IInspectable const& e);
void _ControlGotFocusHandler(winrt::Windows::Foundation::IInspectable const& sender,
winrt::Windows::UI::Xaml::RoutedEventArgs const& e);
void _ControlLostFocusHandler(winrt::Windows::Foundation::IInspectable const& sender,
winrt::Windows::UI::Xaml::RoutedEventArgs const& e);

std::pair<float, float> _CalcChildrenSizes(const float fullSize) const;
SnapChildrenSizeResult _CalcSnappedChildrenSizes(const bool widthOrHeight, const float fullSize) const;
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalApp/TabHeaderControl.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ namespace winrt::TerminalApp::implementation
OBSERVABLE_GETSET_PROPERTY(bool, IsPaneZoomed, _PropertyChangedHandlers);
OBSERVABLE_GETSET_PROPERTY(bool, IsProgressRingActive, _PropertyChangedHandlers);
OBSERVABLE_GETSET_PROPERTY(bool, IsProgressRingIndeterminate, _PropertyChangedHandlers);
OBSERVABLE_GETSET_PROPERTY(bool, BellIndicator, _PropertyChangedHandlers);
OBSERVABLE_GETSET_PROPERTY(uint32_t, ProgressValue, _PropertyChangedHandlers);

private:
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalApp/TabHeaderControl.idl
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ namespace TerminalApp
Boolean IsPaneZoomed { get; set; };
Boolean IsProgressRingActive { get; set; };
Boolean IsProgressRingIndeterminate { get; set; };
Boolean BellIndicator { get; set; };
UInt32 ProgressValue { get; set; };

TabHeaderControl();
Expand Down
6 changes: 6 additions & 0 deletions src/cascadia/TerminalApp/TabHeaderControl.xaml
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@ the MIT License. See LICENSE in the project root for license information. -->
<!--We want the progress ring to 'replace' the tab icon, but we don't have control
over the tab icon here (the tab view item does) - so we hide the tab icon there
and use a negative margin for the progress ring here to put it where the icon would be-->
<FontIcon x:Name="HeaderBellIndicator"
FontFamily="Segoe MDL2 Assets"
Visibility="{x:Bind BellIndicator, Mode=OneWay}"
Glyph="&#xEA8F;"
FontSize="12"
Margin="0,0,8,0"/>
<FontIcon x:Name="HeaderZoomIcon"
FontFamily="Segoe MDL2 Assets"
Visibility="{x:Bind IsPaneZoomed, Mode=OneWay}"
Expand Down
99 changes: 94 additions & 5 deletions src/cascadia/TerminalApp/TerminalTab.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,23 @@ namespace winrt::TerminalApp::implementation
TabViewItem().Header(_headerControl);
}

// Method Description:
// - Called when the timer for the bell indicator in the tab header fires
// - Removes the bell indicator from the tab header
// Arguments:
// - sender, e: not used
void TerminalTab::_BellIndicatorTimerTick(Windows::Foundation::IInspectable const& /*sender*/, Windows::Foundation::IInspectable const& /*e*/)
{
auto weakThis{ get_weak() };

if (auto tab{ weakThis.get() })
Copy link
Member

Choose a reason for hiding this comment

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

This will always pass. Since you constructed the event handler (line 290) with a weak reference, c++/winrt automatically ensures that you have a strong living reference inside this function. You can safely use this or just.. use unqualified members.

{
tab->ShowBellIndicator(false);
tab->_bellIndicatorTimer.value().Stop();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
tab->_bellIndicatorTimer.value().Stop();
tab->_bellIndicatorTimer->Stop();

tab->_bellIndicatorTimer = std::nullopt;
}
}

// Method Description:
// - Initializes a TabViewItem for this Tab instance.
// Arguments:
Expand Down Expand Up @@ -129,6 +146,11 @@ namespace winrt::TerminalApp::implementation
lastFocusedControl.Focus(_focusState);
lastFocusedControl.TaskbarProgressChanged();
}
// When we gain focus, remove the bell indicator if it is active
if (_headerControl.BellIndicator())
{
ShowBellIndicator(false);
}
}
}

Expand Down Expand Up @@ -236,6 +258,41 @@ namespace winrt::TerminalApp::implementation
}
}

// Method Description:
// - Hide or show the bell indicator in the tab header
// Arguments:
// - show: if true, we show the indicator; if false, we hide the indicator
winrt::fire_and_forget TerminalTab::ShowBellIndicator(const bool show)
{
auto weakThis{ get_weak() };

co_await winrt::resume_foreground(TabViewItem().Dispatcher());

if (auto tab{ weakThis.get() })
{
tab->_headerControl.BellIndicator(show);
}
}

// Method Description:
// - Activates the timer for the bell indicator in the tab
// - Called if a bell raised when the tab already has focus
winrt::fire_and_forget TerminalTab::ActivateBellIndicatorTimer()
{
auto weakThis{ get_weak() };

co_await winrt::resume_foreground(TabViewItem().Dispatcher());

if (auto tab{ weakThis.get() })
{
DispatcherTimer bellIndicatorTimer;
bellIndicatorTimer.Interval(std::chrono::milliseconds(2000));
bellIndicatorTimer.Tick({ get_weak(), &TerminalTab::_BellIndicatorTimerTick });
bellIndicatorTimer.Start();
tab->_bellIndicatorTimer.emplace(std::move(bellIndicatorTimer));
Copy link
Member

Choose a reason for hiding this comment

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

can an application DOS us by spamming bell? it will make us start a huge number of timers, and keep overwriting the optional.

Copy link
Member

Choose a reason for hiding this comment

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

(we'll waste memory for up to 2 seconds at which point they'll start to fire)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes good catch! We now only create a new timer if the optional doesn't have a value

}
}

// Method Description:
// - Gets the title string of the last focused terminal control in our tree.
// Returns the empty string if there is no such control.
Expand Down Expand Up @@ -564,10 +621,30 @@ namespace winrt::TerminalApp::implementation
// Do nothing if the Tab's lifetime is expired or pane isn't new.
auto tab{ weakThis.get() };

if (tab && sender != tab->_activePane)
if (tab)
{
tab->_UpdateActivePane(sender);
tab->_RecalculateAndApplyTabColor();
if (sender != tab->_activePane)
{
tab->_UpdateActivePane(sender);
tab->_RecalculateAndApplyTabColor();
}
tab->_focusState = WUX::FocusState::Programmatic;
Copy link
Member

Choose a reason for hiding this comment

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

i hope this is right! what else is _focusState being used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only other places (before this PR) _focusState was being checked/updated: TerminalTab::_RefreshVisualState and TerminalTab::Focus

However, without this addition, the tab does not know when the app itself loses focus (by alt-tab/minimize etc) so it used to still think it is focused even though the app itself and the underlying control(s) are all unfocused.

// This tab has gained focus, remove the bell indicator if it is active
if (tab->_headerControl.BellIndicator())
{
tab->ShowBellIndicator(false);
}
}
});

pane->LostFocus([weakThis](std::shared_ptr<Pane> /*sender*/) {
// Do nothing if the Tab's lifetime is expired or pane isn't new.
auto tab{ weakThis.get() };

if (tab)
{
// update this tab's focus state
tab->_focusState = WUX::FocusState::Unfocused;
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 sure - there will be a moment, when you move from pane A to pane B, that the tab thinks it does not have focus?

}
});

Expand Down Expand Up @@ -601,10 +678,22 @@ namespace winrt::TerminalApp::implementation
// Add a PaneRaiseVisualBell event handler to the Pane. When the pane emits this event,
// we need to bubble it all the way to app host. In this part of the chain we bubble it
// from the hosting tab to the page.
pane->PaneRaiseVisualBell([weakThis](auto&& /*s*/) {
pane->PaneRaiseBell([weakThis](auto&& /*s*/, auto&& visual) {
if (auto tab{ weakThis.get() })
{
tab->_TabRaiseVisualBellHandlers();
if (visual)
{
tab->_TabRaiseVisualBellHandlers();
}
tab->ShowBellIndicator(true);

// If this tab is focused, activate the bell indicator timer, which will
// remove the bell indicator once it fires
// (otherwise, the indicator is removed when the tab gets focus)
if (tab->_focusState != WUX::FocusState::Unfocused)
{
tab->ActivateBellIndicatorTimer();
}
}
});
}
Expand Down
6 changes: 6 additions & 0 deletions src/cascadia/TerminalApp/TerminalTab.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ namespace winrt::TerminalApp::implementation
winrt::fire_and_forget UpdateIcon(const winrt::hstring iconPath);
winrt::fire_and_forget HideIcon(const bool hide);

winrt::fire_and_forget ShowBellIndicator(const bool show);
winrt::fire_and_forget ActivateBellIndicatorTimer();

float CalcSnappedDimension(const bool widthOrHeight, const float dimension) const;
winrt::Microsoft::Terminal::Settings::Model::SplitState PreCalculateAutoSplit(winrt::Windows::Foundation::Size rootSize) const;
bool PreCalculateCanSplit(winrt::Microsoft::Terminal::Settings::Model::SplitState splitType,
Expand Down Expand Up @@ -100,6 +103,9 @@ namespace winrt::TerminalApp::implementation

winrt::TerminalApp::ShortcutActionDispatch _dispatch;

std::optional<Windows::UI::Xaml::DispatcherTimer> _bellIndicatorTimer;
void _BellIndicatorTimerTick(Windows::Foundation::IInspectable const& sender, Windows::Foundation::IInspectable const& e);

void _MakeTabViewItem();

void _SetToolTip(const winrt::hstring& tabTitle);
Expand Down