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

Enable dragging with the entire titlebar #1948

Merged
merged 38 commits into from
Jul 18, 2019

Conversation

zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Jul 12, 2019

Summary of the Pull Request

If a picture is worth 1000 words, then this is 15000 words/second like 18000 words (getting a gif below 10MB was harder than I thought)

dragging-window-004

References

This is a bunch of the work in #1625 and #1375.

PR Checklist

Detailed Description of the Pull Request / Additional comments

This is done with a major change to the NonClientIslandWindow, and the MinMaxControl. I've introduced a new XAML control, the TitlebarControl. The TitlebarControl consists of three parts - Content, Drag Area, and MinMaxControl. The MinMaxControl is now owned by the TitlebarControl, and it does pretty much nothing on it's own. All the hwnd handling has moved up to the TitlebarControl. The MinMaxControl now exists simply as a control with caption buttons.

Instead of the App owning the MinMaxControl, and the titlebar existing as part of the TerminalPage, the TitlebarControl is now owned by the NonClientIslandWindow. The NonClientIslandWindow will initialize it's own UI consisting of a titlebar row and a content row. It'll place a TitlebarControl instance in its titlebar row. It'll also ask the App for content to place in both the client area and the titlebar. When the app is resized, or the titlebar content is resized, the NonClientIslandWindow will update its drag area. The NonClientIslandWindow can also communicate directly with the TitlebarControl, without needing to go through the App to do so. In the future, This should make future windowing-related bugs easier to fix.

