Skip to content

Commit

Permalink
Fix defterm in wake of auto-elevation (#12272)
Browse files Browse the repository at this point in the history
There are a couple places where we now bail immediately on startup, if we think the window is going to get created without any tabs. We do that to prevent a blank window from flashing on the screen when launching auto-elevate profiles. Unfortunately, those broke defterm in a particularly hard to debug way. In the defterm invocation, there actually aren't any tabs when the app completes initialization. We use the initialization to actually accept the defterm handoff. So what would happen is that the window would immediately close itself gracefully, never accepting the handoff.

In my defense, #8514, the original auto-elevated PR, predates defterm merging (906edf7) by a few months, so I totally forgot to test this when rolling it into the subsequent iterations of that PR.

* Related to: 
  * #7489
  * #12137 
  * #12205 
* [x] Closes #12267 
* [x] I work here
* [ ] No tests on this code unfortunately
* [x] Tested manually

Includes a semi-related code fix to #10922 to make that quieter. That is perpetually noisy, and when trying to debug defterm, you've only got about 30s to do that before it bails, so the `sxe eh` breaks in there are quite annoying.
  • Loading branch information
zadjii-msft authored Jan 27, 2022
1 parent 4ccfe0b commit 48d79c7
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 17 deletions.
11 changes: 9 additions & 2 deletions src/cascadia/TerminalApp/TerminalPage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,10 @@ namespace winrt::TerminalApp::implementation
// - true if we're not elevated but all relevant pane-spawning actions are elevated
bool TerminalPage::ShouldImmediatelyHandoffToElevated(const CascadiaSettings& settings) const
{
if (!_startupActions || IsElevated())
// GH#12267: Don't forget about defterm handoff here. If we're being
// created for embedding, then _yea_, we don't need to handoff to an
// elevated window.
if (!_startupActions || IsElevated() || _shouldStartInboundListener)
{
// there arent startup actions, or we're elevated. In that case, go for it.
return false;
Expand Down Expand Up @@ -664,7 +667,11 @@ namespace winrt::TerminalApp::implementation
// However, we need to make sure to close this window in that scenario.
// Since there aren't any _tabs_ in this window, we won't ever get a
// closed event. So do it manually.
if (_tabs.Size() == 0)
//
// GH#12267: Make sure that we don't instantly close ourselves when
// we're readying to accept a defterm connection. In that case, we don't
// have a tab yet, but will once we're initialized.
if (_tabs.Size() == 0 && !(_shouldStartInboundListener || _isEmbeddingInboundListener))
{
_LastTabClosedHandlers(*this, nullptr);
}
Expand Down
32 changes: 17 additions & 15 deletions src/cascadia/WindowsTerminal/AppHost.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1443,24 +1443,26 @@ void AppHost::_WindowMoved()
_logic.DismissDialog();

const auto root{ _logic.GetRoot() };

try
if (root && root.XamlRoot())
{
// This is basically DismissAllPopups which is also in
// TerminalSettingsEditor/Utils.h
// There isn't a good place that's shared between these two files, but
// it's only 5 LOC so whatever.
const auto popups{ Media::VisualTreeHelper::GetOpenPopupsForXamlRoot(root.XamlRoot()) };
for (const auto& p : popups)
try
{
p.IsOpen(false);
// This is basically DismissAllPopups which is also in
// TerminalSettingsEditor/Utils.h
// There isn't a good place that's shared between these two files, but
// it's only 5 LOC so whatever.
const auto popups{ Media::VisualTreeHelper::GetOpenPopupsForXamlRoot(root.XamlRoot()) };
for (const auto& p : popups)
{
p.IsOpen(false);
}
}
catch (...)
{
// We purposely don't log here, because this is exceptionally noisy,
// especially on startup, when we're moving the window into place
// but might not have a real xamlRoot yet.
}
}
catch (...)
{
// We purposely don't log here, because this is exceptionally noisy,
// especially on startup, when we're moving the window into place
// but might not have a real xamlRoot yet.
}
}
}

0 comments on commit 48d79c7

Please sign in to comment.