Re: GSettings backend
- From: Dodji Seketeli <dodji seketeli org>
- To: The mailing list of the Nemiver project <nemiver-list gnome org>
- Subject: Re: GSettings backend
- Date: Tue, 22 Mar 2011 22:49:34 +0100
Hello Fabien,
> I've decided to stop procrastinating and send the patch, that is
> ready for more than a week.
Thank you.
I like the general idea and implementation of the patch. I made a few
comments, mostly because I think your patch shows that Nemiver needs
some re-factoring with respect to configuration keys handling. And if
you agree, I guess this could be a good opportunity for it.
I understand that you might not have time to address all the comments
I am doing below. If that is the case, please tell me where I can
help in implementing what I am proposing.
> - I had to learn more about autotools (I never had to do a lot more
> than adding files until now), so I think a review of this code is
> necessary.
Autotools keep us all very humble, I believe :-) Though, I have yet to
see any build system that is as powerful. Thank you for taking the
time to dive into the realm of that dragon.
> If you think the code is correct enough to push in a new branch,
> please let me know.
It is up to you. You can stick it in a branch on Nemiver's Git if you
want. You can as well, just refresh the patch in your local tree and
send an updated patch to the list when it's necessary. I am fine
either way.
So please find my comments below.
[...]
> diff --git a/configure.ac b/configure.ac
> index 397b645..c20a09f 100644
> --- a/configure.ac
> +++ b/configure.ac
[...]
> @@ -66,6 +66,10 @@ CPPUNIT_VERSION=1.10.0
> AC_SUBST([CPPUNIT_VERSION])
> GTKHEX_VERSION=2.21.4
> AC_SUBST([GTKHEX_VERSION])
> +LIBGSETTINGS_VERSION=2.25.1
It seems to me that this version number is to test that the giomm
component has a version number that is high enough to contain the
gsettings feature. I would propose to make this business more
explicit by naming this macro GIOMM_WITH_GSETTINGS_VERSION instead.
[...]
>
> dnl *********************
> dnl Checks for programs.
> @@ -232,6 +236,22 @@ AC_ARG_ENABLE(symsvis,
> ENABLE_GCC_SYMBOLS_VISIBILITY=$enableval,
> ENABLE_GCC_SYMBOLS_VISIBILITY=no)
>
Here, I think you need to initialize ENABLE_GSETTINGS to, e.g, 'auto',
like in:
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])n
> + [PKG_CHECK_EXISTS([giomm-2.4 >= $LIBGSETTINGS_VERSION gsettings-desktop-schemas >= $GSETTINGS_DESKTOP_SCHEMAS], [HAS_GSETTINGS=yes], [HAS_GSETTINGS=no])])
> +if test x$HAS_GSETTINGS = xyes && test x$HAS_DCONF = xyes && test x$ENABLE_GIO = xyes; then
And the above test would then *also* check that we are in 'auto' mode;
like:
if test x$ENABLE_GSETTINGS=xauto \
&& test x$HAS_GSETTINGS = xyes \
&& test x$HAS_DCONF = xyes \
&& test x$ENABLE_GIO = xyes; then
> + AC_DEFINE([WITH_GSETTINGS], 1, [build with gsettings instead of gconf])
> + CONF_MGR=gsettings
> + ENABLE_GSETTINGS=yes
> +else
> + CONF_MGR=gconf
> + ENABLE_GSETTINGS=no
And the above 'else' branch would become:
else
CONF_MGR=gconf
if x$ENABLE_GSETTINGS=xauto; then
ENABLE_GSETTINGS=no
fi
The idea behind this dance I am proposing is, if he user provided the
--enable-gsettings flag (so if ENABLE_GSETTINGS is set to yes), we
have to honour it without testing if the gsettings, gio and dconf
dependencies are there or not. We do the dependency test only if the
user provided neither --enable-gsettings nor --disable-gsettings.
> +fi
[...]
> -AC_PATH_PROG(GCONFTOOL, gconftool-2)
> +if test x$CONF_MGR = xgsettings ; then
> + dnl library dependencies for the nemiver gsettings plugin
> + DEP_GSETTINGS="giomm-2.4 >= $LIBGSETTINGS_VERSION \
> + gsettings-desktop-schemas >= $GSETTINGS_DESKTOP_SCHEMAS"
> +
> + PKG_CHECK_MODULES(NEMIVERGSETTINGS, $DEP_GSETTINGS)
We have already done this PKG_CHECK_MODULE check to determine the
presence of the gsettings feature and to set the CONF_MGR macro to
gsettings. So doing this test here again seems unnecessary to me. I
think we can remove it, as well as the assignment to the DEP_GSETTINGS
macro above.
> + AC_SUBST(NEMIVERGSETTINGS_LIBS)
> + AC_SUBST(NEMIVERGSETTINGS_CFLAGS)
> +
> + GLIB_GSETTINGS
> +elif test x$CONF_MGR = xgconf ; then
> + PKG_CHECK_MODULES(GCONF,[gconf-2.0 >= $GCONF_VERSION])
> +
> + AC_PATH_PROG(GCONFTOOL, gconftool-2)
> + AC_SUBST(GCONFTOOL)
> +else
> + AC_MSG_ERROR([Configuration Manager undetermined. Abort...])
> +fi
> +
> AM_GCONF_SOURCE_2
Shouldn't we move this AM_GCONF_SOURCE_2 invocation into the 'elif
test x$CONF_MGR = xgconf' branch above?
[...]
> diff --git a/data/Makefile.am b/data/Makefile.am
> index 624c153..55c21d0 100644
> --- a/data/Makefile.am
> +++ b/data/Makefile.am
> @@ -5,6 +5,20 @@ desktop_in_files = nemiver.desktop.in
> desktop_DATA = $(desktop_in_files:.desktop.in=.desktop)
> @INTLTOOL_DESKTOP_RULE@
>
> +gsettingsschema_DATA = org.nemiver.gschema.xml
> +
> +if BUILD_GSETTINGS
> +install-data-local: install-schemas
> +
> +uninstall-local: uninstall-schemas
> +
> +install-schemas: install-gsettingsschemaDATA
> + $(GLIB_COMPILE_SCHEMAS) $(gsettingsschemadir)
> +
> +uninstall-schemas: uninstall-gsettingsschemaDATA
> + $(GLIB_COMPILE_SCHEMAS) $(gsettingsschemadir)
> +endif
> +
GConf schemas for the debugging perspective are handled in
src/persp/dbgperspective/schemas/Makefile.am. Maybe we should move
those schemas from that directory to data/ as well and make use of
your new "if BUILD_GETTINGS" conditional?
[...]
> diff --git a/src/confmgr/nmv-conf-keys.cc b/src/confmgr/nmv-conf-keys.cc
> index b3f13b5..89f81c2 100644
> --- a/src/confmgr/nmv-conf-keys.cc
> +++ b/src/confmgr/nmv-conf-keys.cc
> @@ -27,45 +27,13 @@
>
> NEMIVER_BEGIN_NAMESPACE (nemiver)
>
> -const char* CONF_KEY_NEMIVER_SOURCE_DIRS =
> - "/apps/nemiver/dbgperspective/source-search-dirs";
> -const char* CONF_KEY_SHOW_DBG_ERROR_DIALOGS =
> - "/apps/nemiver/dbgperspective/show-dbg-error-dialogs";
> -const char* CONF_KEY_SHOW_SOURCE_LINE_NUMBERS =
> - "/apps/nemiver/dbgperspective/show-source-line-numbers";
> -const char* CONF_KEY_CONFIRM_BEFORE_RELOAD_SOURCE =
> - "/apps/nemiver/dbgperspective/confirm-before-reload-source";
> -const char* CONF_KEY_ALLOW_AUTO_RELOAD_SOURCE =
> - "/apps/nemiver/dbgperspective/allow-auto-reload-source";
> -const char* CONF_KEY_HIGHLIGHT_SOURCE_CODE =
> - "/apps/nemiver/dbgperspective/highlight-source-code";
> -const char* CONF_KEY_SOURCE_FILE_ENCODING_LIST =
> - "/apps/nemiver/dbgperspective/source-file-encoding-list";
> -const char* CONF_KEY_USE_SYSTEM_FONT =
> - "/apps/nemiver/dbgperspective/use-system-font";
> -const char* CONF_KEY_CUSTOM_FONT_NAME=
> - "/apps/nemiver/dbgperspective/custom-font-name";
> -const char* CONF_KEY_SYSTEM_FONT_NAME=
> - "/desktop/gnome/interface/monospace_font_name";
> -const char* CONF_KEY_USE_LAUNCH_TERMINAL =
> - "/apps/nemiver/dbgperspective/use-launch-terminal";
> -const char* CONF_KEY_STATUS_WIDGET_MINIMUM_WIDTH=
> - "/apps/nemiver/dbgperspective/status-widget-minimum-width";
> -const char* CONF_KEY_STATUS_WIDGET_MINIMUM_HEIGHT=
> - "/apps/nemiver/dbgperspective/status-widget-minimum-height";
> -const char* CONF_KEY_STATUS_PANE_LOCATION=
> - "/apps/nemiver/dbgperspective/status-pane-location";
> -const char* CONF_KEY_DEBUGGER_ENGINE_DYNMOD_NAME =
> - "/apps/nemiver/dbgperspective/debugger-engine-dynmod";
> -const char* CONF_KEY_EDITOR_STYLE_SCHEME =
> - "/apps/nemiver/dbgperspective/editor-style-scheme";
> -const char* CONF_KEY_ASM_STYLE_PURE =
> - "/apps/nemiver/dbgperspective/asm-style-pure";
> -const char* CONF_KEY_DEFAULT_NUM_ASM_INSTRS =
> - "/apps/nemiver/dbgperspective/default-num-asm-instrs";
> -const char *CONF_KEY_GDB_BINARY = "/apps/nemiver/dbgperspective/gdb-binary";
> -const char *CONF_KEY_FOLLOW_FORK_MODE = "/apps/nemiver/dbgperspective"
> - "/follow-fork-mode";
> -const char* CONF_KEY_CONTEXT_PANE_LOCATION =
> - "/apps/nemiver/dbgperspective/context-pane-location";
As I understand it, these keys definitions cannot be left here like
they were previously, because depending on whether we are using
gsettings or gconf, the value or these variables are not the same. So
I agree with this hunk that removes the keys definition from here.
I would even go further and say that each IConfMgr backend (gconf and
gsetting) should then define their own version of key variables, with
their own syntax; i.e for the GConf backend a key definition would be:
const char *CONF_KEY_ONE = "foo/bar";
and the gsettings one. would be:
const char *CONF_KEY_ONE = "foo-bar";
The file src/confmgr/nmv-conf-keys.cc would then be removed
completely.
For this to work, the confmgr module would need to be loaded before
any module that would use a configuration key. So we would need
something like the load_debugger_iface_with_gconf function from
nmv-debugger-utils.cc. Maybe a function named load_iface_with_conf,
that would first load the right configmgr and then load the
module/iface requested. That function would be defined inline in
nmv-i-conf-mgr.h, for example.
What do you think about that?
[...]
> diff --git a/src/confmgr/nmv-conf-keys.h b/src/confmgr/nmv-conf-keys.h
> index 68c29cb..1c5436e 100644
> --- a/src/confmgr/nmv-conf-keys.h
> +++ b/src/confmgr/nmv-conf-keys.h
> @@ -30,27 +30,7 @@
>
> NEMIVER_BEGIN_NAMESPACE (nemiver)
>
> -extern const char* CONF_KEY_NEMIVER_SOURCE_DIRS;
> -extern const char* CONF_KEY_SHOW_DBG_ERROR_DIALOGS;
> -extern const char* CONF_KEY_SHOW_SOURCE_LINE_NUMBERS;
> -extern const char* CONF_KEY_CONFIRM_BEFORE_RELOAD_SOURCE;
> -extern const char* CONF_KEY_ALLOW_AUTO_RELOAD_SOURCE;
> -extern const char* CONF_KEY_HIGHLIGHT_SOURCE_CODE;
> -extern const char* CONF_KEY_SOURCE_FILE_ENCODING_LIST;
> -extern const char* CONF_KEY_USE_SYSTEM_FONT;
> -extern const char* CONF_KEY_CUSTOM_FONT_NAME;
> extern const char* CONF_KEY_SYSTEM_FONT_NAME;
> -extern const char* CONF_KEY_USE_LAUNCH_TERMINAL;
> -extern const char* CONF_KEY_STATUS_WIDGET_MINIMUM_WIDTH;
> -extern const char* CONF_KEY_STATUS_WIDGET_MINIMUM_HEIGHT;
> -extern const char* CONF_KEY_STATUS_PANE_LOCATION;
> -extern const char* CONF_KEY_DEBUGGER_ENGINE_DYNMOD_NAME;
> -extern const char* CONF_KEY_EDITOR_STYLE_SCHEME;
> -extern const char* CONF_KEY_ASM_STYLE_PURE;
> -extern const char* CONF_KEY_GDB_BINARY;
> -extern const char* CONF_KEY_DEFAULT_NUM_ASM_INSTRS;
> -extern const char* CONF_KEY_FOLLOW_FORK_MODE;
> -extern const char* CONF_KEY_CONTEXT_PANE_LOCATION;
>
> NEMIVER_END_NAMESPACE (nemiver)
If we go for the new config key arrangement I proposed above, we'd
keep this src/confmgr/nmv-conf-keys.h file with all the declarations
it contains. Each confmgr backend (gsettings and gconf) would provide
its own set of *definitions* for these declarations. Each set of
definitions would go in a separate nmv-<backend>keys-defs.cc file,
that is compiled with the other backend dynamic module source files to
produce the back-end's dynamic module binary.
[...]
> diff --git a/src/confmgr/nmv-gsettings-mgr.cc b/src/confmgr/nmv-gsettings-mgr.cc
> new file mode 100644
> index 0000000..53b0820
> --- /dev/null
> +++ b/src/confmgr/nmv-gsettings-mgr.cc
> @@ -0,0 +1,253 @@
> +// Author: Fabien Parent
> +/*
> + *This file is part of the Nemiver project
> + *
> + *Nemiver is free software; you can redistribute
> + *it and/or modify it under the terms of
> + *the GNU General Public License as published by the
> + *Free Software Foundation; either version 2,
> + *or (at your option) any later version.
> + *
> + *Nemiver is distributed in the hope that it will
> + *be useful, but WITHOUT ANY WARRANTY;
> + *without even the implied warranty of
> + *MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> + *See the GNU General Public License for more details.
> + *
> + *You should have received a copy of the
> + *GNU General Public License along with Nemiver;
> + *see the file COPYING.
> + *If not, write to the Free Software Foundation,
> + *Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
> + *
> + *See COPYRIGHT file copyright information.
> + */
> +#include <giomm/settings.h>
> +#include "nmv-i-conf-mgr.h"
> +
> +NEMIVER_BEGIN_NAMESPACE (nemiver)
> +
> +class GSettingsMgr : public IConfMgr {
> + GSettingsMgr (const GSettingsMgr &);
> + GSettingsMgr& operator= (const GSettingsMgr &);
> +
> + typedef std::map<const Glib::ustring, Glib::RefPtr<Gio::Settings> > Settings;
> +
> + Settings m_settings;
> + sigc::signal<void, const UString&> m_value_changed_signal;
> +
> +public:
> +
> + GSettingsMgr (DynamicModule *a_dynmod);
> + virtual ~GSettingsMgr ();
> +
> + void set_key_dir_to_notify (const UString &a_key_dir);
> + void add_key_to_notify (const UString &a_key);
> + void add_schema (const UString &a_schema);
> +
> + bool get_key_value (const UString &a_key, UString &a_value);
> + void set_key_value (const UString &a_key, const UString &a_value);
> +
> + bool get_key_value (const UString &a_key, bool &a_value);
> + void set_key_value (const UString &a_key, bool a_value);
> +
> + bool get_key_value (const UString &a_key, int &a_value);
> + void set_key_value (const UString &a_key, int a_value);
> +
> + bool get_key_value (const UString &a_key, double &a_value);
> + void set_key_value (const UString &a_key, double a_value);
> +
> + bool get_key_value (const UString &a_key, std::list<UString> &a_value);
> + void set_key_value (const UString &a_key, const std::list<UString> &a_value);
> +
> + sigc::signal<void, const UString&>& value_changed_signal ();
> +
> +};//end class GSettingsMgr
> +
> +GSettingsMgr::GSettingsMgr (DynamicModule *a_dynmod) :
> + IConfMgr (a_dynmod)
> +{
> +}
> +
> +GSettingsMgr::~GSettingsMgr ()
> +{
> + LOG_D ("delete", "destructor-domain");
> +}
> +
> +
> +void
> +GSettingsMgr::set_key_dir_to_notify (const UString &a_key_dir)
As a_key_dir is not used here, the compiler might warn about an unused
variable. I'd just declare:
GSettingsMgr::set_key_dir_to_notify (const UString &)
> +{
> +}
> +
> +void
> +GSettingsMgr::add_key_to_notify (const UString &a_key)
Same comment as above.
> +{
> +}
> +void
> +GSettingsMgr::add_schema (const UString &a_schema)
> +{
> + Glib::RefPtr<Gio::Settings> settings = Gio::Settings::create (a_schema);
> + THROW_IF_FAIL (settings);
> +
> + settings->signal_changed ().connect
> + (sigc::mem_fun (m_value_changed_signal,
> + &sigc::signal<void, const UString&>::emit));
OK.
> +
> + std::vector<Glib::ustring> keys = settings->list_keys ();
> + for (std::vector<Glib::ustring>::iterator i = keys.begin ();
> + i != keys.end ();
> + ++i)
> + {
This curly bracket should stay on the same line as the closing
parenthesis.
> + THROW_IF_FAIL (m_settings.count (*i) == 0);
> + m_settings[*i] = settings;
> + }
I see. I hope there will never be two different schemas with
identical keys, otherwise, this will break. Won't it?
I guess we can keep this hack for now, as it does seem to work. But I
suspect we will have to force the client code to pass the name of the
schema in parameter to the get/set methods of the IConfMgr
interface. If the schema name is the last parameter of the get/set
function, not passing it would mean "use the default schema name".
This would be backward compatible with the current API for most
cases.
This would argue in favour of merging the two org.nermiver.workbench
and org.nemiver.dbgperspective into one schema named, e.g,
org.nemiver. Client code would then only need to specify a schema
name when it wants to mess with keys coming from other applications or
the desktop; e.g, the org.gnome.desktop.interface schema.
What do you think about this?
[...]
> diff --git a/src/dbgengine/nmv-debugger-utils.cc b/src/dbgengine/nmv-debugger-utils.cc
> index d036060..2ded316 100644
> --- a/src/dbgengine/nmv-debugger-utils.cc
> +++ b/src/dbgengine/nmv-debugger-utils.cc
> @@ -132,11 +132,16 @@ dump_variable_value (IDebugger::VariableSafePtr a_var,
> IDebuggerSafePtr
> load_debugger_iface_with_gconf ()
> {
> -
> // Load the confmgr interface
> +#ifdef WITH_GSETTINGS
> + IConfMgrSafePtr conf_mgr =
> + common::DynamicModuleManager::load_iface_with_default_manager<IConfMgr>
> + ("gsettingsmgr", "IConfMgr");
> +#else
> IConfMgrSafePtr conf_mgr =
> common::DynamicModuleManager::load_iface_with_default_manager<IConfMgr>
> ("gconfmgr", "IConfMgr");
> +#endif /* WITH_GSETTINGS */
I think we can avoid the #ifdef WITH_GSETTINGS altogether by doing an
AC_DEFINE in configure.ac that would define the name of the confmgr
implementation; i.e, either "gconfmgr" or "gsettingsmgr". That name
could be something like CONFIG_MGR_MODULE_NAME. The above
#ifdef would be replaced by just:
IConfMgrSafePtr conf_mgr =
common::DynamicModuleManager::load_iface_with_default_manager<IConfMgr>
(CONFIG_MGR_MODULE_NAME, "IConfMgr");
[...]
> diff --git a/src/persp/dbgperspective/Makefile.am b/src/persp/dbgperspective/Makefile.am
> index 12bf939..1363360 100644
> --- a/src/persp/dbgperspective/Makefile.am
> +++ b/src/persp/dbgperspective/Makefile.am
> @@ -75,7 +75,9 @@ $(h)/nmv-dbg-perspective.h \
> $(h)/nmv-vars-treeview.h \
> $(h)/nmv-vars-treeview.cc \
> $(h)/nmv-call-function-dialog.h \
> -$(h)/nmv-call-function-dialog.cc
> +$(h)/nmv-call-function-dialog.cc \
> +$(h)/nmv-dbg-conf-keys.h \
> +$(h)/nmv-dbg-conf-keys.cc
I would prefer the declaration of all configuration keys to be
centralized in one place: src/confmgr/nmv-conf-keys.h. I have started
a progressive task of centralizing them all there, but I am not sure I
finished that task.
Each confmgr backend would then have it's own definition of these
keys.
So this hunk would be dropped.
[...]
> diff --git a/src/persp/dbgperspective/nmv-call-stack.cc b/src/persp/dbgperspective/nmv-call-stack.cc
> index aa9e429..b731df3 100644
> --- a/src/persp/dbgperspective/nmv-call-stack.cc
> +++ b/src/persp/dbgperspective/nmv-call-stack.cc
> @@ -32,6 +32,7 @@
> #include "nmv-ui-utils.h"
> #include "nmv-i-workbench.h"
> #include "nmv-i-perspective.h"
> +#include "nmv-dbg-conf-keys.h"
... likewise here ...
[...]
> -static const char* CONF_KEY_NEMIVER_CALLSTACK_EXPANSION_CHUNK =
> - "/apps/nemiver/dbgperspective/callstack-expansion-chunk";
> static const char* COOKIE_CALL_STACK_IN_FRAME_PAGING_TRANS =
> "cookie-call-stack-in-frame-paging-trans";
... and here, this key definition should be moved into the gsettings
backend dynamic module.
>
> @@ -107,6 +106,7 @@ typedef map<int, list<IDebugger::VariableSafePtr> > FrameArgsMap;
> typedef map<int, IDebugger::Frame> LevelFrameMap;
> struct CallStack::Priv {
> IDebuggerSafePtr debugger;
> + mutable IConfMgrSafePtr conf_mgr;
Why declaring this as mutable?
> IWorkbench& workbench;
> IPerspective& perspective;
> FrameArray frames;
> @@ -131,6 +131,7 @@ struct CallStack::Priv {
> IWorkbench& a_workbench,
> IPerspective& a_perspective) :
> debugger (a_dbg),
> + conf_mgr(0),
A space is missing before the opening parenthesis.
[...]
> diff --git a/src/persp/dbgperspective/nmv-dbg-conf-keys.cc b/src/persp/dbgperspective/nmv-dbg-conf-keys.cc
> new file mode 100644
> index 0000000..d5e8691
> --- /dev/null
> +++ b/src/persp/dbgperspective/nmv-dbg-conf-keys.cc
> @@ -0,0 +1,79 @@
If we agree with the new keys definition arrangement I proposed
earlier, these keys should be defined in each confmgr dynamic module
(gconf and gsettings). This new file would then become unnecessary.
[...]
> diff --git a/src/persp/dbgperspective/nmv-dbg-conf-keys.h b/src/persp/dbgperspective/nmv-dbg-conf-keys.h
> new file mode 100644
> index 0000000..a3ef172
> --- /dev/null
> +++ b/src/persp/dbgperspective/nmv-dbg-conf-keys.h
> @@ -0,0 +1,57 @@
Likewise for this key declaration file.
[...]
> diff --git a/src/persp/dbgperspective/nmv-dbg-perspective.cc b/src/persp/dbgperspective/nmv-dbg-perspective.cc
> index df2edf7..f31a79a 100644
> --- a/src/persp/dbgperspective/nmv-dbg-perspective.cc
> +++ b/src/persp/dbgperspective/nmv-dbg-perspective.cc
> @@ -94,6 +94,7 @@
> #include "nmv-registers-view.h"
> #include "nmv-call-function-dialog.h"
> #include "nmv-conf-keys.h"
> +#include "nmv-dbg-conf-keys.h"
Likewise for this include directive.
[...]
> diff --git a/src/persp/dbgperspective/nmv-preferences-dialog.cc b/src/persp/dbgperspective/nmv-preferences-dialog.cc
> index c880804..9f2fd54 100644
> --- a/src/persp/dbgperspective/nmv-preferences-dialog.cc
> +++ b/src/persp/dbgperspective/nmv-preferences-dialog.cc
> @@ -30,7 +30,7 @@
> #include "nmv-ui-utils.h"
> #include "nmv-i-conf-mgr.h"
> #include "nmv-i-workbench.h"
> -#include "nmv-conf-keys.h"
> +#include "nmv-dbg-conf-keys.h"
Likewise for this include directive change.
[...]
> diff --git a/src/persp/dbgperspective/schemas/Makefile.am b/src/persp/dbgperspective/schemas/Makefile.am
> index 0074a40..ff77fc9 100644
> --- a/src/persp/dbgperspective/schemas/Makefile.am
> +++ b/src/persp/dbgperspective/schemas/Makefile.am
> @@ -1,34 +1,48 @@
> -files=nemiver-dbgperspective.schemas
> -
> -schemasdir = $(GCONF_SCHEMA_FILE_DIR)
> -schemas_DATA = $(files)
> -
> -GCONFTOOL=@GCONFTOOL@
> +if BUILD_GSETTINGS
> + files=org.nemiver.dbgperspective.gschema.xml
> + gsettingsschema_DATA = $(files)
> +else
> + files=nemiver-dbgperspective.schemas
> + schemasdir = $(GCONF_SCHEMA_FILE_DIR)
> + schemas_DATA = $(files)
> +
> + GCONFTOOL=@GCONFTOOL@
> + export GCONF_CONFIG_SOURCE=$(GCONF_SCHEMA_CONFIG_SOURCE)
> +endif
>
> -export GCONF_CONFIG_SOURCE=$(GCONF_SCHEMA_CONFIG_SOURCE)
> export files
>
> +if BUILD_GSETTINGS
> +install-schemas: install-gsettingsschemaDATA
> + $(GLIB_COMPILE_SCHEMAS) $(gsettingsschemadir)
> +else
> install-schemas:
> if GCONF_SCHEMAS_INSTALL
> if test -z "$(DESTDIR)" ; then \
> - for s in `echo $$files | tr ' ' '\n'` ; do \
> + for s in `echo $$files | tr ' ' '\n'` ; do \
> $(GCONFTOOL) --makefile-install-rule $(top_srcdir)/src/persp/dbgperspective/schemas/$$s ; \
> - done ; \
> - gconfpid=`pidof gconfd-2` ; \
> - if test x$$gconfpid != x ; then \
> + done ; \
> + gconfpid=`pidof gconfd-2` ; \
> + if test x$$gconfpid != x ; then \
> kill -HUP $$gconfpid ; \
> echo "gconf $$gconfpid reloaded its config" ;\
> - fi \
> + fi \
> fi
> -endif
> +endif # GCONF_SCHEMAS_INSTALL
> +endif # BUILD_GSETTINGS
>
>
> +if BUILD_GSETTINGS
> +uninstall-schemas: uninstall-gsettingsschemaDATA
> + $(GLIB_COMPILE_SCHEMAS) $(gsettingsschemadir)
> +else
> uninstall-schemas:
> if GCONF_SCHEMAS_INSTALL
> if test -z "$(DESTDIR)" ; then \
> for s in `echo $$files | tr ' ' '\n'` ; do $(GCONFTOOL) --makefile-uninstall-rule $(top_srcdir)/src/persp/dbgperspective/schemas/$$s ; done \
> fi
> -endif
> +endif # GCONF_SCHEMAS_INSTALL
> +endif # BUILD_GSETTINGS
>
> EXTRA_DIST=$(files)
>
Like I said earlier, maybe we could move all the schemas to src/data?
> diff --git a/src/persp/dbgperspective/schemas/org.nemiver.dbgperspective.gschema.xml b/src/persp/dbgperspective/schemas/org.nemiver.dbgperspective.gschema.xml
> new file mode 100644
> index 0000000..94e1dfc
> --- /dev/null
> +++ b/src/persp/dbgperspective/schemas/org.nemiver.dbgperspective.gschema.xml
> @@ -0,0 +1,149 @@
... and we could merge this schema ...
[...]
> diff --git a/src/workbench/schemas/org.nemiver.workbench.gschema.xml b/src/workbench/schemas/org.nemiver.workbench.gschema.xml
> new file mode 100644
> index 0000000..4db25e1
> --- /dev/null
> +++ b/src/workbench/schemas/org.nemiver.workbench.gschema.xml
> @@ -0,0 +1,45 @@
... with this one. I guess it would make things simpler. What do you
think?
> diff --git a/src/workbench/Makefile.am b/src/workbench/Makefile.am
> index 73689ca..809ee7f 100644
> --- a/src/workbench/Makefile.am
> +++ b/src/workbench/Makefile.am
> @@ -17,12 +17,14 @@ h=$(abs_srcdir)
>
> headers= \
> $(h)/nmv-i-workbench.h \
> -$(h)/nmv-i-pref-mgr.h
> +$(h)/nmv-i-pref-mgr.h \
> +$(h)/nmv-workbench-conf-keys.h
As I said earlier, this hunk would be unnecessary if all the conf keys
stay declared in where they where before this patch ...
> libworkbenchmod_la_SOURCES= \
> $(headers) \
> $(h)/nmv-workbench.cc \
> -$(h)/nmv-pref-mgr.cc
> +$(h)/nmv-pref-mgr.cc \
> +$(h)/nmv-workbench-conf-keys.cc
... and this one would be unnecessary if the conf keys are declared in
confmgr modules.
[...]
> diff --git a/src/workbench/nmv-workbench-conf-keys.cc b/src/workbench/nmv-workbench-conf-keys.cc
> new file mode 100644
> index 0000000..8a44e8e
> --- /dev/null
> +++ b/src/workbench/nmv-workbench-conf-keys.cc
> @@ -0,0 +1,51 @@
Likewise for this file, ...
> diff --git a/src/workbench/nmv-workbench.cc b/src/workbench/nmv-workbench.cc
> index 733d6a6..c87b8f0 100644
> --- a/src/workbench/nmv-workbench.cc
> +++ b/src/workbench/nmv-workbench.cc
> @@ -41,6 +41,7 @@
> #include "nmv-i-workbench.h"
> #include "nmv-i-perspective.h"
> #include "nmv-i-conf-mgr.h"
> +#include "nmv-workbench-conf-keys.h"
... and for this hunk.
[...]
> @@ -53,22 +54,6 @@ using namespace nemiver::common;
>
> NEMIVER_BEGIN_NAMESPACE (nemiver)
>
> -static const UString CONF_KEY_NEMIVER_WINDOW_WIDTH =
> - "/apps/nemiver/workbench/window-width";
> -static const UString CONF_KEY_NEMIVER_WINDOW_HEIGHT =
> - "/apps/nemiver/workbench/window-height";
> -static const UString CONF_KEY_NEMIVER_WINDOW_POSITION_X =
> - "/apps/nemiver/workbench/window-position-x";
> -static const UString CONF_KEY_NEMIVER_WINDOW_POSITION_Y =
> - "/apps/nemiver/workbench/window-position-y";
> -static const UString CONF_KEY_NEMIVER_WINDOW_MAXIMIZED =
> - "/apps/nemiver/workbench/window-maximized";
> -static const UString CONF_KEY_NEMIVER_WINDOW_MINIMUM_WIDTH=
> - "/apps/nemiver/workbench/window-minimum-width";
> -static const UString CONF_KEY_NEMIVER_WINDOW_MINIMUM_HEIGHT=
> - "/apps/nemiver/workbench/window-minimum-height";
> -
> -
This hunk would be a cleanup one to move these key definitions to the
confmgr modules, yes.
[...]
> @@ -540,8 +525,21 @@ Workbench::get_configuration_manager ()
> loader->get_dynamic_module_manager ();
> THROW_IF_FAIL (dynmod_manager);
>
> +#ifdef WITH_GSETTINGS
> + m_priv->conf_mgr = dynmod_manager->load_iface <IConfMgr> ("gsettingsmgr",
> + "IConfMgr");
> +
> + NEMIVER_TRY
> +
> + m_priv->conf_mgr->add_schema ("org.nemiver.workbench");
> + m_priv->conf_mgr->add_schema ("org.nemiver.dbgperspective");
> + m_priv->conf_mgr->add_schema ("org.gnome.desktop.interface");
> +
> + NEMIVER_CATCH_NOX
> +#else
> m_priv->conf_mgr = dynmod_manager->load_iface <IConfMgr> ("gconfmgr",
> "IConfMgr");
> +
Using the CONFIG_MGR_MODULE_NAME I was proposing earlier, we could
avoid the #ifdef directive altogether, considering that with the gconf
backend, IConfMgr::add_schema is a no-op function.
[...]
> diff --git a/src/workbench/schemas/Makefile.am b/src/workbench/schemas/Makefile.am
> index 10754d5..a6d32a4 100644
> --- a/src/workbench/schemas/Makefile.am
> +++ b/src/workbench/schemas/Makefile.am
> @@ -1,28 +1,41 @@
> -files=nemiver-workbench.schemas
> -
> -schemasdir = $(GCONF_SCHEMA_FILE_DIR)
> -schemas_DATA = $(files)
> -
> -GCONFTOOL=@GCONFTOOL@
> +if BUILD_GSETTINGS
> + files=org.nemiver.workbench.gschema.xml
> + gsettingsschema_DATA = $(files)
> +else
> + files=nemiver-workbench.schemas
> + schemasdir = $(GCONF_SCHEMA_FILE_DIR)
> + schemas_DATA = $(files)
> +
> + GCONFTOOL=@GCONFTOOL@
> + export GCONF_CONFIG_SOURCE=$(GCONF_SCHEMA_CONFIG_SOURCE)
> +endif
>
> -export GCONF_CONFIG_SOURCE=$(GCONF_SCHEMA_CONFIG_SOURCE)
> export files
>
> +if BUILD_GSETTINGS
> +install-schemas: install-gsettingsschemaDATA
> + $(GLIB_COMPILE_SCHEMAS) $(gsettingsschemadir)
> +else
> install-schemas:
> if GCONF_SCHEMAS_INSTALL
> if test -z "$(DESTDIR)" ; then \
> for s in `echo $$files | tr ' ' '\n'` ; do \
> $(GCONFTOOL) --makefile-install-rule $(top_srcdir)/src/workbench/schemas/$$s ; done && if test x$$NO_RESTART_GCONF = x ; then $(GCONFTOOL) --shutdown; fi ; \
> fi
> -endif
> -
> +endif # GCONF_SCHEMAS_INSTALL
> +endif # BUILD_GSETTINGS
>
> +if BUILD_GSETTINGS
> +uninstall-schemas: uninstall-gsettingsschemaDATA
> + $(GLIB_COMPILE_SCHEMAS) $(gsettingsschemadir)
> +else
> uninstall-schemas:
> if GCONF_SCHEMAS_INSTALL
> if test -z "$(DESTDIR)" ; then \
> for s in `echo $$files | tr ' ' '\n'` ; do $(GCONFTOOL) --makefile-uninstall-rule $(top_srcdir)/src/workbench/schemas/$$s ; done \
> fi
> -endif
> +endif # GCONF_SCHEMAS_INSTALL
> +endif # BUILD_GSETTINGS
>
Same question as earlier. What would you think about moving these
schemas here into src/data ?
[...]
Thank you for your great work, and pardon my verbiage :-)
--
Dodji
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
