Bug 68439

so if you wish me to split it, would you also please annotate it a bit so I can in the same time improve it?)

A D-Bus service doesn't need to ship bindings. Vala, if it's interested in using Geoclue, can ship its own D-Bus helpers. Most languages will generate the code using the XML file that should be installed by Geoclue, or don't need this sort of crutch.

Comment on attachment 84462 [details] [review]
patch proposal

Review of attachment 84462 [details] [review]:
-----------------------------------------------------------------

Actually this is more than two patches and most of them are unrelated to this bug/subject even:

1. Introduce Api versioning for consisntency
2. Minor enhancements in the configure.ac
3. Vala support + demo (these could also be split but up to you)
4. Libs and Cflags were added to the pc file.
5. EXTRA_DIST were moved out of the "if introspection" condition.
6. ntrospection support were rewritten in a more automake friendly way.

BTW, in commit logs, you want to talk about changes in the patch in present form. i-e 'were rewritten' -> 'are-rewritten'.

::: configure.ac
@@ +17,5 @@
> +
> +AC_SUBST([GCLUE_API_VERSION])
> +AC_SUBST([GCLUE_API_MAJOR_VERSION])
> +AC_SUBST([GCLUE_API_MINOR_VERSION])
> +AC_SUBST([GCLUE_API_VERSION_U],[AS_TR_SH([$GCLUE_API_VERSION])])

whats this GCLUE_API_VERSION_U about?

@@ +39,4 @@
>  # Pkg-config
>  PKG_PROG_PKG_CONFIG([0.22])
>  
> +# I18n support

separate patch.

@@ +120,4 @@
>  fi
>  AM_CONDITIONAL([BUILD_GEOIP_SERVER], [test "x$build_geoip_server" = "xyes"])
>  
> +# Strict compiler

Unneeded but if you insist, just put all these nonfunctional tweaks/corrections into one separate patch.

::: demo/Makefile.am
@@ +9,5 @@
>  		   $(GEOCLUE_LIBS)
> +
> +
> +if ENABLE_VAPIGEN
> +if HAVE_VALAC

I think we should just have one VARIABLE for both: ENABLE_VALA

