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 property to control dropdown speed of global summon #9977

Merged
merged 112 commits into from May 17, 2021

Conversation

zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Apr 28, 2021

Summary of the Pull Request

Adds the dropdownDuration property to globalSummon. This controls how fast the window appears on the screen when summoned from minimized. It similarly controls the speed for sliding out of view when the window is dismissed with "toggleVisibility": true.

dropdownDuration specifies the duration in milliseconds. This defaults to 0 for globalSummon, and defaults to 200 for quakeMode. 200 was picked because, according to AnimateWindow:

Typically, an animation takes 200 milliseconds to play.

Do note that you won't be able to interact with the window during the animation! Input sent during the dropdown will arrive at the end of the animation, but input sent during the slide-up won't. Avoid setting this to large values!

The gifs are in Teams.

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

I had the following previously in the doc comments, but it feels better in the PR body:

  • This was chosen because it was easier to implement and generally nicer than:
    • AnimateWindow, which would show the window borders for the duration of
      the animation, and occasionally just plain not work. Additionally, for
      AnimateWindow to work, the window much not be visible, so we'd need to
      first restore the window, then hide it, then animate it. That would flash
      the taskbar.
    • SetWindowRgn on the root HWND, which caused the xaml content to shift to
      the left, and caused a black bar to be drawn on the right of the window.
      Presumably, SetWindowRgn and DwmExtendFrameIntoClientArea did not play
      well with each other.
    • SetWindowPos(..., SWP_NOSENDCHANGING), which worked the absolute best for
      longer animations, and is the closest to the actual implementation of
      AnimateWindow. This would resize the ROOT window, without sending resizes
      to the XAML island, allowing the content to not reflow. but for a
      duration of 200ms, would only ever display ~2 frames. That's basically
      not even animation anymore, it's now just an "appear". Since that's how
      long the default animation is, if felt silly to have it basically not
      work by default.
  • If a future reader would like to implement this better, they should feel
    free to
    , and not mistake my notes here as expertise. These are research
    notes into the dark and terrible land that is Win32 programming. I'm no expert.

Validation Steps Performed

