From: Vincent C. <vin...@gm...> - 2010-10-10 17:06:19
|
Hi. I will try to port gstpbutilsinstallplugins [1] to Ruby/Gst (Gst::InstallPlugins module) I see that enum constants are (usually?) defined twice by G_DEF_CLASS and G_DEF_CONSTANTS. i.e. : GST_TYPE_MESSAGE_TYPE in rgst-message.c (constantants are defined under Gst::Message::Type and under Gst::Message) Should I do the same ? The gst_install_plugins_async function take a gchar** as the first argument (NULL-terminated array of strings). I was thinking of define Gst::InstallPlugins.async(details, context=nil){block} with details a ruby array. Is there a common way (macro) to convert a ruby array to If not should I define a macro ? In rbgst-install-plugins.c ? In rbgst-install-plugins.h ? In a glib file ? [1]http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-base-libs/html/gst-plugins-base-libs-gstpbutilsinstallplugins.html -- Vincent Carmona |
From: Vincent C. <vin...@gm...> - 2010-10-10 23:23:48
|
I cannot found 2010/10/10 Vincent Carmona <vin...@gm...>: > Hi. > The gst_install_plugins_async function take a gchar** as the first > argument (NULL-terminated array of strings). > I was thinking of define Gst::InstallPlugins.async(details, > context=nil){block} with details a ruby array. > Is there a common way (macro) to convert a ruby array to > If not should I define a macro ? In rbgst-install-plugins.c ? In > rbgst-install-plugins.h ? In a glib file ? > For now I have implemented the conversion directly in the c function. -- Vincent Carmona |
From: Vincent C. <vin...@gm...> - 2010-10-10 23:35:03
|
Sorry about the previous mail. I hit the wrong key! I cannot found a example of G_DEF_CLASS2. Are they no implementations of it ? I want to wrap GstInstallPluginsContext. gst_install_plugins_context_free [1] must be called when GC must be collect the object. [1]http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-base-libs/html/gst-plugins-base-libs-gstpbutilsinstallplugins.html I do not think that rb_cGstInstallPluginsContext=G_DEF_CLASS2(GST_TYPE_INSTALL_PLUGINS_CONTEXT, "Context", mGstInstallPlugins, 0, gst_install_plugins_context_free); is the correct way. It should also free the DATA. I do not know how to initalize Gst::InstallPlugins::Context new objects using gobjet. How work G_INITIALIZE ? I think I miss a lot of background on gobject. 2010/10/11 Vincent Carmona <vin...@gm...>: > I cannot found > > 2010/10/10 Vincent Carmona <vin...@gm...>: >> Hi. > >> The gst_install_plugins_async function take a gchar** as the first >> argument (NULL-terminated array of strings). >> I was thinking of define Gst::InstallPlugins.async(details, >> context=nil){block} with details a ruby array. >> Is there a common way (macro) to convert a ruby array to >> If not should I define a macro ? In rbgst-install-plugins.c ? In >> rbgst-install-plugins.h ? In a glib file ? >> > > For now I have implemented the conversion directly in the c function. > > > > -- > Vincent Carmona > -- Vincent Carmona |
From: Kouhei S. <ko...@co...> - 2010-10-12 01:18:06
|
Hi, In <AANLkTin7Ueh8E-wRjpRcDyp=8x6...@ma...> "[ruby-gnome2-devel-en] gstpbutilsinstallplugins" on Sun, 10 Oct 2010 19:06:12 +0200, Vincent Carmona <vin...@gm...> wrote: > I see that enum constants are (usually?) defined twice by G_DEF_CLASS > and G_DEF_CONSTANTS. > i.e. : GST_TYPE_MESSAGE_TYPE in rgst-message.c (constantants are > defined under Gst::Message::Type and under Gst::Message) > Should I do the same ? G_DEF_CONSTANTS is for convenience. If enum constants are used many times, we will provide shortcuts by G_DEF_CONSTANTS. Otherwise we don't provide shotcuts. > The gst_install_plugins_async function take a gchar** as the first > argument (NULL-terminated array of strings). > I was thinking of define Gst::InstallPlugins.async(details, > context=nil){block} with details a ruby array. > Is there a common way (macro) to convert a ruby array to > If not should I define a macro ? In rbgst-install-plugins.c ? In > rbgst-install-plugins.h ? In a glib file ? Thera is no macro for it. We should define it rbgutil.c and export it by rbgtuil.h. Thanks, -- kou |
From: Kouhei S. <ko...@co...> - 2010-10-12 01:21:18
|
Hi, In <AANLkTinBowP+asNsbxqLr5GT=4Wi...@ma...> "Re: [ruby-gnome2-devel-en] gstpbutilsinstallplugins" on Mon, 11 Oct 2010 01:34:55 +0200, Vincent Carmona <vin...@gm...> wrote: > I cannot found a example of G_DEF_CLASS2. > Are they no implementations of it ? > I want to wrap GstInstallPluginsContext. Please use G_DEF_CLASS_WITH_GC_FUNC instead of G_DEF_CLASS2. G_DEF_CLASS2 is deprecated. You can find some examples by grepping G_DEF_CLASS_WITH_GC_FUNC at glib2/ or gtk2/. Thanks, -- kou |
From: Vincent C. <vin...@gm...> - 2010-10-15 18:25:37
Attachments:
gdb-ruby.txt
gst-install-plugins.patch
|
Hi. Here is a patch for a review. I hope my code is not to ugly. It implementes all gstpbutilsinstallplugins functions. As indicated in comments, the Gst::InstallPlugins::Context#set_id triggers a segfault. It may have something to do with boxed objects. I need some help with this point. To reproduce the segfault : require 'gst' c=Gst::InstallPlugins::Context.new c.set_xid(42) Segfault happens while exicting ruby. I have also implemented some functions of gstpbutilsmissingplugins needed to use Gst::InstallPlugins module. In future should I provide 2 patches instead of one? -- Vincent Carmona |
From: Vincent C. <vin...@gm...> - 2010-10-17 15:44:38
|
2010/10/17 Kouhei Sutou <ko...@co...>: > * Don't declare variables at the middle of block. > e.g.: > + length=RARRAY(details)->len; > + str=RARRAY(details)->ptr; > + gchar *carray[length+1]; <- HERE > I declare this variable here because length is not set before. I will take account of your other comments and send new patches to this list. -- Vincent Carmona |
From: Kouhei S. <ko...@co...> - 2010-10-21 12:10:00
|
Hi, In <AANLkTi=AsT...@ma...> "Re: [ruby-gnome2-devel-en] gstpbutilsinstallplugins" on Sun, 17 Oct 2010 17:44:32 +0200, Vincent Carmona <vin...@gm...> wrote: > 2010/10/17 Kouhei Sutou <ko...@co...>: >> * Don't declare variables at the middle of block. >> e.g.: >> + length=RARRAY(details)->len; >> + str=RARRAY(details)->ptr; >> + gchar *carray[length+1]; <- HERE >> > > I declare this variable here because length is not set before. You can use ALLOCA_N() for dynamic memory allocation. Thanks, -- kou |
From: Vincent C. <vin...@gm...> - 2010-10-21 15:19:19
|
2010/10/21 Kouhei Sutou <ko...@co...>: > Hi, > > In <AANLkTi=AsT...@ma...> > "Re: [ruby-gnome2-devel-en] gstpbutilsinstallplugins" on Sun, 17 Oct 2010 17:44:32 +0200, > Vincent Carmona <vin...@gm...> wrote: > >> 2010/10/17 Kouhei Sutou <ko...@co...>: >>> * Don't declare variables at the middle of block. >>> e.g.: >>> + length=RARRAY(details)->len; >>> + str=RARRAY(details)->ptr; >>> + gchar *carray[length+1]; <- HERE >>> >> >> I declare this variable here because length is not set before. > > You can use ALLOCA_N() for dynamic memory allocation. > > Thanks, > -- > kou > Ok. I will take a look at this macro. Thanks for the info. -- Vincent Carmona |
From: Vincent C. <vin...@gm...> - 2010-10-23 18:38:43
Attachments:
gst-install-plugins.patch
gst-missing-plugins.patch
|
Hi. I joined 2 pathes this time. gst-missing-plugins is to be applyed before gst-install-plugins. gst-missing-plugins handles missing plugin Gst::MessageElement . gst-install-plugins implements gstpbutilsinstallplugins. Thanks for your past and future reviews. 2010/10/17 Kouhei Sutou <ko...@co...>: > * Don't define XXX2() function XXX2() name is too bad name. > * Use G_DEF_SETTERS(); for define XXX= method for set_XXX. > * ... G_DEF_SETTERS needs a class as a argument. I do not see how it can define set_xid and xid= for Gst::InstallPlugins::Context class! I define context_set_xid and context_set_xid2 functions for these setters. How should I rename it if we keep these functions? > We prefer to small patches with test cases instead of a > large patch. Could you split your patch with small path and > add tests for it? test/test_*.rb will help you. How can I run tests ? make test does not work. I had a test for Gst::InstallPlugins::Context#new method and Gst::MessageElement#missing_plugin? There is currently no ruby ways to create a missing plugin Gst::MessageElement so I cannot test other methods. I do not think I should test Gst::InstallPlugins functions (). Am I wrong ? Should I try to define RARRAY2CSTRARRAY(VALUE, ??) macro or is it a too specific to be reused ? (cf : comment in the patch) -- Vincent Carmona |
From: Vincent C. <vin...@gm...> - 2010-11-12 17:09:46
|
2010/10/23 Vincent Carmona <vin...@gm...>: > Hi. > > I joined 2 pathes this time. > gst-missing-plugins is to be applyed before gst-install-plugins. > gst-missing-plugins handles missing plugin Gst::MessageElement . > gst-install-plugins implements gstpbutilsinstallplugins. > > Thanks for your past and future reviews. > > 2010/10/17 Kouhei Sutou <ko...@co...>: >> * Don't define XXX2() function XXX2() name is too bad name. >> * Use G_DEF_SETTERS(); for define XXX= method for set_XXX. >> * ... > > G_DEF_SETTERS needs a class as a argument. I do not see how it can > define set_xid and xid= for Gst::InstallPlugins::Context class! > I define context_set_xid and context_set_xid2 functions for these > setters. How should I rename it if we keep these functions? > >> We prefer to small patches with test cases instead of a >> large patch. Could you split your patch with small path and >> add tests for it? test/test_*.rb will help you. > > How can I run tests ? make test does not work. > I had a test for Gst::InstallPlugins::Context#new method and > Gst::MessageElement#missing_plugin? > There is currently no ruby ways to create a missing plugin > Gst::MessageElement so I cannot test other methods. > I do not think I should test Gst::InstallPlugins functions (). Am I wrong ? > > Should I try to define RARRAY2CSTRARRAY(VALUE, ??) macro or is it a > too specific to be reused ? (cf : comment in the patch) > > -- > Vincent Carmona > Hi team. I write this mail just to remind you this thread. If you do not have time to help me implement gst-missing-plugins I will wait. -- Vincent Carmona |
From: Vincent C. <vin...@gm...> - 2010-11-14 16:02:19
Attachments:
gst-install-plugins2.patch
gst-missing-plugins2.patch
|
2010/11/13 Kouhei Sutou <ko...@co...>: > Hi, > > Sorry for my late response... > # We need more reviewers... I totally understand that you have a life beside ruby-gnome2. > In <AANLkTi=QAZ...@ma...> > "Re: [ruby-gnome2-devel-en] gstpbutilsinstallplugins" on Sat, 23 Oct 2010 20:38:36 +0200, > Vincent Carmona <vin...@gm...> wrote: > >> I joined 2 pathes this time. >> gst-missing-plugins is to be applyed before gst-install-plugins. >> gst-missing-plugins handles missing plugin Gst::MessageElement . >> gst-install-plugins implements gstpbutilsinstallplugins. > > OK. Can we process gst-missing-plugins before > gst-install-plugins? I want to process gst-install-plugins > after gst-missing-plugins is done. I agree that gst-missing-plugin should be processed before gst-install-plugins but all your remarks are based on gst-install-plugins. >> 2010/10/17 Kouhei Sutou <ko...@co...>: >>> * Don't define XXX2() function XXX2() name is too bad name. >>> * Use G_DEF_SETTERS(); for define XXX= method for set_XXX. >>> * ... >> >> G_DEF_SETTERS needs a class as a argument. I do not see how it can >> define set_xid and xid= for Gst::InstallPlugins::Context class! >> I define context_set_xid and context_set_xid2 functions for these >> setters. How should I rename it if we keep these functions? > > What about this? > > - static VALUE > - context_set_xid2(VALUE self, VALUE xid) > - { > - GstInstallPluginsContext *context; > - > - context = (GstInstallPluginsContext *)RVAL2GOBJ(self); > - gst_install_plugins_context_set_xid(context, NUM2INT(xid));/* segfault on ruby exit */ > - return xid; > - } > > - rb_define_method(rb_cGstInstallPluginsContext, "xid=", > - context_set_xid2, 1); > > + G_DEF_SETTERS(rb_cGstInstallPluginsContext); My bad. I should have take a look at the code to see how G_DEF_SETTERS is used. >> I had a test for Gst::InstallPlugins::Context#new method and >> Gst::MessageElement#missing_plugin? >> There is currently no ruby ways to create a missing plugin >> Gst::MessageElement so I cannot test other methods. >> I do not think I should test Gst::InstallPlugins functions (). Am I wrong ? > > Can we use existing Gst::MessageElement? But talking about > Gst::MessageElement is the next step. I think we should talk about it first and then review gst-intall-plugin. Some Gst::MessageElement are missing_plugins some are not. The existing Gst::MessageElement#new methods does not permit to create a missing_plugins object. > I think that we need more tests for added > methods. e.g. Gst::InstallPlugins::return#get_name. It will > not work well. I add a test for this method. >> Should I try to define RARRAY2CSTRARRAY(VALUE, ??) macro or is it a >> too specific to be reused ? (cf : comment in the patch) > > Please define it in the same file for now. I am not sure how ALLOC_N works and how it frees the memory. I do not see how to write this macro (RARRAY2CSTRARRAY). The comment in diff file show how I try to return a char** (same as ALLOC_N) but I failed to see how a local pointer (carray) can be returned. I try to pass a pointer as a second argument but an uninitialized pointer passed to a function does not seem a very good way. Using ALLOC_N in the async function and then calling RARRAY2CSTRARRAY will take away the advantages of writting a macro. The way of wrtting this macro eludes me Sorry but my C knowledge is very thin. > > Other comments: > Other comments have been taken care of. Hope that I have eradicated all style errors. -- Vincent Carmona |
From: Kouhei S. <ko...@co...> - 2010-11-16 22:26:37
|
Hi, In <AANLkTinLeO=JSK...@ma...> "Re: [ruby-gnome2-devel-en] gstpbutilsinstallplugins" on Sun, 14 Nov 2010 17:02:08 +0100, Vincent Carmona <vin...@gm...> wrote: >> Sorry for my late response... >> # We need more reviewers... > I totally understand that you have a life beside ruby-gnome2. Thanks! >> In <AANLkTi=QAZ...@ma...> >> "Re: [ruby-gnome2-devel-en] gstpbutilsinstallplugins" on Sat, 23 Oct 2010 20:38:36 +0200, >> Vincent Carmona <vin...@gm...> wrote: >> >>> I joined 2 pathes this time. >>> gst-missing-plugins is to be applyed before gst-install-plugins. >>> gst-missing-plugins handles missing plugin Gst::MessageElement . >>> gst-install-plugins implements gstpbutilsinstallplugins. >> >> OK. Can we process gst-missing-plugins before >> gst-install-plugins? I want to process gst-install-plugins >> after gst-missing-plugins is done. > > I agree that gst-missing-plugin should be processed before > gst-install-plugins but all your remarks are based on > gst-install-plugins. Ahh!!! Sorry... It's my mistake... I'll re-review your patch again later... Thanks, -- kou |
From: Vincent C. <vin...@gm...> - 2010-11-17 15:21:43
|
2010/11/16 Kouhei Sutou <ko...@co...>: > Hi, > > In <AANLkTinLeO=JSK...@ma...> > "Re: [ruby-gnome2-devel-en] gstpbutilsinstallplugins" on Sun, 14 Nov 2010 17:02:08 +0100, > Vincent Carmona <vin...@gm...> wrote: > >>> Sorry for my late response... >>> # We need more reviewers... >> I totally understand that you have a life beside ruby-gnome2. > > Thanks! > You are welcome. >>> In <AANLkTi=QAZ...@ma...> >>> "Re: [ruby-gnome2-devel-en] gstpbutilsinstallplugins" on Sat, 23 Oct 2010 20:38:36 +0200, >>> Vincent Carmona <vin...@gm...> wrote: >>> >>>> I joined 2 pathes this time. >>>> gst-missing-plugins is to be applyed before gst-install-plugins. >>>> gst-missing-plugins handles missing plugin Gst::MessageElement . >>>> gst-install-plugins implements gstpbutilsinstallplugins. >>> >>> OK. Can we process gst-missing-plugins before >>> gst-install-plugins? I want to process gst-install-plugins >>> after gst-missing-plugins is done. >> >> I agree that gst-missing-plugin should be processed before >> gst-install-plugins but all your remarks are based on >> gst-install-plugins. > > Ahh!!! > Sorry... It's my mistake... > I'll re-review your patch again later... > Thanks for your time. > Thanks, > -- > kou > > ------------------------------------------------------------------------------ > Beautiful is writing same markup. Internet Explorer 9 supports > standards for HTML5, CSS3, SVG 1.1, ECMAScript5, and DOM L2 & L3. > Spend less time writing and rewriting code and more time creating great > experiences on the web. Be a part of the beta today > http://p.sf.net/sfu/msIE9-sfdev2dev > _______________________________________________ > ruby-gnome2-devel-en mailing list > rub...@li... > https://lists.sourceforge.net/lists/listinfo/ruby-gnome2-devel-en > -- Vincent Carmona |
From: Kouhei S. <ko...@co...> - 2010-11-19 14:13:12
|
Hi, In <AANLkTinLeO=JSK...@ma...> "Re: [ruby-gnome2-devel-en] gstpbutilsinstallplugins" on Sun, 14 Nov 2010 17:02:08 +0100, Vincent Carmona <vin...@gm...> wrote: >>> I joined 2 pathes this time. >>> gst-missing-plugins is to be applyed before gst-install-plugins. >>> gst-missing-plugins handles missing plugin Gst::MessageElement . >>> gst-install-plugins implements gstpbutilsinstallplugins. >> >> OK. Can we process gst-missing-plugins before >> gst-install-plugins? I want to process gst-install-plugins >> after gst-missing-plugins is done. > > I agree that gst-missing-plugin should be processed before > gst-install-plugins but all your remarks are based on > gst-install-plugins. I reviewed gst-missing-plugin. I've changed your patch (many) and commit it. Today, I don't have a power to comment your patch... Sorry... Could you check commited change in trunk? gst-install-plugins review will be done later. (I can't review it in this year at least... Sorry...) I hope that someone helps us... Thanks, -- kou |
From: Vincent C. <vin...@gm...> - 2010-11-23 15:41:05
|
> I reviewed gst-missing-plugin. > I've changed your patch (many) and commit it. > Today, I don't have a power to comment your patch... Sorry... > Could you check commited change in trunk? I am updating my local repository. I will modify my test script, test the changes and write back a mail. > gst-install-plugins review will be done later. (I can't > review it in this year at least... Sorry...) Sorry to hear this but I understand it. > I hope that someone helps us... Me too as I need gst-install-plugin for the(/a ?) next release of my audio player. > Thanks, You are welcome. > -- > kou -- Vincent Carmona |
From: Vincent C. <vin...@gm...> - 2010-11-24 15:37:22
|
Since I update my repo I have a segfautl in the Gst.Init function. #code require 'gst.so' Gst.Init #output /usr/local/lib/site_ruby/1.8/i686-linux/gst.so: /usr/local/lib/site_ruby/1.8/i686-linux/gst.so: undefined symbol: rbgobj_id_children - /usr/local/lib/site_ruby/1.8/i686-linux/gst.so (LoadError) -- Vincent Carmona |
From: Kouhei S. <ko...@co...> - 2010-11-13 04:52:34
|
Hi, Sorry for my late response... # We need more reviewers... In <AANLkTi=QAZ...@ma...> "Re: [ruby-gnome2-devel-en] gstpbutilsinstallplugins" on Sat, 23 Oct 2010 20:38:36 +0200, Vincent Carmona <vin...@gm...> wrote: > I joined 2 pathes this time. > gst-missing-plugins is to be applyed before gst-install-plugins. > gst-missing-plugins handles missing plugin Gst::MessageElement . > gst-install-plugins implements gstpbutilsinstallplugins. OK. Can we process gst-missing-plugins before gst-install-plugins? I want to process gst-install-plugins after gst-missing-plugins is done. > 2010/10/17 Kouhei Sutou <ko...@co...>: >> * Don't define XXX2() function XXX2() name is too bad name. >> * Use G_DEF_SETTERS(); for define XXX= method for set_XXX. >> * ... > > G_DEF_SETTERS needs a class as a argument. I do not see how it can > define set_xid and xid= for Gst::InstallPlugins::Context class! > I define context_set_xid and context_set_xid2 functions for these > setters. How should I rename it if we keep these functions? What about this? - static VALUE - context_set_xid2(VALUE self, VALUE xid) - { - GstInstallPluginsContext *context; - - context = (GstInstallPluginsContext *)RVAL2GOBJ(self); - gst_install_plugins_context_set_xid(context, NUM2INT(xid));/* segfault on ruby exit */ - return xid; - } - rb_define_method(rb_cGstInstallPluginsContext, "xid=", - context_set_xid2, 1); + G_DEF_SETTERS(rb_cGstInstallPluginsContext); >> We prefer to small patches with test cases instead of a >> large patch. Could you split your patch with small path and >> add tests for it? test/test_*.rb will help you. > > How can I run tests ? make test does not work. Please try "ruby test/run-test.rb". > I had a test for Gst::InstallPlugins::Context#new method and > Gst::MessageElement#missing_plugin? > There is currently no ruby ways to create a missing plugin > Gst::MessageElement so I cannot test other methods. > I do not think I should test Gst::InstallPlugins functions (). Am I wrong ? Can we use existing Gst::MessageElement? But talking about Gst::MessageElement is the next step. I think that we need more tests for added methods. e.g. Gst::InstallPlugins::return#get_name. It will not work well. > Should I try to define RARRAY2CSTRARRAY(VALUE, ??) macro or is it a > too specific to be reused ? (cf : comment in the patch) Please define it in the same file for now. Other comments: 1. use CSTR2RVAL() instead of rb_str_new2(). static VALUE return_get_name(VALUE self) { - return rb_str_new2(gst_install_plugins_return_get_name( + return CSTR2RVAL(gst_install_plugins_return_get_name( (GstInstallPluginsReturn)RVAL2GOBJ(self))); } 2. use RVAL2GENUM() instead of RVAL2GOBJ() for GEnum. static VALUE return_get_name(VALUE self) { return rb_str_new2(gst_install_plugins_return_get_name( - (GstInstallPluginsReturn)RVAL2GOBJ(self))); + RVAL2GENUM(self, GST_TYPE_INSTALL_PLUGINS_RETURN)))); } 3. don't add "get_" prefix for no argument getter. See: http://ruby-gnome2.sourceforge.jp/hiki.cgi?Naming+and+Conversion+Rules - rb_define_method(rb_cGstInstallPluginsReturn, "get_name", return_get_name, + rb_define_method(rb_cGstInstallPluginsReturn, "name", return_get_name, 0); 4. don't use deep class hierarchy. (nest level shuld be less than 3.) See: http://ruby-gnome2.sourceforge.jp/hiki.cgi?Naming+and+Conversion+Rules GstInstallPluginsReturn -> Gst::InstallPluginsReturn GstInstallPluginsContext -> Gst::InstallPluginsContext 5. please clean style. (please confirm again.) We need clean style for maintaining it. * indent. for example: static VALUE async(int argc, VALUE * argv, VALUE self) { ... if (!NIL_P(rcontext)){ - if (!RVAL2CBOOL(rb_obj_is_kind_of(rcontext, rb_cGstInstallPluginsContext))) + if (!RVAL2CBOOL(rb_obj_is_kind_of(rcontext, rb_cGstInstallPluginsContext))) ... } * space. for example: static VALUE async(int argc, VALUE * argv, VALUE self) { ... - if (!NIL_P(rcontext)){ + if (!NIL_P(rcontext)) { if (!RVAL2CBOOL(rb_obj_is_kind_of(rcontext, rb_cGstInstallPluginsContext))) ... } static VALUE context_initialize(VALUE self) { - GstInstallPluginsContext * context; + GstInstallPluginsContext *context; ... } Thanks, -- kou |
From: Kouhei S. <ko...@co...> - 2010-10-17 13:14:17
|
Hi, nIn <AAN...@ma...> "Re: [ruby-gnome2-devel-en] gstpbutilsinstallplugins" on Fri, 15 Oct 2010 20:25:29 +0200, Vincent Carmona <vin...@gm...> wrote: > Here is a patch for a review. I hope my code is not to ugly. > > It implementes all gstpbutilsinstallplugins functions. > As indicated in comments, the Gst::InstallPlugins::Context#set_id > triggers a segfault. It may have something to do with boxed objects. I > need some help with this point. > To reproduce the segfault : > > require 'gst' > c=Gst::InstallPlugins::Context.new > c.set_xid(42) > > Segfault happens while exicting ruby. > > I have also implemented some functions of gstpbutilsmissingplugins > needed to use Gst::InstallPlugins module. > In future should I provide 2 patches instead of one? Thanks for your patch. I have some comments to your patch but I'll show just a few comments: * Please use PKGConfig for detecting gstpbutils. * Don't declare variables at the middle of block. e.g.: + length=RARRAY(details)->len; + str=RARRAY(details)->ptr; + gchar *carray[length+1]; <- HERE * Don't use C++ style comment "//". Use "/* */" instead. * Please add a space around "=": context=NULL; -> context = NULL; * Use RARRAY_LEN() and RARRAY_PTR() instead of RARRAY()->len and RARRAY()->ptr. * Don't define XXX2() function XXX2() name is too bad name. * Use G_DEF_SETTERS(); for define XXX= method for set_XXX. * ... We prefer to small patches with test cases instead of a large patch. Could you split your patch with small path and add tests for it? test/test_*.rb will help you. Thanks, -- kou |
From: Vincent C. <vin...@gm...> - 2010-10-12 15:03:38
|
2010/10/12 Kouhei Sutou <ko...@co...>: Thanks for your replies. > Hi, > > In <AANLkTinBowP+asNsbxqLr5GT=4Wi...@ma...> > "Re: [ruby-gnome2-devel-en] gstpbutilsinstallplugins" on Mon, 11 Oct 2010 01:34:55 +0200, > Vincent Carmona <vin...@gm...> wrote: > >> I cannot found a example of G_DEF_CLASS2. >> Are they no implementations of it ? >> I want to wrap GstInstallPluginsContext. > > Please use G_DEF_CLASS_WITH_GC_FUNC instead of > G_DEF_CLASS2. G_DEF_CLASS2 is deprecated. You can find some > examples by grepping G_DEF_CLASS_WITH_GC_FUNC at glib2/ or > gtk2/. > It explains why a grep on G_DEF_CLASS2 failled. -- Vincent Carmona |