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

Make the window name _quake special #9785

Merged
11 commits merged into from
Apr 26, 2021
Merged

Conversation

zadjii-msft
Copy link
Member

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

PR Checklist

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.

@github-actions

This comment has been minimized.

Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/cascadia/TerminalApp/AppLogic.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
if (launchMode == LaunchMode::FullscreenMode)
if (IsQuakeWindow())
{
_root->ToggleFocusMode();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
src/cascadia/WindowsTerminal/IslandWindow.cpp Show resolved Hide resolved
// 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
src/cascadia/WindowsTerminal/AppHost.cpp Outdated Show resolved Hide resolved
src/cascadia/WindowsTerminal/AppHost.cpp Outdated Show resolved Hide resolved
src/cascadia/WindowsTerminal/AppHost.cpp Show resolved Hide resolved
@ghost 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 zadjii-msft self-assigned this Apr 13, 2021
@ghost 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
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thanks!

@carlos-zamora carlos-zamora removed their assignment Apr 15, 2021
@@ -416,18 +410,25 @@ void AppHost::_HandleCreateWindow(const HWND hwnd, RECT proposedRect, LaunchMode
const til::size availableSpace = desktopDimensions + nonClientSize;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@zadjii-msft zadjii-msft mentioned this pull request Apr 16, 2021
4 tasks
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@miniksa miniksa removed their assignment Apr 26, 2021
@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 26, 2021
@ghost
Copy link

ghost commented Apr 26, 2021

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.

@ghost ghost merged commit dc66313 into main Apr 26, 2021
@ghost ghost deleted the dev/migrie/f/653-special-_quake-window branch April 26, 2021 19:36
DHowett pushed a commit that referenced this pull request 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
@ghost
Copy link

ghost commented May 25, 2021

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

Handy links:

@ghost
Copy link

ghost commented Jul 14, 2021

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

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants