Re: GSettings backend

[Fabien Parent <parent f gmail com>]

Hello,

> So please find my comments below.

Thank you for all your very usefull comments.


>> -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.

No we haven't already done a PKG_CHECK_MODULE. We previously used
PKG_CHECK_EXISTS to guess automatically if we need to use GSettings or
GConf, but this call doesn't return any error if the dependency is not
met.


> 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?

It would indeed be more clean and easy to have all schemas into a
single location.


>> 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_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 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.

They can stay here.
I noticed that everything was carefully compartmentalized, e.g.
everything (keys, schema) for the workbench was in the workbench
folder, but this was not entirely true for the dbgperspective: only
the schema was inside the dbgperspective folder. I deduced from it,
that all these keys were not in the right place and should have been
in the dbgperspective folder (that's why I moved them).
It seems I understood the opposite of what you wanted to do, so I will
put everything back in confmgr.


> 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";

I think it would be quite redundant to do that. The base name of your
keys are already compatible with GSettings. Why not just add the PATH,
which would be empty with GSettings like this:
const char *CONF_KEY_ONE = PATH "foo-bar";
What do you think ?


> 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?

To answer this question, I need your comments on my comments above.


>> 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.

Likewise,

>> +        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?

Yes, it will break if there is 2 keys with the same name. I like your idea of
using the last parameter with a default value for get/set functions. I
don't know why
I haven't though about it earlier.


>>
>> @@ -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?

No valid reason. Bad copy & paste.


> ... and we could merge this schema ...
[...]
> ... with this one.  I guess it would make things simpler.  What do you
> think?

Yes, if we merge the dbgperspective and workbench schema, it's would
make better and simpler code.
At first I wanted too keep keys in seperate schema, mostly because it
is most easy to navigate through
the keys via a tool like (g|d)conf-editor. But it seems I cannot find
a way to have a code as simple and as reliable than when I am using a
single schema.

I'm sorry about my slow response to your e-mail. I will try to send a
new patch quickly. And thanks again for your enlightened comments.

Fabien Parent