RDEV-8025 - Adapt to different order Avalonia events are triggered #373
Conversation
| // need to subscribe the event this way because close gets called after all elements get detached | ||
| window.AddHandler(Window.WindowClosedEvent, (EventHandler<RoutedEventArgs>)OnHostWindowClosed); | ||
| if (e.Root is Window window && attachedWindow != window) { | ||
| attachedWindow?.RemoveHandler(Window.WindowClosedEvent, (EventHandler<RoutedEventArgs>)OnHostWindowClosed); |
There was a problem hiding this comment.
This new logic means that we are just removing the previous handler when we are attaching a new window to the tree?
There was a problem hiding this comment.
Yes. I don't fully like it because does not allow to reuse the Control unless we first attach it to a new window.
It may be an issue in this use case:
- Detach from logical tree
- Close current window
- Attach to new window (This is not possible)
We are forced to do it in this order:
- Detach from logical tree
- Attach to new window
- Close previous window
There was a problem hiding this comment.
I looked at the Avalonia source and when the detached is called it seems that we can look at some properties to infer that the window is about to close, namelly PlatformImpl and IsVisible
https://github.com/AvaloniaUI/Avalonia/blob/master/src/Avalonia.Controls/WindowBase.cs#L236
https://github.com/AvaloniaUI/Avalonia/blob/master/src/Avalonia.Controls/TopLevel.cs#L227
| using Avalonia.LogicalTree; | ||
|
|
||
| namespace WebViewControl { | ||
| namespace WebViewControl; |
There was a problem hiding this comment.
revert... we should do this for all files in a diff PR
There was a problem hiding this comment.
Fine by me. In the IDE projects we migrate them on the fly when we touch them
heldergoncalves92
force-pushed
the
RDEV-8025-reproduce-avalonia-leak
branch
from
16234e0 to
cd50dd5
Compare
An update: this is the PR which caused the change in behavior: AvaloniaUI/Avalonia#17059
Previously, events were fired in this order:
After #17059, events are fired in this order:
As you can see, previously the
WindowClosedattached event was raised before the window itself was notified that it's been closed. This itself doesn't make much sense, as logically the Closed event on the window should have finished before the attached WindowClosed event is raised, so from a logical point of view the new event ordering makes more sense. This does however cause the problem that you're seeing, where you were using that event to determine whether a control is being removed from the logical tree due to its window closing, or for some other reason.This is just an update: I'll let you know when I have a decision about whether we want to revert to the old (subtly incorrect) ordering, or if we have a better way of doing this.