GNOME Bugzilla – Bug 622664
Vala sample plugin
Last modified: 2011-03-06 16:11:14 UTC
the following patches add a sample vala plugin. valac still needs to be patched to not error on *Interface structs (attachment 164552 [details] [review]) but the more important is here : how to properly write peas_register_types. One could argue that the cast is just a workaround for valac insisting on having exactly a TypeModule (and not a subclass), but it's fine for me.
Created attachment 164554 [details] [review] add single-include headers
Created attachment 164555 [details] [review] add an example vala plugin for peasdemo
Review of attachment 164554 [details] [review]: The patch looks mostly good. A few nitpicks below but it will definitely go in soon. ::: libpeas/Makefile.am @@ +25,3 @@ peas-activatable.h \ + peas-engine.h \ + peas.h nitpick: the position of the \ is not correct @@ +76,3 @@ -include $(INTROSPECTION_MAKEFILE) INTROSPECTION_GIRS = Peas-1.0.gir + INTROSPECTION_SCANNER_ARGS = --c-include=libpeas/peas.h Could you give some information on what this does? A one-liner in the commit message would be enough. ::: libpeasui/Makefile.am @@ +36,3 @@ -include $(INTROSPECTION_MAKEFILE) INTROSPECTION_GIRS = PeasUI-1.0.gir + INTROSPECTION_SCANNER_ARGS = --add-include-path=$(top_builddir)/libpeas --c-include=libpeasui/peas-ui.h Ditto. What does --add-include-path change? didn't it work before?
--c-include is used to put the header name in the gir. Right now, it's only used by vala. The last comment is the same, I just added --c-include to INTROSPECTION_SCANNER_ARGS
sure, but what does --add-include-path=$(top_builddir)/libpeas fix?
ask whoever put it there ;-) Seriously, I just added --c-include to both Makefile.am's : one already had INTROSPECTION_SCANNER_ARGS containing --add-include-path and the other didn't. And if you really want to know, --add-include-path is here so the scanner can find Peas-1.0.gir when generating PeasUI-1.0.gir.
Review of attachment 164555 [details] [review]: Sorry for not having reviewed this patch earlier. I actually did it using the review tool, but I somehow forgot to publish it ::: configure.ac @@ +227,3 @@ +dnl ================================================================ +AC_PATH_PROG(VALAC, valac) +AM_CONDITIONAL([ENABLE_VALA],[test -n "$VALAC"]) Could this be made optional? i.e. I'd like to see a --enable-vala which would exhibit the same behaviour as seed and python ones. ::: libpeas/Makefile.am @@ +76,3 @@ -include $(INTROSPECTION_MAKEFILE) INTROSPECTION_GIRS = Peas-1.0.gir + INTROSPECTION_SCANNER_ARGS = --c-include=libpeas/peas.h --c-include=libpeas/peas-vala.h Is it possible to restrict peas-vala.h usage to actual vala invocations? ::: libpeas/peas-vala.h @@ +22,3 @@ + * It is here just to accomodate code generators (mainly valac) that expect the + * interface struct to be named *Iface rather than *Interface as is expected + * by G_DEFINE_INTERFACE, it may go away after valac is fixed. Please replace "may" with "will" :-) ::: libpeasui/peas-ui-configurable.h @@ +46,3 @@ + * PeasUIConfigurable::create_configure_dialog: + * @conf_dlg: (out): + */ I'd prefer this to go in a separate patch, as I plan to change the prototype of create_configure_dialog to be GtkWidget *(*create_configure_dialog) (PeasUIConfigurable) ::: peas-demo/plugins/Makefile.am @@ +1,1 @@ +SUBDIRS = helloworld valahello if ENABLE_VALA SUBDIRS += valahello endif ::: peas-demo/plugins/valahello/Makefile.am @@ +1,1 @@ +if ENABLE_VALA Remove this as it should already be handled in the upper level makefile (like in seed and python makefiles)
(In reply to comment #7) > Review of attachment 164555 [details] [review]: > > Sorry for not having reviewed this patch earlier. I actually did it using the > review tool, but I somehow forgot to publish it > > ::: configure.ac > @@ +227,3 @@ > +dnl ================================================================ > +AC_PATH_PROG(VALAC, valac) > +AM_CONDITIONAL([ENABLE_VALA],[test -n "$VALAC"]) > > Could this be made optional? i.e. I'd like to see a --enable-vala which would > exhibit the same behaviour as seed and python ones. Since it's just for building an example, I didn't bother : It'll build it whenever valac is available. Thinking again about it, it can be build unconditionally. That would just require developers to have valac installed, but users would get the generated C code > ::: libpeas/Makefile.am > @@ +76,3 @@ > -include $(INTROSPECTION_MAKEFILE) > INTROSPECTION_GIRS = Peas-1.0.gir > + INTROSPECTION_SCANNER_ARGS = --c-include=libpeas/peas.h > --c-include=libpeas/peas-vala.h > > Is it possible to restrict peas-vala.h usage to actual vala invocations? No. That is, unless a vapi is generated. But isn't this enough? As of now, valac is the only program that looks at c:include in the gir AFAIK, not even the typelib compiler. > ::: libpeas/peas-vala.h > @@ +22,3 @@ > + * It is here just to accomodate code generators (mainly valac) that expect > the > + * interface struct to be named *Iface rather than *Interface as is expected > + * by G_DEFINE_INTERFACE, it may go away after valac is fixed. > > Please replace "may" with "will" :-) ok. > ::: libpeasui/peas-ui-configurable.h > @@ +46,3 @@ > + * PeasUIConfigurable::create_configure_dialog: > + * @conf_dlg: (out): > + */ > > I'd prefer this to go in a separate patch, as I plan to change the prototype of > create_configure_dialog to be > GtkWidget *(*create_configure_dialog) (PeasUIConfigurable) The annotation would not be needed then (maybe we'll need transfer-ownership instead). When are you going to change it? > ::: peas-demo/plugins/Makefile.am > @@ +1,1 @@ > +SUBDIRS = helloworld valahello > > if ENABLE_VALA > SUBDIRS += valahello > endif > > ::: peas-demo/plugins/valahello/Makefile.am > @@ +1,1 @@ > +if ENABLE_VALA > > Remove this as it should already be handled in the upper level makefile (like > in seed and python makefiles) ok.
(In reply to comment #8) > > Could this be made optional? i.e. I'd like to see a --enable-vala which would > > exhibit the same behaviour as seed and python ones. > Since it's just for building an example, I didn't bother : It'll build it > whenever valac is available. Thinking again about it, it can be build > unconditionally. That would just require developers to have valac installed, > but users would get the generated C code I thought AM_PROG_VALAC would error out if valac was not available? Also for distro people I think it's important to be able to force or disable vala plugins. For the others it's important to build even if valac is not available. And I like consistency with python and seed plugins. But if it's too hard for you now, it's ok, I think it's only used for the vala plugin example anyway. > > Is it possible to restrict peas-vala.h usage to actual vala invocations? > No. That is, unless a vapi is generated. But isn't this enough? As of now, > valac is the only program that looks at c:include in the gir AFAIK, not even > the typelib compiler. Then that's fine, it would just have been a nice touch. > > I'd prefer this to go in a separate patch, as I plan to change the prototype of > > create_configure_dialog to be > > GtkWidget *(*create_configure_dialog) (PeasUIConfigurable) > > The annotation would not be needed then (maybe we'll need transfer-ownership > instead). When are you going to change it? When I have time. Hopefully this week. It just requires supporting out arguments in peas_method_apply()
(In reply to comment #9) > (In reply to comment #8) > > The annotation would not be needed then (maybe we'll need transfer-ownership > > instead). When are you going to change it? > > When I have time. Hopefully this week. It just requires supporting out > arguments in peas_method_apply() The prototype change has been done in bug 623173
Could you please update the vala plugin patch so it applies on the current master? Thanks
Created attachment 165247 [details] [review] add an example vala plugin for peasdemo
Created attachment 165248 [details] [review] support --enable-vala argument for configure
[sf@noys] (jhbuild) valahello $ make V=1 /opt/gnome2/bin/valac --girdir ../../../libpeas --girdir ../../../libpeasui --pkg gmodule-2.0 --pkg gtk+-3.0 --pkg Peas-1.0 --pkg PeasUI-1.0 -C peasdemo-vala-hello-plugin.vala error: The type name `GLib.TypeInterface' could not be found error: The type name `Atk.ImplementorIface' could not be found error: The type name `GLib.TypeInterface' could not be found Compilation failed: 3 error(s), 0 warning(s) make: *** [libvalahello_la_vala.stamp] Error 1 This is what I get when I try to compile it... I'm on #vala trying to solve it
That's the main use case for --disable-vala :-) Whenever you try something new, you find bugs, and you need to fix valac (and use valac from git rather than a released version). the ImplementorIface problem is already solved in vala git, and the others are solved by patch 164552 I mentionned above (need to bug Jürg again about it). Thinking again about it, I should add a version check to the enable-vala patch.
Created attachment 169603 [details] [review] add an example vala plugin for peasdemo
Abderrahim: Is your latest patch supposed to work "as is" with the latest valac?
I thought I added a comment about this, but maybe I forgot. The patch is supposed to work as is with the switch-to-gir branch of valac since it has a fix for bug 625026 (but it doesn't work right now because it hasn't been ported to the new gir version 1.2) . I'll try to get more information on the status of that branch.
Now (well, since 0.11.2 released last month), vala should have no more problem with this. However, Gtk-3.0.gir still doesn't include the package needed by vala (so the only remaining thing to be fixed is bug 635287, and having libpeas depend on the next version of gtk)
Created attachment 180080 [details] [review] Example Vala plugin Attached is updated vala demo plugin.
Review of attachment 180080 [details] [review]: I also saw a few added trailing whitespaces in the patch. Could you please remove them? ::: configure.ac @@ +265,3 @@ +fi + +AM_CONDITIONAL([ENABLE_VALA],[test "$found_vala" = "yes"]) There is no version check anywhere. I just tested at home and vala breaks with cryptic messages (vala 0.10.0). Also you should add the "vala support" message at the end of the configure.ac script ::: peas-demo/plugins/valahello/peasdemo-vala-hello-plugin.vala @@ +1,1 @@ +public class PeasDemo.ValaHelloPlugin : Peas.ExtensionBase, Peas.Activatable, PeasGtk.Configurable { Could you please create two distinct classes for activatable and configurable? Also I'm somewhat saddened by the fact we need to inherit from Peas.ExtensionBase but I guess we can live with it.
Created attachment 180097 [details] [review] Example Vala plugin Added vala version check, vala support message to configure, split the plugin into two classes.
Review of attachment 180097 [details] [review]: The patch looks good to me. ::: peas-demo/plugins/valahello/valahello.plugin @@ +1,3 @@ +[Plugin] +Module=valahello +IAge=2 Steve, should't we use iage=1 for peas plugins?
(In reply to comment #21) > Also I'm somewhat saddened by the fact we need to inherit from > Peas.ExtensionBase but I guess we can live with it. Why "need"? This limitation was removed from libpeas long ago IIRC. FWIW, a vala plugin using libpeas was committed to totem some time ago: http://git.gnome.org/browse/totem/commit/?id=ea1025b95bec71a1bc5b5af1c93e34801b24fd46
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.