Make the window name `_quake` special by zadjii-msft · Pull Request #9785 · microsoft/terminal

zadjii-msft · 2021-04-12T14:03:22Z

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 Spec for Quake Mode #9274
See also [MEGATHREAD] Quake Mode & Global Summon #8888

PR Checklist

In the pursuit of Feature request: hot key drop down ala quake/guake/tilda #653
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 win+shift+arrow, the window doesn't horizontally re-size to the new monitor dimensions. It should.

src/cascadia/TerminalApp/TerminalPage.cpp

src/cascadia/WindowsTerminal/IslandWindow.cpp

carlos-zamora

A lot of nits, but a few questions here and there that I want answers on before signing off.

src/cascadia/TerminalApp/AppLogic.cpp

src/cascadia/TerminalApp/TerminalPage.cpp

carlos-zamora · 2021-04-12T18:11:19Z

src/cascadia/TerminalApp/AppLogic.cpp

-            if (launchMode == LaunchMode::FullscreenMode)
+            if (IsQuakeWindow())
+            {
+                _root->ToggleFocusMode();


Why ToggleFocusMode and not something like EnableFocusMode or SetFocusMode(true)? QM will always be in focus mode right?

Frankly, I hadn't written _SetFocusMode yet when I added this line 😋

Ah, but that's a private member on TerminalPage... meh, I just didn't feel like adding another method that'll only have one usage right here

src/cascadia/WindowsTerminal/IslandWindow.cpp

carlos-zamora · 2021-04-12T18:32:46Z

src/cascadia/TerminalApp/TerminalPage.cpp

+                // If we're entering QM from ~FM, then this will enter FM
+                // If we're entering QM from FM, then this will do nothing
+                // If we're leaving QM (we're already in FM), then this will do nothing
+                _SetFocusMode(_isInFocusMode);


A continuation of my confusion from above, why not do ToggleFocusMode?

meh, this one gets to be more explicit in what it's trying to do because it's already inside the TerminalPage. Technically, yes, ToggleFocusMode would work, by setting it to false, but then staying in focus mode anyways, but ¯\_(ツ)_/¯

src/cascadia/WindowsTerminal/AppHost.cpp

carlos-zamora

Looks good to me! Thanks!

miniksa · 2021-04-15T17:22:41Z

src/cascadia/WindowsTerminal/AppHost.cpp

        origin = til::point{
-            nearestMonitorInfo.rcWork.left - (nonClientSize.width() / 2),
-            nearestMonitorInfo.rcWork.top
+            ::base::saturated_cast<ptrdiff_t>(nearestMonitorInfo.rcWork.left - (nonClientSize.width() / 2)),


I feel like this is weird. We're doing the math unprotected from over/underflow first then casting the result into ptrdiff_t.

Shouldn't we be doing a saturated operation on these things or casting them into the math wrappers first and then let it do the saturated subtract?

The same goes for some of the other saturated casts below. Though I'm not totally clear on what the source types are here that requires us to do the cast to begin with.

Yea you know, I felt weird writing that. I thinkthe types are LONGs originally. Maybe point is more forgiving than size, which is why I was getting weird "can't use an initializer list" errors.

lemme back that out and spend more than 30 seconds on it

…ial-_quake-window

…ub.com/microsoft/terminal into dev/migrie/f/653-special-_quake-window

miniksa

That's an excellent idea for fixing the math thing. Appreciated. Looks good to me in that respect and otherwise.

ghost · 2021-04-26T19:36:20Z

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (`@msftbot`) and give me an instruction to get started! Learn more here.

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

ghost · 2021-05-25T19:11:48Z

🎉Windows Terminal Preview v1.9.1445.0 has been released which incorporates this pull request.:tada:

Handy links:

ghost · 2021-07-14T17:04:08Z

🎉Windows Terminal v1.9.1942.0 has been released which incorporates this pull request.:tada:

Handy links:

zadjii-msft added 6 commits April 8, 2021 16:38

The _quake window starts in focus mode, on the top half the screen.

7560ee3

Prevent resizing the quake window

8d9c1f1

Prevent the window from being moved as well

2db7fd5

suppress the resize handle on the top

04a4249

Renaming a window to _quake will snap it into quake mode

87c6c2d

Get rid of the margin on either side

34f0dfd

This comment has been minimized.

Sign in to view

miniksa reviewed Apr 12, 2021

View reviewed changes

src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved

miniksa reviewed Apr 12, 2021

View reviewed changes

src/cascadia/WindowsTerminal/IslandWindow.cpp Show resolved Hide resolved

carlos-zamora requested changes Apr 12, 2021

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 12, 2021

zadjii-msft mentioned this pull request Apr 12, 2021

[MEGATHREAD] Quake Mode & Global Summon #8888

Open

61 tasks

zadjii-msft self-assigned this Apr 13, 2021

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 13, 2021

zadjii-msft added 2 commits April 15, 2021 07:18

most of the PR feedback

2a1b468

some of this logic was weird

1e43eef

zadjii-msft assigned carlos-zamora and unassigned zadjii-msft Apr 15, 2021

carlos-zamora approved these changes Apr 15, 2021

View reviewed changes

carlos-zamora removed their assignment Apr 15, 2021

miniksa reviewed Apr 15, 2021

View reviewed changes

zadjii-msft mentioned this pull request Apr 16, 2021

Add globalSummon action #9854

Merged

4 tasks

give til the long superpower

beb96e3

zadjii-msft mentioned this pull request Apr 19, 2021

A "_random_" color scheme option. Any new window chooses randomly from those in settings.schemes. #9845

Closed

zadjii-msft assigned miniksa Apr 19, 2021

zadjii-msft added 2 commits April 26, 2021 10:07

Merge remote-tracking branch 'origin/main' into dev/migrie/f/653-spec…

d3861c5

…ial-_quake-window

Merge branch 'dev/migrie/f/653-special-_quake-window' of https://gith…

02e4ef7

…ub.com/microsoft/terminal into dev/migrie/f/653-special-_quake-window

zadjii-msft mentioned this pull request Apr 26, 2021

Exempt the _quake window from glomming #9956

Merged

4 tasks

miniksa approved these changes Apr 26, 2021

View reviewed changes

miniksa removed their assignment Apr 26, 2021

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 26, 2021

ghost merged commit dc66313 into main Apr 26, 2021

ghost deleted the dev/migrie/f/653-special-_quake-window branch April 26, 2021 19:36

zadjii-msft mentioned this pull request Apr 29, 2021

Feature request: hot key drop down ala quake/guake/tilda #653

Closed

This pull request was closed.

Conversation

zadjii-msft commented Apr 12, 2021

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

TODO!

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

carlos-zamora Apr 12, 2021

Choose a reason for hiding this comment

Uh oh!

zadjii-msft Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

zadjii-msft Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

carlos-zamora Apr 12, 2021

Choose a reason for hiding this comment

Uh oh!

zadjii-msft Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

miniksa Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

zadjii-msft Apr 16, 2021

Choose a reason for hiding this comment

Uh oh!

miniksa left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Apr 26, 2021

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 (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

ghost commented May 25, 2021

Uh oh!

ghost commented Jul 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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 (`@msftbot`) and give me an instruction to get started! Learn more here.