Fix bug #1016 - #1027
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for handling popup scenarios in Avalonia by implementing visual tree traversal through PopupRoot elements and adds a guard to prevent execution of actions on controls that are not yet loaded.
- Added an early return in
TriggerAction.Execute()when the control is not loaded - Modified visual parent traversal logic to handle PopupRoot elements by traversing through the Popup's PlacementTarget
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/Caliburn.Micro.Platform/Platforms/netcore-avalonia/TriggerAction.cs | Adds IsLoaded check to prevent execution of actions before control is loaded |
| src/Caliburn.Micro.Platform/ActionMessage.cs | Adds PopupRoot handling logic to traverse visual tree through popups and imports Avalonia.Controls.Primitives namespace |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| AssociatedObject = (Control)sender; | ||
| } | ||
|
|
||
| if (!AssociatedObject.IsLoaded) |
There was a problem hiding this comment.
Potential null reference exception if AssociatedObject is null. The check at line 57-60 only sets AssociatedObject when sender is a Control, but doesn't guarantee AssociatedObject is non-null. Add a null check before accessing AssociatedObject.IsLoaded.
| if (!AssociatedObject.IsLoaded) | |
| if (AssociatedObject == null || !AssociatedObject.IsLoaded) |
| //Modified parts Begin | ||
| var pElement = BindingScope.GetVisualParent(currentElement); | ||
| if (pElement == null | ||
| && currentElement.GetType().Name.Equals("PopupRoot", StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
Using string comparison with GetType().Name is fragile and should be avoided. Use 'is PopupRoot' type checking instead, which is safer and more maintainable. If PopupRoot might not be accessible at compile time, consider using 'as PopupRoot' pattern.
| && currentElement.GetType().Name.Equals("PopupRoot", StringComparison.OrdinalIgnoreCase)) | |
| && currentElement is PopupRoot) |
| //currentElement = BindingScope.GetVisualParent(currentElement); | ||
|
|
||
| //Modified parts Begin |
There was a problem hiding this comment.
Remove commented-out code and temporary development comments ('Modified parts Begin' and 'End'). These comments add no value and reduce code readability.
| && currentElement.GetType().Name.Equals("PopupRoot", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| if (currentElement is PopupRoot popupRoot && popupRoot.Parent is Popup popup) | ||
| { | ||
| pElement = popup.PlacementTarget; | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
| && currentElement.GetType().Name.Equals("PopupRoot", StringComparison.OrdinalIgnoreCase)) | |
| { | |
| if (currentElement is PopupRoot popupRoot && popupRoot.Parent is Popup popup) | |
| { | |
| pElement = popup.PlacementTarget; | |
| } | |
| && currentElement.GetType().Name.Equals("PopupRoot", StringComparison.OrdinalIgnoreCase) | |
| && currentElement is PopupRoot popupRoot | |
| && popupRoot.Parent is Popup popup) | |
| { | |
| pElement = popup.PlacementTarget; |
|
@869570967 thank you so much for this. Could you please look at this error when building the code in your branch? error CS0246: The type or namespace name 'PopupRoot' could not be found (are you missing a using directive or an assembly reference?) [D:\a\Caliburn.Micro\Caliburn.Micro\src\Caliburn.Micro.WinUI3\Caliburn.Micro.WinUI3.csproj] |
|
I am very sorry, I didn't consider the situation outside of Avalonia. PopupRoot is accessible in Avalonia, but it is not accessible in WPF, so I made some adjustments here. @vb2ae |
|
The code has been submitted, but I didn't see this morning's push entry. Is it the case that after the first push, subsequent pushes will be automatically sent to you for review and merging? @vb2ae |
|
@vb2ae Excluding UWP compilation, I have not developed for UWP and do not know if there are popups on UWP. If you want to fix such issues on UWP, you will need to handle it yourself. |
|
@869570967 thanks. UWP does have a up |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (pElement == null | ||
| && currentElement.GetType().Name.Equals("PopupRoot", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
|
|
||
| #if AVALONIA | ||
| if (currentElement is PopupRoot popupRoot && popupRoot.Parent is Popup popup) | ||
| { | ||
| pElement = popup.PlacementTarget; | ||
| } | ||
|
|
||
| #elif !WINDOWS_UWP | ||
| if (currentElement is FrameworkElement popupRoot && popupRoot.Parent is Popup popup) | ||
| { | ||
| pElement = popup.PlacementTarget; | ||
| } | ||
| #endif | ||
| } |
Check warning
Code scanning / CodeQL
Futile conditional Warning
| { | ||
|
|
||
| #if AVALONIA | ||
| if (currentElement is PopupRoot popupRoot && popupRoot.Parent is Popup popup) | ||
| { | ||
| pElement = popup.PlacementTarget; | ||
| } | ||
|
|
||
| #elif !WINDOWS_UWP | ||
| if (currentElement is FrameworkElement popupRoot && popupRoot.Parent is Popup popup) | ||
| { | ||
| pElement = popup.PlacementTarget; | ||
| } | ||
| #endif | ||
| } |
Check warning
Code scanning / CodeQL
Empty branch of conditional, or empty loop body Warning
| if (pElement == null | ||
| && currentElement.GetType().Name.Equals("PopupRoot", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
|
|
||
| #if AVALONIA | ||
| if (currentElement is PopupRoot popupRoot && popupRoot.Parent is Popup popup) | ||
| { | ||
| pElement = popup.PlacementTarget; | ||
| } | ||
|
|
||
| #elif !WINDOWS_UWP | ||
| if (currentElement is FrameworkElement popupRoot && popupRoot.Parent is Popup popup) | ||
| { | ||
| pElement = popup.PlacementTarget; | ||
| } | ||
| #endif | ||
| } |
Check notice
Code scanning / CodeQL
Nested 'if' statements can be combined Note
|
I merged this in. Want to add a page to the features sample with a popup before I push it to nuget. You can download use the MyGet version of the package in a little while. https://www.myget.org/feed/caliburn-micro-builds/package/nuget/Caliburn.Micro.avalonia |
|
@vb2ae Thank you |
No description provided.