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.

Sign up for GitHub

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

Jump to bottom

Avoid timer ticks on frozen windows #16587

Merged
merged 4 commits into from
Jan 23, 2024
Merged

Avoid timer ticks on frozen windows #16587

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5511a11
Avoid timer ticks on frozen windows
lhecker Jan 22, 2024
1666972
Fix test build
lhecker Jan 22, 2024
6325823
Fix more tests
lhecker Jan 22, 2024
44b77ef
Workaround for the pch dilemma
lhecker Jan 23, 2024
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
View file
Open in desktop
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
18 changes: 6 additions & 12 deletions src/cascadia/TerminalApp/TerminalTab.cpp
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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