-
Notifications
You must be signed in to change notification settings - Fork 8.5k
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
Use feature markers during terminal-by-default handoff #13160
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1082,6 +1082,7 @@ ICore | |
IData | ||
IDCANCEL | ||
IDD | ||
IDefault | ||
IDesktop | ||
IDevice | ||
IDictionary | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ | |
|
||
#include <shellapi.h> | ||
#include <shlwapi.h> | ||
#include <til/latch.h> | ||
|
||
using namespace winrt::Microsoft::Terminal; | ||
using namespace winrt::Microsoft::Terminal::Settings; | ||
|
@@ -1120,12 +1121,27 @@ void CascadiaSettings::CurrentDefaultTerminal(const Model::DefaultTerminal& term | |
// but in the future it might be worthwhile to change the code to use list indices instead. | ||
void CascadiaSettings::_refreshDefaultTerminals() | ||
{ | ||
if (!_defaultTerminals) | ||
if (_defaultTerminals) | ||
{ | ||
auto [defaultTerminals, defaultTerminal] = DefaultTerminal::Available(); | ||
_defaultTerminals = winrt::single_threaded_observable_vector(std::move(defaultTerminals)); | ||
_currentDefaultTerminal = std::move(defaultTerminal); | ||
return; | ||
} | ||
|
||
// This is an extract of extractValueFromTaskWithoutMainThreadAwait | ||
// as DefaultTerminal::Available creates the exact same issue. | ||
std::pair<std::vector<Model::DefaultTerminal>, Model::DefaultTerminal> result{ {}, nullptr }; | ||
til::latch latch{ 1 }; | ||
|
||
std::ignore = [&]() -> winrt::fire_and_forget { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I low key really love this "go do something on a backgrouund thread" lambda, without needing to make a whole function declaration for it. It's like a sneaky way of doing a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But we're not using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
const auto cleanup = wil::scope_exit([&]() { | ||
latch.count_down(); | ||
}); | ||
co_await winrt::resume_background(); | ||
result = DefaultTerminal::Available(); | ||
}(); | ||
|
||
latch.wait(); | ||
_defaultTerminals = winrt::single_threaded_observable_vector(std::move(result.first)); | ||
_currentDefaultTerminal = std::move(result.second); | ||
} | ||
|
||
void CascadiaSettings::ExportFile(winrt::hstring path, winrt::hstring content) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,9 +61,11 @@ static auto extractValueFromTaskWithoutMainThreadAwait(TTask&& task) -> decltype | |
til::latch latch{ 1 }; | ||
|
||
const auto _ = [&]() -> winrt::fire_and_forget { | ||
const auto cleanup = wil::scope_exit([&]() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure I understand why this moved. Just in case There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I realized this mistake when I wrote the code for |
||
latch.count_down(); | ||
}); | ||
co_await winrt::resume_background(); | ||
finalVal.emplace(co_await task); | ||
latch.count_down(); | ||
}(); | ||
|
||
latch.wait(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,7 +26,6 @@ | |
#include "../renderer/base/renderer.hpp" | ||
|
||
#include "../inc/conint.h" | ||
#include "../propslib/DelegationConfig.hpp" | ||
|
||
#include "tracing.hpp" | ||
|
||
|
@@ -64,28 +63,25 @@ try | |
|
||
// Check if this conhost is allowed to delegate its activities to another. | ||
// If so, look up the registered default console handler. | ||
auto isEnabled = false; | ||
if (SUCCEEDED(Microsoft::Console::Internal::DefaultApp::CheckDefaultAppPolicy(isEnabled)) && isEnabled) | ||
if (Globals.delegationPair.IsUndecided() && Microsoft::Console::Internal::DefaultApp::CheckDefaultAppPolicy()) | ||
{ | ||
IID delegationClsid; | ||
if (SUCCEEDED(DelegationConfig::s_GetDefaultConsoleId(delegationClsid))) | ||
{ | ||
Globals.handoffConsoleClsid = delegationClsid; | ||
TraceLoggingWrite(g_hConhostV2EventTraceProvider, | ||
"SrvInit_FoundDelegationConsole", | ||
TraceLoggingGuid(Globals.handoffConsoleClsid.value(), "ConsoleClsid"), | ||
TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), | ||
TraceLoggingKeyword(TIL_KEYWORD_TRACE)); | ||
} | ||
if (SUCCEEDED(DelegationConfig::s_GetDefaultTerminalId(delegationClsid))) | ||
{ | ||
Globals.handoffTerminalClsid = delegationClsid; | ||
TraceLoggingWrite(g_hConhostV2EventTraceProvider, | ||
"SrvInit_FoundDelegationTerminal", | ||
TraceLoggingGuid(Globals.handoffTerminalClsid.value(), "TerminalClsid"), | ||
TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), | ||
TraceLoggingKeyword(TIL_KEYWORD_TRACE)); | ||
} | ||
Globals.delegationPair = DelegationConfig::s_GetDelegationPair(); | ||
|
||
TraceLoggingWrite(g_hConhostV2EventTraceProvider, | ||
"SrvInit_FoundDelegationConsole", | ||
TraceLoggingGuid(Globals.delegationPair.console, "ConsoleClsid"), | ||
TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), | ||
TraceLoggingKeyword(TIL_KEYWORD_TRACE)); | ||
TraceLoggingWrite(g_hConhostV2EventTraceProvider, | ||
"SrvInit_FoundDelegationTerminal", | ||
TraceLoggingGuid(Globals.delegationPair.terminal, "TerminalClsid"), | ||
TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), | ||
TraceLoggingKeyword(TIL_KEYWORD_TRACE)); | ||
} | ||
if (Globals.delegationPair.IsDefault() && Microsoft::Console::Internal::DefaultApp::CheckShouldTerminalBeDefault()) | ||
lhecker marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
Globals.delegationPair = DelegationConfig::TerminalDelegationPair; | ||
Globals.defaultTerminalMarkerCheckRequired = true; | ||
} | ||
|
||
// Create the accessibility notifier early in the startup process. | ||
|
@@ -427,28 +423,15 @@ try | |
auto& g = ServiceLocator::LocateGlobals(); | ||
g.handoffTarget = true; | ||
|
||
IID delegationClsid; | ||
if (SUCCEEDED(DelegationConfig::s_GetDefaultConsoleId(delegationClsid))) | ||
{ | ||
g.handoffConsoleClsid = delegationClsid; | ||
} | ||
if (SUCCEEDED(DelegationConfig::s_GetDefaultTerminalId(delegationClsid))) | ||
{ | ||
g.handoffTerminalClsid = delegationClsid; | ||
} | ||
|
||
if (!g.handoffTerminalClsid) | ||
g.delegationPair = DelegationConfig::s_GetDelegationPair(); | ||
if (!g.delegationPair.IsCustom()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Notes: we've been handed off to (we're openconsole, not conhost). If we get here and there's not a custom defterm set, then it must be because conhost defaulted to us for DxD. Set up Terminal as the thing to handoff too. |
||
{ | ||
TraceLoggingWrite(g_hConhostV2EventTraceProvider, | ||
"SrvInit_ReceiveHandoff_NoTerminal", | ||
TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), | ||
TraceLoggingKeyword(TIL_KEYWORD_TRACE)); | ||
return E_NOT_SET; | ||
g.delegationPair = DelegationConfig::TerminalDelegationPair; | ||
} | ||
|
||
TraceLoggingWrite(g_hConhostV2EventTraceProvider, | ||
"SrvInit_ReceiveHandoff", | ||
TraceLoggingGuid(g.handoffTerminalClsid.value(), "TerminalClsid"), | ||
TraceLoggingGuid(g.delegationPair.terminal, "TerminalClsid"), | ||
TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), | ||
TraceLoggingKeyword(TIL_KEYWORD_TRACE)); | ||
|
||
|
@@ -509,14 +492,14 @@ try | |
|
||
TraceLoggingWrite(g_hConhostV2EventTraceProvider, | ||
"SrvInit_PrepareToCreateDelegationTerminal", | ||
TraceLoggingGuid(g.handoffTerminalClsid.value(), "TerminalClsid"), | ||
TraceLoggingGuid(g.delegationPair.terminal, "TerminalClsid"), | ||
TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), | ||
TraceLoggingKeyword(TIL_KEYWORD_TRACE)); | ||
|
||
RETURN_IF_FAILED(CoCreateInstance(g.handoffTerminalClsid.value(), nullptr, CLSCTX_LOCAL_SERVER, IID_PPV_ARGS(&handoff))); | ||
RETURN_IF_FAILED(CoCreateInstance(g.delegationPair.terminal, nullptr, CLSCTX_LOCAL_SERVER, IID_PPV_ARGS(&handoff))); | ||
TraceLoggingWrite(g_hConhostV2EventTraceProvider, | ||
"SrvInit_CreatedDelegationTerminal", | ||
TraceLoggingGuid(g.handoffTerminalClsid.value(), "TerminalClsid"), | ||
TraceLoggingGuid(g.delegationPair.terminal, "TerminalClsid"), | ||
TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), | ||
TraceLoggingKeyword(TIL_KEYWORD_TRACE)); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,7 +47,7 @@ namespace Microsoft::Console::Internal | |
|
||
namespace DefaultApp | ||
{ | ||
[[nodiscard]] HRESULT CheckDefaultAppPolicy(bool& isEnabled) noexcept; | ||
[[nodiscard]] HRESULT CheckShouldTerminalBeDefault(bool& isEnabled) noexcept; | ||
[[nodiscard]] bool CheckDefaultAppPolicy() noexcept; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IIRC if we change the signature of these methods, we'll have to update the internal-only versions as well, FYI. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep! I felt like this made writing the code more comfortable and noticed that the internal code never returns anything but |
||
[[nodiscard]] bool CheckShouldTerminalBeDefault() noexcept; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: is this redundant with the
Height="20"
on theName
TextBlock
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately we only get the layout in the screenshot above, by specifying the height explicitly twice. The reason is that the flyout (and only the flyout) seemingly doesn't respect the visibility state of the two
TextBlock
s. Due to that the dropdown box will have the correct height (1 line high) but the flyout list will be 2 lines high, even with bothTextBlock
s collapsed. I'm not entirely sure how such a bug can occur, but it does. 😄