[PATCH] Unexpected Behaviour in preference window.

Dirk Hohndel dirk at hohndel.org
Fri Apr 11 07:19:49 PDT 2014


On Fri, 2014-04-11 at 12:31 +0300, Lubomir I. Ivanov wrote:
> On 11 April 2014 11:44, Gehad Elrobey <gehadelrobey at gmail.com> wrote:
> >
> > On Thu, Apr 10, 2014 at 7:19 PM, Dirk Hohndel <dirk at hohndel.org> wrote:
> >>
> >> On Tue, 2014-04-08 at 14:11 +0200, Gehad wrote:
> >> > On 04/07/2014 10:22 PM, Dirk Hohndel wrote:
> >> >
> >> > > I'd really like to have a prefs variable that tracks the currently
> >> > > active prefs and only reference that anywhere in the code. The only
> >> > > place that should mess with s.value should be preferences.cpp
> >> >
> >> > And when will the prefs be synchronized with the QSettings ?
> >> > I found that some QSettings are changed in the middle of the code not
> >> > only from preference window or at startup.
> >>
> >> instead of the writing of QSettings all over the place I'd rather do
> >> this only in one space and have accessor functions that do the right
> >> thing. Maybe I'm overthinking this, but whenever I try to figure out
> >> something that's going wrong with preferences my eyes glaze over...
> >>
> >> /D
> >
> >
> > There are some functions in pref.h that I couldn't find their implementation
> > I think they are not implemented, Right ?
> >
> > extern void subsurface_open_conf(void);
> > extern void subsurface_set_conf(const char *name, const char *value);
> > extern void subsurface_set_conf_bool(const char *name, bool value);
> > extern void subsurface_set_conf_int(const char *name, int value);
> > extern void subsurface_unset_conf(const char *name);
> > extern const char *subsurface_get_conf(const char *name);
> > extern int subsurface_get_conf_bool(const char *name);
> > extern int subsurface_get_conf_int(const char *name);
> > extern void subsurface_flush_conf(void);
> > extern void subsurface_close_conf(void);

Those should almost all be removed (see at the end for exceptions). As
Lubomir said below, they are leftovers from the old config system.

> > Are those the accessor functions you were talking about ? also there is some
> > macros defined in preferences.cpp they do a similar job to these functions,
> > this is confusing me a little bit.
> >
> > I can work on this, But I don't understand how the whole thing is supposed
> > to work.
> 
> the above list of functions are leftovers from the GTK version where
> we wrote ourselves OS native code to handle the settings writing.
> now we can use Qt for that, but the functions still make sense, in a way.

I'm not sure they do, Lubomir. The only ones I'd keep around are the
ones for setting configuration.

> what Dirk describes, i believe, is a local class or a set of global
> C++ functions that will wrap the QSettings integration in Subsurface.
> this would be nice, because we can have better control over things and
> don't need to use the QSettings class directly inside the code.

I think I had something slightly different in mind:

The prefs variable should contain a member for each setting. And we need
to redo those members so they are actually identical to the name of the
setting (it's ridiculous that we have these tiny variations between the
string used as key in QSettings and the member of the struct
preferences).

Whenever the code needs to check a preference, just use 

	if (prefs.whatever == ...)

And when you need to set a preference from the code (we have that in a
few places now) have it call
subsurface_set_conf(const char *name, const char *value)
subsurface_set_conf_bool(const char *name, bool value)
subsurface_set_conf_int(const char *name, int value)
and then implement these three functions so they figure out the matching
settings group and update both.
Actually, I think we may get away with only ever setting things from UI
code... you'll have to double check this, but I don't see how C code
right now could even call into QSettings code, so most likely all you
need is subsurface_set_conf(name, value) and then have three
implementations for different type.
And the subsurface_set_conf() call modifies the matching prefs variable
and then updates the setting.

Once you cleaned up the variable name / settings name inconsistency you
could implement this as a macro

#define subsurface_set_conf(_n, _v) \
	prefs.##_n = _v; \
	subsurfaceSetConf(_n, _v)

Or something along this line. I haven't had enough coffee yet to fully
think this through, but maybe the gist of what I wanted becomes clear?

/D



More information about the subsurface mailing list