Skip to content
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

[MEGATHREAD] Quake Mode & Global Summon #8888

Open
29 of 61 tasks
DHowett opened this issue Jan 26, 2021 · 41 comments
Open
29 of 61 tasks

[MEGATHREAD] Quake Mode & Global Summon #8888

DHowett opened this issue Jan 26, 2021 · 41 comments
Labels
Area-Windowing Window frame, quake mode, tearout Issue-Scenario Product-Terminal The new Windows Terminal.
Milestone

Comments

@DHowett
Copy link
Member

DHowett commented Jan 26, 2021

[Original thread: #653] [Spec: #9274, doc] [Project]

See also Quake Mode FAQ

1.9 tasks

2.0 tasks

{ "name": "Promote to quake window", "command": { "action": "renameWindow", "name": "_quake" } }

Backlog

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Jan 26, 2021
@DHowett DHowett added ⛺ Reserved For future use and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jan 26, 2021
@zadjii-msft zadjii-msft changed the title <camper> [MEGATHREAD] Quake Mode & Global Summon Apr 8, 2021
@zadjii-msft zadjii-msft self-assigned this Apr 8, 2021
@zadjii-msft zadjii-msft added this to the Terminal v1.9 milestone Apr 8, 2021
@zadjii-msft zadjii-msft added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Scenario Product-Terminal The new Windows Terminal. and removed Needs-Tag-Fix Doesn't match tag requirements ⛺ Reserved For future use labels Apr 8, 2021
ghost pushed a commit that referenced this issue Apr 15, 2021
## Summary of the Pull Request

Does what it says on the can. People can now use `win` in a keybinding to
indicate that the chord needs <kbd>win</kbd>.

## References
* Done for #653
* See also #8888

## PR Checklist
* [x] Closes #3184
* [x] I work here
* [ ] Tests added/passed
* [ ] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments

For the record, I hate this. But it's great for quake mode, so _meh_. There's
shockingly more win keys claimed then you think - many more than the shortcut
guide even shows.

* `win+b`: Focus the tray?
* `win+t`: Focus the taskbar
* `win+p`: Project...
* `win+c`: The powertoys color picker
* `win+v`: cloud clipboard

So the list of valid combos is vanishingly small. It's all about that <kbd>win+~</kbd>

## Validation Steps Performed

Bound
```json
        { "keys": [ "win+`" ], "command": "commandPalette" },
```

and yea, it works as expected
mpela81 pushed a commit to mpela81/terminal that referenced this issue Apr 17, 2021
## Summary of the Pull Request

Does what it says on the can. People can now use `win` in a keybinding to
indicate that the chord needs <kbd>win</kbd>.

## References
* Done for microsoft#653
* See also microsoft#8888

## PR Checklist
* [x] Closes microsoft#3184
* [x] I work here
* [ ] Tests added/passed
* [ ] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments

For the record, I hate this. But it's great for quake mode, so _meh_. There's
shockingly more win keys claimed then you think - many more than the shortcut
guide even shows.

* `win+b`: Focus the tray?
* `win+t`: Focus the taskbar
* `win+p`: Project...
* `win+c`: The powertoys color picker
* `win+v`: cloud clipboard

So the list of valid combos is vanishingly small. It's all about that <kbd>win+~</kbd>

## Validation Steps Performed

Bound
```json
        { "keys": [ "win+`" ], "command": "commandPalette" },
```

and yea, it works as expected
ghost pushed a commit that referenced this issue Apr 26, 2021
## Summary of the Pull Request

This PR adds some special behavior to the window named "\_quake".
* When creating the quake window, it ignores "initialRows" and "initialCols" and opens on the top half of the monitor.
  - It uses `initialPosition` to determine which monitor this is
* It cannot be moved
* It can only be vertically resized on the bottom border.
* It's always in focus mode.
  - We should probably have an issue tracking "Allow showing tabs in focus mode"? Maybe?
  - This one element is maybe the one I'm least attached to

When renaming a window to "\_quake", it adopts all those behaviors as well. It does not exit focus mode when leaving QM, nor does it resize back. That seemed unnecessary. 

## References

* As spec'ed in #9274
* See also #8888

## PR Checklist
* [x] In the pursuit of #653 
* [x] I work here
* [ ] Tests added/passed
* [ ] Requires documentation to be updated, but I'm not gonna do any of that till quake mode is totally done. 

## Detailed Description of the Pull Request / Additional comments

Note that this doesn't do things like:
* dropdown
* global hotkey summon 
* summon to the current monitor 
* summon to the current desktop

I'm doing #653 _very_ piecemeal, to try and make the PRs less egregious.

## Validation Steps Performed

* validated that center on launch still works
* validated that QM works on different monitors based on `initialPosition`
* validated entering/exiting QM behaves as expected

## TODO!
* [ ] When snapping the quake window between desktops with <kbd>win+shift+arrow</kbd>, the window doesn't horizontally re-size to the new monitor dimensions. It should.
ghost pushed a commit that referenced this issue Apr 28, 2021
## 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 **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.

## References
* Original thread: #653
* Spec: #9274 
* megathread: #8888

## PR Checklist
* [x] Checks a box in #8888
* [x] closes https://github.com/microsoft/terminal/projects/5#card-59030791
* [x] I work here
* [x] Tests added 
* [n/a] Requires documentation to be updated

## 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.
DHowett pushed a commit that referenced this issue Apr 28, 2021
Adds support for two new actions:
* `globalSummon`, which can be used to activate a window using a _global_ (READ: OS-level) hotkey.
  - accepts an optional `name` argument. When provided, this will attempt to summon with the given name. When omitted, we'll try to summon the most recent window.
* `quakeMode` which is `globalSummon` for the `_quake` window.

These actions are stored in the actions array, but are read by the `WindowsTerminal` level and bound to the OS in `IslandWindow`. The monarch registers for these keybindings with the OS. When one is pressed, the monarch will recieve a `WM_HOTKEY` message. It'll use that to look up the corresponding action args. It'll use those to try and summon the right window.

## References

* #8888: Quake mode megathread
* #9274: Spec (**guys seriously i just need one more ✔️**)
* #9785: The start of granting "\_quake" super powers

## PR Checklist
* [x] Closes #653 - I'm gonna say this closes it for now, though we have _many_ follow-ups in #8888
* [x] I work here
* [x] Tests added/passed

## Validation Steps Performed

* Validated that it works with `win` keys
* Validated that it works without `win` keys
* Validated that it hot-reloads
* Validated that it moves to the new monarch
* Validated that you can bind both `globalSummon` and `quakeMode` at the same time and do different things
* Validated that you can bind `globalSummon` with a name and it creates that name if it doesn't already exist
DHowett pushed a commit that referenced this issue Apr 28, 2021
This adds support for the `desktop` param to the `globalSummon` action. It accepts 3 values:
* `toCurrent` (default): The window moves to the current desktop when it's summoned
* `any`: We don't care what desktop the window is on. We'll go to the desktop the window is on when we summon it.
* `onCurrent`: We'll only try to summon the MRU window on this desktop when summoning a window. 
  * When combined with `name`, if there's a window matching `name`, we'll move it to this desktop. 
  * If there's not a window on this desktop, and `name` is omitted, then we'll make a new window.

`quakeMode` was also updated to use `toCurrent` behavior by default.

## References
* Original thread: #653
* Spec: #9274 
* megathread: #8888

## PR Checklist
* [x] Checks some boxes in #8888
* [x] closes https://github.com/microsoft/terminal/projects/5#card-59030845
* [x] I work here
* [x] Tests added 
* [n/a] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments

S/O to https://github.com/microsoft/PowerToys, who graciously let us use `VirtualDesktopUtils` for figuring out what desktop is the current desktop. Yea, that's all we needed that entire file for. No, there isn't an API for this (_surprised-pikachu.png_)

## Validation Steps Performed

Played with this for a while, and it's amazing.
DHowett pushed a commit that referenced this issue Apr 28, 2021
This adds a `toggleVisibility` parameter to `globalSummon`. 
* When `true` (default): when you press the global summon keybinding, and the window is currently the foreground window, we'll minimize the window.
* When `false`, we'll just do nothing.

## References
* Original thread: #653
* Spec: #9274 
* megathread: #8888

## PR Checklist
* [x] Checks a box in #8888
* [x] closes https://github.com/microsoft/terminal/projects/5#card-59030814
* [x] I work here
* [ ] No tests for this one.
* [ ] yes yes eventually I'll come back on the docs

## Detailed Description of the Pull Request / Additional comments

I've got nothing extra to add here. This one's pretty simple. I'm only targeting #9954 since that one laid so much foundation to build on, with the `SummonBehavior`

## Validation Steps Performed

Played with this for a while, and it's amazing.
Rosefield pushed a commit to Rosefield/terminal that referenced this issue Jul 22, 2021
microsoft#10676)

## Summary of the Pull Request

We were making the quake window exactly the width of the monitor it was on, but that didn't account for the 1px of border on either side.		

## References
* megathread: microsoft#8888

## PR Checklist
* [x] Closes microsoft#10201
* [x] I work here
* [ ] Tests added/passed
* [n/a] Requires documentation to be updated

## Validation Steps Performed

It happened before, it doesn't anymore.
ghost pushed a commit that referenced this issue Aug 2, 2021
## Summary of the Pull Request

<kbd>win+shift+arrows</kbd> can be used to move windows to adjacent monitors. When that happens, we'll new re-calculate the size of the window for the new monitor.

## References
* megathread: #8888

## PR Checklist
* [x] Closes #10274
* [x] I work here
* [ ] Tests added/passed
* [n/a] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments

In `WM_WINDOWPOSCHANGING`, the OS says "hey, I'm about to do {something} to your window. You cool with that?". We handle that message by:
1. checking if the window was _moved_ as a part of this message
2. getting the monitor that the window will be moved onto
3. If that monitor is different than the monitor the window is currently on, then
  * calculate how big the quake window should be on that monitor
  * tell the OS that's where we'd like to be.

## Validation Steps Performed

* <kbd>win+shift+arrows</kbd> works right now
* normal quake summoning still works right
@zadjii-msft zadjii-msft added Area-Windowing Window frame, quake mode, tearout and removed Area-User Interface Issues pertaining to the user interface of the Console or Terminal labels Aug 4, 2021
@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label Aug 4, 2021
ghost pushed a commit that referenced this issue Aug 11, 2021
## Summary of the Pull Request

This isn't a fix for #10875, but it is logging that help identify the root cause here. The logging may additionally be helpful for some of the other issues we're seeing elsewhere in the repo, namely #10340. 

@lhecker is actually working on the fix for #10875, so hopefully this test will help validate.

## References
* Regressed in #10666.
* logging for #8888

## PR Checklist
* [x] Closes nothing
* [x] I work here
* [x] Tests added, and they absolutely fail, but they're localtests, so ¯\\\_(ツ)_/¯
* [n/a] Requires documentation to be updated

## details

While I was here, I noticed that `KeyBindingsTests::KeyChords` has been broken for some time now. So I fixed that too.
@kulkalkul
Copy link

Couldn't find the issue about task manager focus, so posting it here. It also happens with IntelliJ IDEs.

DHowett pushed a commit that referenced this issue Aug 25, 2021
…tor (#10674)

## Summary of the Pull Request

When the quake window is moved to another monitor, re-evaluate it's size for that monitor.

## References
* megathread: #8888
* Similar, but not the same: #10274

## PR Checklist
* [x] Closes #10182
* [x] I work here
* [ ] Tests added/passed
* [n/a] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments

We'll probably need to do this in a few more places, but I'm breaking PRs into small chunks for easier reviews.

## Validation Steps Performed

Summoned the window to a bunch of different resolutions. Where it would use the wrong size before, it no longer does.
DHowett pushed a commit that referenced this issue Aug 25, 2021
#10676)

## Summary of the Pull Request

We were making the quake window exactly the width of the monitor it was on, but that didn't account for the 1px of border on either side.		

## References
* megathread: #8888

## PR Checklist
* [x] Closes #10201
* [x] I work here
* [ ] Tests added/passed
* [n/a] Requires documentation to be updated

## Validation Steps Performed

It happened before, it doesn't anymore.
DHowett pushed a commit that referenced this issue Aug 25, 2021
## Summary of the Pull Request

<kbd>win+shift+arrows</kbd> can be used to move windows to adjacent monitors. When that happens, we'll new re-calculate the size of the window for the new monitor.

## References
* megathread: #8888

## PR Checklist
* [x] Closes #10274
* [x] I work here
* [ ] Tests added/passed
* [n/a] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments

In `WM_WINDOWPOSCHANGING`, the OS says "hey, I'm about to do {something} to your window. You cool with that?". We handle that message by:
1. checking if the window was _moved_ as a part of this message
2. getting the monitor that the window will be moved onto
3. If that monitor is different than the monitor the window is currently on, then
  * calculate how big the quake window should be on that monitor
  * tell the OS that's where we'd like to be.

## Validation Steps Performed

* <kbd>win+shift+arrows</kbd> works right now
* normal quake summoning still works right
@zadjii-msft zadjii-msft added this to the Megathreads milestone Jan 4, 2022
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Feb 28, 2022
@Nacimota
Copy link
Contributor

KNOWN ISSUE: If you try to summon the window while task manager is focused, the window will not get activated. Presumably this can happen with other apps as well. UWP OneNote seems affected as well.

Apologies if this has been discussed elsewhere (I did do a quick search), but I imagine this is UIPI related? If so, my understanding is that you would normally solve that by requesting UIAccess=true, though I wouldn't be able to test that because it needs to be signed and so on to work. I'm not sure how applicable that approach is to this project, so maybe someone who knows more can chime in?

Curiously enough, when I have Task Manager (or any other high integrity/UWP app) in focus, I can summon the quake window if it is not currently shown, but once it is visible on the desktop I cannot dismiss/focus the quake window using the hotkey (if taskmgr/whatever is in focus). I'm not entirely sure why that is, but (assuming my experience isn't unique) I suspect what's actually happening when a user says they can't summon the window is that is was already present on the desktop, but perhaps out of focus and hidden by other windows (like a maximized browser or something) and it's not capturing the hotkey so it doesn't take focus again.

If so that raises a couple of questions in my mind:

  1. Should the quake window be always-on-top?
    I feel like it's kind of transient UI in much the same way that (for example) the start menu is, in the sense that it floats on top of everything else while you're using it and disappears when you're done. But if you don't manually dismiss it with the hotkey and get distracted with other windows, you might mistakenly think you've dismissed it when it's actually still there.
  2. Should the quake window automatically dismiss itself when it loses focus?
    Following on from above, I think this makes a fair bit of sense. I don't think the expectations for how a quake-style window would behave are exactly the same as a regular window (or at least, I don't think they are for me). This also has the added benefit of potentially solving the Task Manager problem, assuming the hotkey does consistently work to summon the window when it isn't currently shown.

The problem also appears to go away for programs like Task Manager if you run the terminal as admin, though that doesn't seem like much of a solution. This doesn't appear to work for UWP apps, though, presumably because of AppContainers and various other things that they do which might as well be magic as far as I know.

@zadjii-msft
Copy link
Member

this is UIPI related? If so, my understanding is that you would normally solve that by requesting UIAccess=true,

I honestly don't know. The way we make this work is admittedly pretty hacky:

const auto windowThreadProcessId = GetWindowThreadProcessId(oldForegroundWindow, nullptr);
const auto currentThreadId = GetCurrentThreadId();
LOG_IF_WIN32_BOOL_FALSE(AttachThreadInput(windowThreadProcessId, currentThreadId, true));
// Just in case, add the thread detach as a scope_exit, to make _sure_ we do it.
auto detachThread = wil::scope_exit([windowThreadProcessId, currentThreadId]() {
LOG_IF_WIN32_BOOL_FALSE(AttachThreadInput(windowThreadProcessId, currentThreadId, false));
});
LOG_IF_WIN32_BOOL_FALSE(BringWindowToTop(_window.get()));
ShowWindow(_window.get(), SW_SHOW);
// Activate the window too. This will force us to the virtual desktop this
// window is on, if it's on another virtual desktop.
LOG_LAST_ERROR_IF_NULL(SetActiveWindow(_window.get()));
// Throw us on the active monitor.
_moveToMonitor(oldForegroundWindow, toMonitor);

I never dug to far into it, but I just assumed that Task Manager was protected s.t. I couldn't AttachThreadInput to it.

Whatever the solution to this is, utimately, it should work for non-quake windows as well. Quake mode is the most visible implementation here, but whatever we come up with should work for any old window.

@Nacimota
Copy link
Contributor

I never dug to far into it, but I just assumed that Task Manager was protected s.t. I couldn't AttachThreadInput to it.

Yeah, I am fairly confident UIPI is the mechanism at play there. My understanding is that you're not allowed to AttachThreadInput (among several other things) to a process running at a higher integrity level than you, which in practice applies not just to Task Manager but presumably any app running as administrator. It's the same reason that PowerToys needs to run as admin for shortcuts to work when elevated windows (like taskmgr) are in focus. PowerToys doesn't seem to have problems with UWP apps though... but UWP is a bit of a mystery to me.

Anyway, requesting UIAccess is basically a way of bypassing UIPI without running as admin, but I don't see it used very often. I suppose because it involves a bunch of extra hurdles (process must be in a trusted location [system32/program files], authenticode signed, etc.) that are less convenient than requiring admin, especially for portable installations. I'm not sure that UIAccess is the right solution here either, I'm just saying I do suspect UIPI is the cause.

Whatever the solution to this is, utimately, it should work for non-quake windows as well. Quake mode is the most visible implementation here, but whatever we come up with should work for any old window.

Yep, that seems reasonable. Though it may not be the best solution for the problem at hand, I'd still propose always-on-top/auto-dismiss behaviours for quake mode have merit regardless, but that is a separate discussion I guess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Windowing Window frame, quake mode, tearout Issue-Scenario Product-Terminal The new Windows Terminal.
Projects
None yet
Development

No branches or pull requests