Skip to content

Commit

Permalink
General improvements in preparation for #16598 (#16601)
Browse files Browse the repository at this point in the history 
This contains all the parts of #16598 that aren't specific to session
restore, but are required for the code in #16598:
* Adds new GUID<>String functions that remove the `{}` brackets.
* Adds `SessionId` to the `ITerminalConnection` interface.
* Flush the `ApplicationState` before we terminate the process.
* Not monitoring `state.json` for changes is important as it prevents
  disturbing the session state while session persistence is ongoing.
  That's because when `ApplicationState` flushes to disk, the FS
  monitor will be triggered and reload the `ApplicationState` again.
  • Loading branch information
@lhecker
lhecker authored Feb 6, 2024
1 parent b70fd5e commit 5dda507
Show file tree
Hide file tree
Showing 29 changed files with 173 additions and 147 deletions.
1 change: 1 addition & 0 deletions .github/actions/spelling/expect/expect.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ chh
chshdng
CHT
Cic
CLASSSTRING
CLE
cleartype
CLICKACTIVE
Expand Down
7 changes: 1 addition & 6 deletions src/cascadia/TerminalApp/AppLogic.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,7 @@ namespace winrt::TerminalApp::implementation
return appLogic->GetSettings();
}

AppLogic::AppLogic() :
_reloadState{ std::chrono::milliseconds(100), []() { ApplicationState::SharedInstance().Reload(); } }
AppLogic::AppLogic()
{
// For your own sanity, it's better to do setup outside the ctor.
// If you do any setup in the ctor that ends up throwing an exception,
Expand Down Expand Up @@ -327,10 +326,6 @@ namespace winrt::TerminalApp::implementation
{
_reloadSettings->Run();
}
else if (ApplicationState::SharedInstance().IsStatePath(modifiedBasename))
{
_reloadState();
}
});
}

Expand Down
1 change: 0 additions & 1 deletion src/cascadia/TerminalApp/AppLogic.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,6 @@ namespace winrt::TerminalApp::implementation
::TerminalApp::AppCommandlineArgs _settingsAppArgs;

std::shared_ptr<ThrottledFuncTrailing<>> _reloadSettings;
til::throttled_func_trailing<> _reloadState;

std::vector<Microsoft::Terminal::Settings::Model::SettingsLoadWarnings> _warnings{};

Expand Down
10 changes: 10 additions & 0 deletions src/cascadia/TerminalApp/DebugTapConnection.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ namespace winrt::Microsoft::TerminalApp::implementation
void TerminalOutput(const winrt::event_token& token) noexcept { _wrappedConnection.TerminalOutput(token); };
winrt::event_token StateChanged(const TypedEventHandler<ITerminalConnection, IInspectable>& handler) { return _wrappedConnection.StateChanged(handler); };
void StateChanged(const winrt::event_token& token) noexcept { _wrappedConnection.StateChanged(token); };
winrt::guid SessionId() const noexcept { return {}; }
ConnectionState State() const noexcept { return _wrappedConnection.State(); }

private:
Expand Down Expand Up @@ -98,6 +99,15 @@ namespace winrt::Microsoft::TerminalApp::implementation
_wrappedConnection = nullptr;
}

guid DebugTapConnection::SessionId() const noexcept
{
if (const auto c = _wrappedConnection.get())
{
return c.SessionId();
}
return {};
}

ConnectionState DebugTapConnection::State() const noexcept
{
if (auto strongConnection{ _wrappedConnection.get() })
Expand Down
2 changes: 2 additions & 0 deletions src/cascadia/TerminalApp/DebugTapConnection.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ namespace winrt::Microsoft::TerminalApp::implementation
void WriteInput(const hstring& data);
void Resize(uint32_t rows, uint32_t columns);
void Close();

winrt::guid SessionId() const noexcept;
winrt::Microsoft::Terminal::TerminalConnection::ConnectionState State() const noexcept;

void SetInputTap(const Microsoft::Terminal::TerminalConnection::ITerminalConnection& inputTap);
Expand Down
65 changes: 29 additions & 36 deletions src/cascadia/TerminalApp/TerminalPage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1210,7 +1210,7 @@ namespace winrt::TerminalApp::implementation
TerminalConnection::ITerminalConnection connection{ nullptr };

auto connectionType = profile.ConnectionType();
winrt::guid sessionGuid{};
Windows::Foundation::Collections::ValueSet valueSet;

if (connectionType == TerminalConnection::AzureConnection::ConnectionType() &&
TerminalConnection::AzureConnection::IsAzureConnectionAvailable())
Expand All @@ -1226,23 +1226,16 @@ namespace winrt::TerminalApp::implementation
connection = TerminalConnection::ConptyConnection{};
}

