Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Proposal][WinUI] Easily create dependency properties #472

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

[Proposal][WinUI] Easily create dependency properties #472

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
661e980
compilable stub
citelao Sep 6, 2024
3cb550f
local static?
citelao Sep 6, 2024
f454113
support default value too
citelao Sep 6, 2024
8124844
support strings
citelao Sep 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 112 additions & 0 deletions include/wil/cppwinrt_authoring.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,5 +330,117 @@ struct single_threaded_notifying_property : single_threaded_rw_property<T>
*/
#define INIT_NOTIFYING_PROPERTY(NAME, VALUE) NAME(&m_propertyChanged, *this, L"" #NAME, VALUE)

namespace details
{
#ifdef WINRT_Microsoft_UI_Xaml_Data_H
using Xaml_DependencyProperty = winrt::Microsoft::UI::Xaml::DependencyProperty;
using Xaml_PropertyChangedCallback = winrt::Microsoft::UI::Xaml::PropertyChangedCallback;
using Xaml_PropertyMetadata = winrt::Microsoft::UI::Xaml::PropertyMetadata;
#elif defined(WINRT_Windows_UI_Xaml_Data_H)
using Xaml_DependencyProperty = winrt::Windows::UI::Xaml::DependencyProperty;
using Xaml_PropertyChangedCallback = winrt::Windows::UI::Xaml::PropertyChangedCallback;
using Xaml_PropertyMetadata = winrt::Windows::UI::Xaml::PropertyMetadata;
#endif

using Xaml_DependencyObject_GetValue = std::function<winrt::Windows::Foundation::IInspectable(Xaml_DependencyProperty const&)>;
using Xaml_DependencyObject_SetValue = std::function<void(Xaml_DependencyProperty const&, winrt::Windows::Foundation::IInspectable const&)>;
Copy link
Contributor

Choose a reason for hiding this comment

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

These seem unused. Also why the type erasure?


template <typename PropertyType, typename OwnerType>
inline Xaml_DependencyProperty register_dependency_property(
const std::wstring_view& propertyNameString)
Copy link
Contributor

Choose a reason for hiding this comment

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

std::wstring_view is trivially copyable. I think it is actually more efficient to pass it by value because it is so cheap to copy.

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct, taking it by value avoids a stack spill on ARM64: https://godbolt.org/z/e5WbGKz1q

Copy link
Member

Choose a reason for hiding this comment

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

+1. std::wstring_view is cheaply-copied so F16 applies

{
return Xaml_DependencyProperty::Register(
propertyNameString,
winrt::template xaml_typename<PropertyType>(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is the definition of xaml_typename coming from?

Copy link
Member

Choose a reason for hiding this comment

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

@dmachaj see https://learn.microsoft.com/en-us/uwp/cpp-ref-for-winrt/xaml-typename

winrt::template xaml_typename<OwnerType>(),
nullptr);
}

template <typename PropertyType, typename OwnerType, typename DefaultValueType = PropertyType>
inline Xaml_DependencyProperty register_dependency_property(
const std::wstring_view& propertyNameString,
const DefaultValueType& defaultValue,
const Xaml_PropertyChangedCallback& propertyChangedCallback = nullptr)
{
// TODO: assert T and PropertyType are compatible
Copy link
Member

Choose a reason for hiding this comment

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

put a typename = std::enable_if_t<is_assignable_v<PropertyType, DefaultValueType>> to filter it out

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: require C++20 and use std::convertible_to<PropertyType> DefaultValueType

Nit: C++/WinRT has no formal documented way to 100% check this, see microsoft/cppwinrt#609

return Xaml_DependencyProperty::Register(
propertyNameString,
winrt::template xaml_typename<PropertyType>(),
winrt::template xaml_typename<OwnerType>(),
Xaml_PropertyMetadata{winrt::box_value(defaultValue), propertyChangedCallback});
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a static_cast<PropertyType> for the default value here, in case nullptr is passed.

}
}

//template <typename T>
//struct single_threaded_dependency_property
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of this big comment? It seems like a full implementation but it is entirely commented out and unused.

//{
// using Type = T;
//
// single_threaded_dependency_property(
// wil::details::Xaml_DependencyProperty const& dp,
// wil::details::Xaml_DependencyObject_GetValue const& getValue,
// wil::details::Xaml_DependencyObject_SetValue const& setValue) :
// m_dp(dp), m_getValue(getValue), m_setValue(setValue)
// {
// }
//
// template <typename Q>
// auto& operator()(Q&& q)
// {
// return winrt::unbox_value<Type>(m_getValue(m_dp));
// }
//
// template <typename Q>
// auto& operator=(Q&& q)
// {
// m_setValue(m_dp, winrt::box_value(value));
// }
//
//private:
// wil::details::Xaml_DependencyProperty const& m_dp{ nullptr };
// wil::details::Xaml_DependencyObject_GetValue m_getValue{nullptr};
// wil::details::Xaml_DependencyObject_SetValue m_setValue{nullptr};
//};

#define WIL_DEFINE_DP(baseClass, type, name) \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have to pass baseClass to WIL_DEFINE_DP because we need the winrt::xaml_typename<> of the base class. This seems like we could derive automatically from any real winrt::implements class---is there not some sort of this->BaseType property via C++/WinRT?

Copy link
Member

@asklar asklar Sep 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

we need the xaml_typename but we don't need the class itself.
xaml_typename takes the type as a template param and no arguments
but you could define something like:

template<typename T>
auto xaml_typename_from_object(T&&) {
  return winrt::xaml_typename<T>();
}

and just pass xaml_typename_from_object(*this) or similar

Copy link
Contributor

Choose a reason for hiding this comment

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

winrt::implements has a class_type typedef which seems to be what you want here.

My own macro does the following:

# define DECL_DEPENDENCY_PROPERTY_FIELD(TYPE, NAME, METADATA) \
	inline static wux::DependencyProperty DEPENDENCY_PROPERTY_FIELD(NAME) = \
		wux::DependencyProperty::Register( \
			UTIL_STRINGIFY(NAME), \
			winrt::xaml_typename<TYPE>(), \
			winrt::xaml_typename<class_type>(), \
			METADATA);

Copy link
Member

Choose a reason for hiding this comment

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

the naming here is confusing because I think by baseClass you mean the class that owns the DP, not the base class in an inheritance sense? Same with type, it's ambiguous, I like the naming you had above (propertyType vs ownerType)

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#define WIL_DEFINE_DP(baseClass, type, name) \
#define WIL_DEFINE_XAML_DP(baseClass, type, name) \

Better namespacing

Copy link
Member

Choose a reason for hiding this comment

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

naming wise we probably want to be more specific than DP, i.e. spell out DEPENDENCY_PROPERTY

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a mouthful, and I think XAML_DP makes it pretty unambiguous.

static wil::details::Xaml_DependencyProperty name##Property() \
{ \
static wil::details::Xaml_DependencyProperty s_##name##Property = \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose to use function-local statics for a few reasons:

  1. XAML doesn't seem to really care when these statics are initialized: in production, I've seen folks set the statics in the element's constructor, in an "Ensure" function that runs during app load, and even just in the root of the .cpp itself (which I know is dangerous).
  2. I'm sure this differs from global statics in some respects (is this specific to the translation unit?)---but I'm not sure if that matters for this type of call. What is different because of this change?
  3. Using function-local statics dramatically simplifies this code.

What should we do here?

Copy link
Contributor

Choose a reason for hiding this comment

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

which I know is dangerous

Do you mind explaining how this is dangerous? It's what I've mostly seen around.

XAML doesn't seem to really care when these statics are initialized

Is this also true when e.g. the first instance of the class being created is through a .xaml file and that specific file assigns some DPs? I would've expected XAML to have difficulty there if lazily created since it is gonna try to assign properties that don't have registered metadata yet.

What I do here in my own macro is use C++17's inline statics, which end up emulating global static behavior but allows you to define it in the header (so I'm wondering if I should move away from this because your statement of them being dangerous).

Also, inline statics aren't specific to the TU.

wil::details::register_dependency_property<type, baseClass>(L"" #name); \
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest using a stringify helper macro here, in case the dependency property name itself is a macro (it wouldn't be expanded here, leading to a wrong name)

return s_##name##Property; \
} \
auto name() const \
{ \
return winrt::unbox_value<type>(GetValue(name##Property())); \
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing I dislike about overly long macros is that you can't debug them with source code. It can be useful to set debug breakpoints in a getter or setter to see who is changing the value. It won't be possible to (source) debug these DPs.

} \
void name(type value) const \
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems a bit odd that the setter is marked const. I guess it technically is because the pointer to the DP is not changing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend not marking it const yeah - it would be extremely odd and break typical assumptions to have a const foo but being able to set properties, even if technically it's possible.

{ \
SetValue(name##Property(), winrt::box_value(value)); \
} \
static void Ensure##name##Property() \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I provide the Ensure###Property() method to match previous guidance for creating custom DPs. But in practice, calling this in the constructor or near first-use seems to work fine. What are the actual repercussions here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there ever a case where an "unensure" function would be needed? For example what happens if XAML is torn down before the module unloads? Destructing the function level static will not be happy.

{ \
name##Property(); \
}

#define WIL_DEFINE_DP_WITH_DEFAULT_VALUE_AND_CALLBACK(baseClass, type, name, defaultValue, propertyChangedCallback) \
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it valid to provide a default value but not care about the callback (passing null)? Or vice-versa.

Copy link
Contributor

Choose a reason for hiding this comment

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

From XAML's point of view, yes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest folding this and WIL_DEFINE_DP together, lots of repetition otherwise. For example, have them both summon the same macro which uses variadic arguments for the parameters of register_dependency_property.

static wil::details::Xaml_DependencyProperty name##Property() \
{ \
static wil::details::Xaml_DependencyProperty s_##name##Property = \
wil::details::register_dependency_property<type, baseClass>(L"" #name, defaultValue, propertyChangedCallback); \
return s_##name##Property; \
} \
auto name() const \
{ \
return winrt::unbox_value<type>(GetValue(name##Property())); \
} \
void name(type value) const \
{ \
SetValue(name##Property(), winrt::box_value(value)); \
} \
static void Ensure##name##Property() \
{ \
name##Property(); \
}

#endif // !defined(__WIL_CPPWINRT_AUTHORING_INCLUDED_XAML_DATA) && (defined(WINRT_Microsoft_UI_Xaml_Data_H) || defined(WINRT_Windows_UI_Xaml_Data_H))
} // namespace wil
67 changes: 67 additions & 0 deletions tests/CppWinRTAuthoringTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@
#if _MSVC_LANG >= 201703L
#include <winrt/Windows.Foundation.h>
#include <winrt/Windows.System.h>
#include <winrt/Windows.UI.Xaml.h>
#include <winrt/Windows.UI.Xaml.Data.h>
#include <winrt/Windows.UI.Xaml.Input.h>
#include <winrt/Windows.UI.Xaml.Interop.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think WIL has a WinAppSDK dependency to test against. This coverage is limited to System XAML types.

I think there are already some WinAppSDK helpers in WIL that are similarly impossible to cover with tests. That's unfortunate.

#endif

#include <wil/cppwinrt_authoring.h>
Expand Down Expand Up @@ -173,6 +175,71 @@ TEST_CASE("CppWinRTAuthoringTests::InStruct", "[property]")
REQUIRE(test.Prop2() == 33);
}

// Simple mock of GetValue & SetValue for testing dependency properties.
struct mock_dependency_object
{
winrt::Windows::Foundation::IInspectable GetValue(winrt::Windows::UI::Xaml::DependencyProperty const& dp) const
{
return m_values.at(dp);
}
void SetValue(winrt::Windows::UI::Xaml::DependencyProperty dp, winrt::Windows::Foundation::IInspectable const& value) const
{
// We have to do a const_cast because ordinarily SetValue is const---
// it's delegated to WinRT, and C++/WinRT marks most of its functions
// const.
Comment on lines +187 to +189
Copy link
Member

Choose a reason for hiding this comment

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

this seems odd... SetValue's job is to change a property on the object so the method should not be const...
if it must be, maybe changing m_values to be mutable instead of the const_cast?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is cause of the macro itself using const for the property setter. This wouldn't be needed if it weren't.

const_cast<mock_dependency_object*>(this)->m_values[dp] = value;
}

std::map<winrt::Windows::UI::Xaml::DependencyProperty, winrt::Windows::Foundation::IInspectable> m_values{};
};

struct my_dependency_properties : mock_dependency_object
{
WIL_DEFINE_DP(my_dependency_properties, int32_t, MyProperty);
WIL_DEFINE_DP_WITH_DEFAULT_VALUE_AND_CALLBACK(my_dependency_properties, int32_t, MyPropertyWithDefault, 42, nullptr);
WIL_DEFINE_DP(my_dependency_properties, winrt::hstring, MyStringProperty);
};

namespace winrt
{
// Fake the xaml_typename specialization
template<>
inline Windows::UI::Xaml::Interop::TypeName xaml_typename<my_dependency_properties>()
{
static const Windows::UI::Xaml::Interop::TypeName name{hstring{L"my_dependency_properties"}, Windows::UI::Xaml::Interop::TypeKind::Custom};
return name;
}
}

TEST_CASE("CppWinRTAuthoringTests::DependencyProperties", "[property]")
{
// Throws if not ensured (?)
auto obj = my_dependency_properties{};
// REQUIRE_FAILFAST_MSG(E_NOTIMPL, [&obj] { obj.MyProperty(); });
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these intended to get uncommented later?

// REQUIRE_FAILFAST_MSG(E_NOTIMPL, [&obj] { obj.MyProperty(42); });
// REQUIRE_FAILFAST_MSG(E_NOTIMPL, [&obj] { obj.MyPropertyWithDefault(); });
// REQUIRE_FAILFAST_MSG(E_NOTIMPL, [&obj] { obj.MyPropertyWithDefault(42); });
// REQUIRE_FAILFAST_MSG(E_NOTIMPL, [&obj] { obj.MyStringProperty(); });
// REQUIRE_FAILFAST_MSG(E_NOTIMPL, [&obj] { obj.MyStringProperty(L"foo"); });

// Register the dependency property
my_dependency_properties::EnsureMyPropertyProperty();
my_dependency_properties::EnsureMyPropertyWithDefaultProperty();
my_dependency_properties::EnsureMyStringPropertyProperty();

// Now it should work
obj.MyProperty(42);
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the behavior of querying a property that has not been set yet? Undefined behavior? Should this macro require that the properties be default-constructible or they have to provide a default value if not?

Copy link
Member

Choose a reason for hiding this comment

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

when you register the property you give it its default value

Copy link
Contributor

Choose a reason for hiding this comment

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

The macro without a default value passes nullptr for the Windows::UI::Xaml::PropertyMetadata. That leaves it ambiguous as to the default value. We'd have to check the XAML documentation (assuming the behavior here is publicly documented).

Copy link
Member

Choose a reason for hiding this comment

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

Passing null for this parameter is equivalent to passing a new PropertyMetadata instance created by calling PropertyMetadata.Create with null as the default value parameter.

See https://learn.microsoft.com/en-us/uwp/api/windows.ui.xaml.dependencyproperty.register?view=winrt-26100#definition

Copy link
Contributor

Choose a reason for hiding this comment

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

So calling get without specifying a default value or setting it first would result in winrt::unbox_value<T>(nullptr) getting called. And that seems to throw E_NOINTERFACE. https://github.com/microsoft/cppwinrt/blob/2e4bdb05da1fe86080c523a2e00f81d62f209b17/strings/base_reference_produce.h#L430

Copy link
Contributor

Choose a reason for hiding this comment

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

https://learn.microsoft.com/en-us/uwp/api/windows.ui.xaml.dependencyproperty.register?view=winrt-26100

Passing null for this parameter is equivalent to passing a new PropertyMetadata instance created by calling PropertyMetadata.Create with null as the default value parameter.

This means that for ref types, you get a null default value. For value types, unbox_value will throw hresult_no_interface until a value is set (because you'll try to unbox nullptr).

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, those comments hadn't showed up on my end when I was writing this. @dmachaj is correct, with the addition that it wouldn't throw with ref types, but just return nullptr.

REQUIRE(obj.MyProperty() == 42);

// TODO: handle defaults
// REQUIRE(obj.MyPropertyWithDefault() == 42);
obj.MyPropertyWithDefault(43);
REQUIRE(obj.MyPropertyWithDefault() == 43);

obj.MyStringProperty(L"foo");
REQUIRE(obj.MyStringProperty() == L"foo");
}

#ifdef WINRT_Windows_Foundation_H
TEST_CASE("CppWinRTAuthoringTests::Events", "[property]")
{
Expand Down
Loading