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

Add desktop param to globalSummon; set _quake = toCurrent #9954

Merged
merged 1 commit into from
Apr 28, 2021

Conversation

zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Apr 26, 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

PR Checklist

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.

@zadjii-msft

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.

I want to approve, but this comment is concerning. That's really the only concern I have. Please keep me in the loop and I'll be an approve for you.

src/cascadia/WindowsTerminal/IslandWindow.cpp Outdated Show resolved Hide resolved
src/cascadia/Remoting/SummonWindowBehavior.h Show resolved Hide resolved
src/cascadia/Remoting/SummonWindowBehavior.h Show resolved Hide resolved
src/cascadia/Remoting/Peasant.cpp Outdated Show resolved Hide resolved
@carlos-zamora
Copy link
Member

I guess don't forget the docs/schema too. But I think you wanted to go back and do that later? idk

@zadjii-msft
Copy link
Member Author

I guess don't forget the docs/schema too. But I think you wanted to go back and do that later? idk

yea I got a bunch more to do haha. Plus, I'm gonna have that big ol' table in the docs. Makes sense to do it atomically.

@github-actions

This comment has been minimized.

@zadjii-msft
Copy link
Member Author

🤮 now the commit history is extra crazy. sorry about that. Probably just best to review #9854, get that in, then let me rebase this one.

The commits that actually comprise this PR are:
305e627..045a8db, c95a429, 24bfbe6

@zadjii-msft
Copy link
Member Author

Okay ignore the above - I managed to fix that with some creative merges.

@zadjii-msft zadjii-msft mentioned this pull request Apr 28, 2021
4 tasks
{
_SummonRequestedHandlers(*this, nullptr);
auto localCopy = winrt::make_self<implementation::SummonWindowBehavior>(summonBehavior);
Copy link
Member

Choose a reason for hiding this comment

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

Why are we copying it? Is it because it's an OOP object, and we can get a local?

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 basically. Stash the local copy so that if during the SummonRequestedHandlers, the Monarch dies, then the Peasant can still complete the summon, without the handler knowing that the args are OOP.

src/cascadia/WindowsTerminal/AppHost.cpp Outdated Show resolved Hide resolved
src/cascadia/WindowsTerminal/AppHost.cpp Outdated Show resolved Hide resolved
src/cascadia/WindowsTerminal/IslandWindow.cpp Outdated Show resolved Hide resolved
@ghost ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Apr 28, 2021
Base automatically changed from dev/migrie/f/653-QUAKE-MODE to main April 28, 2021 22:13
(cherry picked from commit 9e94d348efa0783fa8381bc0297e0cec8153133a)
@DHowett DHowett force-pushed the dev/migrie/f/quake-toCurrent-desktop branch from 6c424f7 to fe51444 Compare April 28, 2021 22:19
@DHowett
Copy link
Member

DHowett commented Apr 28, 2021

I did a squash-merge-onto-base-branch cherry-pick-onto-main force-push manoeuvre.

@DHowett DHowett changed the title Add support for desktop param in globalSummon, Quake window moves to current desktop Add desktop param to globalSummon; set _quake = toCurrent Apr 28, 2021
@DHowett DHowett merged commit 65b22b9 into main Apr 28, 2021
@DHowett DHowett deleted the dev/migrie/f/quake-toCurrent-desktop branch April 28, 2021 22:25
DHowett pushed a commit that referenced this pull request 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.
ghost pushed a commit that referenced this pull request May 4, 2021
… already (#10025)

## Summary of the Pull Request

This is to mitigate MSFT:33035972. If you call `MoveWindowToDesktop` while an app is set to "Show windows from this app on all desktops", the OS will clear that "Show windows from this app on all desktops" state. But it _won't_ clear that state from the task view, so it'll just plain look broken.

We can mitigate this just by checking if we're already on the current desktop first. "Show windows from this app on all desktops" windows will _always_ be on every desktop, so that API will return true, and we can avoid tearing the state.

## References
* added in #9954 

## PR Checklist
* [x] Closes https://github.com/microsoft/terminal/projects/5#card-60325102
* [x] I work here
* [ ] Tests aren't possible
* [n/a] Requires documentation to be updated

## Validation Steps Performed
* it works again
@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:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants