ObjectBuilder: add property_if(), property_if_some(), property_from_iter() ... #1377

fengalin · 2024-04-29T17:05:19Z

... & property_if_not_empty()

This commit adds an ObjectBuilderPropertySetter trait which implements
functions to improve usability when setting properties.

property_if() allows setting a property from an Option only if a given
predicate evaluates to true. Otherwise, the Object's default value or
any previously set value is kept.

property_if_some() allows setting a property from an Option only if the
Option is Some. Otherwise, the Object's default value or any previously
set value is kept.

For instance, assuming an Args struct which was built from the application's
CLI arguments:

let args = Args {
    name: Some("test"),
    anwser: None,
};

... without this change, we need to write something like this:

let mut builder = Object::builder::<SimpleObject>();

if let Some(ref name) = args.name {
    builder = builder.property("name", name)
}

if let Some(ref answer) = args.answer {
    builder = builder.property("answer", &args.answer)
}

let obj = builder.build();

With property_if_some(), we can write:

let obj = Object::builder::<SimpleObject>()
    .property_if_some("name", &args.name)
    .property_if_some("answer", &args.answer)
    .build();

property_from_iter() allows building a collection-like Value property
from a collection of items. In GStreamer this allows things like:

let src = gst::ElementFactory::make("webrtcsrc")
    .property_from_iter::<gst::Array>("audio-codecs", ["L24", "OPUS"])
    .build()
    .unwrap();

property_if_not_empty() allows building a collection-like Value property
from a collection of items but does nothing if the provided collection if empty,
thus keeping the default value of the property, if any. In GStreamer this allows
things like:

    .property_if_not_empty::<gst::Array>("audio-codecs", &args.audio_codecs)
    .build()
    .unwrap();

bilelmoussaoui · 2024-04-29T17:51:49Z

Haven't had the time to review the PR yet but at some point there was a discussion about adding the possibility to set a property only if a condition is truthful. The use case was setting a property only if a certain gstreamer version is available iirc

cc @sdroege, do you remember where was that needed? or maybe it was someone else.

fengalin · 2024-04-29T18:44:46Z

@bilelmoussaoui: indeed that would be useful. Something like Option::take_if in reverse? I can add that too yes.

fengalin · 2024-04-29T18:49:01Z

Noteworthy annoyance with these changes is that we need to use glib::prelude::*; (or gst::prelude::*;) in some places were it wasn't before.

felinira · 2024-04-29T18:49:33Z

I propose renaming property_if_any() to property_if_not_empty() to clarify its purpose and to be more similar to the common is_empty() collection method.

bilelmoussaoui · 2024-04-29T18:50:09Z

Noteworthy annoyance with these changes is that we need to use glib::prelude::*; (or gst::prelude::*;) in some places were it wasn't before.

That is ok, I already migrated a bunch of files to use prelude instead of including traits manually. Going forward in that direction is good from my point of view

bilelmoussaoui · 2024-04-29T18:50:35Z

I propose renaming property_if_any() to property_if_not_empty() to clarify its purpose and to be more similar to the common is_empty() collection method.

I would name it property_if_some instead.

fengalin · 2024-04-29T18:53:30Z

I propose renaming property_if_any() to property_if_not_empty() to clarify its purpose and to be more similar to the common is_empty() collection method

Makes sense.

I would name it property_if_some instead.

Already taken by the Option<_> variant :)

sdroege · 2024-04-30T06:34:34Z

glib/src/object.rs

+    }
+}
+
+impl<'a, O: IsA<Object> + IsClass> ObjectBuilderPropertySetter<'a> for ObjectBuilder<'a, O> {


Why is this an extension trait instead of just implementing it directly on ObjectBuilder?

Because you suggested this :): https://gitlab.freedesktop.org/gstreamer/gstreamer-rs/-/merge_requests/1431#note_2392609

I think it's convenient to have a single definition too: I can use this trait for gst::ElementFactory, implement property() and get all the other implementations for free.

I used a similar pattern gst::StructureBuilderPropertySetter which is applicable to gst::StructureBuilder, gst::CapsBuilder, gst_audio::CapsBuilder, gst_video::CapsBuilder, ...

I suggested this because I thought of adding this extension trait in gstreamer-rs, but here it could also be a normal impl block.

If it makes anything easier and you're going to implement the trait on other types too that's fine. (Btw, could you also add this to the appsrc/appsink builders then? I just noticed that they don't have the general property method)

I suggested this because I thought of adding this extension trait in gstreamer-rs, but here it could also be a normal impl block.

As far as this PR is concerned, I can only see one implementation here. @bilelmoussaoui can you think of other builders which would use property() and would benefit from this?

I can only see one implementation here

But more in gstreamer-rs?

And here, I think all the autogenerated builders for specific types could make use of this. I think they all don't have a generic property method right now but that would be useful.

A macro that is exported from the glib crate? Not sure I like adding more macro public API. @bilelmoussaoui ?

The way I see it: due to the Value / SendValue differences, I will handle this separately in gtk-rs & GStreamer.

In gtk-rs: the new methods property_* can be added to the ObjectBuilder impl directly. For the generated Builders, a gir update could add the additional methods on a per-property basis.

In GStreamer, I think I can use the macro I was mentioning in my previous comment. It would be transparent to users since it would only be used in the bindings for the ElementFactory & the Structure-like struct builders, not something like glib::clone! for instance.

Sounds like a plan

I don't see which other types that could use this. I think the only use case would be either gstreamer-rs or gegl-rs(which is not maintained anyways...). So do we really need this to be an extension trait?

We don't. See my comment: #1377 (comment)

glib/src/object.rs

sdroege

Looks good to me otherwise and the method names also make sense. @bilelmoussaoui @felinira do you prefer any different names, or would like to have something else changed?

sdroege · 2024-04-30T06:38:20Z

I propose renaming property_if_any() to property_if_not_empty() to clarify its purpose and to be more similar to the common is_empty() collection method

Makes sense.

Also seems fine to me

…ter() ... ... & property_if_not_empty() This commit adds an `ObjectBuilderPropertySetter` `trait` which implements functions to improve usability when setting properties. **`property_if()`** allows setting a property from an `Option` only if a given `predicate` evaluates to `true`. Otherwise, the `Object`'s default value or any previously set value is kept. **`property_if_some()`** allows setting a property from an `Option` only if the `Option` is `Some`. Otherwise, the `Object`'s default value or any previously set value is kept. For instance, assuming an `Args` `struct` which was built from the application's CLI arguments: ```rust let args = Args { name: Some("test"), anwser: None, }; ``` ... without this change, we need to write something like this: ```rust let mut builder = Object::builder::<SimpleObject>(); if let Some(ref name) = args.name { builder = builder.property("name", name) } if let Some(ref answer) = args.answer { builder = builder.property("answer", &args.answer) } let obj = builder.build(); ``` With `property_if_some()`, we can write: ```rust let obj = Object::builder::<SimpleObject>() .property_if_some("name", &args.name) .property_if_some("answer", &args.answer) .build(); ``` **`property_from_iter()`** allows building a collection-like `Value` property from a collection of items. In GStreamer this allows things like: ```rust let src = gst::ElementFactory::make("webrtcsrc") .property_from_iter::<gst::Array>("audio-codecs", ["L24", "OPUS"]) .build() .unwrap(); ``` **`property_if_not_empty()`** allows building a collection-like `Value` property from a collection of items but does nothing if the provided collection if empty, thus keeping the default value of the property, if any. In GStreamer this allows things like: ```let src = gst::ElementFactory::make("webrtcsrc") .property_if_not_empty::<gst::Array>("audio-codecs", &args.audio_codecs) .build() .unwrap(); ```

bilelmoussaoui

otherwise lgtm, can't comment on the namings because i am bad at that

bilelmoussaoui · 2024-05-06T14:58:25Z

glib/src/value_array.rs

@@ -23,6 +24,25 @@ impl ValueArray {
        unsafe { from_glib_full(gobject_ffi::g_value_array_new(n_prealloced)) }
    }

+    pub fn from_items(values: impl IntoIterator<Item = impl Into<Value> + Send>) -> Self {


Those changes should go to a different commit if possible

bilelmoussaoui · 2024-05-06T15:06:29Z

glib/src/object.rs

+    }
+}
+
+impl<'a, O: IsA<Object> + IsClass> ObjectBuilderPropertySetter<'a> for ObjectBuilder<'a, O> {


