GNOME.org
 

Re: GSettings backend

> I'm really sorry for the spam. I swear it's my last e-mail (at least
>  for now).

No need to be sorry.  Thank you!

So, the patch looks good to me, besides some minor comments I made
below.  Whenever I found something that I felt was needing some
changes, I went ahead and did the change.

Roughly speaking, the changes I made fall into two categories.

[IConfMgr]

I leveraged on your work to hammer (a little bit) the interface of
IConfMgr w.r.t IConfMgr::set_key_dir_to_notify,
IConfMgr::add_key_to_notify, and IConfMgr::add_schema.

It seems to me that the purpose of these functions is to make IConfMgr
be aware of the namespace(s) of the configuration keys we want it to
deal with.  I have thus replaced these three functions by a new
function named IConfMgr::register_namespace.

For the GConf backend, a namespace would be e.g, "/apps/nemiver".  The
GConfMgr::register_namespace implementation listens to the value
changes of the keys that are below the path "/apps/nemiver".  The
implementation of IConfMgr::{get,set}_key_value just ignores the
namespace parameter.

For the GSettings backend, a namespace would be e.g, "org.nemiver".
The GSettings::register_namespace creates an instance of GSettings
against the schema named "org.nemiver".  The implementation of the
IConfMgr::{get,set}_key_value uses the namespace as the name of the
schema to consider.

I have defined a new configuration key CONF_NAMESPACE_NEMIVER that is
defined to "/apps/nemiver" when using the gconf backend and to
"org.nemiver" when using the gsettings backend.  I replaced The
IConfMgr::s_default_schema static variable by a
IConfMgr::get_default_namespace function that returns the
CONF_NAMESPACE_NEMIVER key.

The default argument of the namespace parameter of the {se,ge}tter is
now "".  The implementations of the {se,ge}tters use
IConfMgr::get_default_namespace to get the default namespace when they
are given an empty string namespace.

[Regression tests]

I have noticed that regression tests (under tests/) were broken.  I.e,
'make check' would yield many errors.  That's because the tests that
load the implementation of the IDebugger interface need to initialize
it with a configuration manager; so they load a configuration manager;
and until now they were unconditionally loading the gconf backend.  I
have made the necessary changes to make the regression tests load the
configuration backend that is selected at configure time.

I have added a helper function template in nmv-i-conf-mgr.h named
load_iface_and_confmgr.  It first loads the right configuration
manager and then loads a dynamic module that contains the
implementation of a given interface [using the the default dynamic
module manager].  This ensures that the needed configuration keys
related symbols are resolved before the latter dynamic module is
loaded.  All the regression tests that load the implementation of the
IDebugger interface now indirectly use this helper function template.

Note that to compile the regression tests, you need to have the
environment variable NEMIVER_DEVEL=on set prior to running either
autogen.sh or configure.  You also need to have the boost.test package
installed.

Besides these two main changes, I have a made other changes as well,
such as:

     - Use the new load_iface_and_confmgr in the main function to
       load the workbench dynamic module.

     - Remove a useless invocations of the (now defunct)
       IConfMgr::add_key_to_notify from CallStack::Priv::init_conf

Please find my specific comments below.

