Inconsistent behavior for [Logical|Visual]Tree extensions

[Sergio0694]

Describe the bug

There are a number of inconsistencies in the various extensions in the LogicalTree and VisualTree extensions. Specifically, in the way input arguments are validated and used to perform the various requested operations. For instance:

https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/ca461ca17112c5967730b56f34187d9c5d5cc3ac/Microsoft.Toolkit.Uwp.UI/Extensions/Tree/LogicalTree.cs#L208-L213

If element is null, the method will gracefully bail and return null.

https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/ca461ca17112c5967730b56f34187d9c5d5cc3ac/Microsoft.Toolkit.Uwp.UI/Extensions/Tree/LogicalTree.cs#L234-L240

If element is null, the method will throw a NullReferenceException on 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 .tail opcode 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:

• Marking input arguments as non-nullable reference types, and use the implicit NRE if they're null
• Marking the returned value as nullable, ie. FrameworkElement?

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, from master

[michael-hawker]

@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. 😋

[michael-hawker]

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.

[michael-hawker]

For instance with the Gaze code, if we had an alternate FindAscendant method which could take a filter type function (to look for some statement about the item in the tree), we could do something like this:

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.

[michael-hawker]

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.

[michael-hawker]

Synced with @Sergio0694, we agreed:

• All methods should just naturally throw a NRE if the underlying root element is null: this should usually indicate a development error (invoking in non-standard way with null or not using a element?.method() calling pattern from the parent code), this also doesn't involve an additional safety check for performance scenarios (i.e. Gaze (in the future))
• All method should be defined with a 'Self' version and a non-self version to clarify if the calling element should be checked first or not before walking up/down the tree. (We should align naming scheme to how Roslyn does it (e.g. might be FindAscendentOrSelf).)
• For method looking up, we'll remove recursion
• 'ByName' methods will switch to use StringComparison.Ordinal by default (fastest) but provide overload parameter to modify.
• Make use of nullable annotations from new C# lang features
• Add Unit Tests

Proposals:

• Rename Find*ByName to just the same as the root Find* method taking string as overload just implies search by name...
• Expose function delegate versions which could be used to simplify the ByName functions as well as support new scenarios as commented here These could also expose TState versions as well. Strongly typed versions will only call the delegate on the matched types, but still crawl past them (just as the regular methods work).
• Check if we should write other helpers (Name, Type) as a function of delegate root or have specific code this is a maintenance vs. performance question
• P2: Do we expose plural versions of the Logic functions?
• P2: Do we need any additional helpers to detect/find VisualStateManagers on elements?