Skip to content

Conversation

@kennykerr
Copy link
Collaborator

Fixes #427 by adding unboxing helpers for maps.

@kennykerr
Copy link
Collaborator Author

/azp run

@kennykerr kennykerr requested a review from jonwis July 10, 2020 22:20
@microsoft microsoft deleted a comment from azure-pipelines bot Jul 10, 2020
@microsoft microsoft deleted a comment from azure-pipelines bot Jul 10, 2020
Copy link
Member

@jonwis jonwis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! (Also, cc @DrusTheAxe)


// TryLookupAs edge cases

REQUIRE_FALSE(m.TryLookupAs<bool>(L"INVALID").has_value());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does "nullptr" here represent the invalid value? Or should there be a test for m.TryLookupAs<Uri>(L"INVALID") here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TryLookupAs<bool> will return a std::optional<bool>, and we are verifying that it returns a valueless object.

In the second test, TryLookupAs<Uri> will return a Uri, and we are using the Uri == nullptr test to see if the Uri object is empty.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about a test that validates TryLookupAs<IInspectableDerivedThing>(L"VALUE THAT IS NOT IN THE MAP")?

Copy link
Member

@oldnewthing oldnewthing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I preferred the proposal to add support for value types to IInspectable.as<> and IInspectable.try_as<> (treating them as unboxing), because that is useful in places other than map lookups. For example, converting XAML tags and navigation parameters.

// old and busted
auto bounds = unbox_value<Rect>(SelectedItem().as<FrameworkElement>().Tag());
auto userid = unbox_value<hstring>(navigationEventArgs.Parameter());


// new hotness
auto bounds = SelectedItem().as<FrameworkElement>().Tag().as<Rect>();
auto userid = navigationEventArgs.Parameter().as<hstring>();

Even further, try_as could take a second parameter that is the fallback value. This is extremely handy when doing map lookups.

// with this proposal
auto optional_priority = settings.TryLookupAs<PriorityLevel>(L"priority");
auto priority = maybe_priority ? maybe_priority.value() : PriorityLevel::Normal;

// newer hotness
auto priority = settings.TryLookup(L"priority").try_as<PriorityLevel>(PriorityLevel::Normal);

If we are hesitant to overload as and try_as in that way, we could call the new methods unbox_value, try_unbox_value, and unbox_value_or to match the free functions.

auto bounds = SelectedItem().as<FrameworkElement>().Tag().unbox_value<Rect>();
auto userid = navigationEventArgs.Parameter().unbox_value<hstring>();
auto priority = settings.TryLookup(L"priority").unbox_value_or<PriorityLevel>(PriorityLevel::Normal):


// TryLookupAs edge cases

REQUIRE_FALSE(m.TryLookupAs<bool>(L"INVALID").has_value());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TryLookupAs<bool> will return a std::optional<bool>, and we are verifying that it returns a valueless object.

In the second test, TryLookupAs<Uri> will return a Uri, and we are using the Uri == nullptr test to see if the Uri object is empty.

@mlalic
Copy link
Contributor

mlalic commented Jul 11, 2020

I preferred the proposal to add support for value types to IInspectable.as<> and IInspectable.try_as<> (treating them as unboxing), because that is useful in places other than map lookups. For example, converting XAML tags and navigation parameters.

FWIW, in addition to the Xaml examples Raymond listed, we have a new UIAutomation platform API (currently in insider preview SDK builds) that would also benefit from a uniform way of converting an IInspectable to a runtimeclass, COM pointer, or an unboxed value.

For example;

// today
auto elementResult = result.GetOperand(a).as<IUIAutomationElement>(); // this is a COM interface
auto textRangeResult = result.GetOperand(b).as<winrt::AutomationTextRange>(); // this is a WinRT runtimeclass
auto stringResult = winrt::unbox_value<winrt::hstring>(result.GetOperand(c));


// new hotness :)
auto elementResult = result.GetOperand(a).as<IUIAutomationElement>();
auto textRangeResult = result.GetOperand(b).as<winrt::AutomationTextRange>();
auto stringResult = result.GetOperand(c).as<winrt::hstring>();

The consistency looks really appealing from a consumer's perspective, IMO.

@kennykerr
Copy link
Collaborator Author

Yes, I also prefer overloading as and try_as - that was my original suggestion. The trouble is (beyond the tricky metaprogramming) that this introduces a dependency from winrt/base.h (where IUnknown/IInspectable/as/try_as live) to winrt/windows.foundation.h (where boxing support lives). And that's just a much bigger change than I'd like to do for this feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Maps (propertysets) of K to Object could get an unboxing [Try]Lookup

5 participants