Conversation
zadjii-msft
changed the base branch from
dev/migrie/f/653-special-_quake-window
to
main
| // that the window size is _first_ set up as something sensible, so | ||
| // leaving fullscreen returns to a reasonable size. | ||
| // | ||
| // We know at the start, that the root TerminalPage definitely isn't |
There was a problem hiding this comment.
But why do we know that? Is it because we reset the focus mode when we hide the quake window?
| // If we're the quake window, we must always be in focus mode. | ||
| // Prevent leaving focus mode here. |
There was a problem hiding this comment.
I know we're on a tight budget and I'd be happy to approve this PR regardless, but technically...
This doesn't strike me as the right kind of abstraction for me. It's like a... inverse leaky abstraction. Basically a leaky injection.
I'd suggest to keep TerminalPage unaware about what meaning a window name has, as such logic should be controlled centrally in a single place if possible. Instead a "DisableFocusModeChanges" property might be appropriate.
lhecker
left a comment
lhecker
left a comment
There was a problem hiding this comment.
I couldn't quite follow the AppHost::_HandleCreateWindow diff. I'll review at a later time again, but it looks good so far (just a few comments).
We don't want it acting as the "most recent window" for windowing behavior. The most recent window should always be some other window. This is being made as an atomic commit because we're probably 50% sure on this one. Maybe people do want new tabs to open up in the quake window! If they're running from the commandline, that's easy. If they're running from the shell context menu, that's **H**ard / impossible currently. $20 someone asks for that if we ship this. That of course might just fall into "explorer context menu settings" though.
zadjii-msft
force-pushed
the
dev/migrie/f/excempt-_quake-from-glomming
branch
from
35b964d to
c31d1b6
Compare
|
Okay, that's better now. |
DHowett
left a comment
DHowett
left a comment
There was a problem hiding this comment.
It's a hack, but I'm OK with it.
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 (
|
ghost
merged commit ## Summary of the Pull Request This fixes a bug where if you had the `_quake` window open, and you tried to `globalSummon` it (not with the `quakeMode` action, but just with a plain-old `globalSummon` to activate the MRU window), we'd _create a new window_ instead of just summoning the `_quake` window. ## References * regressed in #9956 ## PR Checklist * [x] Closes https://github.com/microsoft/terminal/projects/5#card-60325142 * [x] I work here * [x] Tests added/passed * [n/a] Requires documentation to be updated ## Detailed Description of the Pull Request / Additional comments It's basically a one-line fix, I just had to update the function signature for `_getMostRecentPeasantID` to allow us to use it differently for glomming vs summoning. When glomming, `ignoreQuakeWindow` should be true. When summoning, `ignoreQuakeWindow` should be false.
|
🎉 Handy links: |
|
🎉 Handy links: |
3 participants
Summary of the Pull Request
We don't want it acting as the "most recent window" for windowing behavior.
The most recent window should always be some other window.
This is being made as an atomic commit because we're probably 50% sure on this
one. Maybe people do want new tabs to open up in the quake window! If they're
running from the commandline, that's easy. If they're running from the shell
context menu, that's Hard / impossible currently. $20 someone asks for
that if we ship this. That of course might just fall into "explorer context
menu settings" though.
References
PR Checklist
Detailed Description of the Pull Request / Additional comments
I mean, this one's super straightforward, not sure what else there is to add.
Validation Steps Performed
Played with this, it works exactly as you'd think.