UiaOperationScope.ForEach fetches the array element as const

[michaelDCurran]

UiaOperationScope has a useful utility method: ForEach which executes the given lambda on each element in the given array, whether running locally or remotely.

However, this method fetches each element from the array and passes it to the visitor function as const, which means it is impossible to modify the element from within the visitor function if the visitor function accepts the element by reference.

And because none of the Uia types have methods marked as const, it is therefore impossible to do much at all in the visitor function. For example,

scope.ForEach(myArray, [](auto& element){
 auto element.GetName();
});

Results in an error like the following:

build\x86\UIARemote\UIARemote.cpp(367): error C2662: 'UiaOperationAbstraction::UiaString UiaOperationAbstraction::UiaElement::GetName(bool)': cannot convert 'this' pointer from 'const ItemWrapperType' to 'UiaOperationAbstraction::UiaElement &'
        with
        [
            ItemWrapperType=UiaOperationAbstraction::UiaElement
        ]
build\x86\UIARemote\UIARemote.cpp(367): note: Conversion loses qualifiers
build\x86\microsoft-ui-uiautomation\include\UiaOperationAbstraction\UiaTypeAbstraction.g.h(2065): note: see declaration of 'UiaOperationAbstraction::UiaElement::GetName'
build\x86\microsoft-ui-uiautomation\include\UiaOperationAbstraction/UiaOperationAbstraction.h(2379): note: see reference to function template instantiation 'auto _remoteable_getTextContent::<lambda_c4afe82d3e528f79f41517a2c2e0fcb4>::operator ()<const ItemWrapperType>(const ItemWrapperType &) const' being compiled
        with
        [
            ItemWrapperType=UiaOperationAbstraction::UiaElement
        ]

A workaround is to have the lambda accept the element by value rather than reference:

scope.ForEach(myArray, [](auto element){
 auto element.GetName();
});

As I believe that copying the Uia* types is remotely cheap and ends up using the same underlying object anyway.

But personally I do prefer to code my functions taking Uia* types by reference as it just feels more symantically correct, and removes any guessing about whether copying is in deed cheep or not.

I'm also really not sure why scope.forEach deliberately fetches the element from the array as a const variable? I don't think the concept of const really makes any sense for these types, especially when none of the methods on them are marked as const.

Removing const here would be a one line change. I can open a pr to do this, but I first wanted to better understand why it was made const in the first place.

[michaelDCurran]

the change would be as follows:

diff --git a/src/UIAutomation/UiaOperationAbstraction/UiaOperationAbstraction.h b/src/UIAutomation/UiaOperationAbstraction/UiaOperationAbstraction.h
index eb831d4..86c843d 100644
--- a/src/UIAutomation/UiaOperationAbstraction/UiaOperationAbstraction.h
+++ b/src/UIAutomation/UiaOperationAbstraction/UiaOperationAbstraction.h
@@ -2375,7 +2375,7 @@ namespace UiaOperationAbstraction
                 [&]() { index += 1; } /* modification */,
                 [&]() /* body */
                 {
-                    const auto element = array.GetAt(index);
+                    auto element = array.GetAt(index);
                     body(element);
                 });
         }

[michaelDCurran]

I'm also happy to accept an answer such that it is recommended that all Uia* types should always be accepted by value in functions, but in this case I'd appreciate a clear explanation as to why.

[beweedon]

Hmm, thanks for the feedback! Honestly, my guess as to why we made that const is just that we're getting the array item by value from GetAt and didn't think too hard about what the signature of body would be.

I think I agree with you that it shouldn't be const. That way we allow body to have whatever signature it wants, which matches the capabilities of normal C++ foreach loops (i.e. the item can be const auto, const auto&, auto& etc.).

@pengyuanjin @mavangur @zhayuli Do you have any particular thoughts on this?

[beweedon]

@michaelDCurran I'd say go ahead and put up a PR for this if you want (along with some tests validating multiple constness-es). Otherwise we'll get to it when we have the chance.