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

Fixed self capture in TermControl #3908

Merged
5 commits merged into from
Dec 13, 2019
Merged

Fixed self capture in TermControl #3908

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bb67a00
Fixed TermControl lambda captures
mkitzan Dec 11, 2019
8afce7f
Should've ran Format Document the first time
mkitzan Dec 11, 2019
a81fdc1
Changes for auto_revoke
mkitzan Dec 11, 2019
d271c33
PR review revision commit
mkitzan Dec 11, 2019
7729685
Tick now takes get_weak()
mkitzan Dec 13, 2019
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
169 changes: 88 additions & 81 deletions src/cascadia/TerminalControl/TermControl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,10 +117,11 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
// in any layout change chain. That gives us great flexibility in finding the right point
// at which to initialize our renderer (and our terminal).
// Any earlier than the last layout update and we may not know the terminal's starting size.

if (_InitializeTerminal())
{
// Only let this succeed once.
this->_layoutUpdatedRevoker.revoke();
_layoutUpdatedRevoker.revoke();
}
});

Expand All @@ -144,8 +145,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation

// These are important:
// 1. When we get tapped, focus us
this->Tapped([this](auto&, auto& e) {
this->Focus(FocusState::Pointer);
_tappedRevoker = this->Tapped(winrt::auto_revoke, [this](auto&, auto& e) {
Focus(FocusState::Pointer);
e.Handled(true);
});
// 2. Make sure we can be focused (why this isn't `Focusable` I'll never know)
Expand All @@ -156,11 +157,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
// DON'T CALL _InitializeTerminal here - wait until the swap chain is loaded to do that.

// Subscribe to the connection's disconnected event and call our connection closed handlers.
_connectionStateChangedRevoker = _connection.StateChanged(winrt::auto_revoke, [weakThis = get_weak()](auto&& /*s*/, auto&& /*v*/) {
if (auto strongThis{ weakThis.get() })
{
strongThis->_ConnectionStateChangedHandlers(*strongThis, nullptr);
}
_connectionStateChangedRevoker = _connection.StateChanged(winrt::auto_revoke, [this](auto&& /*s*/, auto&& /*v*/) {
_ConnectionStateChangedHandlers(*this, nullptr);
});
}
DHowett-MSFT marked this conversation as resolved.
Show resolved Hide resolved

Expand All @@ -176,34 +174,38 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation

// Dispatch a call to the UI thread to apply the new settings to the
// terminal.
_root.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [this]() {
// Update our control settings
_ApplyUISettings();
_root.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [weakThis{ get_weak() }, this]() {
// If 'weakThis' is locked, then we can safely work with 'this'
if (auto control{ weakThis.get() })
{
// Update our control settings
_ApplyUISettings();

// Update DxEngine's SelectionBackground
_renderEngine->SetSelectionBackground(_settings.SelectionBackground());
// Update DxEngine's SelectionBackground
_renderEngine->SetSelectionBackground(_settings.SelectionBackground());

// Update the terminal core with its new Core settings
_terminal->UpdateSettings(_settings);
// Update the terminal core with its new Core settings
_terminal->UpdateSettings(_settings);

// Refresh our font with the renderer
_UpdateFont();
// Refresh our font with the renderer
_UpdateFont();

const auto width = _swapChainPanel.ActualWidth();
const auto height = _swapChainPanel.ActualHeight();
if (width != 0 && height != 0)
{
// If the font size changed, or the _swapchainPanel's size changed
// for any reason, we'll need to make sure to also resize the
// buffer. _DoResize will invalidate everything for us.
auto lock = _terminal->LockForWriting();
_DoResize(width, height);
}
const auto width = _swapChainPanel.ActualWidth();
const auto height = _swapChainPanel.ActualHeight();
if (width != 0 && height != 0)
{
// If the font size changed, or the _swapchainPanel's size changed
// for any reason, we'll need to make sure to also resize the
// buffer. _DoResize will invalidate everything for us.
auto lock = _terminal->LockForWriting();
_DoResize(width, height);
}

// set TSF Foreground
Media::SolidColorBrush foregroundBrush{};
foregroundBrush.Color(ColorRefToColor(_settings.DefaultForeground()));
_tsfInputControl.Foreground(foregroundBrush);
// set TSF Foreground
Media::SolidColorBrush foregroundBrush{};
foregroundBrush.Color(ColorRefToColor(_settings.DefaultForeground()));
_tsfInputControl.Foreground(foregroundBrush);
}
});
}

Expand Down Expand Up @@ -361,30 +363,34 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
// - <none>
void TermControl::_BackgroundColorChanged(const uint32_t color)
{
_root.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [this, color]() {
const auto R = GetRValue(color);
const auto G = GetGValue(color);
const auto B = GetBValue(color);
_root.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [weakThis{ get_weak() }, this, color]() {
// If 'weakThis' is locked, then we can safely work with 'this'
if (auto control{ weakThis.get() })
{
const auto R = GetRValue(color);
const auto G = GetGValue(color);
const auto B = GetBValue(color);

winrt::Windows::UI::Color bgColor{};
bgColor.R = R;
bgColor.G = G;
bgColor.B = B;
bgColor.A = 255;
winrt::Windows::UI::Color bgColor{};
bgColor.R = R;
bgColor.G = G;
bgColor.B = B;
bgColor.A = 255;

if (auto acrylic = _root.Background().try_as<Media::AcrylicBrush>())
{
acrylic.FallbackColor(bgColor);
acrylic.TintColor(bgColor);
}
else if (auto solidColor = _root.Background().try_as<Media::SolidColorBrush>())
{
solidColor.Color(bgColor);
}
if (auto acrylic = _root.Background().try_as<Media::AcrylicBrush>())
{
acrylic.FallbackColor(bgColor);
acrylic.TintColor(bgColor);
}
else if (auto solidColor = _root.Background().try_as<Media::SolidColorBrush>())
{
solidColor.Color(bgColor);
}

// Set the default background as transparent to prevent the
// DX layer from overwriting the background image or acrylic effect
_settings.DefaultBackground(ARGB(0, R, G, B));
// Set the default background as transparent to prevent the
// DX layer from overwriting the background image or acrylic effect
_settings.DefaultBackground(ARGB(0, R, G, B));
}
});
}

Expand Down Expand Up @@ -437,10 +443,15 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
}

auto chain = _renderEngine->GetSwapChain();
_swapChainPanel.Dispatcher().RunAsync(CoreDispatcherPriority::High, [=]() {
auto lock = _terminal->LockForWriting();
auto nativePanel = _swapChainPanel.as<ISwapChainPanelNative>();
nativePanel->SetSwapChain(chain.Get());

_swapChainPanel.Dispatcher().RunAsync(CoreDispatcherPriority::High, [weakThis{ get_weak() }, this, chain]() {
// If 'weakThis' is locked, then we can safely work with 'this'
if (auto control{ weakThis.get() })
{
auto lock = _terminal->LockForWriting();
auto nativePanel = _swapChainPanel.as<ISwapChainPanelNative>();
nativePanel->SetSwapChain(chain.Get());
}
});
}

Expand Down Expand Up @@ -512,6 +523,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
THROW_IF_FAILED(dxEngine->Enable());
_renderEngine = std::move(dxEngine);

// This event is explicitly revoked in the destructor: does not need weak_ref
auto onRecieveOutputFn = [this](const hstring str) {
_terminal->Write(str.c_str());
};
Expand All @@ -521,11 +533,15 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
_terminal->SetWriteInputCallback(inputFn);

auto chain = _renderEngine->GetSwapChain();
_swapChainPanel.Dispatcher().RunAsync(CoreDispatcherPriority::High, [this, chain]() {
_terminal->LockConsole();
auto nativePanel = _swapChainPanel.as<ISwapChainPanelNative>();
nativePanel->SetSwapChain(chain.Get());
_terminal->UnlockConsole();

_swapChainPanel.Dispatcher().RunAsync(CoreDispatcherPriority::High, [weakThis{ get_weak() }, this, chain]() {
if (auto control{ weakThis.get() })
{
_terminal->LockConsole();
auto nativePanel = _swapChainPanel.as<ISwapChainPanelNative>();
nativePanel->SetSwapChain(chain.Get());
_terminal->UnlockConsole();
}
});

// Set up the height of the ScrollViewer and the grid we're using to fake our scrolling height
Expand Down Expand Up @@ -599,7 +615,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation

static constexpr auto AutoScrollUpdateInterval = std::chrono::microseconds(static_cast<int>(1.0 / 30.0 * 1000000));
_autoScrollTimer.Interval(AutoScrollUpdateInterval);
_autoScrollTimer.Tick({ this, &TermControl::_UpdateAutoScroll });
_autoScrollTimer.Tick({ get_weak(), &TermControl::_UpdateAutoScroll });

// Set up blinking cursor
int blinkTime = GetCaretBlinkTime();
Expand All @@ -608,7 +624,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
// Create a timer
_cursorTimer = std::make_optional(DispatcherTimer());
_cursorTimer.value().Interval(std::chrono::milliseconds(blinkTime));
_cursorTimer.value().Tick({ this, &TermControl::_BlinkCursor });
_cursorTimer.value().Tick({ get_weak(), &TermControl::_BlinkCursor });
_cursorTimer.value().Start();
}
else
Expand Down Expand Up @@ -1480,15 +1496,19 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
}

// Update our scrollbar
_scrollBar.Dispatcher().RunAsync(CoreDispatcherPriority::Low, [=]() {
_scrollBar.Dispatcher().RunAsync(CoreDispatcherPriority::Low, [weakThis{ get_weak() }, this, viewTop, viewHeight, bufferSize]() {
// Even if we weren't closed/closing few lines above, we might be
// while waiting for this block of code to be dispatched.
if (_closing.load())
// If 'weakThis' is locked, then we can safely work with 'this'
if (auto control{ weakThis.get() })
{
return;
}
if (_closing.load())
{
return;
}

_ScrollbarUpdater(_scrollBar, viewTop, viewHeight, bufferSize);
_ScrollbarUpdater(_scrollBar, viewTop, viewHeight, bufferSize);
}
});

// Set this value as our next expected scroll position.
Expand Down Expand Up @@ -1576,19 +1596,6 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
_connection.TerminalOutput(_connectionOutputEventToken);
_connectionStateChangedRevoker.revoke();

// Clear out the cursor timer, so it doesn't trigger again on us once we're destructed.
if (auto localCursorTimer{ std::exchange(_cursorTimer, std::nullopt) })
{
localCursorTimer->Stop();
// cursorTimer timer, now stopped, is destroyed.
}

if (auto localAutoScrollTimer{ std::exchange(_autoScrollTimer, nullptr) })
{
localAutoScrollTimer.Stop();
// _autoScrollTimer timer, now stopped, is destroyed.
}

Copy link
Contributor

Choose a reason for hiding this comment

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

i'm surprised tearing down the timers isn't necessary anymore, but I'm willing to believe it 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those Stop calls were only throwing in the event that TermControl was being destructed at the end of an event handler, which was strange. Could just wrap them in try-catch blocks if the Stop calls really should be there 🤔

Copy link
Member

Choose a reason for hiding this comment

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

The most correct solution might be to make both the timers Tick events take a get_weak() as opposed to this, right? If we had done that from the start, we probably wouldn't have ever needed these lines...

if (auto localConnection{ std::exchange(_connection, nullptr) })
{
localConnection.Close();
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalControl/TermControl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
TSFInputControl _tsfInputControl;

event_token _connectionOutputEventToken;
TermControl::Tapped_revoker _tappedRevoker;
TerminalConnection::ITerminalConnection::StateChanged_revoker _connectionStateChangedRevoker;

std::unique_ptr<::Microsoft::Terminal::Core::Terminal> _terminal;
Expand Down