-
-
Notifications
You must be signed in to change notification settings - Fork 103
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.
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
ObjectBuilder: add property_if(), property_if_some(), property_from_iter() ... #1377
base: master
Are you sure you want to change the base?
Conversation
e81405d
to
86bbd20
Compare
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. |
@bilelmoussaoui: indeed that would be useful. Something like |
Noteworthy annoyance with these changes is that we need to |
I propose renaming |
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 |
I would name it |
Makes sense.
Already taken by the |
} | ||
} | ||
|
||
impl<'a, O: IsA<Object> + IsClass> ObjectBuilderPropertySetter<'a> for ObjectBuilder<'a, O> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this an extension trait instead of just implementing it directly on ObjectBuilder
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
, ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A macro that is exported from the glib crate? Not sure I like adding more macro public API. @bilelmoussaoui ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 methodsproperty_*
can be added to theObjectBuilder
impl
directly. For the generatedBuilder
s, agir
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 theElementFactory
& theStructure
-likestruct
builders, not something likeglib::clone!
for instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a plan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't. See my comment: #1377 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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(); ```
86bbd20
to
23be9af
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
otherwise lgtm, can't comment on the namings because i am bad at that
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those changes should go to a different commit if possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
} | ||
} | ||
|
||
impl<'a, O: IsA<Object> + IsClass> ObjectBuilderPropertySetter<'a> for ObjectBuilder<'a, O> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Sorry for the lack of progress on this one. fwiw, I'm planning on working on it during the hackfest. |
... & property_if_not_empty()
This commit adds an
ObjectBuilderPropertySetter
trait
which implementsfunctions to improve usability when setting properties.
property_if()
allows setting a property from anOption
only if a givenpredicate
evaluates totrue
. Otherwise, theObject
's default value orany previously set value is kept.
property_if_some()
allows setting a property from anOption
only if theOption
isSome
. Otherwise, theObject
's default value or any previouslyset value is kept.
For instance, assuming an
Args
struct
which was built from the application'sCLI arguments:
... without this change, we need to write something like this:
With
property_if_some()
, we can write:property_from_iter()
allows building a collection-likeValue
propertyfrom a collection of items. In GStreamer this allows things like:
property_if_not_empty()
allows building a collection-likeValue
propertyfrom 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: