After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 539772 - gst_pad_template_new() does more than call g_object_new()
gst_pad_template_new() does more than call g_object_new()
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal normal
: 0.10.21
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2008-06-23 16:06 UTC by José Alburquerque
Modified: 2008-08-01 10:06 UTC
See Also:
GNOME target: ---
GNOME version: 2.21/2.22


Attachments
padtemplate-properties.diff (4.84 KB, patch)
2008-06-24 07:52 UTC, Sebastian Dröge (slomo)
none Details | Review
Patch adding 'name-template', 'direction', 'presence' and 'caps' construct-only properties to GstPadTemplate (6.35 KB, patch)
2008-06-24 15:04 UTC, Tim-Philipp Müller
none Details | Review
Patch adding 'name-template', 'direction', 'presence' and 'caps' construct-only properties to GstPadTemplate (updated) (6.36 KB, patch)
2008-06-24 15:07 UTC, Tim-Philipp Müller
committed Details | Review

Description José Alburquerque 2008-06-23 16:06:54 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.
Comment 1 Sebastian Dröge (slomo) 2008-06-24 05:59:38 UTC
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 :)
Comment 2 Sebastian Dröge (slomo) 2008-06-24 07:52:12 UTC
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?)
Comment 3 Tim-Philipp Müller 2008-06-24 15:04:45 UTC
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.
Comment 4 Tim-Philipp Müller 2008-06-24 15:07:14 UTC
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.
Comment 5 José Alburquerque 2008-06-24 15:15:09 UTC
(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.
Comment 6 Murray Cumming 2008-07-26 09:49:01 UTC
Thanks for the fix. Will this be committed soon?
Comment 7 Sebastian Dröge (slomo) 2008-07-27 11:13:26 UTC
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.
Comment 8 Murray Cumming 2008-07-27 11:36:51 UTC
Thanks. What does that even do? ./configure --help just says 
"
  --disable-enumtypes     disable enum types
"
Comment 9 Sebastian Dröge (slomo) 2008-07-27 12:05:01 UTC
(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* :)
Comment 10 Stefan Sauer (gstreamer, gtkdoc dev) 2008-07-27 17:08:44 UTC
+1 for dropping -disable-enumtypes. I think it just break building things and fixing that is not worth it.
Comment 11 Sebastian Dröge (slomo) 2008-07-31 15:24:36 UTC
This patch can now be committed as disabling of the enum types is not possible anymore in CVS.
Comment 12 Tim-Philipp Müller 2008-08-01 10:06:43 UTC
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)