Skip to content

Conversation

@zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Nov 3, 2021

Adds snap layout support to the Terminal's maximize button. This PR is
full of BODGY, so brace yourselves.

Big thanks to Chris Swan in #11134 for building the prototype.
I don't believe this solves #8795, because XAML islands can't get
nchittest messages

  • The window procedure for the drag bar forwards clicks on its client
    area to its parent as non-client clicks.
  • BODGY: It also manually handles the caption buttons. They exist in
    the titlebar, and work reasonably well with just XAML, if the drag bar
    isn't covering them.
  • However, to get snap layout support, we need to actually return
    HTMAXBUTTON where the maximize button is. If the drag bar doesn't
    cover the caption buttons, then the core input site (which takes up
    the entirety of the XAML island) will steal the WM_NCHITTEST before
    we get a chance to handle it.
  • So, the drag bar covers the caption buttons, and manually handles
    hovering and pressing them when needed. This gives the impression that
    they're getting input as they normally would, even if they're not
    really getting input via XAML.
  • We also need to manually display the button tooltips now, because XAML
    doesn't know when they've been hovered for long enough. Hence, the
    _displayToolTip ThrottledFuncTrailing

Validation

Minimized, maximized, restored down, hovered the buttons slowly, moved
the mouse over them quickly, they feel the same as before. But now with
snap layouts appearing.

TODO!

Closes #9443
I thought this took care of #8587 as a bonus, because I was here, and the fix is now trivial, but looking at the latest commit that regressed.

Co-authored-by: Chris Swan [email protected]

@ghost ghost added Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Nov 3, 2021
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@miniksa
Copy link
Member

miniksa commented Nov 4, 2021

This PR is full of BODGY, so brace yourselves.

Oh man am I excited.

@miniksa

This comment has been minimized.

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.

Does Alt+Space also still work for the system menu? This was only positioning for mouse events right? I don't think the mouse-variant of the system menu in the top left ever worked for us (though we could make it work maybe by returning HTSYSMENU, eh?

@DHowett
Copy link
Member

DHowett commented Nov 17, 2021

This is also going out in a selfhost build this evening. 😄

@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Nov 23, 2021
@ghost
Copy link

ghost commented Nov 23, 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.

@DHowett
Copy link
Member

DHowett commented Nov 23, 2021

Alright, I've only caught one issue with this and that's: when you click down on close, move to maximize, then release, it performs maximize instead of sinking the event.
If it's an easy fix, I'd like it, and if not... we can fix it later?

@DHowett
Copy link
Member

DHowett commented Nov 23, 2021

before you merge this, drive those todos to 0 and delete them? 😄

<data name="WindowCloseButton.[using:Windows.UI.Xaml.Automation]AutomationProperties.Name" xml:space="preserve">
<value>Close</value>
</data>
<data name="WindowCloseButton.[using:Windows.UI.Xaml.Controls]ToolTipService.ToolTip" xml:space="preserve">
Copy link
Member

Choose a reason for hiding this comment

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

too bad we can't reuse the existing tooltip machinery

@DHowett DHowett removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Nov 23, 2021
@miniksa
Copy link
Member

miniksa commented Nov 24, 2021

before you merge this, drive those todos to 0 and delete them? 😄

This is why I didn't hit approve here... waiting for Mike's input

EDIT: wait nvm Dustin killed automerge... clicking....

@DHowett
Copy link
Member

DHowett commented Nov 24, 2021

Moved from body:

gh-9447-prototype-000

@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Nov 29, 2021
@ghost ghost merged commit f2ebb21 into main Nov 29, 2021
@ghost ghost deleted the dev/migrie/nc-titlebar-chswan branch November 29, 2021 21:10
DHowett pushed a commit that referenced this pull request Dec 13, 2021
Adds snap layout support to the Terminal's maximize button. This PR is
full of BODGY, so brace yourselves.

Big thanks to Chris Swan in #11134 for building the prototype.
I don't believe this solves #8795, because XAML islands can't get
nchittest messages

- The window procedure for the drag bar forwards clicks on its client
  area to its parent as non-client clicks.
- BODGY: It also _manually_ handles the caption buttons. They exist in
  the titlebar, and work reasonably well with just XAML, if the drag bar
  isn't covering them.
- However, to get snap layout support, we need to actually return
  `HTMAXBUTTON` where the maximize button is. If the drag bar doesn't
  cover the caption buttons, then the core input site (which takes up
  the entirety of the XAML island) will steal the `WM_NCHITTEST` before
  we get a chance to handle it.
- So, the drag bar covers the caption buttons, and manually handles
  hovering and pressing them when needed. This gives the impression that
  they're getting input as they normally would, even if they're not
  _really_ getting input via XAML.
- We also need to manually display the button tooltips now, because XAML
  doesn't know when they've been hovered for long enough. Hence, the
  `_displayToolTip` `ThrottledFuncTrailing`

## Validation
Minimized, maximized, restored down, hovered the buttons slowly, moved
the mouse over them quickly, they feel the same as before. But now with
snap layouts appearing.

## TODO!
* [x] I'm working on getting the ToolTips on the caption buttons back. Alas, I needed a demo of this _today_, so I'll fix that tomorrow morning.
* [x] mild concern: I should probably test Win 10 to make sure there wasn't weird changes to the message loop in win11 that means this is broken on win10.
* [x] I think I used the wrong issue number for tons of my comments throughout this PR. Double check that. Should be #9443, not #9447.

Closes #9443
I thought this took care of #8587 ~as a bonus, because I was here, and the fix is _now_ trivial~, but looking at the latest commit that regressed.

Co-authored-by: Chris Swan <[email protected]>
(cherry picked from commit f2ebb21)
@ghost
Copy link

ghost commented Dec 14, 2021

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

Handy links:

@augustresende
Copy link

If anyone is interested, snap layouts does not work in flutter desktop apps if title bar is customized/disabled:

flutter/flutter#156791

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-UserInterface Issues pertaining to the user interface of the Console or Terminal AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Issue-Task It's a feature request, but it doesn't really need a major design. Priority-3 A description (P3) Product-Terminal The new Windows Terminal.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Snap Layouts Support: WM_NCHITTEST should return HTMAXBUTTON, HTMINBUTTON, ... for caption buttons

5 participants