> Subject: [PATCH] Add GSettings backend to the configuration manager.
> 
> 	* configure.ac:
> 	* data/Makefile.am:
> 	* data/schemas/Makefile.am:
> 	* data/schemas/gconf/Makefile.am:
> 	* data/schemas/gconf/nemiver-dbgperspective.schemas:
> 	* data/schemas/gconf/nemiver-workbench.schemas:
> 	* data/schemas/gsettings/Makefile.am:
> 	* data/schemas/gsettings/org.nemiver.gschema.xml:
> 	* src/confmgr/gsettingsmgr.conf.in:
> 	* src/confmgr/Makefile.am:
> 	* src/confmgr/nmv-conf-keys.cc:
> 	* src/confmgr/nmv-conf-keys.h:
> 	* src/confmgr/nmv-gconf-mgr.cc:
> 	* src/confmgr/nmv-gsettings-mgr.cc:
> 	* src/confmgr/nmv-i-conf-mgr.h:
> 	* src/dbgengine/nmv-debugger-utils.cc:
> 	* src/dbgengine/nmv-gdb-engine.cc:
> 	* src/main.cc:
> 	* src/persp/dbgperspective/Makefile.am:
> 	* src/persp/dbgperspective/nmv-call-stack.cc:
> 	* src/persp/dbgperspective/nmv-dbg-perspective.cc:
> 	* src/persp/dbgperspective/schemas/Makefile.am:
> 	* src/persp/dbgperspective/schemas/nemiver-dbgperspective.schemas:
> 	* src/workbench/Makefile.am:
> 	* src/workbench/nmv-workbench.cc:
> 	* src/workbench/schemas/Makefile.am:
> 	* src/workbench/schemas/nemiver-workbench.schemas: Add GSettings Backend for
> 	the Configuration Manager.

This commit log doesn't follow the guidelines at
http://git.gnome.org/browse/nemiver/tree/git-commit-messages.README.
I have amended it to mention every single function that got modify and
say what was changed.

[...]

> diff --git a/configure.ac b/configure.ac
> index bdd0928..818308d 100644
> --- a/configure.ac
> +++ b/configure.ac

> @@ -64,6 +64,10 @@ CPPUNIT_VERSION=1.10.0
>  AC_SUBST([CPPUNIT_VERSION])
>  GTKHEX_VERSION=2.21.4
>  AC_SUBST([GTKHEX_VERSION])
> +LIBGIOMM_WITH_GSETTINGS_VERSION=2.25.1
> +AC_SUBST([GIOMM_WITH_GSETTINGS_VERSION])
> +GSETTINGS_DESKTOP_SCHEMAS=0.0.1
> +AC_SUBST([GSETTINGS_DESKTOP_SCHEMAS])
>  
>  dnl *********************
>  dnl Checks for programs.
> @@ -195,6 +199,29 @@ AC_ARG_ENABLE(symsvis,
>                ENABLE_GCC_SYMBOLS_VISIBILITY=$enableval,
>                ENABLE_GCC_SYMBOLS_VISIBILITY=no)
>  
> +
> +ENABLE_GSETTINGS=auto
> +AC_ARG_ENABLE(gsettings,
> +              AS_HELP_STRING([--enable-gsettings=yes|no],
> +                             [use gsettings instead of gconf (default is auto)]),
> +              ENABLE_GSETTINGS=$enableval,
> +              AC_CHECK_PROG([HAS_DCONF], [dconf], [yes], [no])
> +              [PKG_CHECK_EXISTS([giomm-2.4 >= $LIBGIOMM_WITH_GSETTINGS_VERSION gsettings-desktop-schemas >= $GSETTINGS_DESKTOP_SCHEMAS], [HAS_GSETTINGS=yes], [HAS_GSETTINGS=no])])
> +if test x$ENABLE_GSETTINGS = xyes || (test x$ENABLE_GSETTINGS = xauto \
> +      && test x$HAS_GSETTINGS = xyes \
> +      && test x$HAS_DCONF = xyes \
> +      && test x$ENABLE_GIO != xno); then

After the patches from Kaleb, GIO is always enabled now.  So the
variable ENABLE_GIO is not present anymore.  I have thus removed the
&& test x$ENABLE_GIO != xno branch from the version I am applying.

[...]


> diff --git a/src/confmgr/Makefile.am b/src/confmgr/Makefile.am
> index 84a739a..b3d75ce 100644
> --- a/src/confmgr/Makefile.am
> +++ b/src/confmgr/Makefile.am

[...]

