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

Replace the HRGN-based titlebar cutout with an overlay window #5485

Merged
merged 23 commits into from
Apr 24, 2020

Conversation

DHowett-MSFT
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT commented Apr 23, 2020

Copying the description from the inimitable @greg904 in #4778.

Summary of the Pull Request

My understanding is that the XAML framework uses another way of getting mouse input that doesn't work with WM_SYSCOMMAND with SC_MOVE. It looks like it "steals" our mouse messages like WM_LBUTTONDOWN.

Before, we were cutting (with HRGNs) the drag bar part of the XAML islands window in order to catch mouse messages and be able to implement the drag bar that can move the window. However this "cut" doesn't only apply to input (mouse messages) but also to the graphics so we had to paint behind with the same color as the drag bar using GDI to hide the fact that we were cutting the window.

The main issue with this is that we have to replicate exactly the rendering on the XAML drag bar using GDI and this is bad because:

  1. it's hard to keep track of the right color: if a dialog is open, it will cover the whole window including the drag bar with a transparent white layer and it's hard to keep track of those things.
  2. we can't do acrylic with GDI

So I found another method, which is to instead put a "drag window" exactly where the drag bar is, but on top of the XAML islands window (in Z order). I've found that this lets us receive the WM_LBUTTONDOWN messages.

PR Checklist

Detailed Description of the Pull Request / Additional comments

Picture's worth a thousand words.

Available with this PR:
image

Available after version 1, if we so desire:

image

Validation Steps Performed

DH: Tested manually in all configurations (debug, release) with snap, drag, move, double-click and double-click on the resize handle. Tested at 200% scale.

  • Need to test DPI transitions still.

Greg Depoire--Ferrer and others added 21 commits March 1, 2020 22:13
* Don't recreate the window every time; it's possible to use
  SetWindowPos and show/hide it on-demand
* WIL-ize and document a couple things
* make the class registration a static inline
* make class reg/creation happen as part of MakeWindow
* remove the entire tabview color key (let it fall back to the actual
  color specified in MUX)
@DHowett DHowett mentioned this pull request Apr 23, 2020
3 tasks
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

  • Does this work with enter/exiting fullscreen?
  • Enter/exiting fullscreen from maximized?
  • 1.1 I'm assuming?

I think I approve of this

Comment on lines +542 to +547
// Get the cursor position from the _last message_ and not from
// `GetCursorPos` (which returns the cursor position _at the
// moment_) because if we're lagging behind the cursor's position,
// we still want to get the cursor position that was associated
// with that message at the time it was sent to handle the message
// correctly.
Copy link
Member

Choose a reason for hiding this comment

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

this is mental

POINT screenPt = clientPt;
if (ClientToScreen(window, &screenPt))
{
const auto parentWindow{ GetAncestor(window, GA_PARENT) };
Copy link
Member

Choose a reason for hiding this comment

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

This is just a formality, right? Like, we know this is going to just return GetWindowsHandle() right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the trick here is this isn’t a member function and doesn’t have access to this

Copy link
Member

Choose a reason for hiding this comment

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

Oh derp, thanks for clearing that up ☺️

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth commenting to that effect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You know, given our plans for this it might be better to just make this a member and do the window data pointer thing anyway.

@DHowett-MSFT
Copy link
Contributor Author

I tested fullscreen, but not from maximized. I’ll add it to the list.

@DHowett-MSFT
Copy link
Contributor Author

@zadjii-msft tested, passed:

  • DPI switch
  • fullscreen from maximize
  • really leaning hard on the f11 button to try to break it

@DHowett-MSFT DHowett-MSFT marked this pull request as ready for review April 23, 2020 17:53
@DHowett-MSFT DHowett-MSFT added this to the Terminal v1.0 milestone Apr 23, 2020
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.

Looks alright to me. I think the fake-out HWND over the top and just gently nudging all the window messages into the other format isn't that bad. Just one little comment but signing off anyway.

src/cascadia/WindowsTerminal/NonClientIslandWindow.cpp Outdated Show resolved Hide resolved
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

Okay I just futzed with this enough to be happy with it. Still feels risky to be touching this code this close to release, but :shipit:

@DHowett-MSFT DHowett-MSFT changed the title Kill HRGN II: Kills Regions Dead Replace the HRGN-based titlebar cutout with an overlay window Apr 24, 2020
@DHowett-MSFT DHowett-MSFT merged commit 214163e into master Apr 24, 2020
@DHowett-MSFT DHowett-MSFT deleted the dev/duhowett/kill_hrgn_4778 branch April 24, 2020 22:22
@ghost
Copy link

ghost commented May 5, 2020

🎉Windows Terminal Release Candidate v0.11.1251.0 (1.0rc1) has been released which incorporates this pull request.:tada:

Handy links:

@yoshiask
Copy link

How does this work with Tablet Mode? Does it still use acrylic in the titlebar?

@DHowett
Copy link
Member

DHowett commented Aug 26, 2020

This changes nothing about tablet mode.

@fembiba
Copy link

fembiba commented Dec 1, 2020

Did these changes make it into the release?

Do they exist at the moment? I do not see acrylic in versions 1.4+

Best regards,
Roman S

@DHowett
Copy link
Member

DHowett commented Dec 1, 2020

@rovecode this change did not add acrylic to the title bar. This change removed a technical roadblock to having acrylic in the title bar.

@fembiba
Copy link

fembiba commented Dec 1, 2020

@DHowett. OK. But acrylic in the title bar is not planned?

@zadjii-msft
Copy link
Member

@rovecode We're planning for acrylic in the titlebar as a part of a broader "theming" feature set, see #3327 and #5772 for more details

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