Created attachment 153062 [details] [review]
Slightly modified patch to export GtkSource, rather than GtkSourceView

I know gtk has merged GI so I am ok with merging it too in gtksourceview, however I'd like to know more about the current support this patch adds:

 - does the patch add all the needed things like in gtk? can it be enabled/disabled at compile time? do we have to add other files? I seem to recall talk about a .m4 file on gtk list...

 - I see no chamges to the code, while this is cool, it seems a bit surprising... do we have to add annotations for some specific functions/signals? How much of the API has been tested?

 - is there some small program that tests this?

Thanks for having a look (I'm no expert on it) - so I can only give a rough idea of what I've understood so far..

- does the patch add all the needed things like in gtk?
*  It just adds the generates the gir/xml file (from the c comments/sources) and compiles it to a typelib file

- can it be enabled/disabled at compile time
* nope - if the infrastructre for Introspection is set up properly, ./configure reports that INTROSPECTION will be generated, otherwise it will not bother generating the code.

- I seem to recall talk about a .m4 file on gtk list...
Sorry no idea about that bit.

 - I see no chamges to the code, while this is cool, it seems a bit
surprising... do we have to add annotations for some specific
functions/signals?
Maybe - but usually it's just the edge cases (ownership transfer / inout parameters, and array's of C types) - As the bindings in theory will be the default way all language bind (hopefully) - you should get bug reports for specific issues.

How much of the API has been tested?
Not alot probably.. - 
- is there some small program that tests this?
I'm working with Seed on it. (very much part/spare time..) - I've got the basic editor up with highlighting.. Code is not amazingly clean though as I'm experimenting with coding/design methods.

These two are 'non-working'/'live-but-changing' - but could be used as a starting point for tests.
http://www.akbkhome.com/svn/seed/gtksourceview.js
http://www.akbkhome.com/svn/seed/Builder/RightEditor.js

Regards
Alan

Created attachment 159851 [details] [review]
Update introspection support

This patch uses what AFAICS is the recommended way of generating the typelib

Review of attachment 159851 [details] [review]:

Minor comments below, but what concerns me more is lack of annotations added to the api. I am ok if the api is exposed to introspection incrementally, but what I am not sure of are the backward compat implications: e.g. if we tomorrow we add an annotation that specifies the ownership of an output param, will we break programs that started using the introspection before the annotation was added?

::: configure.ac
@@ +14,3 @@
 AC_CONFIG_MACRO_DIR([m4])
 
+AM_INIT_AUTOMAKE([1.10.3 dist-bzip2 no-dist-gzip -Wno-portability])

Why is -Wno-portability needed? I do not like the idea of turning of portability warnings very much...

@@ +108,3 @@
 
+GOBJECT_INTROSPECTION_CHECK([0.6.7])
+

if not support for explicit enable/disable I would at least like that a propery summary line is given at the end of configure, like we do for the compiler etc

(In reply to comment #5)
> Review of attachment 159851 [details] [review]:
> 
> Minor comments below, but what concerns me more is lack of annotations added to
> the api. I am ok if the api is exposed to introspection incrementally, but what
> I am not sure of are the backward compat implications: e.g. if we tomorrow we
> add an annotation that specifies the ownership of an output param, will we
> break programs that started using the introspection before the annotation was
> added?

I'm afraid we can only say that the introspected API is unstable for now and it will mature with time.

> ::: configure.ac
> @@ +14,3 @@
>  AC_CONFIG_MACRO_DIR([m4])
> 
> +AM_INIT_AUTOMAKE([1.10.3 dist-bzip2 no-dist-gzip -Wno-portability])
> 
> Why is -Wno-portability needed? I do not like the idea of turning of
> portability warnings very much...

This just hides some warnings about not being compatible with non-GNU Make, which GNOME doesn't try to support. Some background:

http://mail.gnome.org/archives/desktop-devel-list/2009-February/msg00111.html

> @@ +108,3 @@
> 
> +GOBJECT_INTROSPECTION_CHECK([0.6.7])
> +
> 
> if not support for explicit enable/disable I would at least like that a propery
> summary line is given at the end of configure, like we do for the compiler etc

Done.

Created attachment 159877 [details] [review]
Update introspection support

Just putting that there is another bug about this in gedit and it is also for gtksourceview:

https://bugzilla.gnome.org/show_bug.cgi?id=604840

(In reply to comment #6)
> (In reply to comment #5)
> > Review of attachment 159851 [details] [review] [details]:
> > 
> > Minor comments below, but what concerns me more is lack of annotations added to
> > the api. I am ok if the api is exposed to introspection incrementally, but what
> > I am not sure of are the backward compat implications: e.g. if we tomorrow we
> > add an annotation that specifies the ownership of an output param, will we
> > break programs that started using the introspection before the annotation was
> > added?
> 
> I'm afraid we can only say that the introspected API is unstable for now and it
> will mature with time.

You can see here what telepathy-glib has done about it:

http://git.collabora.co.uk/?p=telepathy-glib.git;a=blob;f=NEWS;h=ac7ed8c4c9569b169bb40659a49ae7f327bd513c;hb=HEAD#l60

Is there anything else a module can do?

I think we should get introspection in on the unstable branch and work from there

Created attachment 161191 [details] [review]
Add introspection support to build

This version includes introspection.m4 so the build will not fail when it is not installed, conditionally builds the introspection data and the output of configure is cleaner.

Review of attachment 161191 [details] [review]:

Comments added bellow.

::: Makefile.am
@@ +61,3 @@
 	fi
 
+if HAVE_INTROSPECTION

what about indenting here?

::: configure.ac
@@ +14,3 @@
 AC_CONFIG_MACRO_DIR([m4])
 
+AM_INIT_AUTOMAKE([1.10.3 dist-bzip2 no-dist-gzip -Wno-portability])

-Wno-portability <- what's that for?

I'd say to skip the copy of introspection.m4, since I asked and also pango etc do not ship a copy so one needs it installed anyway.

Apart from the that and nacho's comment about indentation, it looks fine to me.

Tomeu already replied about no-portability

What is left to do here? If I update Garrett's patch to not include introspection.m4 and fix indentation, it could go in? Or is there any other possibility?

We discussed this on irc we were about to commit this (with the change you mention), but the g-i was not building for any of us, so we put this on hold.

Sure, go ahead

Created attachment 161767 [details] [review]
Add introspection support to build

Adds -Wno-portability because we don't presume to support
non-GNU make,
http://mail.gnome.org/archives/desktop-devel-list/2009-February/msg00111.html.

Attachment 161767 [details] pushed as 89279ac - Add introspection support to build

*** Bug 604840 has been marked as a duplicate of this bug. ***

*** Bug 626237 has been marked as a duplicate of this bug. ***