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 646854 - Port to GSettings
Port to GSettings
Status: RESOLVED FIXED
Product: libgweather
Classification: Core
Component: general
3.2.x
Other All
: Normal normal
: 2.22.0
Assigned To: libgweather-maint
libgweather-maint
: 547478 (view as bug list)
Depends on: 587017
Blocks: 622558
 
 
Reported: 2011-04-05 19:39 UTC by Giovanni Campagna
Modified: 2012-03-09 17:50 UTC
See Also:
GNOME target: 3.4
GNOME version: 3.3/3.4


Attachments
Port to GSettings (69.21 KB, patch)
2011-04-05 19:41 UTC, Giovanni Campagna
none Details | Review
Port to GSettings (74.40 KB, patch)
2011-04-05 20:48 UTC, Giovanni Campagna
none Details | Review
Port to GSettings (85.84 KB, patch)
2011-11-15 15:51 UTC, Giovanni Campagna
reviewed Details | Review
Port to GSettings (86.60 KB, patch)
2011-12-14 11:40 UTC, Giovanni Campagna
none Details | Review
Port to GSettings (85.96 KB, patch)
2011-12-14 17:56 UTC, Giovanni Campagna
needs-work Details | Review
Port to GSettings (119.61 KB, patch)
2012-02-06 17:26 UTC, Giovanni Campagna
committed Details | Review

Description Giovanni Campagna 2011-04-05 19:39:32 UTC
As part of https://live.gnome.org/GnomeGoals/GSettingsMigration, we should drop our dependency on GConf and move to GSettings.

This is on top of bug 587017
Comment 1 Giovanni Campagna 2011-04-05 19:41:12 UTC
Created attachment 185230 [details] [review]
Port to GSettings

Kill GWeatherPrefs and GWeatherGConf, as it is very easy to just
construct a GSettings object. Make GWeatherInfo use a default
settings object for its string methods. Make GWeatherLocation
serializable to a GVariant (in a GtkTreePath-like fashion) and store
that in the settings, killing WeatherLocation.
Comment 2 Giovanni Campagna 2011-04-05 20:48:31 UTC
Created attachment 185233 [details] [review]
Port to GSettings

Kill GWeatherPrefs and GWeatherGConf, as it is very easy to just
construct a GSettings object. Make GWeatherInfo use a default
settings object for its string methods. Make GWeatherLocation
serializable to a GVariant (in a GtkTreePath-like fashion) and store
that in the settings, killing WeatherLocation.

Previous patch was missing two files.
Comment 3 Bastien Nocera 2011-04-15 13:09:18 UTC
You probably want to look at bug 547478 as well, as Dan had some comments there.
Comment 4 Giovanni Campagna 2011-04-21 14:02:57 UTC
*** Bug 547478 has been marked as a duplicate of this bug. ***
Comment 5 Giovanni Campagna 2011-04-21 14:05:41 UTC
Responding to comments on the other bugs:

I think it is worth to keep settings separate between the clock applet (and the yet to be Weather App) and the library, as things like the preferred temperature unit should not change according to what viewer is used. Same for default-location, although it may be just removed at some time, in favor of GeoClue.
Comment 6 Javier Jardón (IRC: jjardon) 2011-09-07 12:31:32 UTC
Hey Giovanni, any news on this? Plan to fix this to 3.2 / 3.4 ?

This is one of the latest core modules still depending on gconf.
Comment 7 Giovanni Campagna 2011-09-07 12:50:16 UTC
(In reply to comment #6)
> Hey Giovanni, any news on this? Plan to fix this to 3.2 / 3.4 ?
> 
> This is one of the latest core modules still depending on gconf.

It's pending review from the libgweather maintainer, it may change according to the results of the GSoC on location aware shell (I don't know how much of that will be merged, and what requirement it has on libgweather), and it is on top of the introspection/GObject rewrite, which is a big API break. So, no, I don't think it's 3.2 material.
Comment 8 Matthias Clasen 2011-11-01 21:59:55 UTC
Looks like the patch needs some rebasing to apply to current master, unfortunately.
Comment 9 Giovanni Campagna 2011-11-15 15:51:32 UTC
Created attachment 201451 [details] [review]
Port to GSettings

Kill GWeatherPrefs and GWeatherGConf, as it is very easy to just
construct a GSettings object. Make GWeatherInfo use a default
settings object for its string methods.
Make the default location composed of a localized name, a metar code
and a pair of coordinates, of which the only really needed is the metar
code, which is looked up the in the database.

Rebased to current master.
Comment 10 Dan Winship 2011-12-13 15:27:37 UTC
Comment on attachment 201451 [details] [review]
Port to GSettings

Matthias asked me to review this...

>+AM_PROG_CC_C_O

pretty sure that's automatically implied by either LT_INIT() or AM_INIT_AUTOMAKE()

>+dnl -- check for gio (required) -----------------------------------------
>+PKG_CHECK_MODULES(GIO, 
>+		  [gio-2.0 >= $GLIB_REQUIRED])

This is implied by the combination of AM_PATH_GLIB_2_0($GLIB_REQUIRED) and GLIB_GSETTINGS

>+    <key name="radar" type="s">
>+      <default>""</default>

'' not "" isn't it?

>+      <_summary>Url for the radar map</_summary>

>+	The custom url from where to retrieve a radar map, or empty

"URL"

>+    <key name="temperature-unit" enum="org.gnome.GWeather.GWeatherTemperatureUnit">
>+      <!-- TRANSLATORS: pick a temperature unit that should be used by default in your
>+      locale; values must be quoted -->
>+      <_default l10n="messages">'kelvin'</_default>

given that there's no en_US.po, the defaults need to be correct for en_US.

>+	The unit of speed used for showing weather (for example for wind speed). Valid
>+	values are 'ms' (meters per second), 'kpm' (kilometers per hour), 'mph' (miles

'ms' seems wrong for 'meters per second', and 'kpm' is definitely wrong for 'kilometers per *hour*'.

>+      <_default>('', 'KPIT', nothing)</_default>

let's pick a less random default location...

maybe go with Washington DC or New York, and suggest in the translation note that a capital/largest city is a good default?

>+	The second field is the metar code for the default weather station. It must not be

"METAR"

>+typedef enum { /*< underscore_name=gweather_forecast_type >*/
>+    FORECAST_STATE,
>+    FORECAST_ZONE,
>+    FORECAST_LIST
>+} GWeatherForecastType;

if these are being installed then the values should be properly namespaced too

>diff --git a/libgweather/weather-iwin.c b/libgweather/weather-iwin.c

The changes in this file seem unrelated to the rest of the patch...


I mostly just skimmed the code changes. It all seems plausible if it works...

If gweather-applet still exists, it's going to need a lot of updating for this patch, since a lot of the gweather-gconf stuff was actually code that belonged in gweather-applet.
Comment 11 Giovanni Campagna 2011-12-14 11:31:34 UTC
(In reply to comment #10)
> (From update of attachment 201451 [details] [review])
> >+dnl -- check for gio (required) -----------------------------------------
> >+PKG_CHECK_MODULES(GIO, 
> >+		  [gio-2.0 >= $GLIB_REQUIRED])

No, it's needed for compiler flags.

> 
> >+    <key name="radar" type="s">
> >+      <default>""</default>
> 
> '' not "" isn't it?

glib docs say that both are accepted, and equivalent. Anyway, I'll change to ''.

> >+	The unit of speed used for showing weather (for example for wind speed). Valid
> >+	values are 'ms' (meters per second), 'kpm' (kilometers per hour), 'mph' (miles
> 
> 'ms' seems wrong for 'meters per second', and 'kpm' is definitely wrong for
> 'kilometers per *hour*'.

kpm was a typo (the actual enum is kph), ms is autogenerated from the C enum, so I left it as it was.

> >+      <_default>('', 'KPIT', nothing)</_default>
> 
> let's pick a less random default location...
> 
> maybe go with Washington DC or New York, and suggest in the translation note
> that a capital/largest city is a good default?

Went with New York City, Central Park.

> >diff --git a/libgweather/weather-iwin.c b/libgweather/weather-iwin.c
> 
> The changes in this file seem unrelated to the rest of the patch...

No, they're needed because I removed coordinates from the internal WeatherLocation structure and replaced it with latlon_valid.

> 
> I mostly just skimmed the code changes. It all seems plausible if it works...

It seems to... At least, it works for Pittsburgh. :)

> If gweather-applet still exists, it's going to need a lot of updating for this
> patch, since a lot of the gweather-gconf stuff was actually code that belonged
> in gweather-applet.

This code is on top of the introspection changes, so yes, there will be some porting to do in gweather-applet, but I don't think it will be that much, as GWeatherInfo abstracts most of the settings stuff.
Comment 12 Giovanni Campagna 2011-12-14 11:40:43 UTC
Created attachment 203443 [details] [review]
Port to GSettings

Kill GWeatherPrefs and GWeatherGConf, as it is very easy to just
construct a GSettings object. Make GWeatherInfo use a default
settings object for its string methods. Make GWeatherLocation
serializable to a GVariant (in a GtkTreePath-like fashion) and store
that in the settings, killing WeatherLocation.
Comment 13 Dan Winship 2011-12-14 14:13:25 UTC
(In reply to comment #11)
> > >diff --git a/libgweather/weather-iwin.c b/libgweather/weather-iwin.c
> > 
> > The changes in this file seem unrelated to the rest of the patch...
> 
> No, they're needed because I removed coordinates from the internal
> WeatherLocation structure and replaced it with latlon_valid.

ah, ok. I was going to say "well at least fix the indentation in the patch then", but now I see that it's actually the rest of the file that has broken indentation (no tabs), so ok. Remove the g_debug() though.
Comment 14 Giovanni Campagna 2011-12-14 17:56:55 UTC
Created attachment 203500 [details] [review]
Port to GSettings

Kill GWeatherPrefs and GWeatherGConf, as it is very easy to just
construct a GSettings object. Make GWeatherInfo use a default
settings object for its string methods. Make GWeatherLocation
serializable to a GVariant and store that in the settings,
killing WeatherLocation.
Comment 15 André Klapper 2012-01-18 20:13:19 UTC
Dan: Could you give the last patch a review if you find some time?
Comment 16 Dan Winship 2012-01-19 16:36:43 UTC
Comment on attachment 203500 [details] [review]
Port to GSettings

>+      <_default l10n="messages">'meters'</_default>
>+      <_default l10n="messages">'ms'</_default>
>+      <_default l10n="messages">'kpa'</_default>

These need to be fixed to en_US defaults as well. (miles, knots, inch_hg)

>+typedef enum { /*< underscore_name=gweather_forecast_type >*/
>+    GWEATHER_FORECAST_STATE,
>+    GWEATHER_FORECAST_ZONE,
>+    GWEATHER_FORECAST_LIST
>+} GWeatherForecastType;
>+
>+typedef enum { /*< underscore_name=gweather_temperature_unit >*/
>+    TEMP_UNIT_INVALID = 0,

Likewise, I meant that *every* type in gweather-enums.h needed properly-namespaced values, not just the one I'd pointed out.
Comment 17 Giovanni Campagna 2012-02-06 17:26:30 UTC
(In reply to comment #16)
> (From update of attachment 203500 [details] [review])
> >+      <_default l10n="messages">'meters'</_default>
> >+      <_default l10n="messages">'ms'</_default>
> >+      <_default l10n="messages">'kpa'</_default>
> 
> These need to be fixed to en_US defaults as well. (miles, knots, inch_hg)

Probably the most correct way to fix this is to have an en_US.po file, since meters and kilopascals are an International Standard, and the thus the only correct default for the portable locale. Included anyway

> >+typedef enum { /*< underscore_name=gweather_forecast_type >*/
> >+    GWEATHER_FORECAST_STATE,
> >+    GWEATHER_FORECAST_ZONE,
> >+    GWEATHER_FORECAST_LIST
> >+} GWeatherForecastType;
> >+
> >+typedef enum { /*< underscore_name=gweather_temperature_unit >*/
> >+    TEMP_UNIT_INVALID = 0,
> 
> Likewise, I meant that *every* type in gweather-enums.h needed
> properly-namespaced values, not just the one I'd pointed out.

I fixed also the enums in gweather-weather.h, although it makes the patch a little bigger. No code changes really, just search and replace.
Comment 18 Giovanni Campagna 2012-02-06 17:26:50 UTC
Created attachment 206917 [details] [review]
Port to GSettings

Kill GWeatherPrefs and GWeatherGConf, as it is very easy to just
construct a GSettings object. Make GWeatherInfo use a default
settings object for its string methods. Make GWeatherLocation
serializable to a GVariant and store that in the settings,
killing WeatherLocation.
Comment 19 Dan Winship 2012-02-10 17:53:33 UTC
Comment on attachment 206917 [details] [review]
Port to GSettings

i just skimmed this, but it looks right
Comment 20 Javier Jardón (IRC: jjardon) 2012-03-05 14:57:27 UTC
(In reply to comment #19)
> (From update of attachment 206917 [details] [review])
> i just skimmed this, but it looks right

Could this be committed then?
Comment 21 André Klapper 2012-03-09 11:34:02 UTC
Javier: Who did you ask?
Comment 22 Javier Jardón (IRC: jjardon) 2012-03-09 17:49:28 UTC
Comment on attachment 206917 [details] [review]
Port to GSettings

commit 7d32d907d0997d8dbdafc08ca807f132820660e3
Comment 23 Javier Jardón (IRC: jjardon) 2012-03-09 17:50:54 UTC
This was commited in master, now that libgweather is branched for gnome-3-4.

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