Conversation
mou-haz
force-pushed
the
ExpanderInPopupBug
branch
2 times, most recently
from
5d48ddd to
bfabfa4
Compare
mou-haz
force-pushed
the
ExpanderInPopupBug
branch
from
bfabfa4 to
4dddcd5
Compare
…F.Modern into ExpanderInPopupBug
|
Not sure adding |
There was a problem hiding this comment.
Pull request overview
This PR fixes a NullReferenceException that occurred when displaying a ContentDialog containing an Expander with the ExpanderCardStyleKey style. The issue stemmed from the animation helper attempting to access a template part before the template was applied.
Changes:
- Adds
ApplyTemplate()call to ensure the Expander's template is loaded before searching for animation target parts - Implements caching mechanism to store the found animation target control and avoid repeated template searches
- Replaces silent null return with a descriptive exception when the animation target part cannot be found
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| expander.ApplyTemplate(); | ||
| var toAnimateControl = expander.Template?.FindName(GetToAnimateControlName(expander), expander) as FrameworkElement ?? | ||
| throw new ArgumentNullException("ToAnimateControl", $"Couldn't find the part to animate either update the ExpanderAnimationsHelper.ToAnimateControlName or rename the animation target part to default: {DefaultAnimationTargetPartName}"); |
There was a problem hiding this comment.
The exception type should be InvalidOperationException rather than ArgumentNullException. An ArgumentNullException is used when a method parameter is null, but in this case, the issue is that a required control part cannot be found in the template, which is an invalid state or operation issue, not a null argument issue.
| throw new ArgumentNullException("ToAnimateControl", $"Couldn't find the part to animate either update the ExpanderAnimationsHelper.ToAnimateControlName or rename the animation target part to default: {DefaultAnimationTargetPartName}"); | |
| throw new InvalidOperationException($"Couldn't find the part to animate either update the ExpanderAnimationsHelper.ToAnimateControlName or rename the animation target part to default: {DefaultAnimationTargetPartName}"); |
| private static readonly DependencyProperty CachedToAnimateControlProperty = | ||
| DependencyProperty.RegisterAttached( | ||
| "CachedToAnimateControl", | ||
| typeof(FrameworkElement), | ||
| typeof(ExpanderAnimationsHelper), | ||
| new PropertyMetadata(null)); |
There was a problem hiding this comment.
The caching mechanism doesn't account for runtime template changes. If an Expander's Template property is changed after it has been loaded, the cached control will still reference the old template's element. Consider either: 1) Listening to the TemplateApplied event or overriding OnApplyTemplate to clear the cache, 2) Adding a property changed callback on Control.TemplateProperty to clear the cache, or 3) Accepting this as a known limitation if runtime template changes are not a supported scenario.
|
|
||
| expander.ApplyTemplate(); | ||
| var toAnimateControl = expander.Template?.FindName(GetToAnimateControlName(expander), expander) as FrameworkElement ?? | ||
| throw new ArgumentNullException("ToAnimateControl", $"Couldn't find the part to animate either update the ExpanderAnimationsHelper.ToAnimateControlName or rename the animation target part to default: {DefaultAnimationTargetPartName}"); |
There was a problem hiding this comment.
The error message could be clearer. Consider rephrasing to: "Couldn't find the part to animate. Either update ExpanderAnimationsHelper.ToAnimateControlName, or rename the animation target part to the default: {DefaultAnimationTargetPartName}"
| throw new ArgumentNullException("ToAnimateControl", $"Couldn't find the part to animate either update the ExpanderAnimationsHelper.ToAnimateControlName or rename the animation target part to default: {DefaultAnimationTargetPartName}"); | |
| throw new ArgumentNullException("ToAnimateControl", $"Couldn't find the part to animate. Either update ExpanderAnimationsHelper.ToAnimateControlName, or rename the animation target part to the default: {DefaultAnimationTargetPartName}"); |
|
Hey @NotYoojun , i removed the caching property. can you see if it's good now ? |
2 participants
Fix #421