Merged
Conversation
(cherry picked from commit 46c6403)
(cherry picked from commit 5614fde95f927428a6acc38baeea64588953c2a5)
I think this is a fix for #12190 intentionally, and for #12169 by happenstance. I'm no longer creating the window when the actions at the commandline are just creating new tabs or panes that need to be elevated. I still have more to check here. For ex: ``` wtd nt -p admin ; sp -p admin ; sp -p foo ``` That... should create a new tab then split that with foo, right? Unsure if that'll work * [ ] fix that * [ ] I don't think I need HookupKeyBindings
ghost
added
Area-Remoting
This comment has been minimized.
This comment has been minimized.
Contributor
|
I've checked out and built this branch and can no longer reproduce the crash in #12169 🎉. |
Member
Author
|
@ianjoneill thanks for going the extra mile to confirm! (and also for selfhosting directly on |
DHowett
approved these changes
Jan 21, 2022
Member
DHowett
left a comment
DHowett
left a comment
There was a problem hiding this comment.
This seems like a very thorny area in which we're gonna continue seeing issues -- let's monitor.
ghost
requested review from
PankajBhojwani,
carlos-zamora,
leonMSFT,
lhecker and
miniksa
lhecker
approved these changes
Jan 24, 2022
| // - <none> | ||
| // Return Value: | ||
| // - <none> | ||
| bool TerminalPage::ShouldImmediatelyHandoffToElevated(CascadiaSettings& settings) const |
Member
There was a problem hiding this comment.
This doesn't seem to need a mutable reference to settings. Make it a const ref?
Same for the one below.
Comment on lines
+306
to
+309
| // - This is a bit of trickiness: If we're running unelevated, and the user | ||
| // passed in only --elevate actions, the we don't _actually_ want to | ||
| // restore the layouts here. We're not _actually_ about to create the | ||
| // window. We're simply going to toss the commandlines |
Member
There was a problem hiding this comment.
Just so I get it: This returns true if we're not elevated but all relevant pane-spawning actions are elevated. Is that correct? Would such a comment be helpful for others in the future?
zadjii-msft
added
the
AutoMerge
|
Hello @zadjii-msft! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
This was referenced Jan 27, 2022
ghost
pushed a commit
that referenced
this pull request
Jan 27, 2022
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.
DHowett
pushed a commit
that referenced
this pull request
Feb 18, 2022
(cherry picked from commit dd213a5)
|
🎉 Handy links: |
This was referenced Mar 25, 2022
This pull request was closed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a collection of fixes:
split-paneto a new tab, in the case that it's preceded with onlynew-tabactions that opened elevated windows.checklist