::: demo/find-location.vala
@@ +1,1 @@
> +class FindLocation : Object {

If this demo does the same as where-am-i, I'd just call it where-am-i-vala to make it clear.

@@ +11,5 @@
> +        Environment.set_application_name ("Find Location");
> +    }
> +
> +    public void search () {
> +        GClue.ManagerProxy.create_for_bus.begin (GLib.BusType.SYSTEM,

We should use 'yield' so it shows the true beauty of Vala. :) Same applies to all .begin calls below.

::: src/Geoclue-2.0-custom.vala
@@ +1,1 @@
> +namespace GClue {

I'd document a bit more in the commit log what exactly is the issue with async constructors to justify this w/ bug reference.

::: src/Geoclue-2.0.metadata
@@ +1,4 @@
> +* cheader_filename="geoclue.h"
> +*.*.cancellable#parameter nullable default=null
> +
> +// Bug #659886: Async constructors don't work

You really want to add this reference to commit log.

::: src/Makefile.am
@@ +43,5 @@
> +geoclue_CFLAGS =                        \
> +	$(GEOCLUE_CFLAGS)               \
> +	-DLOCALEDIR="\"$(localedir)\""  \
> +	$(NULL)
> +

This deserves a separate patch.

@@ +62,3 @@
>  
>  if HAVE_INTROSPECTION
> +-include $(INTROSPECTION_MAKEFILE)

Where is this veriable coming from? I don't see any new makefile added to this patch.

@@ +88,3 @@
>  
>  typelibsdir = $(libdir)/girepository-1.0
> +typelibs_DATA = $(INTROSPECTION_GIRS:.gir=.typelib)

Please:

1. Diff generated GIRs w/ and w/o your change
2. test your change against gnome-maps

@@ +91,5 @@
>  
>  CLEANFILES += $(gir_DATA) $(typelibs_DATA)
> +
> +if ENABLE_VAPIGEN
> +-include $(VAPIGEN_MAKEFILE)

Same about this one? where is it coming from?

::: src/geoclue-2.0.pc.in
@@ +7,5 @@
>  Description: The Geoinformation Service
>  Version: @VERSION@
> +Requires: gio-unix-2.0 libsoup-2.4 json-glib-1.0
> +Libs: -L${libdir} -lgeoclue-@GCLUE_API_MAJOR_VERSION@
> +Cflags: -I${includedir}/geoclue-@GCLUE_API_VERSION@

Can't tell from the patch but do you ensure include files are getting installed in the new prefix?

(In reply to comment #2)
> A D-Bus service doesn't need to ship bindings.

It does if it provides a library to abstract the D-Bus. That is why we provide the GIR (which is a binding too).

> Vala, if it's interested in
> using Geoclue, can ship its own D-Bus helpers.

Its not Vala that is interested but the apps written in Vala.

> Most languages will generate
> the code using the XML file that should be installed by Geoclue, or don't
> need this sort of crutch.

Ideally speaking, yes but there is not a single example of any other compiled high-level language that bothers to support GIR in the first place. So far we have only one example and since it cant readily use GIR, we blame the language and use this as nice excuse to bash it. I can assure you that all such languages will need such "crutches" in practice.

Anyways, I don't expect to change your opinion on this but just wanted to get things straight on the record.

(In reply to comment #4)
> (In reply to comment #2)
> > A D-Bus service doesn't need to ship bindings.
> 
> It does if it provides a library to abstract the D-Bus. That is why we
> provide the GIR (which is a binding too).

It shouldn't. C users can poke at the service directly, or use pkg-config to find the XML file describing the service, and generate code with gdbus-codegen themselves.

The API isn't going to be great for any other language, and I really don't think you want to add a layer on top of machine generated code which you won't be able to tweak. Python, Javascript, C, C++, won't need the library.

> > Vala, if it's interested in
> > using Geoclue, can ship its own D-Bus helpers.
> 
> Its not Vala that is interested but the apps written in Vala.

So, vala.

> > Most languages will generate
> > the code using the XML file that should be installed by Geoclue, or don't
> > need this sort of crutch.
> 
> Ideally speaking, yes but there is not a single example of any other
> compiled high-level language that bothers to support GIR in the first place.
> So far we have only one example and since it cant readily use GIR, we blame
> the language and use this as nice excuse to bash it. I can assure you that
> all such languages will need such "crutches" in practice.

I can say this one more time, and I have no problems being on record about it. Vala needs to sort out its GIR support. Python and Javascript can live just fine with using the GIR provided and don't need anything else. People should fix the Vala GIR parser if it needs to be fixed instead of requiring people to sprinkle vapi files everywhere. It's 1998 and people are sprinkling Python bindings using Cython.

> Anyways, I don't expect to change your opinion on this but just wanted to
> get things straight on the record.

We shouldn't ship a library. The pkg-config file should give a pointer to the xml file so that C/Vala/whatever apps can generate their own helper code.

(In reply to comment #2)
> A D-Bus service doesn't need to ship bindings. Vala, if it's interested in
> using Geoclue, can ship its own D-Bus helpers. Most languages will generate
> the code using the XML file that should be installed by Geoclue, or don't
> need this sort of crutch.

I thought that one of the key benefits of switching from geocode-glib to geoclue2 is that I can get rid of Bastien as maintainer:)

> BTW, in commit logs, you want to talk about changes in the patch in present
> form. i-e 'were rewritten' -> 'are-rewritten'.
> 
I do not really want)) but if it should be this way, then I do not mind.

> > +
> > +AC_SUBST([GCLUE_API_VERSION])
> > +AC_SUBST([GCLUE_API_MAJOR_VERSION])
> > +AC_SUBST([GCLUE_API_MINOR_VERSION])
> > +AC_SUBST([GCLUE_API_VERSION_U],[AS_TR_SH([$GCLUE_API_VERSION])])
> 
> whats this GCLUE_API_VERSION_U about?
> 
it's there for turning names like "geoclue-2.0" into "geoclue_2_0" so they can be used as names in makefile for making it consistent. It is a common practice even in gnome projects like: evince, librsvg, vte, gucharmap and many others
please refer to http://codesearch.debian.net/search?q=%5C%5BAS_TR_SH%5C%28%5C%5B

> > +    public void search () {
> > +        GClue.ManagerProxy.create_for_bus.begin (GLib.BusType.SYSTEM,
> 
> We should use 'yield' so it shows the true beauty of Vala. :) Same applies
> to all .begin calls below.
> 
you are totally right about this point!

Created attachment 84502 [details]
vala example, with yield it looks clear

(In reply to comment #6)
> (In reply to comment #2)
> > A D-Bus service doesn't need to ship bindings. Vala, if it's interested in
> > using Geoclue, can ship its own D-Bus helpers. Most languages will generate
> > the code using the XML file that should be installed by Geoclue, or don't
> > need this sort of crutch.
> 
> I thought that one of the key benefits of switching from geocode-glib to
> geoclue2 is that I can get rid of Bastien as maintainer:)

Some respect please! Bastien has been maintaining geoclue when original devs abandoned it and he is the one who came up with the idea and design for geoclue2. I only implemented it (not even completely yet). While I might not agree with Bastien, he does have a point here.

Interacting with D-Bus directly in Vala is pretty easy and given our simple interface, it should be pretty darn simple. Something like this:

https://git.gnome.org/browse/rygel/tree/src/librygel-server/rygel-dbus-thumbnailer.vala#n23

So I'm gonna drop the library and gir now (after porting demo and Maps to use D-Bus directly). You non-vala and non-gir related changes are still going to be useful so please do put them in separate bugs.

(In reply to comment #7)
> > BTW, in commit logs, you want to talk about changes in the patch in present
> > form. i-e 'were rewritten' -> 'are-rewritten'.
> > 
> I do not really want)) but if it should be this way, then I do not mind.

AFAIK thats the usual git commit log practice (Can't recall where I first learnt it from) and BTW I made a typo 'are-rewritten' -> 'are rewritten' (no dash).

(In reply to comment #6)
> (In reply to comment #2)
> > A D-Bus service doesn't need to ship bindings. Vala, if it's interested in
> > using Geoclue, can ship its own D-Bus helpers. Most languages will generate
> > the code using the XML file that should be installed by Geoclue, or don't
> > need this sort of crutch.
> 
> I thought that one of the key benefits of switching from geocode-glib to
> geoclue2 is that I can get rid of Bastien as maintainer:)

Geoclue2 is always where the place where the location detection was going to live, see:
http://www.hadess.net/2013/04/geocluing-desktop-slowly.html