Inconsistent behavior for [Logical|Visual]Tree extensions #3487
Comments
|
@Sergio0694 I was referring to the Visual/Logic tree helpers before, so appreciate you taking a look. Agree we should be consistent here, I think originally I was thinking the element would never be null as an extension method, but it doesn't mean it can't just be called directly either. Happy for you to take a stab at working your magic here to optimize and improve these methods. I think we have some unit tests??? If not, we certainly should. |
|
Yeah, think the Gaze usages would be a prime example of scenarios where performance can matter. Not sure if those scenarios can be better optimized independently or require new methods/features to our existing set to better centralize code. Something to look at though. |
|
For instance with the Gaze code, if we had an alternate element = element.FindSelfOrAscendant<UIElement>(current => GazeTargetItem.GetOrCreate(current).IsInvokable);
invokable = GazeTargetItem.GetOrCreate(element);vs. invokable = null;
if (element != null)
{
invokable = GazeTargetItem.GetOrCreate(element);
while (element != null && !invokable.IsInvokable)
{
element = VisualTreeHelper.GetParent(element) as UIElement;
if (element != null)
{
invokable = GazeTargetItem.GetOrCreate(element);
}
}
}This could work for a variety of scenarios, for instance looking for a visible or collapsed element, etc... Kind of a broader case than some of our named element finders. Thinking in the above scenario for the typed info, I'd see the filter being strongly typed, as you'd still only be looking for elements of the specified type and then further checking if they meet this extra criteria. |
|
I think we lack testing (surprisingly) for these functions as well, so a suite of test cases for these functions would be good to have as well. |
Describe the bug
There are a number of inconsistencies in the various extensions in the
LogicalTreeandVisualTreeextensions. Specifically, in the way input arguments are validated and used to perform the various requested operations. For instance:WindowsCommunityToolkit/Microsoft.Toolkit.Uwp.UI/Extensions/Tree/LogicalTree.cs
Lines 208 to 213 in ca461ca
If
elementisnull, the method will gracefully bail and returnnull.WindowsCommunityToolkit/Microsoft.Toolkit.Uwp.UI/Extensions/Tree/LogicalTree.cs
Lines 234 to 240 in ca461ca
If
elementisnull, the method will throw aNullReferenceExceptionon line 237.Similarly, the former will match the name with the input element as a first step, and then navigate upwards, whereas the second one will not check the input element at all and only look for matches in the visual hierarchy. I'd argue the latter is more correct and appropriate for the name of the method, but regardless, the behavior should be consistent.
There are a number of similar issues in the two classes, plus some optimizations that could be done while we're at it (namely, to manually unroll some recursive tail calls, since the C# compiler doesn't support the
.tailopcode automatically).cc. @michael-hawker If I'm not mistaking, these would be the ones you asked me about a while back too?🤔
Also, I think we should agree on a standard handling of arguments in order for them to be properly annotated.
Personally I would suggest:
nullFrameworkElement?I can help with this and make a PR when we agree on how to approach these various points.😊
The changes would go into 7.0 anyway, so I think it would be ok for us to make breaking changes
Package Version(s):
Microsoft.Toolkit.Uwp.UI 6.1.1, frommasterThe text was updated successfully, but these errors were encountered: