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

Avoid timer ticks on frozen windows #16587

Merged
merged 4 commits into from
Jan 23, 2024
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
2 changes: 2 additions & 0 deletions src/cascadia/LocalTests_TerminalApp/pch.h
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ Author(s):
// Manually include til after we include Windows.Foundation to give it winrt superpowers
#include "til.h"

#include <SafeDispatcherTimer.h>

// Common includes for most tests:
#include "../../inc/conattrs.hpp"
#include "../../types/inc/utils.hpp"
Expand Down
2 changes: 0 additions & 2 deletions src/cascadia/TerminalApp/ColorHelper.cpp
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
#include "pch.h"
#include "ColorHelper.h"
#include <limits>

using namespace winrt::TerminalApp;

Expand Down
3 changes: 1 addition & 2 deletions src/cascadia/TerminalApp/ColorHelper.h
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
#pragma once
#include "pch.h"

#include <winrt/windows.ui.core.h>
#include <winrt/Windows.UI.h>
Copy link
Member

Choose a reason for hiding this comment

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

jeepers I don't love this, presumably this should be in the pch for both this project and the tests, but also those tests really haven't had love in years. So, this is fine for now. (I can only hope that it also being in the pch.h for TerminalAppLib means it won't actually get re-parsed here)

Copy link
Member Author

Choose a reason for hiding this comment

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

The Windows.UI header is a smaller than the Windows.UI.Core header so this at least makes it a tiny bit better.


namespace winrt::TerminalApp
{
Expand Down
4 changes: 3 additions & 1 deletion src/cascadia/TerminalApp/TerminalAppLib.vcxproj
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,9 @@
</ClCompile>
<ClCompile Include="Pane.cpp" />
<ClCompile Include="Pane.LayoutSizeNode.cpp" />
<ClCompile Include="ColorHelper.cpp" />
<ClCompile Include="ColorHelper.cpp">
<PrecompiledHeader>NotUsing</PrecompiledHeader>
</ClCompile>
<ClCompile Include="DebugTapConnection.cpp" />
<ClCompile Include="pch.cpp">
<PrecompiledHeader>Create</PrecompiledHeader>
Expand Down
18 changes: 6 additions & 12 deletions src/cascadia/TerminalApp/TerminalTab.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -121,12 +121,7 @@ namespace winrt::TerminalApp::implementation
void TerminalTab::_BellIndicatorTimerTick(const Windows::Foundation::IInspectable& /*sender*/, const Windows::Foundation::IInspectable& /*e*/)
{
ShowBellIndicator(false);
// Just do a sanity check that the timer still exists before we stop it
if (_bellIndicatorTimer.has_value())
{
_bellIndicatorTimer->Stop();
_bellIndicatorTimer = std::nullopt;
}
_bellIndicatorTimer.Stop();
}

// Method Description:
Expand Down Expand Up @@ -356,14 +351,13 @@ namespace winrt::TerminalApp::implementation
{
ASSERT_UI_THREAD();

if (!_bellIndicatorTimer.has_value())
if (!_bellIndicatorTimer)
{
DispatcherTimer bellIndicatorTimer;
bellIndicatorTimer.Interval(std::chrono::milliseconds(2000));
bellIndicatorTimer.Tick({ get_weak(), &TerminalTab::_BellIndicatorTimerTick });
bellIndicatorTimer.Start();
_bellIndicatorTimer.emplace(std::move(bellIndicatorTimer));
_bellIndicatorTimer.Interval(std::chrono::milliseconds(2000));
_bellIndicatorTimer.Tick({ get_weak(), &TerminalTab::_BellIndicatorTimerTick });
}

_bellIndicatorTimer.Start();
}

// Method Description:
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalApp/TerminalTab.h
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ namespace winrt::TerminalApp::implementation

void _Setup();

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

void _MakeTabViewItem() override;
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalApp/Toast.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,5 +34,5 @@ class Toast : public std::enable_shared_from_this<Toast>

private:
winrt::Microsoft::UI::Xaml::Controls::TeachingTip _tip;
winrt::Windows::UI::Xaml::DispatcherTimer _timer;
SafeDispatcherTimer _timer;
};
2 changes: 2 additions & 0 deletions src/cascadia/TerminalApp/pch.h
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ TRACELOGGING_DECLARE_PROVIDER(g_hTerminalAppProvider);
// Manually include til after we include Windows.Foundation to give it winrt superpowers
#include "til.h"

#include <SafeDispatcherTimer.h>

#include <cppwinrt_utils.h>
#include <wil/cppwinrt_helpers.h> // must go after the CoreDispatcher type is defined

Expand Down
75 changes: 33 additions & 42 deletions src/cascadia/TerminalControl/TermControl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
_isInternalScrollBarUpdate{ false },
_autoScrollVelocity{ 0 },
_autoScrollingPointerPoint{ std::nullopt },
_autoScrollTimer{},
_lastAutoScrollUpdateTime{ std::nullopt },
_cursorTimer{},
_blinkTimer{},
_searchBox{ nullptr }
{
InitializeComponent();
Expand Down Expand Up @@ -1087,10 +1084,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
if (blinkTime != INFINITE)
{
// Create a timer
DispatcherTimer cursorTimer;
cursorTimer.Interval(std::chrono::milliseconds(blinkTime));
cursorTimer.Tick({ get_weak(), &TermControl::_CursorTimerTick });
_cursorTimer.emplace(std::move(cursorTimer));
_cursorTimer.Interval(std::chrono::milliseconds(blinkTime));
Copy link
Member

Choose a reason for hiding this comment

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

note to self: Interval will correctly re-initialize the underlying Timer if it was already Destroy'ed

_cursorTimer.Tick({ get_weak(), &TermControl::_CursorTimerTick });
// As of GH#6586, don't start the cursor timer immediately, and
// don't show the cursor initially. We'll show the cursor and start
// the timer when the control is first focused.
Expand All @@ -1105,13 +1100,12 @@ namespace winrt::Microsoft::Terminal::Control::implementation
_core.CursorOn(_focused || _displayCursorWhileBlurred());
if (_displayCursorWhileBlurred())
{
_cursorTimer->Start();
_cursorTimer.Start();
}
}
else
{
// The user has disabled cursor blinking
_cursorTimer = std::nullopt;
_cursorTimer.Destroy();
}

// Set up blinking attributes
Expand All @@ -1120,16 +1114,14 @@ namespace winrt::Microsoft::Terminal::Control::implementation
if (animationsEnabled && blinkTime != INFINITE)
{
// Create a timer
DispatcherTimer blinkTimer;
blinkTimer.Interval(std::chrono::milliseconds(blinkTime));
blinkTimer.Tick({ get_weak(), &TermControl::_BlinkTimerTick });
blinkTimer.Start();
_blinkTimer.emplace(std::move(blinkTimer));
_blinkTimer.Interval(std::chrono::milliseconds(blinkTime));
_blinkTimer.Tick({ get_weak(), &TermControl::_BlinkTimerTick });
_blinkTimer.Start();
}
else
{
// The user has disabled blinking
_blinkTimer = std::nullopt;
_blinkTimer.Destroy();
}

// Now that the renderer is set up, update the appearance for initialization
Expand Down Expand Up @@ -1498,7 +1490,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// Manually show the cursor when a key is pressed. Restarting
// the timer prevents flickering.
_core.CursorOn(_core.SelectionMode() != SelectionInteractionMode::Mark);
_cursorTimer->Start();
_cursorTimer.Start();
}

return handled;
Expand Down Expand Up @@ -1973,12 +1965,12 @@ namespace winrt::Microsoft::Terminal::Control::implementation
{
// When the terminal focuses, show the cursor immediately
_core.CursorOn(_core.SelectionMode() != SelectionInteractionMode::Mark);
_cursorTimer->Start();
_cursorTimer.Start();
}

if (_blinkTimer)
{
_blinkTimer->Start();
_blinkTimer.Start();
}

// Only update the appearance here if an unfocused config exists - if an
Expand Down Expand Up @@ -2021,13 +2013,13 @@ namespace winrt::Microsoft::Terminal::Control::implementation

if (_cursorTimer && !_displayCursorWhileBlurred())
{
_cursorTimer->Stop();
_cursorTimer.Stop();
_core.CursorOn(false);
}

if (_blinkTimer)
{
_blinkTimer->Stop();
_blinkTimer.Stop();
}

// Check if there is an unfocused config we should set the appearance to
Expand Down Expand Up @@ -2278,7 +2270,16 @@ namespace winrt::Microsoft::Terminal::Control::implementation

// Disconnect the TSF input control so it doesn't receive EditContext events.
TSFInputControl().Close();

// At the time of writing, closing the last tab of a window inexplicably
// does not lead to the destruction of the remaining TermControl instance(s).
// On Win10 we don't destroy window threads due to bugs in DesktopWindowXamlSource.
// In turn, we leak TermControl instances. This results in constant HWND messages
// while the thread is supposed to be idle. Stop these timers avoids this.
_autoScrollTimer.Stop();
_bellLightTimer.Stop();
_cursorTimer.Stop();
_blinkTimer.Stop();

if (!_detached)
{
Expand Down Expand Up @@ -3129,20 +3130,13 @@ namespace winrt::Microsoft::Terminal::Control::implementation
_bellDarkAnimation.Duration(winrt::Windows::Foundation::TimeSpan(std::chrono::milliseconds(TerminalWarningBellInterval)));
}

// Similar to the animation, only initialize the timer here
if (!_bellLightTimer)
{
_bellLightTimer = {};
_bellLightTimer.Interval(std::chrono::milliseconds(TerminalWarningBellInterval));
_bellLightTimer.Tick({ get_weak(), &TermControl::_BellLightOff });
}

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.IsEnabled())
{
// Start the timer, when the timer ticks we switch off the light
_bellLightTimer.Interval(std::chrono::milliseconds(TerminalWarningBellInterval));
_bellLightTimer.Tick({ get_weak(), &TermControl::_BellLightOff });
_bellLightTimer.Start();

// Switch on the light and animate the intensity to fade out
Expand All @@ -3162,15 +3156,12 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void TermControl::_BellLightOff(const Windows::Foundation::IInspectable& /* sender */,
const Windows::Foundation::IInspectable& /* e */)
{
if (_bellLightTimer)
{
// Stop the timer and switch off the light
_bellLightTimer.Stop();
// Stop the timer and switch off the light
_bellLightTimer.Stop();

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

Expand Down Expand Up @@ -3729,9 +3720,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation
{
// If we should be ALWAYS displaying the cursor, turn it on and start blinking.
_core.CursorOn(true);
if (_cursorTimer.has_value())
if (_cursorTimer)
{
_cursorTimer->Start();
_cursorTimer.Start();
}
}
else
Expand All @@ -3740,9 +3731,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// blinking. (if we're focused, then we're already doing the right
// thing)
const auto focused = FocusState() != FocusState::Unfocused;
if (!focused && _cursorTimer.has_value())
if (!focused && _cursorTimer)
{
_cursorTimer->Stop();
_cursorTimer.Stop();
}
_core.CursorOn(focused);
}
Expand Down
8 changes: 4 additions & 4 deletions src/cascadia/TerminalControl/TermControl.h
Original file line number Diff line number Diff line change
Expand Up @@ -236,16 +236,16 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// viewport. View is then scrolled to 'follow' the cursor.
double _autoScrollVelocity;
std::optional<Windows::UI::Input::PointerPoint> _autoScrollingPointerPoint;
Windows::UI::Xaml::DispatcherTimer _autoScrollTimer;
SafeDispatcherTimer _autoScrollTimer;
std::optional<std::chrono::high_resolution_clock::time_point> _lastAutoScrollUpdateTime;
bool _pointerPressedInBounds{ false };

winrt::Windows::UI::Composition::ScalarKeyFrameAnimation _bellLightAnimation{ nullptr };
winrt::Windows::UI::Composition::ScalarKeyFrameAnimation _bellDarkAnimation{ nullptr };
Windows::UI::Xaml::DispatcherTimer _bellLightTimer{ nullptr };
SafeDispatcherTimer _bellLightTimer;

std::optional<Windows::UI::Xaml::DispatcherTimer> _cursorTimer;
Copy link
Member

Choose a reason for hiding this comment

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

thank you for killing these optional<nullable thing>s

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

winrt::Windows::UI::Xaml::Controls::SwapChainPanel::LayoutUpdated_revoker _layoutUpdatedRevoker;
bool _showMarksInScrollbar{ false };
Expand Down
3 changes: 2 additions & 1 deletion src/cascadia/TerminalControl/pch.h
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@ TRACELOGGING_DECLARE_PROVIDER(g_hTerminalControlProvider);
#include <til/mutex.h>
#include <til/winrt.h>

#include "ThrottledFunc.h"
#include <SafeDispatcherTimer.h>
#include <ThrottledFunc.h>

#include <cppwinrt_utils.h>
#include <wil/cppwinrt_helpers.h> // must go after the CoreDispatcher type is defined
3 changes: 2 additions & 1 deletion src/cascadia/WinRTUtils/WinRTUtils.vcxproj
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
<Import Project="$(OpenConsoleDir)src\cppwinrt.build.pre.props" />
<!-- ========================= Headers ======================== -->
<ItemGroup>
<ClInclude Include="inc\SafeDispatcherTimer.h" />
<ClInclude Include="pch.h" />
<ClInclude Include="inc\ScopedResourceLoader.h" />
<ClInclude Include="inc\LibraryResources.h" />
Expand Down Expand Up @@ -52,4 +53,4 @@
<!-- ========================= Globals ======================== -->
<Import Project="$(OpenConsoleDir)src\cppwinrt.build.post.props" />
<Import Project="$(OpenConsoleDir)src\common.nugetversions.targets" />
</Project>
</Project>
8 changes: 4 additions & 4 deletions src/cascadia/WinRTUtils/WinRTUtils.vcxproj.filters
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,21 @@
<Project ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<ItemGroup>
<Natvis Include="$(SolutionDir)tools\ConsoleTypes.natvis" />
<Natvis Include="$(MSBuildThisFileDirectory)..\..\natvis\wil.natvis" />
</ItemGroup>
<ItemGroup>
<ClCompile Include="pch.cpp" />
<ClCompile Include="LibraryResources.cpp" />
<ClCompile Include="ScopedResourceLoader.cpp" />
<ClCompile Include="Utils.cpp" />
</ItemGroup>
<ItemGroup>
<ClInclude Include="pch.h" />
<ClInclude Include="ScopedResourceLoader.h" />
<ClInclude Include="inc\LibraryResources.h" />
<ClInclude Include="inc\SafeDispatcherTimer.h" />
<ClInclude Include="inc\ScopedResourceLoader.h" />
<ClInclude Include="inc\ThrottledFunc.h" />
<ClInclude Include="inc\Utils.h" />
<ClInclude Include="inc\WtExeUtils.h" />
</ItemGroup>
<ItemGroup>
<None Include="packages.config" />
</ItemGroup>
</Project>
Loading
Loading