auto valueSet = TerminalConnection::ConptyConnection::CreateSettings(azBridgePath.native(),
L".",
L"Azure",
false,
L"",
nullptr,
settings.InitialRows(),
settings.InitialCols(),
winrt::guid(),
profile.Guid());

if constexpr (Feature_VtPassthroughMode::IsEnabled())
{
valueSet.Insert(L"passthroughMode", Windows::Foundation::PropertyValue::CreateBoolean(settings.VtPassthrough()));
}

connection.Initialize(valueSet);
valueSet = TerminalConnection::ConptyConnection::CreateSettings(azBridgePath.native(),
L".",
L"Azure",
false,
L"",
nullptr,
settings.InitialRows(),
settings.InitialCols(),
winrt::guid(),
profile.Guid());
}

else
Expand All @@ -1267,38 +1260,38 @@ namespace winrt::TerminalApp::implementation
// process until later, on another thread, after we've already
// restored the CWD to its original value.
auto newWorkingDirectory{ _evaluatePathForCwd(settings.StartingDirectory()) };
auto conhostConn = TerminalConnection::ConptyConnection();
auto valueSet = TerminalConnection::ConptyConnection::CreateSettings(settings.Commandline(),
newWorkingDirectory,
settings.StartingTitle(),
settings.ReloadEnvironmentVariables(),
_WindowProperties.VirtualEnvVars(),
environment,
settings.InitialRows(),
settings.InitialCols(),
winrt::guid(),
profile.Guid());

valueSet.Insert(L"passthroughMode", Windows::Foundation::PropertyValue::CreateBoolean(settings.VtPassthrough()));
connection = TerminalConnection::ConptyConnection{};
valueSet = TerminalConnection::ConptyConnection::CreateSettings(settings.Commandline(),
newWorkingDirectory,
settings.StartingTitle(),
settings.ReloadEnvironmentVariables(),
_WindowProperties.VirtualEnvVars(),
environment,
settings.InitialRows(),
settings.InitialCols(),
winrt::guid(),
profile.Guid());

if (inheritCursor)
{
valueSet.Insert(L"inheritCursor", Windows::Foundation::PropertyValue::CreateBoolean(true));
}
}

conhostConn.Initialize(valueSet);

sessionGuid = conhostConn.Guid();
connection = conhostConn;
if constexpr (Feature_VtPassthroughMode::IsEnabled())
{
valueSet.Insert(L"passthroughMode", Windows::Foundation::PropertyValue::CreateBoolean(settings.VtPassthrough()));
}

connection.Initialize(valueSet);

TraceLoggingWrite(
g_hTerminalAppProvider,
"ConnectionCreated",
TraceLoggingDescription("Event emitted upon the creation of a connection"),
TraceLoggingGuid(connectionType, "ConnectionTypeGuid", "The type of the connection"),
TraceLoggingGuid(profile.Guid(), "ProfileGuid", "The profile's GUID"),
TraceLoggingGuid(sessionGuid, "SessionGuid", "The WT_SESSION's GUID"),
TraceLoggingGuid(connection.SessionId(), "SessionGuid", "The WT_SESSION's GUID"),
TraceLoggingKeyword(MICROSOFT_KEYWORD_MEASURES),
TelemetryPrivacyDataTag(PDT_ProductAndServiceUsage));

