-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Implement Mac TitleBar #25670
Implement Mac TitleBar #25670
Conversation
This one just needs the screenshots updated in the UITests when they are available! |
Testing this out: 2024-11-05.16.58.25.mov2024-11-05.16.44.54.movThe top Titlebar area is subject to hit detection from the mouse and can be grabbed. The clicks do go through for things like Buttons, but you can also drag the window while doing so. Likewise, I would expect the entire titlebar area (The gray background) to be draggable, but only the top area where the window controls are is. I'm unsure if it's designed to be this way or if I'm missing an API for the tracking areas for where the hit detection is. |
ee31ad2
to
d6cf6cc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some alignment issues, but this is way lot better than noting I love this
Based on the TitleBar content behavior, is draggable, I think we need some comments and include in the docs that:
|
@@ -266,6 +266,18 @@ private void TitleBar_PropertyChanged(object? sender, System.ComponentModel.Prop | |||
Window.Activated += Window_Activated; | |||
Window.Deactivated += Window_Deactivated; | |||
} | |||
else if (e.PropertyName == nameof(LeadingContent)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these all be moved into the OnPropertyChanged delegates for these BPs?
For example, OnLeadingChanged
// However we need this when the titlebar is added or removed or the titlebar may not be fully laid out. | ||
if (!isInitalizing) | ||
{ | ||
View.LayoutIfNeeded(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SetNeedsLayout doesn't work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it seems that it misses the layout update when using SetNeedsLayout and when a new TitleBar is created after one already existed, the layout is not updated in time until there is another layout pass.
Using SetNeedsLayout:
UsingSetNeedsLayout2.mov
Using LayoutIfNeeded:
WithLayoutIfNeeded.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be correct? It looks like the first button is cut off by a few pixels at the top.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is because the titleBar is now not there at this point so the page's content is all the way at the top of the window and the UITests cut off the top pixels to get rid of the titlebar
0fb8ee8
to
c11a659
Compare
src/Core/src/Core/IWindow.cs
Outdated
@@ -134,5 +134,9 @@ public interface IWindow : ITitledElement | |||
ITitleBar? TitleBar => null; | |||
Rect[]? TitleBarDragRectangles => null; | |||
#endif | |||
|
|||
#if MACCATALYST |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can simplify this. ITitlebar is on Windows and Catalyst. Just need:
ITitleBar? TitleBar => null;
#if WINDOWS
Rect[]? TitleBarDragRectangles => null;
#endif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So something like this?
#if WINDOWS || MACCATALYST
ITitleBar? TitleBar => null;
#endif
#if WINDOWS
Rect[]? TitleBarDragRectangles => null;
#endif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 8 out of 18 changed files in this pull request and generated 1 suggestion.
Files not reviewed (10)
- src/Controls/samples/Controls.Sample/Pages/Controls/TitleBarPage.xaml: Language not supported
- src/Controls/tests/TestCases.HostApp/Issues/Issue24489.xaml: Language not supported
- src/Core/src/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt: Language not supported
- src/Core/src/Handlers/Window/WindowHandler.cs: Evaluated as low risk
- src/Controls/samples/Controls.Sample/ViewModels/PlatformSpecificsViewModel.cs: Evaluated as low risk
- src/Controls/src/Core/TitleBar/TitleBar.cs: Evaluated as low risk
- src/Core/src/Core/IWindow.cs: Evaluated as low risk
- src/Core/src/Handlers/Window/WindowHandler.iOS.cs: Evaluated as low risk
- src/Controls/src/Core/Layout/LayoutExtensions.cs: Evaluated as low risk
- src/Controls/samples/Controls.Sample/Pages/Controls/TitleBarPage.xaml.cs: Evaluated as low risk
3c61b1a
to
de0119a
Compare
227ed81
to
90a7372
Compare
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
Description of Change
This PR implements the easily customizable TitleBar control to MacCatalyst similar to how it was implemented to Windows here.
As @drasticactions mentioned, it is worth noting here that in this PR, dragging every part of titlebar does not cause the window to move, just the top part of the titlebar where it would have been draggable before. It is also worth noting that the top part of the titlebar is draggable inside of buttons as well.
A sample is to test is available in the Control.Sample project under "Platform Specifics -> TitleBar"
Screen.Recording.2024-11-07.at.9.31.15.AM.mov
Issues Fixed
Fixes #24489