GNOME Bugzilla – Bug 646854
Port to GSettings
Last modified: 2012-03-09 17:50:54 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
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.
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.
You probably want to look at bug 547478 as well, as Dan had some comments there.
*** Bug 547478 has been marked as a duplicate of this bug. ***
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.
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.
(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.
Looks like the patch needs some rebasing to apply to current master, unfortunately.
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 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.
(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.
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.
(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.
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.
Dan: Could you give the last patch a review if you find some time?
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.
(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.
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 on attachment 206917 [details] [review] Port to GSettings i just skimmed this, but it looks right
(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?
Javier: Who did you ask?
Comment on attachment 206917 [details] [review] Port to GSettings commit 7d32d907d0997d8dbdafc08ca807f132820660e3
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