I forgot to mention above (though it is set on Bugzilla) - this depends on bug 587016 (in gobject-introspection) being fixed. That said, that's a one-liner which should be committed quite quickly.

Created attachment 137403 [details] [review]
Generate gobject-introspection data

So I'll get around to testing this at some point, but... what does the generated introspection data look like? WeatherInfo is sort of a disaster API-wise. Does it turn into something usable? (Even the notification callbacks?) Or were you only interested in the location/timezone APIs (which ought to be completely introspectable because I already made usable python bindings of them at one point).

There's a bug about making a cleaner API to replace WeatherInfo at some point, which I was assuming would be a prereq for introspectability.

Another issue is that libgweather is one of those super-unstable APIs, where you have to "#define LIBGWEATHER_I_KNOW_THIS_IS_UNSTABLE" in order to include the headers. It would be nice if the introspected form preserved this information somehow, which I guess would require gir support. Let me file a bug for that...

So! I was initially mostly interested in location data, which mostly seems to work (for some reason children of a location don't work, but the location entry field works, and returns a reasonable location with accessible, useful data). However, it doesn't make sense to commit this until more of the rest of the library works... I tried to extract weather data, and got a ways, but you're right, the API isn't the most introspectable thing in the world...

About the unstability thing... that actually sounds like a great thing to have. libgames-support is the same way (they don't promise stability, and thus don't want people outside of gnome-games using the library, but I had to make it install so that the Seed-based games I'm working on for GSoC could access it), and we were slightly worried about people taking the existence of a GIR for that library the wrong way... something like this would be a reasonable way to fix that.

Do we really have to add introspection.m4 to the module? I'm not a big fan of this, since it will just get out of date...

(In reply to comment #5)
> Do we really have to add introspection.m4 to the module? I'm not a big fan of
> this, since it will just get out of date...

Yes, because this macro is required for bootstrapping from git, or autoreconf'ing from a tarball, but you can't rely on a system macro being present because GObjectIntrospection support is optional.

yeah, currently modules are adding introspection.m4 directly; presumably when introspection moves into glib (or at least, becomes part of the platform) then everyone will remove it and just it via aclocal at autogen time.

being able to have unstable introspection is bug 591600, though I'm not sure this really should be marked depending on that, since the highly-unstable parts of libgweather are pretty much identical to the highly-non-introspectable parts...

Review of attachment 137403 [details] [review]:

This patch should be updated to follow the conventions listed here:
http://live.gnome.org/GObjectIntrospection/AutotoolsIntegration

I was looking into implementing a weather information view for Gnome Shell in the sense of [1] and landed here.

(In reply to comment #3)
> There's a bug about making a cleaner API to replace WeatherInfo at some point,
> which I was assuming would be a prereq for introspectability.

I have browsed the open bugs for this module but didn't find such an entry. What would be the plans for this cleaner API? The main reason that WeatherInfo currently does not work with introspection is because the symbols of 'weather.h' have a different namespace ('Weather' vs. 'GWeather').

[1] http://live.gnome.org/GnomeShell/Design/Guidelines/MessageTray/Weather

(In reply to comment #9)
> I have browsed the open bugs for this module but didn't find such an entry.

Hm... I guess bug 538787 was what I was thinking of, but that got committed without really fixing up a lot of things.

> What would be the plans for this cleaner API? The main reason that WeatherInfo
> currently does not work with introspection is because the symbols of
> 'weather.h' have a different namespace ('Weather' vs. 'GWeather').

Fixing up namespacing is part of it (both for WeatherInfo and for all of the enum types). Also, the memory management and use of callbacks is sort of ugly; the new GWeatherInfo should either be a GObject, or else a refcounted GBoxed type. You also need to remove the redundancy between, eg weather_info_get_sky() and weather_info_get_value_sky(). (You want to keep the name "weather_info_get_sky", but the signature of weather_info_get_value_sky().)

weather_info_update() and weather_info_abort() should go away; the new GWeatherInfo should just be a static object representing the weather at one point in time. To get updated weather, the app should just call g_weather_location_get_weather() again. And g_weather_location_get_weather() needs to be split into _sync and _async variants, gio-style, with GCancellables and GErrors.

gweather-gconf.h, gweather-prefs.h, and gweather-xml.h can probably just be excluded from introspection, although at some point we need to figure out how gnome-panel 3.0 (and gnome-applets 3.0 assuming it exists) store their weather location info. The way it currently is in libgweather is sort of broken, because it was never really completely abstracted away when libgweather was split out of weather-applet. If gnome-panel 3.0 can be made happy without using any of those files (which it probably can) then they should just go away.

Created attachment 184124 [details] [review]
API Break: correct namespacing and make WeatherInfo more GObject friendly

WeatherInfo becomes GWeatherInfo, a GObject, with private data (it
was already opaque), properties (write only for now, I plan to improve
on that later, when I'll convert all getters) and signals. WeatherLocation
has been removed, you can now either pass a GWeatherLocation (obtained from
the database), or use the default from preferences. WeatherPrefs was
removed, and GWeatherPrefs made opaque. GWeatherPrefs can be reintroduced
when we figure out the story of locations.

(In reply to comment #10)
> (In reply to comment #9)
> Fixing up namespacing is part of it (both for WeatherInfo and for all of the
> enum types). Also, the memory management and use of callbacks is sort of ugly;
> the new GWeatherInfo should either be a GObject, or else a refcounted GBoxed
> type. You also need to remove the redundancy between, eg weather_info_get_sky()
> and weather_info_get_value_sky(). (You want to keep the name
> "weather_info_get_sky", but the signature of weather_info_get_value_sky().)

Made into a GObject. Getters still missing, but for now are not needed, and in fact you would still need "get_sky_string", because it does some non trivial processing on the data.

> weather_info_update() and weather_info_abort() should go away; the new
> GWeatherInfo should just be a static object representing the weather at one
> point in time. To get updated weather, the app should just call
> g_weather_location_get_weather() again. And g_weather_location_get_weather()
> needs to be split into _sync and _async variants, gio-style, with GCancellables
> and GErrors.

For now, gweather_info_update is there, as a normal method. All operations are async, and apps should connect to 'updated'. I can add 'cancellable' if needed.
This resulted in a somehow mixed API, in which apps call gweather_info_new, which does all the normal stuff, initiates libsoup sessions, etc., whereas libgweather internally creates other half-breed GWeatherInfos for forecasts.

> gweather-gconf.h, gweather-prefs.h, and gweather-xml.h can probably just be
> excluded from introspection, although at some point we need to figure out how
> gnome-panel 3.0 (and gnome-applets 3.0 assuming it exists) store their weather
> location info. The way it currently is in libgweather is sort of broken,
> because it was never really completely abstracted away when libgweather was
> split out of weather-applet. If gnome-panel 3.0 can be made happy without using
> any of those files (which it probably can) then they should just go away.

I removed GWeatherPrefs, because it exposed WeatherLocation (which I had to make private because of GWeatherLocation). I think GWeatherGConf can be removed in the move to GSettings, but GWeatherPrefs is useful to tweak the behaviour of GWeatherInfo (in particular default units).

Created attachment 201450 [details] [review]
API Break: correct namespacing and make WeatherInfo more GObject friendly

WeatherInfo becomes GWeatherInfo, a GObject, with private data (it
was already opaque), properties (write only for now, I plan to improve
on that later, when I'll convert all getters) and signals. WeatherLocation
has been removed, you can now either pass a GWeatherLocation (obtained from
the database), or use the default from preferences. WeatherPrefs was
removed, and GWeatherPrefs made opaque. GWeatherPrefs can be reintroduced
when we figure out the story of locations.

Rebased on current master.

ping to the maintainers - can this please get in?

(In reply to comment #14)
> ping to the maintainers - can this please get in?

libgweather is basically unmaintained at this point. I won't complain if this code goes in, but I won't fix it if it breaks either. (I guess I should remove myself from the maintainers file...)

(In reply to comment #15)
> I won't complain if this code goes in

With that out of the way, I don't think this should go in. :-). libgweather is a bad implementation of a solution to the wrong problem, and we should just throw it away and start over, not reusing any of the GPLed code/data in libgweather (so that, among other things, the new library is compatible with the CC-licenses geonames database) and keep only a small local database of cities/weather stations (with both names and translations from geonames), and just expect the user to be online most of the time and have access to the full database that way.

Created attachment 203498 [details] [review]
Weather: move weather.h to gweather-weather.h

Public headers should have a namespaced name, and should also be
installed.

Created attachment 203499 [details] [review]
GWeatherXML: use meaningful column types in the tree model

Now that WeatherLocation is no longer a public type, we should avoid
using it in public places like the gweather-xml tree store.

Found while porting gweather applet to the new API.

(In reply to comment #16)
> (In reply to comment #15)
> > I won't complain if this code goes in
> 
> With that out of the way, I don't think this should go in. :-).

Which boils down... yes or no?
(I'm asking because it blocks the GSettings patch, and I'd like to avoid a long and probably dangerous rebase)

(In reply to comment #19)
> Which boils down... yes or no?

It wasn't an answer, it was just my opinion. I'm not a libgweather maintainer any more anyway.

(In reply to comment #20)
> I'm not a libgweather maintainer any more anyway.

http://git.gnome.org/browse/libgweather/tree/MAINTAINERS says differently. :P

pfft. MAINTAINERS is deprecated. I'm not in the doap file. :)

Review of attachment 203499 [details] [review]:

::: libgweather/gweather-xml.c
@@ +89,3 @@
 	    gtk_tree_store_set (store, &iter,
+				GWEATHER_XML_COL_METAR_CODE, wloc->code,
+				GWEATHER_XML_COL_LATLON_VALID, wloc->latlon_valid,

This doesnt seems to be correct.
WeatherLocation doesnt have a latlon_valid member

@@ +109,3 @@
 	gtk_tree_store_set (store, &iter,
+			    GWEATHER_XML_COL_METAR_CODE, wloc->code,
+			    GWEATHER_XML_COL_LATLON_VALID, wloc->latlon_valid,

Same here

(In reply to comment #23)
> Review of attachment 203499 [details] [review]:
> 
> ::: libgweather/gweather-xml.c
> @@ +89,3 @@
>          gtk_tree_store_set (store, &iter,
> +                GWEATHER_XML_COL_METAR_CODE, wloc->code,
> +                GWEATHER_XML_COL_LATLON_VALID, wloc->latlon_valid,
> 
> This doesnt seems to be correct.
> WeatherLocation doesnt have a latlon_valid member

Yes it does (weather-priv.h:58)
Bad rebase?

In case the patch sequence between this and the gsettings bug is hard to follow, I've setup a branch at github: https://github.com/gcampax/libgweather/tree/introspectable-api

I don't see that this will block 3.4 (see "GNOME Target" field), still it's a nice to have even if it's not perfect.
Javier?

(In reply to comment #25)
> In case the patch sequence between this and the gsettings bug is hard to
> follow, I've setup a branch at github:
> https://github.com/gcampax/libgweather/tree/introspectable-api

Oh, ok. So these 2 patches are applied after the GSettings one.
Some missing things:

- Update doc/libgweather-sections.txt and doc/libgweather-docs.sgml (gweather-gconf apis doesnt exist anymore)
- Remove gconf dependency from libgweather/gweather-3.0-uninstalled.pc and libgweather/gweather-3.0.pc.in
- Update po-locations/POTFILES.skip

All this work was pushed to master after branch libgweather for gnome-3-4

Thanks Giovanni for your work on this and Dan for the reviews

This broke jhbuild, see bug 674714