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

citelao · 2024-09-06T16:37:02Z

Today, creating a custom dependency property requires a lot of boilerplate. This change gets rid of most of it. With this change, you can add a new dependency property quickly:

// idl
runtimeclass BgLabelControl : Windows.UI.Xaml.Controls.Control
{
    // ...
    static Windows.UI.Xaml.DependencyProperty LabelProperty{ get; };
    String Label;
}

// .h
namespace winrt::BgLabelControlApp::implementation
{
    struct BgLabelControl : BgLabelControlT<BgLabelControl>
    {
        // ...

        // At some point in your app initialization, you want to call `BgLabelControl::EnsureLabelProperty`
        WIL_DEFINE_DP(BgLabelControl, Label, winrt::hstring);
    };
}

Versus before you'd have to (see example on MSDN):

(Unchanged) IDL: Declare the IDL
.h: Declare a static DP class variable
.h: Declare a static DP getter function
.cpp: Define memory for the static DP
(If you want to be correct) .cpp: In your app, initialize the static DP during startup
.h: Declare & define member getter & setter functions

What changed?

Introduce new macros WIL_DEFINE_DP and WIL_DEFINE_DP_WITH_DEFAULT_VALUE_AND_CALLBACK that automatically create idiomatic implementations of the 3 static & member methods needed for a dependency property.
To avoid any problems with statics, these macros use function-local static storage instead of global static storage.

How tested?

New tests pass
TODO: consumed in actual XAML project

citelao

Left my initial questions

citelao · 2024-09-06T16:38:51Z

include/wil/cppwinrt_authoring.h

+//    wil::details::Xaml_DependencyObject_SetValue m_setValue{nullptr};
+//};
+
+#define WIL_DEFINE_DP(baseClass, type, name) \


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?

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

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);

citelao · 2024-09-06T16:41:57Z

include/wil/cppwinrt_authoring.h

+#define WIL_DEFINE_DP(baseClass, type, name) \
+    static wil::details::Xaml_DependencyProperty name##Property() \
+    { \
+        static wil::details::Xaml_DependencyProperty s_##name##Property = \


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

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).

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?

Using function-local statics dramatically simplifies this code.

What should we do here?

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.

citelao · 2024-09-06T16:44:48Z

include/wil/cppwinrt_authoring.h

+    { \
+        SetValue(name##Property(), winrt::box_value(value)); \
+    } \
+    static void Ensure##name##Property() \


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?

asklar · 2024-09-06T16:57:57Z

include/wil/cppwinrt_authoring.h

+        const DefaultValueType& defaultValue,
+        const Xaml_PropertyChangedCallback& propertyChangedCallback = nullptr)
+    {
+        // TODO: assert T and PropertyType are compatible


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

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

asklar · 2024-09-06T17:02:09Z

include/wil/cppwinrt_authoring.h

+//    wil::details::Xaml_DependencyObject_SetValue m_setValue{nullptr};
+//};
+
+#define WIL_DEFINE_DP(baseClass, type, name) \


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)

dunhor · 2024-09-06T17:05:14Z

include/wil/cppwinrt_authoring.h

+//    wil::details::Xaml_DependencyObject_SetValue m_setValue{nullptr};
+//};
+
+#define WIL_DEFINE_DP(baseClass, type, name) \


Suggested change

#define WIL_DEFINE_DP(baseClass, type, name) \

#define WIL_DEFINE_XAML_DP(baseClass, type, name) \

Better namespacing

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

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

asklar · 2024-09-06T17:15:26Z

tests/CppWinRTAuthoringTests.cpp

+        // 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.


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?

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

dmachaj · 2024-09-06T17:13:33Z

include/wil/cppwinrt_authoring.h

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


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.

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

dmachaj · 2024-09-06T17:14:30Z

include/wil/cppwinrt_authoring.h