TODOs

  • There's definitely a bug with the bottom margin. A single pixel of your accent color will appear at the bottom of the window. With different MARGINs passed to DwmExtendframIntoClientArea, the amount changes. This regressed between 1905852 and 96c5da2. I want to track this down before we check this in.
    • I was able to fix this. We need to not paint the entirety of the root window. As long as it's not all painted, then the border won't be drawn. Click "details" to see my ramblings as I debugged this.
  • Definitely also regressed after 60a444c 7b8cf10 b9cc819 0219781 02e8389 (wait there's only f4e02d8 between those two??)
  • Oh no, it's broken in master. Must've regressed in f4e02d8, thank god I have that branch locally still.
  • It's fine in 9040940, which was the last commit in that PR before merging into master. I'll bisect from there to f4e02d8, we'll see where this regressed
    • 122f0de: ✔ (Just the TerminalPage changes)
    • 122f0de + 5fd4191: ✔ (TerminalPage + Most of the NC painting but not the whole titlebar thing)
    • Okay, so 9040940, where I paint the entire titlebar, is what broke it. That's definitely the commit that breaks it.
    • 9040940 + 122f0de: ❌ (Fix the NC painting, paint the entire titlebar + TerminalPage)
    • 9040940 + 60a444c: ❌ Well this was surprising
  • This is actually related to how GDI blends with the nonclient area. @DHowett-MSFT was seeing this previously when trying to paint our caption buttons himself. You won't see this bug in light mode, but in dark mode, where the color is #000000, the window frame bleeds through entirely. For example, I have a bright cyan accent color. If I change the backgroundColor to #ff00000, then the single pixel at the bottom will appear almost white:
    image

  Don't do anything in NCPAINT. If you do, you have to do everything. But the
  whole point of DwmExtendFrameIntoClientArea is to let you paint the NC area in
  your normal paint. So just do that dummy.

  * This doesn't transition across monitors.
  * This has a window style change I think is wrong.
  * I'm not sure the margins change is important.
  I'm not entirely sure why. But if we only update the titlebar drag region when
  that actually changes, it's a _lot_ smoother. I'm not super happy with the
  duplicated work in _UpdateDragRegion and OnSize, but checking this in in case
  I can't figure that out.
…tton

* Make the new tab button transparent, to see how that looks
* Make sure the TabView doesn't push the MMC off the window
  * The TitlebarControl is owned by the NCIW. It consists of a Content, DragBar,
    and MMCControl.
  * The App instatntiates a TabRowControl at runtime, and either places it in
    the UI (for tabs below titlebar) or hangs on to it, and gives it to the NCIW
    when the NCIW creates its UI.
  * When the NCIW is created, it creates a grid with two rows, one for the
    titlebar and one for the app content.
  * The MMCControl is only responsible for Min Max Close now, and is closer to
    the window implementation.
  * The drag bar takes up all the space from the right of the TabRow to the left
    of the MMC
  * Things that **DON'T** work:
    - When you add tabs, the drag bar doesn't update it's size. It only updates
      OnSize
    - The MMCControl's Min and Max buttons don't seem to work anymore.
      - They should probably just expose their OnMinimizeClick and
        OnMaximizeClick events for the Titlebar to handle minimizing and
        maximizing.
    - The drag bar is Magenta (#ff00ff) currently.
    - I'm not _sure_ we need a TabRowControl. We could probably get away with
      removing it from the UI tree, I was just being dumb before.
  I forgot to plumb the window handle through
…ie/f/newtab-btn-float-left

# Conflicts:
#	src/cascadia/WindowsTerminal/NonClientIslandWindow.cpp
…-btn-float-left

# Conflicts:
#	src/cascadia/WindowsTerminal/NonClientIslandWindow.cpp
@zadjii-msft zadjii-msft added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Product-Terminal The new Windows Terminal. labels Jul 12, 2019
@zadjii-msft zadjii-msft changed the title Enable fragging with the entire titlebar Enable dragging with the entire titlebar Jul 12, 2019
src/cascadia/TerminalApp/App.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/App.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/App.h Outdated Show resolved Hide resolved
@@ -53,10 +54,11 @@ namespace winrt::TerminalApp::implementation
// (which is a root when the tabs are in the titlebar.)
Windows::UI::Xaml::Controls::Control _root{ nullptr };
Microsoft::UI::Xaml::Controls::TabView _tabView{ nullptr };
Windows::UI::Xaml::Controls::Grid _tabRow{ nullptr };

TerminalApp::TabRowControl _tabRow{ nullptr };
Copy link
Contributor

Choose a reason for hiding this comment

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

this was not necessary any longer, yes?

Copy link
Member Author

Choose a reason for hiding this comment

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

technically yes, the TabRowControl isn't totally necessary, but I was inclined to keep it, to keep each piece more atomic. If you feel strongly, I can change it back

src/cascadia/TerminalApp/MinMaxCloseControl.cpp Outdated Show resolved Hide resolved

Windows::UI::Xaml::UIElement TitlebarControl::Content()
{
return ContentRoot().Children().Size() > 0 ? ContentRoot().Children().GetAt(0) : nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is technically something you should use a control template and a template binding for.

Copy link
Contributor

Choose a reason for hiding this comment

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

so that people can do <TitleBar>Whatever</TitleBar> and it will put Whatever in the Content

Copy link
Member Author

Choose a reason for hiding this comment

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

That's certainly the direction I was going with this, but I wanted to try and split that into a different task - this one's already getting hefty. I can file a follow-up issue, yea?

src/cascadia/TerminalApp/TitlebarControl.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 Jul 12, 2019
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 15, 2019
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 16, 2019
zadjii-msft and others added 3 commits July 16, 2019 11:15
…-btn-float-left

# Conflicts:
#	src/cascadia/TerminalApp/TerminalApp.vcxproj
Co-Authored-By: Michael Niksa <miniksa@microsoft.com>
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

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

I like it. A lot.

<ColumnDefinition Width="Auto"/>
</Grid.ColumnDefinitions>

<Grid x:Name="ContentRoot" Grid.Column="0" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Try out a ContentPresenter. Its sole job is to present content; perhaps it'll help you not have to interact with Children 😄

// calling. We know that our content is a Grid, so we don't need to worry
// about this.
const auto fwe = content.try_as<winrt::Windows::UI::Xaml::FrameworkElement>();
if (fwe)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (fwe)
if (const auto fwe = ...)

@mdtauk
Copy link

mdtauk commented Jul 17, 2019

Some thoughts about the Min/Max/Close buttons, and supporting FullScreen or Tablet Mode

Some properties to consider for this TitlebarControl. This could be also be useful for the Settings UI, if it is presented in a window.

  • CanMaximise
  • CanMinimise
  • CanRestore

Which would hide those buttons if you wanted to make a settings screen which only shows a close button. Also consider how these TitleBar Controls will handle FullScreen/Tablet Mode

image
Full Screen

Screenshot (17)
Tablet Mode

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.

:shipit:

@zadjii-msft
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@zadjii-msft
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@zadjii-msft zadjii-msft merged commit 8ffff8e into master Jul 18, 2019
mcpiroman pushed a commit to mcpiroman/terminal that referenced this pull request Jul 23, 2019
* This definitely works for getting shadow, pointy corners back

  Don't do anything in NCPAINT. If you do, you have to do everything. But the
  whole point of DwmExtendFrameIntoClientArea is to let you paint the NC area in
  your normal paint. So just do that dummy.

  * This doesn't transition across monitors.
  * This has a window style change I think is wrong.
  * I'm not sure the margins change is important.

* The window style was _not_ important

* Still getting a black xaml islands area (the HRGN) when we switch to high DPI

* I don't know if this affects anything.

* heyo this works.

  I'm not entirely sure why. But if we only update the titlebar drag region when
  that actually changes, it's a _lot_ smoother. I'm not super happy with the
  duplicated work in _UpdateDragRegion and OnSize, but checking this in in case
  I can't figure that out.

* Add more comments and cleanup

* Try making the button RightCustomContent

* * Make the MinMaxClose's drag bar's min size the same as a caption button
* Make the new tab button transparent, to see how that looks
* Make sure the TabView doesn't push the MMC off the window

* Create a TitlebarControl

  * The TitlebarControl is owned by the NCIW. It consists of a Content, DragBar,
    and MMCControl.
  * The App instatntiates a TabRowControl at runtime, and either places it in
    the UI (for tabs below titlebar) or hangs on to it, and gives it to the NCIW
    when the NCIW creates its UI.
  * When the NCIW is created, it creates a grid with two rows, one for the
    titlebar and one for the app content.
  * The MMCControl is only responsible for Min Max Close now, and is closer to
    the window implementation.
  * The drag bar takes up all the space from the right of the TabRow to the left
    of the MMC
  * Things that **DON'T** work:
    - When you add tabs, the drag bar doesn't update it's size. It only updates
      OnSize
    - The MMCControl's Min and Max buttons don't seem to work anymore.
      - They should probably just expose their OnMinimizeClick and
        OnMaximizeClick events for the Titlebar to handle minimizing and
        maximizing.
    - The drag bar is Magenta (#ff00ff) currently.
    - I'm not _sure_ we need a TabRowControl. We could probably get away with
      removing it from the UI tree, I was just being dumb before.

* Fix the MMC buttons not working

  I forgot to plumb the window handle through

* Make the titlebar less magenta

* Resize the drag region as we add/remove tabs

* Move the actual MMC handling to the TitlebarControl

* Some PR nits, fix the titlebar painting on maximize

* Put the TabRow in our XAML

* Remove dead code in preparation for review

* Horrifyingly try Gdi Plus as a solution, that is _wrong_ though

* Revert "Horrifyingly try Gdi Plus as a solution, that is _wrong_ though"

This reverts commit e038b5d.

* This fixes the bottom border but breaks the titlebar painting

* Fix the NC bottom border

* A bunch of the more minor PR nits

* Add a MinimizeClick event to the MMCControl

  This works for Minimize. This is what I wanted to do originally.

* Add events for _all_ of the buttons, not just the Minimize btn

* Change hoe setting the titlebar content works

  Now the app triggers a callcack on the host to set the content, instead of the host querying the app.

* Move the tab row to the bottom of it's available space

* Fix the theme reloading

* PR nits from @miniksa

* Update src/cascadia/WindowsTerminal/NonClientIslandWindow.cpp

Co-Authored-By: Michael Niksa <miniksa@microsoft.com>

* This needed to be fixed, was missed in other PR nits

* runformat

  wait _what_

* Does this fix the CI build?
@DHowett-MSFT DHowett-MSFT deleted the dev/migrie/f/newtab-btn-float-left branch July 23, 2019 23:25
@ghost
Copy link

ghost commented Aug 3, 2019

🎉Windows Terminal Preview v0.3.2142.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-User Interface Issues pertaining to the user interface of the Console or Terminal Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use tab-bar/title bar for dragging window around
5 participants