DYN-9823: This Visual is not connected to a PresentationSource.#16958
DYN-9823: This Visual is not connected to a PresentationSource.#16958chubakueno wants to merge 2 commits intoDynamoDS:masterfrom
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-9823
There was a problem hiding this comment.
Pull request overview
Fixes a WPF crash in NodeView where Node AutoComplete popup placement could call PointToScreen while the visual tree is transient/disconnected, triggering “This Visual is not connected to a PresentationSource.”
Changes:
- Replaced a direct coordinate conversion helper with
TryPointToLocal(...)to safely handle transient WPF visual-tree/disconnection conditions. - Updated Node AutoComplete popup placement logic to only set
Window.Left/Topwhen coordinate conversion succeeds. - Updated cluster autocomplete placement to early-return when placement coordinates can’t be computed.
You can also share your feedback on Copilot code review. Take the survey.
| try | ||
| { | ||
| Point positionFromScreen = target.PointToScreen(new Point(x, y)); | ||
| PresentationSource source = PresentationSource.FromVisual(target); | ||
| result = source.CompositionTarget.TransformFromDevice.Transform(positionFromScreen); | ||
| return true; | ||
| } | ||
| catch | ||
| { | ||
| // DYN-9823: PointToScreen can throw when the visual is not connected to a PresentationSource | ||
| // (e.g. during layout, tab switch, or node deletion). Popup placement is non-critical, | ||
| // so just return false for any exception and let the caller handle it. | ||
| return false; | ||
| } |
There was a problem hiding this comment.
TryPointToLocal currently uses a blanket catch and relies on exceptions (including NullReference from PresentationSource.FromVisual(target)/CompositionTarget) for normal control flow. This can mask unexpected failures and also adds overhead during rapid UI updates. Consider adding explicit target/source/source.CompositionTarget null checks and catching only the expected WPF exception(s) from PointToScreen (e.g., InvalidOperationException when the visual is not connected).
| try | |
| { | |
| Point positionFromScreen = target.PointToScreen(new Point(x, y)); | |
| PresentationSource source = PresentationSource.FromVisual(target); | |
| result = source.CompositionTarget.TransformFromDevice.Transform(positionFromScreen); | |
| return true; | |
| } | |
| catch | |
| { | |
| // DYN-9823: PointToScreen can throw when the visual is not connected to a PresentationSource | |
| // (e.g. during layout, tab switch, or node deletion). Popup placement is non-critical, | |
| // so just return false for any exception and let the caller handle it. | |
| return false; | |
| } | |
| if (target == null) | |
| { | |
| return false; | |
| } | |
| Point positionFromScreen; | |
| try | |
| { | |
| positionFromScreen = target.PointToScreen(new Point(x, y)); | |
| } | |
| catch (InvalidOperationException) | |
| { | |
| // DYN-9823: PointToScreen can throw when the visual is not connected to a PresentationSource | |
| // (e.g. during layout, tab switch, or node deletion). Popup placement is non-critical, | |
| // so just return false for this expected exception and let the caller handle it. | |
| return false; | |
| } | |
| PresentationSource source = PresentationSource.FromVisual(target); | |
| if (source?.CompositionTarget == null) | |
| { | |
| return false; | |
| } | |
| result = source.CompositionTarget.TransformFromDevice.Transform(positionFromScreen); | |
| return true; |
| private void ViewModel_RequestAutoCompletePopupPlacementTarget(Window window, PortModel portModel, double spacing) | ||
| { | ||
| if (portModel.PortType == PortType.Input) | ||
| { | ||
| var portView = inputPortControl.ItemContainerGenerator.ContainerFromIndex(portModel.Index) as FrameworkElement; | ||
| window.Top = PointToLocal(0, 0, portView).Y; | ||
| window.Left = PointToLocal(0, 0, this).X - window.Width - spacing; | ||
| if (TryPointToLocal(0, 0, portView, out var portTop) && TryPointToLocal(0, 0, this, out var nodeLeft)) | ||
| { | ||
| window.Top = portTop.Y; | ||
| window.Left = nodeLeft.X - window.Width - spacing; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| var portView = outputPortControl.ItemContainerGenerator.ContainerFromIndex(portModel.Index) as FrameworkElement; | ||
| window.Top = PointToLocal(0, 0, portView).Y; | ||
| window.Left = PointToLocal(ActualWidth, 0, this).X + spacing; | ||
| if (TryPointToLocal(0, 0, portView, out var portTop) && TryPointToLocal(ActualWidth, 0, this, out var nodeRight)) | ||
| { | ||
| window.Top = portTop.Y; | ||
| window.Left = nodeRight.X + spacing; | ||
| } |
There was a problem hiding this comment.
If either TryPointToLocal call fails here, the window position is left unchanged (or stays at the default for a newly created window), but the autocomplete window may still be shown by the caller. That can result in the popup appearing in an arbitrary/stale location. Consider a fallback behavior when placement can't be computed (e.g., hide/close the window for that tick, or place it relative to the owner/workspace until a valid target is available).
|
| return targetPoints; | ||
| result = default; | ||
| try | ||
| { |
There was a problem hiding this comment.
Does it make sense to do some extra checks here to avoid an exception ?
for source, source.RootVisual, source.CompositionTarget
| // DYN-9823: PointToScreen can throw when the visual is not connected to a PresentationSource | ||
| // (e.g. during layout, tab switch, or node deletion). Popup placement is non-critical, | ||
| // so just return false for any exception and let the caller handle it. | ||
| return false; |
There was a problem hiding this comment.
So the side effect will be that we position the window in a weird position ?
Since we still continue with the window setup.
But I guess this happens so rare that we can accept this.
There was a problem hiding this comment.
Yeah the autocomplete popup could end up in a weird position if the user stops moving the node around just in time where the exception happens. It should then continue following the node if the node is moved again. So its like a single frame thing in very weird cases.
|
LGTM! |
Purpose
DYN-9823: - Fix crash when the Node AutoComplete popup placement runs while the visual tree is in a transient state.
PointToScreencan throw "This Visual is not connected to a PresentationSource" during layout, rapid node movement/deletion, or tab switching.This is a very tricky error to reproduce and while i was able to repro locally, i was not able to do so consistently. One way is to move the node attached to the autocomplete view very fast and deleting the node while doing so, but that only crashes 1 out of 20 times when done right and using some luck.
Declarations
Check these if you believe they are true
Release Notes
Fixed crash when moving a node very fast and simultaneously deleting a node with the Node AutoComplete bar open.
Reviewers
@DynamoDS/synapse
FYIs
@avidit