This is the blob of json I'm testing with these days:

        { "keys": "ctrl+`", "command": { "action": "quakeMode" } },
        { "keys": "ctrl+1", "command": { "action": "globalSummon" } },
        // { "keys": "ctrl+2", "command": { "action": "globalSummon", "desktop": "toCurrent" } },
        // { "keys": "ctrl+2", "command": { "action": "globalSummon", "toggleVisibility": false } },
        { "keys": "ctrl+2", "command": { "action": "globalSummon", "dropdownDuration": 2000 } },
        { "keys": "ctrl+3", "command": { "action": "globalSummon", "desktop": "onCurrent" } },
        { "keys": "ctrl+4", "command": { "action": "globalSummon", "desktop": "any" } },
  • ctrl+` will summon the quake window with a quick animation
  • ctrl+2 will summon the window with a s l o w animation

…!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
…ndow should be. It just always says 0 for now, but in the future it could actually give us useful info.
@DHowett DHowett force-pushed the dev/migrie/f/quake-toggleVisibility branch 2 times, most recently from 04752e5 to 1032dbb Compare April 28, 2021 22:26
Base automatically changed from dev/migrie/f/quake-toggleVisibility to main April 28, 2021 23:57
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.

You know what, the two "issues" I had are minor and can just be fixed in post if we even want it. At this point, you've implemented everything according to the agreed upon spec, and any small design changes can be done before/during v1.9. Good work :)

@carlos-zamora
Copy link
Member

@zadjii-msft Is it time to update the docs/schema now? haha

@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label May 13, 2021
@zadjii-msft zadjii-msft added the Area-Remoting Communication layer between windows. Often for windowing behavior, quake mode, etc. label May 14, 2021
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

Signing off with changes requested because you have to do a conflict resolution anyway. The WINDOWPLACEMENT one can cause really weird behavior in the future depending on how the compiler is feeling.

@@ -1045,10 +1045,12 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
WINRT_PROPERTY(winrt::hstring, Name, L"");
WINRT_PROPERTY(Model::DesktopBehavior, Desktop, Model::DesktopBehavior::ToCurrent);
WINRT_PROPERTY(bool, ToggleVisibility, true);
WINRT_PROPERTY(uint32_t, DropdownDuration, 0);
Copy link
Member

Choose a reason for hiding this comment

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

it's always you three

lol

@@ -757,7 +758,7 @@ bool AppHost::_LazyLoadDesktopManager()
void AppHost::_HandleSummon(const winrt::Windows::Foundation::IInspectable& /*sender*/,
const Remoting::SummonWindowBehavior& args)
{
_window->SummonWindow(args.ToggleVisibility());
_window->SummonWindow(args.ToggleVisibility(), args.DropdownDuration());
Copy link
Member

Choose a reason for hiding this comment

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

Please do not make IslandWindow more specific to Terminal. I'd prefer it to be less specific to Terminal IMO 😄

src/cascadia/WindowsTerminal/IslandWindow.cpp Outdated Show resolved Hide resolved
src/cascadia/WindowsTerminal/IslandWindow.cpp Outdated Show resolved Hide resolved
@zadjii-msft zadjii-msft merged commit 6e11780 into main May 17, 2021
@zadjii-msft zadjii-msft deleted the dev/migrie/f/quake-dropdown-final branch May 17, 2021 12:28
ghost pushed a commit that referenced this pull request May 17, 2021
…10092)

#### ⚠️ this pr targets #9977

## Summary of the Pull Request

This adds support for part of the `monitor` property for `globalSummon`. It also goes a little off-spec:

```json
"monitor": "any"|"toCurrent"|"toMouse"
```

* `monitor`: This controls the monitor that the window will be summoned from/to
  - `"any"`: Summon the MRU window, regardless of which monitor it's currently on.
  - `"toCurrent"`/omitted: (_default_): Summon the MRU window **TO** the monitor with the current **foreground** window.
  - [**NEW**] `"toMouse"`: Summon the MRU window **TO** the monitor where the **mouse** cursor is.

When I was playing with this, It felt like `toMouse` was always what I wanted, not `toCurrent`. We can always just comment that out if we think that's contentious - I'm aware I didn't originally spec that.

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

## PR Checklist
* [x] Closes https://github.com/microsoft/terminal/projects/5#card-60325291
* [x] I work here
* [ ] Tests added/passed
* [ ] Requires documentation to be updated 😢 

## Detailed Description of the Pull Request / Additional comments

I made `toMouse` the default because it felt better. fite-me.jpg 

## Validation Steps Performed
my ever evolving blob:

```jsonc
    { "keys": "ctrl+`", "command": { "action": "quakeMode" } },
    { "keys": "ctrl+1", "command": { "action": "globalSummon" } },
    // { "keys": "ctrl+2", "command": { "action": "globalSummon", "desktop": "toCurrent" } },
    // { "keys": "ctrl+2", "command": { "action": "globalSummon", "toggleVisibility": false } },
    // { "keys": "ctrl+2", "command": { "action": "globalSummon", "dropdownDuration": 2000 } },
    { "keys": "ctrl+2", "command": { "action": "globalSummon", "monitor": "any" } },
    // { "keys": "ctrl+3", "command": { "action": "globalSummon", "desktop": "onCurrent" } },
    { "keys": "ctrl+3", "command": { "action": "globalSummon", "monitor": "toMouse" } },
    // { "keys": "ctrl+4", "command": { "action": "globalSummon", "desktop": "any" } },
    { "keys": "ctrl+4", "command": { "action": "globalSummon", "monitor": "toMouse", "dropdownDuration": 500 } },
    { "keys": "ctrl+5", "command": { "action": "globalSummon", "dropdownDuration": 500 } },
```
@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:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Remoting Communication layer between windows. Often for windowing behavior, quake mode, etc. Needs-Second It's a PR that needs another sign-off
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants