GNOME Bugzilla – Bug 624485
port to gsettings
Last modified: 2010-10-08 14:15:35 UTC
ssia
Created attachment 168145 [details] [review] egg-editable-toolbar: don't force visibility on actions GtkActions set the visibility of their proxies by themselves, we don't have to force the visibility with gtk_widget_show. Bug #624485
Created attachment 168146 [details] [review] ephy-action-helper: minor change in the sensitivity API ephy_action_change_sensitivity_flags now returns the current value of the 'sensitive' property of the GtkAction. Bug #624485
Created attachment 168147 [details] [review] gsettings: port epiphany to gsettings This commit includes all the smaller/simpler ports. Bug #624485
The first two patches are minor bugfixes of stuff I hit when porting. The third one is the actuar porting. It's huge (ignore the bogus commit message, I already rewrote it locally).
Comment on attachment 168146 [details] [review] ephy-action-helper: minor change in the sensitivity API What is this used for? Also, I don't think any of the comments you have added add much to the code. The ones in the if/else are trivial, teh other one is duplicated in the doc.
(In reply to comment #5) > (From update of attachment 168146 [details] [review]) > What is this used for? Also, I don't think any of the comments you have added > add much to the code. The ones in the if/else are trivial, teh other one is > duplicated in the doc. Agree about comments. It's used for ephy-lockdown.c porting, check sensitive_get_mapping: +static gboolean +sensitive_get_mapping (GValue *value, + GVariant *variant, + gpointer data) + { + gboolean active, sensitive; + active = g_variant_get_boolean (variant); + sensitive = ephy_action_change_sensitivity_flags (GTK_ACTION (data), + LOCKDOWN_FLAG, + active); + + g_value_set_boolean (value, sensitive); + + return TRUE; +} This function sets/unsets the "LOCKDOWN_FLAG" in "data" (a GtkAction), however, our "sensitivity_flags" API only allows sensitiveness when no more flags are blocking the GtkAction. The get_mapping must set the new @value (when the GSetting change) for the ::sensitive property of the widget it's connected to. The problem arises when the setting releases the lock (i.e. the LOCKDOWN_FLAG is no more) *but* there's some other flag blocking (see ephy-window.c). Hence, if we were to use a 1-to-1 mapping we would end with elements that suddenly turn sensitive when its associated GSetting changed; despite any other sensitivity flag blocking it. Summary: we need to know the new ::sensitive property value after ephy_action_change_sensitivity_flags, to cover that I just made it return the new state of the GtkAction. I could have used a g_object_get () but I thought it was an useful hint to have in the API.
Comment on attachment 168145 [details] [review] egg-editable-toolbar: don't force visibility on actions I believe you. You should push this to the egg repo?
(In reply to comment #7) > (From update of attachment 168145 [details] [review]) > I believe you. You should push this to the egg repo? We need to sync with libegg upstream. We have a somewhat big diff right now. But it requires careful checking of the diff. But that's another bug. It's likely that this fix is needed anyway though. I can work on that after getting this out of the inbox.
Comment on attachment 168145 [details] [review] egg-editable-toolbar: don't force visibility on actions This was committed to libegg and we just updated our libegg copy.
Ok, some pointers to test this: - dconf needs to be running, it's the binary in $PREFIX/libexec/ not in $PREFIX/bin/ - migration is done progressively, it's safe to interrupt it, if you want to ensure it starts again from scratch, nuke: + ~/.local/share/gsettings-data-convert (.ini file with migrations done) + ~/.config/dconf/user (dconf db) - you need gconf from git master to have reliable migration binaries, you don't need the newer gconf daemon running, this migration binaries are standalone. - settings are kept in memory if dconf is not running, that means they are lost when you close epiphany. - REMEMBER! prefix/libexec ; NOT prefix/bin. - gsettings set|get <schema> <key> <value> is your friend. + consider that "string" (s) values need double quotes around single quotes: "'new value'" + string lists are similar, they require some funky combination like: "'['a', 'b']'" or something, just fool around until you hit it, or try "gsettings get" and add quotes to whatever it spits.
This is everything I had to apply to your patch just to make it compile: diff --git a/Makefile.am b/Makefile.am index 6fe44ea..68235f7 100644 --- a/Makefile.am +++ b/Makefile.am @@ -4,7 +4,7 @@ if ENABLE_TESTS SUBDIRS += tests endif -ACLOCAL_AMFLAGS = -I m4 +ACLOCAL_AMFLAGS = -I m4 ${ACLOCAL_FLAGS} NULL = diff --git a/configure.ac b/configure.ac index dcfd3b1..a79f277 100644 --- a/configure.ac +++ b/configure.ac @@ -78,6 +78,8 @@ AC_PATH_PROG([GLIB_MKENUMS],[glib-mkenums]) IT_PROG_INTLTOOL([0.40.0]) +GLIB_GSETTINGS + PKG_PROG_PKG_CONFIG GNOME_DEBUG_CHECK diff --git a/data/epiphany.convert b/data/epiphany.convert index a4fd6a3..764efd9 100644 --- a/data/epiphany.convert +++ b/data/epiphany.convert @@ -55,3 +55,6 @@ print-setup = /desktop/gnome/lockdown/disable_print_setup command-line = /desktop/gnome/lockdown/disable_command_line quit = /apps/epiphany/lockdown/disable_quit javascript-chrome = /apps/epiphany/lockdown/disable_javascript_chrome +hide-menubar = /apps/epiphany/lockdown/hide_menubar + + diff --git a/data/org.gnome.Epiphany.gschema.xml b/data/org.gnome.Epiphany.gschema.xml index 97980cb..23d2606 100644 --- a/data/org.gnome.Epiphany.gschema.xml +++ b/data/org.gnome.Epiphany.gschema.xml @@ -293,5 +293,8 @@ <key name="javascript-chrome" type="b"> <default>false</default> </key> + <key name="hide-menubar" type="b"> + <default>false</default> + </key> </schema> </schemalist> diff --git a/lib/Makefile.am b/lib/Makefile.am index 1e41625..f191be9 100644 --- a/lib/Makefile.am +++ b/lib/Makefile.am @@ -76,6 +76,7 @@ libephymisc_la_CPPFLAGS = \ -I$(top_builddir)/lib \ -I$(top_builddir)/lib/egg \ -I$(top_srcdir)/lib/egg \ + -I$(top_srcdir)/embed \ -DDATADIR="\"$(datadir)\"" \ -DSHARE_DIR=\"$(pkgdatadir)\" \ -DEXTENSIONS_DIR=\""$(libdir)/epiphany/$(EPIPHANY_API_VERSION)/extensions"\" \ diff --git a/lib/ephy-prefs.h b/lib/ephy-prefs.h index 6c28073..e36cfb7 100644 --- a/lib/ephy-prefs.h +++ b/lib/ephy-prefs.h @@ -115,6 +115,7 @@ typedef enum #define EPHY_PREFS_LOCKDOWN_COMMAND_LINE "command-line" #define EPHY_PREFS_LOCKDOWN_QUIT "quit" #define EPHY_PREFS_LOCKDOWN_JAVASCRIPT_CHROME "javascript-chrome" +#define EPHY_PREFS_LOCKDOWN_HIDE_MENUBAR "hide-menubar" G_END_DECLS diff --git a/src/prefs-dialog.c b/src/prefs-dialog.c index 3884ff9..8b8ab21 100644 --- a/src/prefs-dialog.c +++ b/src/prefs-dialog.c @@ -537,7 +537,6 @@ setup_add_language_dialog (PrefsDialog *pd) NULL)); ephy_dialog_construct (dialog, - NULL, ephy_file ("prefs-dialog.ui"), "add_language_dialog", NULL); -- GLIB_GSETTINGS being completely missing made me waste a lot of time in particular, please try to be more careful. Also you made code in /lib depend on /embed (although the -I was missing, how could it compile for you?). This is not right in general, we need to figure out to avoid it. Perhaps the accessors for the settings could me moved to lib/ and be made completely generic, accessible to all epiphany.
Created attachment 169328 [details] [review] gsettings: port epiphany to gsettings This new patch addresses most of what we talked before on jabber: - created ephy_settings_get (char *schema) instead of ephy_embed_shell_get_settings (shell, schema), this lives in lib/ and has an ephy_settings_shutdown () function to clear the GHashTable (sounds right?) - fixed the build failure - removed an useless SHOW_STATUSBAR item What I left out from our previous conversation was moving stuff from ephy-prefs.h to ephy-embed-prefs.h. IMHO, keeping all the settings in one place is more organised, and anyway we were using settings defined in embed/ -not only in lib/- already. Also consider that we were using some settings defined in ephy-embed-prefs.h in some places in src/, this means that they were not really "exclusive" to ephy-embed-prefs (user css for example); I believe this is just old chaos from migration days.
(In reply to comment #12) > Created an attachment (id=169328) [details] [review] > gsettings: port epiphany to gsettings > > This new patch addresses most of what we talked before on jabber: > - created ephy_settings_get (char *schema) instead of > ephy_embed_shell_get_settings (shell, schema), this lives in lib/ and has an > ephy_settings_shutdown () function to clear the GHashTable (sounds right?) > - fixed the build failure > - removed an useless SHOW_STATUSBAR item > And also removed that change to sensitivity API :).
Review of attachment 169328 [details] [review]: ::: data/Makefile.am @@ +76,3 @@ $(m4data_DATA) \ + $(convert_DATA) \ + $(gsettings_SCHEMAS) \ Try to align the '\' @@ +85,3 @@ $(pkgconfig_DATA) \ + gschemas.compiled + $(default_bookmarks_DATA) \ Same. ::: lib/Makefile.am @@ +78,1 @@ -I$(top_builddir)/lib \ This shouldn't be needed now. ::: lib/ephy-dialog.c @@ -72,3 @@ - gboolean sane_state; -} PropertyInfo; - There's a shitload of code removed in ephy-dialog. Care to give a general overview of what you are doing and why? I don't see any obvious replacement for all that functionality. This is the only really scary thing of the patch IMHO, everything else looks pretty straightforward. ::: lib/ephy-settings.c @@ +62,3 @@ + + return gsettings; +} Epic indentation fail. ::: lib/ephy-settings.h @@ +37,3 @@ +#define EPHY_SETTINGS_LOCKDOWN ephy_settings_get (EPHY_PREFS_LOCKDOWN_SCHEMA) +#define EPHY_SETTINGS_STATE ephy_settings_get (EPHY_PREFS_STATE_SCHEMA) + Align those. @@ +40,3 @@ + +GSettings *ephy_settings_get (char *schema); + That can be const char*. ::: src/ephy-extensions-manager.c @@ -701,1 @@ } Don't mix this kind of thing in the patch... ::: src/ephy-lockdown.c @@ +286,1 @@ } Useless? ::: src/ephy-main.c @@ +578,3 @@ + EPHY_PREFS_LOCKDOWN_ARBITRARY_URL); + arbitrary_url = g_settings_get_boolean (lockdown_settings, + lockdown_settings = g_settings_new (EPHY_PREFS_LOCKDOWN_SCHEMA); Can probably unref it right here.
Created attachment 169391 [details] [review] gsettings: port epiphany to gsettings This addresses your comments. Now, for the ephy-dialog explanation: EphyDialog code that was removed was helper code to bind GtkWidgets to GConf settings. The replacement for all that is g_settings_bind_* API. The most complex cases not covered by plain GSettings API are at most 3 in all our code iirc. The reason to remove it then was that it's functionality is deprecated by GSettings API. In the future, if we need something like this, we would have had to rewire the whole thing for GSettings anyway.
(In reply to comment #15) > Created an attachment (id=169391) [details] [review] > gsettings: port epiphany to gsettings > > This addresses your comments. Now, for the ephy-dialog explanation: > > EphyDialog code that was removed was helper code to bind GtkWidgets to GConf > settings. The replacement for all that is g_settings_bind_* API. > The most complex cases not covered by plain GSettings API are at most 3 in all > our code iirc. > > The reason to remove it then was that it's functionality is deprecated by > GSettings API. In the future, if we need something like this, we would have had > to rewire the whole thing for GSettings anyway. That was a bit confusing. So is g_settings_bind et al effectively reimplementing all that or are there things left to do? If there are, which ones? Was all that code only wiring gconf changes to the widgets and viceversa?
(In reply to comment #16) > > That was a bit confusing. So is g_settings_bind et al effectively > reimplementing all that or are there things left to do? If there are, which > ones? Was all that code only wiring gconf changes to the widgets and viceversa? Yes. The GSettings API replaces all of what I removed. I also removed a ephy_dialog_get|set_modal, since the modal property is useless/duplicate. GSettings gives us g_settings_bind () and g_settings_bind_with_mapping (); first one does a automagic binding between properties, this is what replaced 95% of ephy-dialog code. g_settings_bind_with_mapping () covered the other 5%. For example for encoding selection, GtkComboBox::active is an int, but in GSettings we store a string; similar with cookie accept policy, we store an enum (int) but WebKitSettings requires a string. We are forced to do mappings only for cookies and encodings iirc. In some other places we used bind_with_mapping but for totally unrelated reasons like in ephy-embed-prefs.c
Created attachment 169407 [details] [review] ephy-dialog: remove useless modal property Bug #624485
Created attachment 169408 [details] [review] gsettings: port epiphany to gsettings Adds our own schemas, a migration file and removes old gconf API and files. Bug #624485
As discussed on jabber, I moved the modal property removal to another patch.
Comment on attachment 169407 [details] [review] ephy-dialog: remove useless modal property I hope this doesn't break anything. Attachment 169407 [details] pushed as 43e7d50 - ephy-dialog: remove useless modal property
Created attachment 169469 [details] [review] gsettings: port epiphany to gsettings Fixes lolvims and i18n. Also consider patch for e-e in bug #628752
I'm worried about some apparently random changes in the preference names: +warn-unsubmitted-data = /apps/epiphany/dialogs/warn_on_close_unsubmitted_data This is less descriptive to the point of being hard to tell what it's talking about. +middle-click-open = /apps/epiphany/general/middle_click_open_url Same. +search-url = /apps/epiphany/general/url_search Uh? +enable-smooth-scrolling = /apps/epiphany/web/smooth_scroll +enable-web-inspector = /apps/epiphany/web/inspector_enabled +enable-caret-browsing = /apps/epiphany/web/browse_with_caret +enabled-extensions = /apps/epiphany/general/active_extensions So all boolean preferences are to be positive and with 'enabled' prefix? +show-tabs-bar-always = /apps/epiphany/general/always_show_tabs_bar I guess not :) Why the change in wording here? +encoding-default = /apps/epiphany/web/default_encoding Seems even stranger than search-url +javascript-chrome = /apps/epiphany/lockdown/disable_javascript_chrome This preference used to be negative. You have removed that from the name but have not changed the way it's used, so this is really confusing now IMHO. The same applies for many prefs in lockdown section. I think in general we need some written guidelines here, at least in the bug, and stick to those.
Ok, idea: - boolean preferences should be prefixed with enable- - preferences under lockdown should (have|have not) a disable- prefix - key names should describe the consequence of the key: click-icon-quits; double-click-reloads; - 'things' (not booleans) should describe what they are for: keyword-search-url; default-encoding; cookie-policy; 1 and 3 conflict a bit I guess. Something else I'm missing (likely I am)?
Created attachment 169886 [details] [review] gsettings: port epiphany to gsettings Adds our own schemas, a migration file and removes old gconf API and files. Bug #624485
+[org.gnome.Epiphany.web] +rendering-font = /apps/epiphany/web/font +rendering-min-font-size = /apps/epiphany/web/minimum_font_size +rendering-language = /apps/epiphany/web/language +rendering-use-own-fonts = /apps/epiphany/web/use_own_fonts +rendering-use-own-colors = /apps/epiphany/web/use_own_colors I think the rendering- prefix is not needed here. +disable-hide-menubar = /apps/epiphany/lockdown/hide_menubar Just disable-menubar? Can you please add to the patch (in NEWS or somewhere) the instructions to migrate your settings manually?
Created attachment 170732 [details] [review] gsettings: port epiphany to gsettings Adds our own schemas, a migration file and removes old gconf API and files. Bug #624485 This addresses the last comments. I have a fix for epiphany-extensions ready in bug #628752 for when we remove the ephy-dialog stuff.
You are still missing the tutorial to do the migration manually AFAICT.
OK, after a bunch of fixes that I made locally I have pushed this to master. I've checked that the migration works and that running with dconf gives you persistent settings and all that :)