+    {
+        return Xaml_DependencyProperty::Register(
+            propertyNameString,
+            winrt::template xaml_typename<PropertyType>(),


Where is the definition of xaml_typename coming from?

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

dmachaj · 2024-09-06T17:20:08Z

include/wil/cppwinrt_authoring.h

+    { \
+        return winrt::unbox_value<type>(GetValue(name##Property())); \
+    } \
+    void name(type value) const \


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.

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.

dmachaj · 2024-09-06T17:21:47Z

include/wil/cppwinrt_authoring.h

+    } \
+    auto name() const \
+    { \
+        return winrt::unbox_value<type>(GetValue(name##Property())); \


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.

dmachaj · 2024-09-06T17:23:46Z

include/wil/cppwinrt_authoring.h

+    { \
+        SetValue(name##Property(), winrt::box_value(value)); \
+    } \
+    static void Ensure##name##Property() \


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.

dmachaj · 2024-09-06T17:24:26Z

include/wil/cppwinrt_authoring.h

+        name##Property(); \
+    }
+
+#define WIL_DEFINE_DP_WITH_DEFAULT_VALUE_AND_CALLBACK(baseClass, type, name, defaultValue, propertyChangedCallback) \


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

From XAML's point of view, yes.

dmachaj · 2024-09-06T17:25:49Z

tests/CppWinRTAuthoringTests.cpp

 #include <winrt/Windows.UI.Xaml.Data.h>
 #include <winrt/Windows.UI.Xaml.Input.h>
+#include <winrt/Windows.UI.Xaml.Interop.h>


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.

dmachaj · 2024-09-06T17:27:07Z

tests/CppWinRTAuthoringTests.cpp

+{
+    // Throws if not ensured (?)
+    auto obj = my_dependency_properties{};
+    // REQUIRE_FAILFAST_MSG(E_NOTIMPL, [&obj] { obj.MyProperty(); });


Are these intended to get uncommented later?

dmachaj · 2024-09-06T17:27:52Z

tests/CppWinRTAuthoringTests.cpp

+    my_dependency_properties::EnsureMyStringPropertyProperty();
+
+    // Now it should work
+    obj.MyProperty(42);


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?

when you register the property you give it its default value

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).

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

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

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).

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.

dmachaj · 2024-09-06T17:30:26Z

include/wil/cppwinrt_authoring.h

+}
+
+//template <typename T>
+//struct single_threaded_dependency_property


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

sylveon · 2024-09-06T19:55:24Z

include/wil/cppwinrt_authoring.h

+#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&)>;


These seem unused. Also why the type erasure?

sylveon · 2024-09-06T20:00:21Z

include/wil/cppwinrt_authoring.h

+            propertyNameString,
+            winrt::template xaml_typename<PropertyType>(),
+            winrt::template xaml_typename<OwnerType>(),
+            Xaml_PropertyMetadata{winrt::box_value(defaultValue), propertyChangedCallback});


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

sylveon · 2024-09-06T20:03:05Z

include/wil/cppwinrt_authoring.h

+        name##Property(); \
+    }
+
+#define WIL_DEFINE_DP_WITH_DEFAULT_VALUE_AND_CALLBACK(baseClass, type, name, defaultValue, propertyChangedCallback) \


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.

sylveon · 2024-09-06T20:03:56Z

include/wil/cppwinrt_authoring.h

+    static wil::details::Xaml_DependencyProperty name##Property() \
+    { \
+        static wil::details::Xaml_DependencyProperty s_##name##Property = \
+            wil::details::register_dependency_property<type, baseClass>(L"" #name); \


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)

sylveon · 2024-09-06T20:10:42Z

Suggestion: detect the IDL compiler in the header using #ifdef __midl and provide the same macros but for IDL. Or make a separate header for that.

citelao added 4 commits September 6, 2024 09:46

compilable stub

661e980

local static?

3cb550f

support default value too

f454113

support strings

8124844

citelao commented Sep 6, 2024

View reviewed changes

asklar reviewed Sep 6, 2024

View reviewed changes

dunhor reviewed Sep 6, 2024

View reviewed changes

asklar reviewed Sep 6, 2024

View reviewed changes

dmachaj reviewed Sep 6, 2024

View reviewed changes

sylveon reviewed Sep 6, 2024

View reviewed changes

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

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

Are you sure you want to change the base?

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

Conversation

citelao commented Sep 6, 2024

What changed?

How tested?

citelao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asklar Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sylveon commented Sep 6, 2024

asklar Sep 6, 2024 •

edited

Loading