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

Just trigger SizeChanged when Window size changes #22413

Merged
merged 13 commits into from
May 22, 2024

Conversation

pictos
Copy link
Contributor

@pictos pictos commented May 15, 2024

Description of Change

Issues Fixed

Fixes #17991
Fixes #21306

@pictos pictos requested a review from a team as a code owner May 15, 2024 01:12
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label May 15, 2024
@jsuarezruiz
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Co-authored-by: Shane Neuville <shane94@hotmail.com>
@jsuarezruiz
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@@ -199,7 +200,7 @@ public double MinimumHeight
}

int _batchFrameUpdate = 0;

bool disableHandlerUpdate;
Copy link
Member

Choose a reason for hiding this comment

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

I chatted with @mattleibow about this one a bit, and I think my suggestions are somewhat just digging us into a deeper hole here :-)

I think we've been trying to fix this issue the wrong way for some time now and need to revert this entire method back to what it originally looked like here

https://github.com/dotnet/maui/pull/4942/files#diff-07d5a2c0b5069d448623e88d890e41bc2c21e12f99bd01f1349903a332d4130cR202-R222

And then change this method

private protected override void UpdateHandlerValue(string property)
		{
			if (disableHandlerUpdate)
				return;

			base.UpdateHandlerValue(property);
		}

to

private protected override void UpdateHandlerValue(string property)
		{
			if (_batchFrameUpdate > 0)
				return;

			base.UpdateHandlerValue(property);
		}

@PureWeen
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen PureWeen self-requested a review May 17, 2024 20:51
@pictos
Copy link
Contributor Author

pictos commented May 19, 2024

Looks like the error are unrelated with my changes, from logs I could see something about Cake

@MartyIX
Copy link
Collaborator

MartyIX commented May 20, 2024

I wonder if this PR fixes #21306 as well. My PR #21357 touches src/Controls/src/Core/Window/Window.cs as this PR.

I don't have my macOS machine with me now to test it but to verify if the bug is fixed or not, one can:

  1. Run src/Controls/samples/Controls.Sample project
  2. In the app, navigate to Core -> Multi-Window and click Set window size to 700x500 and
  3. the application crashes.

@PureWeen PureWeen merged commit 7948366 into dotnet:main May 22, 2024
49 checks passed
@pictos pictos deleted the pj/window-size-fix branch May 22, 2024 04:16
@mattleibow
Copy link
Member

@MartyIX it did not it seems. I added a comment to the issue with a tiny bit more detail, but yeah, we need to fix this sooner rather than later.

@MartyIX
Copy link
Collaborator

MartyIX commented May 22, 2024

@MartyIX it did not it seems. I added a comment to the issue with a tiny bit more detail, but yeah, we need to fix this sooner rather than later.

Yes, I added some debug info in #21306 (comment). So I think it's clear what happens. Not so much clear how to fix it (at least to me).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-controls-window Window community ✨ Community Contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[macOS] Crash in multi-window mode in the Controls.sample project Window.SizeChanged fired moving MainWindow
5 participants