Introduce PresentationSource, move some responsibilities from TopLevel#20624
Introduce PresentationSource, move some responsibilities from TopLevel#20624
Conversation
# Conflicts: # tests/Avalonia.Controls.UnitTests/TabControlTests.cs
d1e9aa1 to
5286505
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 104 out of 104 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
src/Avalonia.Controls/TopLevel.cs:665
- In
HandleClosed,_sourceis disposed before clearing_source.RootVisual, but clearing the root visual currently updates the renderer/composition root and presentation-source attachment. Because the renderer/layout manager are already disposed, this can throw or leave the visual tree attached. Clear/detach the root visual (and unsubscribe renderer events) before disposing thePresentationSource, and avoid disposing the same layout manager twice (it’s already disposed by_source.Dispose()).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 109 out of 109 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/Avalonia.Controls/PresentationSource/PresentationSource.Cursor.cs
Outdated
Show resolved
Hide resolved
| // TODO: The check for bounds is no longer correct | ||
|
|
||
| var root = (InputElement?)element.VisualRoot; | ||
| if (root == null) | ||
| return true; | ||
|
|
||
| // Check if the element is within the visible area of the window | ||
| var visibleBounds = new Rect(0, 0, root.Bounds.Width, root.Bounds.Height); |
There was a problem hiding this comment.
The TODO comment mentions "The check for bounds is no longer correct" but the implementation still relies on this check. The code now uses element.VisualRoot which may no longer be the TopLevel (as per the PR's design changes), making the bounds check against root.Bounds potentially incorrect. This should either be fixed or documented more clearly with a follow-up issue reference.
There was a problem hiding this comment.
TODO's from this PR will be addressed in following PRs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 113 out of 113 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public void Dispose() | ||
| { | ||
| _layoutDiagnosticBridge?.Dispose(); | ||
| _layoutDiagnosticBridge = null; | ||
| LayoutManager.Dispose(); | ||
| Renderer.SceneInvalidated -= SceneInvalidated; | ||
| // We need to wait for the renderer to complete any in-flight operations | ||
| Renderer.Dispose(); | ||
|
|
||
| PlatformImpl = null; | ||
| _pointerOverPreProcessor?.OnCompleted(); | ||
| _pointerOverPreProcessorSubscription?.Dispose(); | ||
| if (((IInputRoot)this).PointerOverElement is AvaloniaObject pointerOverElement) | ||
| pointerOverElement.PropertyChanged -= PointerOverElement_PropertyChanged; | ||
| } |
There was a problem hiding this comment.
The PresentationSource.Dispose() method does not set RootVisual to null or call SetPresentationSourceForRootVisual(null) on the current RootVisual. This means that after disposal, the RootVisual may still have a reference to this disposed PresentationSource. This could lead to issues if code tries to access the PresentationSource after disposal. The Dispose method should detach the RootVisual before disposing.
There was a problem hiding this comment.
This makes sense, but might currently break TopLevel's assumption about event order, i. e. I think it expects to detach the logical tree before visual tree.
Will need to think more about this in further PRs
| else if (e.Root.RootElement == _tipControl?.VisualRoot) | ||
| { | ||
| _lastWindowEventTime = pointerEvent.Timestamp; | ||
| } |
There was a problem hiding this comment.
The comparison on line 46 checks if e.Root.RootElement equals _tipControl?.VisualRoot. However, RootElement is an InputElement (from IInputRoot.RootElement) while VisualRoot is a Visual? (from the VisualRoot property). These might not be reference-equal even when they represent the same logical root, since RootElement could be a different type. This comparison should likely use TopLevel.GetTopLevel or compare PresentationSource instances instead.
| @@ -49,20 +49,29 @@ protected override void OnAttachedToVisualTree(VisualTreeAttachmentEventArgs e) | |||
| { | |||
| base.OnAttachedToVisualTree(e); | |||
There was a problem hiding this comment.
This change set seems unrelated, but this code started to trigger compilation errors for me for some reason, so fixed along the way.
|
You can test this PR using the following package version. |
|
GH shows commits out of order, 34ac9c9 (the one with green tests) is actually the latest one. This is ready for review, I think |
|
I have some notes for later. Those aren't super required to be addressed with this PR since TopLevel still always sits at the root of the visual tree.
|
| var root = visual.VisualRoot ?? | ||
| throw new ArgumentException("Control does not belong to a visual tree.", nameof(visual)); | ||
| var source = visual.PresentationSource; | ||
| var root = source?.RootVisual ?? |
There was a problem hiding this comment.
Nit: the cast below can now be removed.
| { | ||
| Parent = parent ?? throw new ArgumentNullException(nameof(parent)); | ||
| Root = root ?? throw new ArgumentNullException(nameof(root)); | ||
| Debug.Assert(presentationSource.RootVisual != null); |
There was a problem hiding this comment.
I think the Debug.Assert isn't enough here: this constructor is public. We should check and throw if the presentation source does not have a root visual.
There was a problem hiding this comment.
Should we make the ctor private?
There was a problem hiding this comment.
I don't think so, let's keep the event args constructible. Checking the state should be enough.
| /// The root visual of the tree this visual is being attached to or detached from. | ||
| /// This is guaranteed to be non-null and will be the same as <see cref="IPresentationSource.RootVisual"/>. | ||
| /// </summary> | ||
| public Visual RootVisual => PresentationSource.RootVisual!; |
There was a problem hiding this comment.
Consider snapshotting the root visual in a field in the constructor. This would allow the event args to be stable even if RootVisual is accessed at a later point (where it could be null).
| RawDragEvent rawEvent = new RawDragEvent(_dragDrop, type, root, pt, _draggedData!, _allowedEffects, modifiers); | ||
| var tl = (root as Visual)?.GetSelfAndVisualAncestors().OfType<TopLevel>().FirstOrDefault(); | ||
| tl?.PlatformImpl?.Input?.Invoke(rawEvent); | ||
| var source = (PresentationSource?)root.RootElement.PresentationSource; |
There was a problem hiding this comment.
Avoid this cast if possible, as PresentationSource looks safe for the call below.
There was a problem hiding this comment.
It's casting from IPresentationSource to PresentationSource, what is wrong with that?
There was a problem hiding this comment.
Not assuming the impl, and simply being safe when everything in this chain is?
| void TakeFocus(); | ||
|
|
||
| [Obsolete("For unit tests only")] | ||
| internal IKeyboardNavigationHandler Tests_KeyboardNavigationHandler { get; } |
There was a problem hiding this comment.
Remove this to keep the interface clean. There's only one use of it in PopupTests where we control the IPopupHost implementation: we can cast to the correct type there.
| protected virtual Size Measure(Size constraint) | ||
| { | ||
| var l = (Layoutable) InputRoot!; | ||
| var l = (Layoutable)InputRoot!.RootElement!; |
There was a problem hiding this comment.
The Layoutable cast isn't necessary anymore and RootElement is never null.
There was a problem hiding this comment.
Note that RootElement will be nullable in subsequent PRs.
| var bounds = Bounds; | ||
| // Native window is not rendered by Avalonia | ||
| var transformToVisual = this.TransformToVisual(_currentRoot); | ||
| var transformToVisual = _currentRoot.RootVisual != null ? this.TransformToVisual(_currentRoot.RootVisual) : null; |
There was a problem hiding this comment.
Note: PresentationSource.RootVisual is not nullable, but its implementation accepts null. I read the comment there about keeping it not nullable for now. However, this becomes confusing here: should we expose an additional nullable property for now?
|
|
||
| [MemberNotNull(nameof(_groupManager))] | ||
| private void EnsureRadioGroupManager(IRenderRoot? root = null) | ||
| private void EnsureRadioGroupManager(IPresentationSource? source = null) |
There was a problem hiding this comment.
The parameter should probably be a root visual. Managing the group should have nothing to do with the presentation source.
There was a problem hiding this comment.
It was previously managed per-toplevel, so there is no behavior change here.
There was a problem hiding this comment.
It's not about the behavioral change, but rather the usage: since the group is scoped to a visual, the presentation source feels like the wrong layer here.
There was a problem hiding this comment.
Previously RadioGroupManager was only associated with attached trees, I think it's better to keep it as is
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
See #20622
This PR mostly moves stuff around, no observable behavioral changes yet, TopLevel is currently still always the root of the tree.
Based on another PR, so the diff is bigg