> +if BUILD_GSETTINGS
> +    libgsettingsmgrmod_la_SOURCES=$(headers) $(h)/nmv-gsettings-mgr.cc\
> +            $(h)/nmv-gsettings-keys-defs.cc
> +    libgsettingsmgrmod_la_LDFLAGS=-module -avoid-version -Wl,--as-needed
> +    libgsettingsmgrmod_la_LIBADD=@NEMIVERCOMMON_LIBS@ @NEMIVERGSETTINGS_LIBS@ \
> +            $(abs_top_builddir)/src/common/libnemivercommon.la
> +
> +    gsettingsmgrmod_LTLIBRARIES=libgsettingsmgrmod.la
> +    gsettingsmgrmoddir=@NEMIVER_MODULES_DIR@
> +
> +    config_DATA=gsettingsmgr.conf
> +    EXTRA_DIST=gsettingsmgr.conf.in

I believe EXTRA_DIST should be defined "globally" outside of the 'if
UILD_GSETTINGS' otherwise when I do "make dist" to build the tarball
before the release, either gsettingsmgr.conf.in *or* gconfmgr.conf.in
will be present in the tarball that is shipped.  What we want is to
have both to be shipped, independently of whether I built with or
without gsettings.

So I have defined it outside of the 'if' as
"EXTRA_DIST=gconfmgr.conf.in gsettingsmgr.conf.in", and removed this
definition from within the 'if'.

[...]

> +else

[...]

> +    EXTRA_DIST=gconfmgr.conf.in

I have removed this too.

[...]

> diff --git a/src/confmgr/nmv-gconf-mgr.cc b/src/confmgr/nmv-gconf-mgr.cc
> index eee2aa0..1fe90ee 100644
> --- a/src/confmgr/nmv-gconf-mgr.cc
> +++ b/src/confmgr/nmv-gconf-mgr.cc

[...]

> +const UString IConfMgr::s_default_schema = "";

I have changed this into a virtual pure function returning a const
string named IConfMgr::get_default_namespace and changed the visibility
to make it public because client code might need to refer to it, at
least to invoke IConfMgr::add_schema.

[...]

