Created attachment 215910 [details] [review]
librsvg-2.0: switch to GIR

Created attachment 215911 [details] [review]
librsvg-2.0: switch to GIR

Oops.  One of the files was wrong.

Distributing these with librsvg instead of valac raises an interesting issue.  There are a several deprecated symbols which are #ifdef'd out while g-ir-scanner is running so that they aren't presented to consumers, but Vala's existing librsvg bindings do include these symbols (Vala's librsvg bindings are considerably older than librsvg's introspection support).  The way I see it there are three solutions:

1. Break backwards compatibility with the bindings distributed with Vala.
2. Use an Rsvg-2.0-custom.vala file to add the symbols to the Vala bindings only.
3. Expose the deprecated symbols to G-I and properly annotate them (including the fact that they are deprecated).

I went with option #2 for the patch above since I had to work with what librsvg provided.  Option #3 has the rather nice property that we can eliminate the need for a *-custom.vala and is my preferred choice.  Which would you like?

I don't like #3. I didn't expose these to the GIR because I want to remove them, and not making them available to bindings will make this easier.

If you do need to keep backward compat, #2 would be the best. Or we could make the librsvg VAPI version be different than the GIR/API version so the new bindings could be parallel-installed with the old custom ones, and go with #1.

Created attachment 216242 [details] [review]
introspection: clean up lots of warnings emitted by g-ir-scanner

This takes care of a bunch of warnings emitted when passing --warn-all to g-ir-scanner.  Really it's just fixing gtk-doc syntax, so documentation should benefit as well.

Created attachment 216243 [details] [review]
Add Vala bindings

endif # HAVE_INTROSPECTION
 
+if ENABLE_VAPIGEN

The ENABLE_VAPIGEN should be inside the HAVE_INTROSPECTION, since it requires it.

+librsvg-$(RSVG_API_VERSION).vapi: Rsvg-$(RSVG_API_VERSION).gir
+
+VAPIGEN_VAPIS = librsvg-$(RSVG_API_VERSION).vapi
+
+librsvg_@RSVG_API_VERSION_U@_vapi_DEPS = gio-2.0 cairo
+librsvg_@RSVG_API_VERSION_U@_vapi_METADATADIRS = $(srcdir)
+librsvg_@RSVG_API_VERSION_U@_vapi_FILES = Rsvg-$(RSVG_API_VERSION).gir Rsvg-$(RSVG_API_VERSION)-custom.vala

Let's define RSVG_VAPI_VERSION[_U] for this in configure, set to RSVG_API_VERSION for now.

+	namespace Version {
+		[CCode (cname = "LIBRSVG_CHECK_VERSION")]
+		public static bool check (int major, int minor, int micro);
+	}
+

Is that a runtime check? We could add that to librsvg, if you have a use case for it.

+	[CCode (lower_case_cprefix = "librsvg_have_")]
+	namespace Features {
+		public static bool SVGZ;
+		public static bool CSS;
+	}

Those should be deprecated, since we now always support SVGZ and CSS (removed the optional configure switches in 2.35).

\ No newline at end of file

Fix this :-)

+# Vala bindings
+VAPIGEN_CHECK([0.17.1.26])

Unfortunately, this macro is automagic by default. So before calling it add:

# No automagic please!
if test -z "$enable_vala"; then
  enable_vala=yes
fi

(or perhaps =no; not sure yet).

Created attachment 216476 [details] [review]
Add Vala bindings

(In reply to comment #7)
> The ENABLE_VAPIGEN should be inside the HAVE_INTROSPECTION, since it requires
> it.

vapigen.m4 checks to make sure introspection is enabled so this isn't strictly necessary, but it doesn't hurt and it may help people grok the Makefile.am
 
> Let's define RSVG_VAPI_VERSION[_U] for this in configure, set to
> RSVG_API_VERSION for now.

As discussed with you in IRC, leaving as it is.

> +    namespace Version {
> +        [CCode (cname = "LIBRSVG_CHECK_VERSION")]
> +        public static bool check (int major, int minor, int micro);
> +    }
> +
> 
> Is that a runtime check? We could add that to librsvg, if you have a use case
> for it.

From Vala's it is a check using the version numbers from the header--you might be able to use it as a compile-time check by using GLib.static_assert (although that may not work due to Vala's aggressive use of temporary variables).  The generated VAPI also has the MAJOR/MINOR/MICRO constants (see http://pastebin.com/CRAEcKj9 for a copy)

It's already part of librsvg (the LIBRSVG_CHECK_VERSION macro in librsvg-features.h), it just isn't in the GIR because G-I doesn't support macros, so I added it to the custom.vala.  If you want to expose the functionality to other introspection consumers librsvg would have to provide a function for that... sqlite3 does that (see http://sqlite.org/c3ref/libversion.html).  Even in C it can be useful to check the library version against the header version.  I've also used it in sqlheavy to enable features in conjunction with dlopen/dlsym.

> +    [CCode (lower_case_cprefix = "librsvg_have_")]
> +    namespace Features {
> +        public static bool SVGZ;
> +        public static bool CSS;
> +    }
> 
> Those should be deprecated, since we now always support SVGZ and CSS (removed
> the optional configure switches in 2.35).

Okay, then they can just be removed.  The versions of these in the old vapi are broken (they'll generate RSVG_LIBRSVG_HAVE_*, so backwards compatibility isn't a concern here.

> \ No newline at end of file
> 
> Fix this :-)

Oops.

> +# Vala bindings
> +VAPIGEN_CHECK([0.17.1.26])
> 
> Unfortunately, this macro is automagic by default. So before calling it add:
> 
> # No automagic please!
> if test -z "$enable_vala"; then
>   enable_vala=yes
> fi

As discussed in IRC, I'm including an updated vapigen.m4 in this patch which allows you to pass the default value so ./configure --help doesn't lie about the default value.  Assuming you're happy with it, I'll push the updated version to Vala, too.

> (or perhaps =no; not sure yet).

I think "no" makes more sense than "yes", otherwise you're adding vala/vapigen as a dependency for the default build.  Also, this depends on a very recent version of vala/vapigen (I had to commit push a small change to properly handle multiple symbol prefixes.  See http://git.gnome.org/browse/vala/commit/?id=d01d6da7584e1c8836cbc175d4e2c9b6be67aea6).

Fixed on master.