Skip to content

Call RootViewChanged when setting ContentView to null#20052

Merged
PureWeen merged 5 commits intodotnet:mainfrom
drasticactions:dev/timill/adorner-layer-fix
Jun 1, 2024
Merged

Call RootViewChanged when setting ContentView to null#20052
PureWeen merged 5 commits intodotnet:mainfrom
drasticactions:dev/timill/adorner-layer-fix

Conversation

@drasticactions
Copy link
Contributor

image

Looking at the original issue as I tried to fix it, I think other code was refactored that caused the the RootManager.RootView to remain null in the new code I introduced. Stepping through it, I saw that OnWindowContentPlatformViewCreated invoked RootViewChanged, which the Visual Diagnostics Overlay uses to then create its overlay for. This is never called for Shell, which is what my original code tried to force, but since the RootManager.RootView was null, it still wasn't being called.

This fixes it by revering the changes for OnWindowContentPlatformViewCreated and instead calling RootViewChanged inside of SetContentView. For Shell, the ContentView is set to null, as its fragments are handled elsewhere. By calling RootViewChanged here, the VisualDiagnosticsOverlay can then access RootManager with RootView initialized.

Description of Change

Issues Fixed

Fixes #17677

@drasticactions drasticactions requested a review from a team as a code owner January 22, 2024 05:59
@samhouts samhouts added the area-controls-shell Shell Navigation, Routes, Tabs, Flyout label Jan 25, 2024
@jsuarezruiz jsuarezruiz requested a review from PureWeen January 26, 2024 12:11
@PureWeen
Copy link
Member

/rebase

@github-actions github-actions bot force-pushed the dev/timill/adorner-layer-fix branch from 304756f to ec99792 Compare February 21, 2024 19:29

_viewFragment = null;

RootViewChanged?.Invoke(this, EventArgs.Empty);
Copy link
Member

Choose a reason for hiding this comment

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

Did you validate this with page swapping variations?

If the user swaps the main page for a different shell page
If they swap it for non shell then swap back to a shell page?

The main thing I'm curious about is if the "remove" path is followed if we need to wait for that to finish before calling RootViewChanged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be fixed with 2f4cd0c, as I wrote below it's unrelated to what I was addressing with this PR (you would have hit this in current MAUI stable switching any Window main page.)

The issue is the way the layers work with Android and where the graphics layout is put. When we initialize the VisualDiagnosticsOverlay for Android, it goes on the top most layout of the view, which just so happens to be whatever the current Page content for a given Window is. That is unlike iOS/Mac/Windows where it's put above every MAUI view so it works regardless of switching out the page. With Android we need to recreate the overlay if you swap out the MainPage of a given window, since swapping it takes out the graphics layer.

Copy link
Member

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

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

If you swap out the MainPage LVT still breaks.

image

# Conflicts:
#	src/Core/src/Platform/Android/Navigation/NavigationRootManager.cs
@drasticactions
Copy link
Contributor Author

drasticactions commented Mar 19, 2024

I am not sure what this image is meant to represent. Having a series of steps you took would be nice rather than a png.

e. Okay, I think I understand.

If you take an existing Shell page, and swap it for something else, the WindowOverlay view does "break" and doesn't render new content... I tried it in the current MAUI 8.x stable and it does the same thing. It's also unrelated to Shell, you can swap two normal pages in a window and it will do the same thing. That's because WindowOverlay doesn't get deinitialized when the page is removed, meaning that when the new page is set, the WindowOverlay. IsPlatformViewInitialized is already set to true, so the new content isn't tracked.

It's unrelated to LVT and the Hot Reload stack, and it's a separate issue from what I'm fixing here.

e2. should be fixed with 2f4cd0c

@drasticactions drasticactions requested a review from PureWeen March 19, 2024 08:03
@drasticactions
Copy link
Contributor Author

@PureWeen Can you please look at this again? The root page issue should be fixed.

@PureWeen
Copy link
Member

/rebase

@PureWeen
Copy link
Member

PureWeen commented Jun 1, 2024

/rebase

@PureWeen PureWeen merged commit fcabff1 into dotnet:main Jun 1, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jul 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Android] VisualDiagnostics/IWindowOverlay with Shell still has issues loading

3 participants