I don't see which other types that could use this. I think the only use case would be either gstreamer-rs or gegl-rs(which is not maintained anyways...). So do we really need this to be an extension trait?

fengalin · 2024-05-20T10:38:59Z

Sorry for the lack of progress on this one. fwiw, I'm planning on working on it during the hackfest.

fengalin force-pushed the iterable-value branch from e81405d to 86bbd20 Compare April 29, 2024 17:18

sdroege reviewed Apr 30, 2024

View reviewed changes

glib/src/object.rs Show resolved Hide resolved

sdroege reviewed Apr 30, 2024

View reviewed changes

glib/src/object.rs Show resolved Hide resolved

sdroege reviewed Apr 30, 2024

View reviewed changes

glib/src/object.rs Outdated Show resolved Hide resolved

sdroege reviewed Apr 30, 2024

View reviewed changes

fengalin force-pushed the iterable-value branch from 86bbd20 to 23be9af Compare April 30, 2024 06:55

fengalin changed the title ~~ObjectBuilder: add property_if_some(), property_from_iter() & …~~ ObjectBuilder: add property_if(), property_if_some(), property_from_iter() ... Apr 30, 2024

bilelmoussaoui reviewed May 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ObjectBuilder: add property_if(), property_if_some(), property_from_iter() ... #1377

ObjectBuilder: add property_if(), property_if_some(), property_from_iter() ... #1377

fengalin commented Apr 29, 2024 •

edited

bilelmoussaoui commented Apr 29, 2024

fengalin commented Apr 29, 2024

fengalin commented Apr 29, 2024

felinira commented Apr 29, 2024

bilelmoussaoui commented Apr 29, 2024

bilelmoussaoui commented Apr 29, 2024 •

edited

fengalin commented Apr 29, 2024

sdroege Apr 30, 2024

fengalin Apr 30, 2024 •

edited

sdroege Apr 30, 2024

fengalin Apr 30, 2024

sdroege Apr 30, 2024

sdroege Apr 30, 2024

fengalin May 1, 2024

sdroege May 1, 2024

bilelmoussaoui May 6, 2024

fengalin May 6, 2024

sdroege left a comment

sdroege commented Apr 30, 2024

bilelmoussaoui left a comment

bilelmoussaoui May 6, 2024

fengalin May 6, 2024

bilelmoussaoui May 6, 2024

fengalin commented May 20, 2024

ObjectBuilder: add property_if(), property_if_some(), property_from_iter() ... #1377

Are you sure you want to change the base?

ObjectBuilder: add property_if(), property_if_some(), property_from_iter() ... #1377

Conversation

fengalin commented Apr 29, 2024 • edited

bilelmoussaoui commented Apr 29, 2024

fengalin commented Apr 29, 2024

fengalin commented Apr 29, 2024

felinira commented Apr 29, 2024

bilelmoussaoui commented Apr 29, 2024

bilelmoussaoui commented Apr 29, 2024 • edited

fengalin commented Apr 29, 2024

Choose a reason for hiding this comment

fengalin Apr 30, 2024 • edited

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

sdroege left a comment

Choose a reason for hiding this comment

sdroege commented Apr 30, 2024

bilelmoussaoui left a 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

fengalin commented May 20, 2024

fengalin commented Apr 29, 2024 •

edited

bilelmoussaoui commented Apr 29, 2024 •

edited

fengalin Apr 30, 2024 •

edited