Expand Down
10 changes: 8 additions & 2 deletions src/cascadia/TerminalConnection/AzureConnection.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,14 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
{
if (settings)
{
_initialRows = gsl::narrow<til::CoordType>(winrt::unbox_value_or<uint32_t>(settings.TryLookup(L"initialRows").try_as<Windows::Foundation::IPropertyValue>(), _initialRows));
_initialCols = gsl::narrow<til::CoordType>(winrt::unbox_value_or<uint32_t>(settings.TryLookup(L"initialCols").try_as<Windows::Foundation::IPropertyValue>(), _initialCols));
_initialRows = unbox_prop_or<uint32_t>(settings, L"initialRows", _initialRows);
_initialCols = unbox_prop_or<uint32_t>(settings, L"initialCols", _initialCols);
_sessionId = unbox_prop_or<guid>(settings, L"sessionId", _sessionId);
}

if (_sessionId == guid{})
{
_sessionId = Utils::CreateGuid();
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/cascadia/TerminalConnection/AzureConnection.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@
#include <mutex>
#include <condition_variable>

#include "ConnectionStateHolder.h"
#include "BaseTerminalConnection.h"
#include "AzureClient.h"

namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
{
struct AzureConnection : AzureConnectionT<AzureConnection>, ConnectionStateHolder<AzureConnection>
struct AzureConnection : AzureConnectionT<AzureConnection>, BaseTerminalConnection<AzureConnection>
{
static winrt::guid ConnectionType() noexcept;
static bool IsAzureConnectionAvailable() noexcept;
Expand Down
21 changes: 19 additions & 2 deletions ...erminalConnection/ConnectionStateHolder.h → ...rminalConnection/BaseTerminalConnection.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,28 @@
namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
{
template<typename T>
struct ConnectionStateHolder
struct BaseTerminalConnection
{
public:
ConnectionState State() const noexcept { return _connectionState; }
winrt::guid SessionId() const noexcept
{
return _sessionId;
}

ConnectionState State() const noexcept
{
return _connectionState;
}

TYPED_EVENT(StateChanged, ITerminalConnection, winrt::Windows::Foundation::IInspectable);

protected:
template<typename U>
U unbox_prop_or(const Windows::Foundation::Collections::ValueSet& blob, std::wstring_view key, U defaultValue)
{
return winrt::unbox_value_or<U>(blob.TryLookup(key).try_as<Windows::Foundation::IPropertyValue>(), defaultValue);
}

#pragma warning(push)
#pragma warning(disable : 26447) // Analyzer is still upset about noexcepts throwing even with function level try.
// Method Description:
Expand Down Expand Up @@ -86,6 +101,8 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
return _isStateOneOf(ConnectionState::Connected);
}

winrt::guid _sessionId{};

private:
std::atomic<ConnectionState> _connectionState{ ConnectionState::NotConnected };
mutable std::mutex _stateMutex;
Expand Down
40 changes: 11 additions & 29 deletions src/cascadia/TerminalConnection/ConptyConnection.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,18 +85,12 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
auto environment = _initialEnv;

{
// Convert connection Guid to string and ignore the enclosing '{}'.
auto wsGuid{ Utils::GuidToString(_guid) };
wsGuid.pop_back();

const auto guidSubStr = std::wstring_view{ wsGuid }.substr(1);

// Ensure every connection has the unique identifier in the environment.
environment.as_map().insert_or_assign(L"WT_SESSION", guidSubStr.data());
// Convert connection Guid to string and ignore the enclosing '{}'.
environment.as_map().insert_or_assign(L"WT_SESSION", Utils::GuidToPlainString(_sessionId));

// The profile Guid does include the enclosing '{}'
const auto profileGuid{ Utils::GuidToString(_profileGuid) };
environment.as_map().insert_or_assign(L"WT_PROFILE_ID", profileGuid.data());
environment.as_map().insert_or_assign(L"WT_PROFILE_ID", Utils::GuidToString(_profileGuid));

// WSLENV is a colon-delimited list of environment variables (+flags) that should appear inside WSL
// https://devblogs.microsoft.com/commandline/share-environment-vars-between-wsl-and-windows/
Expand Down Expand Up @@ -171,7 +165,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
g_hTerminalConnectionProvider,
"ConPtyConnected",
TraceLoggingDescription("Event emitted when ConPTY connection is started"),
TraceLoggingGuid(_guid, "SessionGuid", "The WT_SESSION's GUID"),
TraceLoggingGuid(_sessionId, "SessionGuid", "The WT_SESSION's GUID"),
TraceLoggingWideString(_clientName.c_str(), "Client", "The attached client process"),
TraceLoggingKeyword(MICROSOFT_KEYWORD_MEASURES),
TelemetryPrivacyDataTag(PDT_ProductAndServiceUsage));
Expand All @@ -189,7 +183,6 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
TERMINAL_STARTUP_INFO startupInfo) :
_rows{ 25 },
_cols{ 80 },
_guid{ Utils::CreateGuid() },
_inPipe{ hIn },
_outPipe{ hOut }
{
Expand Down Expand Up @@ -249,12 +242,6 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
return vs;
}

template<typename T>
T unbox_prop_or(const Windows::Foundation::Collections::ValueSet& blob, std::wstring_view key, T defaultValue)
{
return winrt::unbox_value_or<T>(blob.TryLookup(key).try_as<Windows::Foundation::IPropertyValue>(), defaultValue);
}

void ConptyConnection::Initialize(const Windows::Foundation::Collections::ValueSet& settings)
{
if (settings)
Expand All @@ -268,7 +255,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
_startingTitle = unbox_prop_or<winrt::hstring>(settings, L"startingTitle", _startingTitle);
_rows = unbox_prop_or<uint32_t>(settings, L"initialRows", _rows);
_cols = unbox_prop_or<uint32_t>(settings, L"initialCols", _cols);
_guid = unbox_prop_or<winrt::guid>(settings, L"guid", _guid);
_sessionId = unbox_prop_or<winrt::guid>(settings, L"sessionId", _sessionId);
_environment = settings.TryLookup(L"environment").try_as<Windows::Foundation::Collections::ValueSet>();
if constexpr (Feature_VtPassthroughMode::IsEnabled())
{
Expand Down Expand Up @@ -299,17 +286,12 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
_initialEnv = til::env::from_current_environment();
}
}

if (_guid == guid{})
{
_guid = Utils::CreateGuid();
}
}
}

winrt::guid ConptyConnection::Guid() const noexcept
{
return _guid;
if (_sessionId == guid{})
{
_sessionId = Utils::CreateGuid();
}
}

winrt::hstring ConptyConnection::Commandline() const
Expand Down Expand Up @@ -382,7 +364,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
g_hTerminalConnectionProvider,
"ConPtyConnectedToDefterm",
TraceLoggingDescription("Event emitted when ConPTY connection is started, for a defterm session"),
TraceLoggingGuid(_guid, "SessionGuid", "The WT_SESSION's GUID"),
TraceLoggingGuid(_sessionId, "SessionGuid", "The WT_SESSION's GUID"),
TraceLoggingWideString(_clientName.c_str(), "Client", "The attached client process"),
TraceLoggingKeyword(MICROSOFT_KEYWORD_MEASURES),
TelemetryPrivacyDataTag(PDT_ProductAndServiceUsage));
Expand Down Expand Up @@ -686,7 +668,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
TraceLoggingWrite(g_hTerminalConnectionProvider,
"ReceivedFirstByte",
TraceLoggingDescription("An event emitted when the connection receives the first byte"),
TraceLoggingGuid(_guid, "SessionGuid", "The WT_SESSION's GUID"),
TraceLoggingGuid(_sessionId, "SessionGuid", "The WT_SESSION's GUID"),
TraceLoggingFloat64(delta.count(), "Duration"),
TraceLoggingKeyword(MICROSOFT_KEYWORD_MEASURES),
TelemetryPrivacyDataTag(PDT_ProductAndServicePerformance));
Expand Down
6 changes: 2 additions & 4 deletions src/cascadia/TerminalConnection/ConptyConnection.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,14 @@
#pragma once

#include "ConptyConnection.g.h"
#include "ConnectionStateHolder.h"
#include "BaseTerminalConnection.h"

#include "ITerminalHandoff.h"
#include <til/env.h>

namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
{
struct ConptyConnection : ConptyConnectionT<ConptyConnection>, ConnectionStateHolder<ConptyConnection>
struct ConptyConnection : ConptyConnectionT<ConptyConnection>, BaseTerminalConnection<ConptyConnection>
{
ConptyConnection(const HANDLE hSig,
const HANDLE hIn,
Expand All @@ -36,7 +36,6 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation

void ReparentWindow(const uint64_t newParent);

winrt::guid Guid() const noexcept;
winrt::hstring Commandline() const;
winrt::hstring StartingTitle() const;
WORD ShowWindow() const noexcept;
Expand Down Expand Up @@ -77,7 +76,6 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
hstring _startingTitle{};
bool _initialVisibility{ true };
Windows::Foundation::Collections::ValueSet _environment{ nullptr };
guid _guid{}; // A unique session identifier for connected client
hstring _clientName{}; // The name of the process hosted by this ConPTY connection (as of launch).

bool _receivedFirstByte{ false };
Expand Down
1 change: 0 additions & 1 deletion src/cascadia/TerminalConnection/ConptyConnection.idl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ namespace Microsoft.Terminal.TerminalConnection
[default_interface] runtimeclass ConptyConnection : ITerminalConnection
{
ConptyConnection();
Guid Guid { get; };
String Commandline { get; };
String StartingTitle { get; };
UInt16 ShowWindow { get; };
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalConnection/EchoConnection.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation

void Initialize(const Windows::Foundation::Collections::ValueSet& /*settings*/) const noexcept {};

winrt::guid SessionId() const noexcept { return {}; }
ConnectionState State() const noexcept { return ConnectionState::Connected; }

WINRT_CALLBACK(TerminalOutput, TerminalOutputHandler);
Expand Down
Loading

0 comments on commit 5dda507

Please sign in to comment.