After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 587017 - Add GObject Introspection bindings for libgweather
Add GObject Introspection bindings for libgweather
Status: RESOLVED FIXED
Product: libgweather
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: future
Assigned To: libgweather-maint
libgweather-maint
Depends on: 587016
Blocks: 585444 646854
 
 
Reported: 2009-06-26 06:12 UTC by Tim Horton
Modified: 2012-05-12 18:06 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Generate gobject-introspection data (5.69 KB, patch)
2009-06-26 06:14 UTC, Tim Horton
needs-work Details | Review
API Break: correct namespacing and make WeatherInfo more GObject friendly (135.35 KB, patch)
2011-03-22 20:57 UTC, Giovanni Campagna
none Details | Review
API Break: correct namespacing and make WeatherInfo more GObject friendly (135.58 KB, patch)
2011-11-15 15:50 UTC, Giovanni Campagna
none Details | Review
Weather: move weather.h to gweather-weather.h (5.03 KB, patch)
2011-12-14 17:55 UTC, Giovanni Campagna
none Details | Review
GWeatherXML: use meaningful column types in the tree model (4.45 KB, patch)
2011-12-14 17:55 UTC, Giovanni Campagna
reviewed Details | Review

Description Tim Horton 2009-06-26 06:12:36 UTC
Here's an initial patch to generate gobject-introspection data for libgweather.
Comment 1 Tim Horton 2009-06-26 06:13:32 UTC
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.
Comment 2 Tim Horton 2009-06-26 06:14:04 UTC
Created attachment 137403 [details] [review]
Generate gobject-introspection data
Comment 3 Dan Winship 2009-06-26 13:58:27 UTC
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...
Comment 4 Tim Horton 2009-06-29 23:50:32 UTC
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.
Comment 5 Vincent Untz 2009-09-08 23:50:49 UTC
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...
Comment 6 Yaakov Selkowitz 2009-09-09 01:06:41 UTC
(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.
Comment 7 Dan Winship 2009-09-09 01:26:38 UTC
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...
Comment 8 Johan (not receiving bugmail) Dahlin 2009-12-17 13:26:52 UTC
Review of attachment 137403 [details] [review]:

This patch should be updated to follow the conventions listed here:
http://live.gnome.org/GObjectIntrospection/AutotoolsIntegration
Comment 9 Frederik Zipp 2011-01-10 19:05:21 UTC
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
Comment 10 Dan Winship 2011-01-10 19:40:22 UTC
(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.
Comment 11 Giovanni Campagna 2011-03-22 20:57:42 UTC
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.
Comment 12 Giovanni Campagna 2011-03-22 21:03:49 UTC
(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).
Comment 13 Giovanni Campagna 2011-11-15 15:50:06 UTC
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.
Comment 14 André Klapper 2011-11-29 21:08:20 UTC
ping to the maintainers - can this please get in?
Comment 15 Dan Winship 2011-12-03 10:54:31 UTC
(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...)
Comment 16 Dan Winship 2011-12-03 11:06:41 UTC
(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.
Comment 17 Giovanni Campagna 2011-12-14 17:55:00 UTC
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.
Comment 18 Giovanni Campagna 2011-12-14 17:55:23 UTC
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.
Comment 19 Giovanni Campagna 2012-02-13 16:01:21 UTC
(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)
Comment 20 Dan Winship 2012-02-13 19:12:48 UTC
(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.
Comment 21 André Klapper 2012-02-13 20:26:00 UTC
(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
Comment 22 Dan Winship 2012-02-13 21:23:35 UTC
pfft. MAINTAINERS is deprecated. I'm not in the doap file. :)
Comment 23 Javier Jardón (IRC: jjardon) 2012-03-05 17:53:54 UTC
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
Comment 24 Giovanni Campagna 2012-03-05 20:48:17 UTC
(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?
Comment 25 Giovanni Campagna 2012-03-05 20:54:14 UTC
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
Comment 26 André Klapper 2012-03-06 13:03:22 UTC
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?
Comment 27 Javier Jardón (IRC: jjardon) 2012-03-06 17:07:14 UTC
(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
Comment 28 Javier Jardón (IRC: jjardon) 2012-03-09 17:56:45 UTC
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
Comment 29 Ray Strode [halfline] 2012-05-12 18:06:31 UTC
This broke jhbuild, see bug 674714