Hello,
As promised, here is the new patch. What do you think of this one ?
Fabien Parent
On Fri, Mar 25, 2011 at 21:43, Fabien Parent <parent f gmail com> wrote:
> 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
>
Attachment:
0001-Add-GSettings-backend-to-the-configuration-manager.patch
Description: Binary data