GNOME Bugzilla – Bug 539772
gst_pad_template_new() does more than call g_object_new()
Last modified: 2008-08-01 10:06:43 UTC
gst_pad_template_new() does a few more things than just calling g_object_new(). I also noticed that its parameters don't correspond to the construction properties used in the call to g_object_new(). This is the code: GstPadTemplate * gst_pad_template_new (const gchar * name_template, GstPadDirection direction, GstPadPresence presence, GstCaps * caps) { GstPadTemplate *new; g_return_val_if_fail (name_template != NULL, NULL); g_return_val_if_fail (caps != NULL, NULL); g_return_val_if_fail (direction == GST_PAD_SRC || direction == GST_PAD_SINK, NULL); g_return_val_if_fail (presence == GST_PAD_ALWAYS || presence == GST_PAD_SOMETIMES || presence == GST_PAD_REQUEST, NULL); if (!name_is_valid (name_template, presence)) { gst_caps_unref (caps); return NULL; } new = g_object_new (gst_pad_template_get_type (), "name", name_template, NULL); GST_PAD_TEMPLATE_NAME_TEMPLATE (new) = g_strdup (name_template); GST_PAD_TEMPLATE_DIRECTION (new) = direction; GST_PAD_TEMPLATE_PRESENCE (new) = presence; GST_PAD_TEMPLATE_CAPS (new) = caps; return new; } As in bug #539108, I suppose the extra code would be better placed in an _init() function also. Thanks.
Part of the reason why this function doesn't simply call g_object_new() is bug #539886, i.e. GObject not allowing property getters/setters and object instanciation to fail. Also GstPadTemplate doesn't even implement any GObject properties so g_object_new() call is wrong ;) In this case I guess we can implement all except the name validity checking as GObject properties and probably should... patch follows later :)
Created attachment 113313 [details] [review] padtemplate-properties.diff This patch adds name-template, direction, presence and caps properties to GstPadTemplate. As GstPadTemplate are meant to be immutable these properties are construct-only, is this fine for you? (Is there a way to have properties that are writable at construction time and readonly afterwards?)
Created attachment 113334 [details] [review] Patch adding 'name-template', 'direction', 'presence' and 'caps' construct-only properties to GstPadTemplate Same patch as above basically, but with additional gtk-doc blurbs (for the Since:) and a property getter. Before this can be applied we need to remove the --disable-enumtypes configure option though.
Created attachment 113336 [details] [review] Patch adding 'name-template', 'direction', 'presence' and 'caps' construct-only properties to GstPadTemplate (updated) Same as above, just without the copy'n'paste error in the gtk-doc blurbs.
(In reply to comment #2) > This patch adds name-template, direction, presence and caps properties to > GstPadTemplate. As GstPadTemplate are meant to be immutable these properties > are construct-only, is this fine for you? > > (Is there a way to have properties that are writable at construction time and > readonly afterwards?) Do you mean me? If so this solution is just fine for c++ bindings (I've just tested and results are consistent both in C and C++). As you say, these will will not be readily modifiable by programmer unless they can somehow get the properties with property system and change them that way, but I don't see why they would want to do that. If the changes are fine with you they are also fine here. Thanks.
Thanks for the fix. Will this be committed soon?
The only problem with this is, that (to quote Tim) "Before this can be applied we need to remove the --disable-enumtypes configure option though." This should be discussed first, other than that I see no reason not to commit it.
Thanks. What does that even do? ./configure --help just says " --disable-enumtypes disable enum types "
(In reply to comment #8) > Thanks. What does that even do? ./configure --help just says > " > --disable-enumtypes disable enum types > " It doesn't register GEnums for the different enum types in core. No idea why this is very useful but on embedded systems it maybe makes a difference *shrug* :)
+1 for dropping -disable-enumtypes. I think it just break building things and fixing that is not worth it.
This patch can now be committed as disabling of the enum types is not possible anymore in CVS.
Committed: 2008-08-01 Tim-Philipp Müller <tim.muller at collabora co uk> * gst/gstpadtemplate.c: (gst_pad_template_class_init), (gst_static_pad_template_get), (gst_pad_template_new), (gst_pad_template_pad_created), (gst_pad_template_set_property), (gst_pad_template_get_property): Add "name-template", "direction", "presence" and "caps" properties, so that gst_pad_template_new() is just a thin wrapper around g_object_new(), which is better for bindings. (Fixes: #539772)