> @@ -95,32 +118,7 @@ client_notify_func (GConfClient *a_client,

[...]


> +void
> +GConfMgr::add_schema (const UString &)

I renamed this function got renamed into GConfMgr::register_namespace.

[...]

>  bool
> -GConfMgr::get_key_value (const UString &a_key, UString &a_value)
> +GConfMgr::get_key_value (const UString &a_key,
> +                         UString &a_value,
> +                         const UString &a_schema)

The a_schema parameter is not used in the function so the compiler
complains when invoked with the relevant warnings turned on.  So I
changed this into:

-GConfMgr::get_key_value (const UString &a_key, UString &a_value)
+GConfMgr::get_key_value (const UString &a_key,
+                         UString &a_value,
+                         const UString &)

I did a similar change for the subsequent similar hunks.

[...]


> diff --git a/src/confmgr/nmv-i-conf-mgr.h b/src/confmgr/nmv-i-conf-mgr.h
> index f7c9f06..63647ee 100644
> --- a/src/confmgr/nmv-i-conf-mgr.h
> +++ b/src/confmgr/nmv-i-conf-mgr.h

[...]

> @@ -48,6 +49,8 @@ class NEMIVER_API IConfMgr : public DynModIface {
>      IConfMgr (const IConfMgr &);
>      IConfMgr& operator= (const IConfMgr &);
>  
> +    static const UString s_default_schema;
> +

I removed this and replaced it by this function with public
visibility:

virtual const UString& get_default_namespace () const = 0;

> @@ -62,26 +65,46 @@ public:
>  
>      virtual void set_key_dir_to_notify (const UString &a_key_dir) = 0;
>      virtual void add_key_to_notify (const UString &a_key) = 0;
> +    virtual void add_schema (const UString &a_schema) = 0;

I replaced the three functions above with this function:

virtual void register_namespace
        (const UString &a_namespace = /*default namespace*/"") = 0;

[...]

> -    virtual bool get_key_value (const UString &a_key, UString &a_value) = 0;
> -    virtual void set_key_value (const UString &a_key, const UString &a_value) = 0;
> +    virtual bool get_key_value (const UString &a_key,
> +                                UString &a_value,
> +                                const UString &a_schema = s_default_schema) = 0;

I changed the parameter name a_schema into a_namespace.  And the
default argument is now the empty string, as s_default_schema doesn't
exist anymore.  I did a similar change for the similar subsequent
hunks.  I believe I did the a_schema -> a_namespace renaming in all
the patch, actually.

[...]

> diff --git a/src/main.cc b/src/main.cc
> index 305a9d0..51a8170 100644
> --- a/src/main.cc
> +++ b/src/main.cc

[...]

> @@ -607,6 +608,7 @@ main (int a_argc, char *a_argv[])
>      //load and init the workbench dynamic module
>      //********************************************
>      DynamicModuleManager module_manager;
> +    module_manager.load_iface<IConfMgr> (CONFIG_MGR_MODULE_NAME, "IConfMgr");
>      IWorkbenchSafePtr workbench =
>          module_manager.load_iface<IWorkbench> ("workbench",
>      "IWorkbench");

I have changed the two lines above by:

IWorkbenchSafePtr workbench =
        nemiver::load_iface_and_confmgr<IWorkbench> ("workbench",
                                                     "IWorkbench");


[...]

> diff --git a/src/persp/dbgperspective/Makefile.am b/src/persp/dbgperspective/Makefile.am
> index 12bf939..75f7690 100644
> --- a/src/persp/dbgperspective/Makefile.am
> +++ b/src/persp/dbgperspective/Makefile.am

[...]

> diff --git a/src/persp/dbgperspective/nmv-dbg-perspective.cc b/src/persp/dbgperspective/nmv-dbg-perspective.cc
> index bcbda7e..ffa38af 100644
> --- a/src/persp/dbgperspective/nmv-dbg-perspective.cc
> +++ b/src/persp/dbgperspective/nmv-dbg-perspective.cc

[...]

> @@ -4819,7 +4836,8 @@ DBGPerspective::read_default_config ()
>      conf_mgr.get_key_value (CONF_KEY_CUSTOM_FONT_NAME,
>                              m_priv->custom_font_name);
>      conf_mgr.get_key_value (CONF_KEY_SYSTEM_FONT_NAME,
> -                            m_priv->system_font_name);
> +                            m_priv->system_font_name,
> +                            "org.gnome.desktop.interface");

I changed the reference to "org.gnome.desktop.interface" into
CONF_NAMESPACE_DESKTOP_INTERFACE.


[...]

> diff --git a/src/workbench/nmv-workbench.cc b/src/workbench/nmv-workbench.cc
> index 73444ba..86ef9a1 100644
> --- a/src/workbench/nmv-workbench.cc
> +++ b/src/workbench/nmv-workbench.cc

[...]

> @@ -529,13 +514,17 @@ Workbench::get_configuration_manager ()

[...]


> +        m_priv->conf_mgr->add_schema ("org.nemiver");
> +        m_priv->conf_mgr->add_schema ("org.gnome.desktop.interface");

I changed this into:

+        m_priv->conf_mgr->register_namespace (/*default nemiver namespace*/);
+        m_priv->conf_mgr->register_namespace (CONF_NAMESPACE_DESKTOP_INTERFACE);


>          m_priv->conf_mgr->set_key_dir_to_notify ("/apps/nemiver");

I have removed this line ...

> -        m_priv->conf_mgr->add_key_to_notify (
> -                "/desktop/gnome/interface/monospace_font_name");
> +        m_priv->conf_mgr->add_key_to_notify
> -                (CONF_KEY_SYSTEM_FONT_NAME);

... as well as this one.


Thank you for the great work.  I find it nice that Nemiver now has
support for the GConf *and* GSettings, without having a single #ifdef
GCONF/GSETTING in the client code.

I applied the patch to the master branch at f3b5b02. Following my
discussion with Kaleb, there is also a gtk2-branch now.  I have applied
this patch there as well.  The hash is 548fdad.  More on that branch
later.

Thanks!

-- 
		Dodji
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]