Merge pull request #1023 from ranfdev/nullable_props

Add nullable attribute on properties macro

Author: sdroege

glib-macros/src/lib.rs

@@ -872,6 +872,8 @@ pub fn cstr_bytes(item: TokenStream) -> TokenStream {
 /// `PropertySet` and `PropertySetNested` if possible.
 ///
 /// The type `Option<T>` is supported as a property only if `Option<T>` implements `ToValueOptional`.
+/// Optional types also require the `nullable` attribute: without it, the generated setter on the wrapper type
+/// will take `T` instead of `Option<T>`, preventing the user from ever calling the setter with a `None` value.
 /// If your type doesn't support `PropertySet`, you can't use the generated setter, but you can
 /// always define a custom one.
 ///
@@ -913,7 +915,7 @@ pub fn cstr_bytes(item: TokenStream) -> TokenStream {
 ///         numeric_builder: RefCell<u32>,
 ///         #[property(get, set, builder('c'))]
 ///         builder_with_required_param: RefCell<char>,
-///         #[property(get, set)]
+///         #[property(get, set, nullable)]
 ///         optional: RefCell<Option<String>>,
 ///         #[property(get, set)]
 ///         smart_pointer: Rc<RefCell<String>>,

glib-macros/src/properties.rs

@@ -86,6 +86,9 @@ enum PropAttr {
     // Builder(Punctuated(required_params), Optionals(TokenStream))
     Builder(Punctuated<syn::Expr, Token![,]>, TokenStream2),
 
+    // ident
+    Nullable,
+
     // ident [= expr]
     Get(Option<syn::Expr>),
     Set(Option<syn::Expr>),
@@ -151,6 +154,7 @@ impl Parse for PropAttr {
         } else {
             // attributes with only the identifier name
             match &*name_str {
+                "nullable" => PropAttr::Nullable,
                 "get" => PropAttr::Get(None),
                 "set" => PropAttr::Set(None),
                 "readwrite" | "read_only" | "write_only" => {
@@ -171,6 +175,7 @@ impl Parse for PropAttr {
 
 #[derive(Default)]
 struct ReceivedAttrs {
+    nullable: bool,
     get: Option<MaybeCustomFn>,
     set: Option<MaybeCustomFn>,
     override_class: Option<syn::Type>,
@@ -197,6 +202,7 @@ impl Parse for ReceivedAttrs {
 impl ReceivedAttrs {
     fn set_from_attr(&mut self, attr: PropAttr) {
         match attr {
+            PropAttr::Nullable => self.nullable = true,
             PropAttr::Get(some_fn) => self.get = Some(some_fn.into()),
             PropAttr::Set(some_fn) => self.set = Some(some_fn.into()),
             PropAttr::Name(lit) => self.name = Some(lit),
@@ -223,6 +229,7 @@ struct PropDesc {
     name: syn::LitStr,
     override_class: Option<syn::Type>,
     override_interface: Option<syn::Type>,
+    nullable: bool,
     get: Option<MaybeCustomFn>,
     set: Option<MaybeCustomFn>,
     member: Option<syn::Ident>,
@@ -238,6 +245,7 @@ impl PropDesc {
         attrs: ReceivedAttrs,
     ) -> syn::Result<Self> {
         let ReceivedAttrs {
+            nullable,
             get,
             set,
             override_class,
@@ -280,6 +288,7 @@ impl PropDesc {
             name,
             override_class,
             override_interface,
+            nullable,
             get,
             set,
             member,
@@ -514,8 +523,23 @@ fn expand_wrapper_getset_properties(props: &[PropDesc]) -> TokenStream2 {
         let is_construct_only = p.builder_fields.iter().any(|(k, _)| *k == "construct_only");
         let setter = (p.set.is_some() && !is_construct_only).then(|| {
             let ident = format_ident!("set_{}", ident);
-            quote!(pub fn #ident<'a>(&self, value: impl std::borrow::Borrow<<<#ty as #crate_ident::Property>::Value as #crate_ident::HasParamSpec>::SetValue>) {
-                self.set_property_from_value(#name, &::std::convert::From::from(std::borrow::Borrow::borrow(&value)))
+            let target_ty = quote!(<<#ty as #crate_ident::Property>::Value as #crate_ident::HasParamSpec>::SetValue);
+            let set_ty = if p.nullable {
+               quote!(Option<impl std::borrow::Borrow<#target_ty>>)
+            } else {
+               quote!(impl std::borrow::Borrow<#target_ty>)
+            };
+            let upcasted_borrowed_value = if p.nullable {
+                quote!(
+                    value.as_ref().map(|v| std::borrow::Borrow::borrow(v))
+                )
+            } else {
+                quote!(
+                    std::borrow::Borrow::borrow(&value)
+                )
+            };
+            quote!(pub fn #ident<'a>(&self, value: #set_ty) {
+                self.set_property_from_value(#name, &::std::convert::From::from(#upcasted_borrowed_value))
             })
         });
         let span = p.attrs_span;

glib-macros/tests/properties.rs

@@ -139,9 +139,9 @@ mod foo {
             boxed: RefCell<SimpleBoxedString>,
             #[property(get, set, builder(SimpleEnum::One))]
             fenum: RefCell<SimpleEnum>,
-            #[property(get, set)]
+            #[property(get, set, nullable)]
             object: RefCell<Option<glib::Object>>,
-            #[property(get, set)]
+            #[property(get, set, nullable)]
             optional: RefCell<Option<String>>,
             #[property(get, set)]
             smart_pointer: Rc<RefCell<String>>,
@@ -305,11 +305,6 @@ fn props() {
         foo::SimpleBoxedString("".into())
     );
 
-    // optional
-    assert_eq!(myfoo.property::<Option<String>>("optional"), None,);
-
-    myfoo.connect_optional_notify(|_| println!("notified"));
-
     // Test `FooPropertiesExt`
     // getters
     {
@@ -349,9 +344,6 @@ fn props() {
             "setter working".to_string()
         );
 
-        // object subclass
-        myfoo.set_object(glib::BoxedAnyObject::new(""));
-
         // custom
         myfoo.set_fake_field("fake setter");
         assert_eq!(
@@ -374,4 +366,14 @@ fn props() {
         let not_overridden: u32 = myfoo.property("not-overridden");
         assert_eq!(not_overridden, 42);
     }
+
+    // optional
+    myfoo.set_optional(Some("Hello world"));
+    assert_eq!(myfoo.optional(), Some("Hello world".to_string()));
+    myfoo.connect_optional_notify(|_| println!("notified"));
+
+    // object subclass
+    let myobj = glib::BoxedAnyObject::new("");
+    myfoo.set_object(Some(myobj.upcast_ref()));
+    assert_eq!(myfoo.object(), Some(myobj.upcast()))
 }