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.

Sign up for GitHub

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

Jump to bottom

[Mac Catalyst] Set window dimensions #24001

Merged
merged 14 commits into from
Dec 19, 2024
Merged

[Mac Catalyst] Set window dimensions #24001

merged 14 commits into from
Dec 19, 2024
+112 −11

Conversation

MartyIX
Copy link
Contributor

@MartyIX MartyIX commented Aug 4, 2024 •
edited
Loading

Description of Change

This PR demonstrates that Mac Catalyst applications can change window dimensions.

Demo

Screen.Recording.2024-08-14.at.15.00.53.mov

Issues Fixed

Fixes #9704

Resources

@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Aug 4, 2024
@MartyIX MartyIX changed the title [MacCatyst] Set window dimensions [MacCatalyst] Set window dimensions Aug 4, 2024
@MartyIX MartyIX changed the title [MacCatalyst] Set window dimensions [Mac Catalyst] Set window dimensions Aug 4, 2024
@MartyIX MartyIX force-pushed the feature/2024-08-03-maccatyst-window-dimensions branch 2 times, most recently from 3862e00 to 72401f9 Compare August 14, 2024 12:58
@MartyIX MartyIX marked this pull request as ready for review August 14, 2024 13:08
@MartyIX MartyIX requested a review from a team as a code owner August 14, 2024 13:08
@MartyIX MartyIX requested review from jsuarezruiz, tj-devel709 and drasticactions and removed request for tj-devel709 August 14, 2024 13:08
mattleibow
mattleibow reviewed Aug 14, 2024
View reviewed changes
src/Core/src/Platform/iOS/WindowExtensions.cs Outdated Show resolved Hide resolved
src/Core/src/Handlers/Window/WindowHandler.iOS.cs Outdated
protected override void ConnectHandler(UIWindow platformView)
{
base.ConnectHandler(platformView);

UpdateVirtualViewFrame(platformView);

_effectiveGeometryObserver = platformView.WindowScene?.AddObserver("effectiveGeometry", NSKeyValueObservingOptions.OldNew, HandleEffectiveGeometryObserved);
Copy link
Member

Choose a reason for hiding this comment

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

Referencing the handler directly from the native view - the observer now holds a reference to the handler - will most certainly cause a leak.

Please use our amazing and totally not verbose way to do events: https://github.com/dotnet/maui/blob/main/src/Core/src/Handlers/Switch/SwitchHandler.iOS.cs#L57-L79

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I attempted to update it.

@mattleibow
Copy link
Member

mattleibow commented Aug 14, 2024 •
edited
Loading

I think all this looks good, just had a nit about collapsing and reducing the if block complexity. Then you are going to have to use the proxy event system as anything referencing maui from the native world will leak the entire universe as they are all linked.

Although now with @PureWeen magic in net9 this may not be the case. But, for net8 we are going to have to proxy all the things.

@MartyIX MartyIX force-pushed the feature/2024-08-03-maccatyst-window-dimensions branch from 251d4fa to cb0eae4 Compare August 14, 2024 19:48
@MartyIX
Copy link
Contributor Author

MartyIX commented Aug 14, 2024 •
edited
Loading

I think all this looks good, just had a nit about collapsing and reducing the if block complexity. Then you are going to have to use the proxy event system as anything referencing maui from the native world will leak the entire universe as they are all linked.

I noticed an issue when initial window dimension was not correctly handled (window was placed always to X=0 and Y=0). I'm not sure how else to fix it than to do: 3e17515 (where I basically wait until the new API sends an event with proper window dimensions, at first, some values are NaNs.)

@MartyIX
Copy link
Contributor Author

MartyIX commented Aug 14, 2024

tl;dr: The controls sample app I use for testing appears to work correctly with the latest commit (3e17515).

But I guess there are more scenarios which should be tested, hopefully tests cover some of those scenarios.

@mattleibow
Copy link
Member

mattleibow commented Aug 15, 2024 •
edited by jfversluis
Loading

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@MartyIX

This comment was marked as outdated.

Sign in to view
@MartyIX
Copy link
Contributor Author

MartyIX commented Aug 20, 2024

@mattleibow Could you take a look please?

@kubaflo
Copy link
Contributor

kubaflo commented Oct 16, 2024

@MartyIX to run the failing test you need to set this in your vs code
Screenshot 2024-10-16 at 20 10 06

after a successful build, you should see the following screen:

Screenshot 2024-10-16 at 20 11 24

then to run this particular test do as I did in this video:

Screen.Recording.2024-10-16.at.20.12.08.mov

@MartyIX
Copy link
Contributor Author

MartyIX commented Oct 16, 2024

@MartyIX to run the failing test you need to set this in your vs code

Yeah, I figured it out in the meanwhile. Sorry for not updating the post. However, I'm very much unsure how to fix the test.

@MartyIX

This comment was marked as outdated.

Sign in to view
@MartyIX MartyIX force-pushed the feature/2024-08-03-maccatyst-window-dimensions branch 2 times, most recently from 3e49f32 to 89976d9 Compare October 18, 2024 06:53
@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@github-actions github-actions bot force-pushed the feature/2024-08-03-maccatyst-window-dimensions branch from 89976d9 to 9449842 Compare October 19, 2024 11:01
@MartyIX MartyIX force-pushed the feature/2024-08-03-maccatyst-window-dimensions branch from 9449842 to e3bc904 Compare October 19, 2024 11:01
@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen PureWeen added the p/1 Work that is important, and has been scheduled for release in this or an upcoming sprint label Nov 27, 2024
@MartyIX
Copy link
Contributor Author

MartyIX commented Dec 4, 2024

@mattleibow Could you take a look please?

@axolmain
Copy link

axolmain commented Dec 5, 2024

Would this also fix the window positioning error?

@MartyIX
Copy link
Contributor Author

MartyIX commented Dec 5, 2024

Would this also fix the window positioning error?

Could you explain more in detail what you mean?

@axolmain
Copy link

axolmain commented Dec 6, 2024

Would this also fix the window positioning error?

Could you explain more in detail what you mean?

If I understand right from the video (I haven't cloned and ran this), your changes would fix the error of being unable to change window dimensions programmatically on mac. If that's right, does your change also address moving the location of the app's window itself? (App.X / App.Y)

@PureWeen PureWeen modified the milestones: .NET 9 SR2, .NET 9 SR3 Dec 6, 2024
@MartyIX
Copy link
Contributor Author

MartyIX commented Dec 7, 2024

Would this also fix the window positioning error?

Could you explain more in detail what you mean?

If I understand right from the video (I haven't cloned and ran this), your changes would fix the error of being unable to change window dimensions programmatically on mac. If that's right, does your change also address moving the location of the app's window itself? (App.X / App.Y)

image

"Center window" works for me, so it appears it works.

@rmarinho
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@MartyIX
Copy link
Contributor Author

MartyIX commented Dec 17, 2024

It appears that

dbug: 2024-12-17 02:30:28.774949-0800 Microsoft.Maui.Controls.DeviceTests[21489:51508457]    at Microsoft.Maui.DeviceTests.ShellTests.PageLayoutDoesNotExceedWindowBounds() in /_/src/Controls/tests/DeviceTests/Elements/Shell/ShellTests.cs:line 73
dbug: --- End of stack trace from previous location ---
dbug: 2024-12-17 02:30:28.775023-0800 Microsoft.Maui.Controls.DeviceTests[21489:51508457]    Execution time: 0.1023795
dbug: 2024-12-17 02:30:28.775085-0800 Microsoft.Maui.Controls.DeviceTests[21489:51508457]    Test trait name: Category
dbug: 2024-12-17 02:30:28.775152-0800 Microsoft.Maui.Controls.DeviceTests[21489:51508457]       value: Shell
dbug: 2024-12-17 02:30:28.775204-0800 Microsoft.Maui.Controls.DeviceTests[21489:51508457]       value: Shell
dbug: 2024-12-17 02:30:28.775260-0800 Microsoft.Maui.Controls.DeviceTests[21489:51508457]       value: Shell
dbug: 2024-12-17 02:30:28.775348-0800 Microsoft.Maui.Controls.DeviceTests[21489:51508457]       value: Shell
dbug: 2024-12-17 02:30:28.775406-0800 Microsoft.Maui.Controls.DeviceTests[21489:51508457]       value: Shell
dbug: 2024-12-17 02:30:28.775471-0800 Microsoft.Maui.Controls.DeviceTests[21489:51508457] 	[FAIL] PageLayoutDoesNotExceedWindowBounds
dbug: 2024-12-17 02:30:28.775538-0800 Microsoft.Maui.Controls.DeviceTests[21489:51508457] 	[FAIL] PageLayoutDoesNotExceedWindowBounds   Test name: PageLayoutDoesNotExceedWindowBounds

fails on iOS.

@MartyIX MartyIX force-pushed the feature/2024-08-03-maccatyst-window-dimensions branch from d98c564 to 1650bee Compare December 17, 2024 21:12
@PureWeen
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@mattleibow
Copy link
Member

PR looks ok, not sure about the observers @PureWeen? Are we allowed to do that?

My review is a bit nitty, but I think adding the logging will help future us and customers when things break.

@MartyIX
Copy link
Contributor Author

MartyIX commented Dec 18, 2024

@mattleibow I have addressed your feedback. Hopefully, it's OK.

@rmarinho
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@MartyIX
Copy link
Contributor Author

MartyIX commented Dec 19, 2024

@mattleibow Tests seem to be green.

@mattleibow mattleibow merged commit b2a3cec into dotnet:main Dec 19, 2024
109 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattleibow mattleibow mattleibow approved these changes

@jsuarezruiz jsuarezruiz Awaiting requested review from jsuarezruiz jsuarezruiz is a code owner automatically assigned from dotnet/dotnet-maui-reviewers

@drasticactions drasticactions Awaiting requested review from drasticactions

@PureWeen PureWeen Awaiting requested review from PureWeen

Assignees

@mattleibow mattleibow

Labels
area-controls-window Window community ✨ Community Contribution p/1 Work that is important, and has been scheduled for release in this or an upcoming sprint platform/macOS 🍏 macOS / Mac Catalyst
Projects
MAUI SDK Ongoing
Status: Done
Milestone
.NET 9 SR3
Development

Successfully merging this pull request may close these issues.

Mac Catalyst does not support resizing or repositioning windows programmatically
7 participants
@MartyIX @mattleibow @kubaflo @PureWeen @axolmain @rmarinho @jfversluis