Qsettings

[kain88-de]

This adds read and write functions for Qt QSettings object and a small wrapper class to use QSettings with mixxx.

I haven't touched any of the config options so far. This should show as a proof-of-concept how we can switch to QSettings and get rid of the m_pConfig pointer that we pass around everywhere. If we want to switch I would suggest merging this PR and then creating a PR for each config-option that has been ported to the new interface. To keep track of the porting progress the QSettings object currently saves it's progress in a file called Mixxx.cfg instead of mixxx.cfg (notice the capital M).

Once we have gotten every access to a config option switch the mixxx.cfg file should be empty when we run mixxx with an empty settings-folder (do an rm -r ~/.mixxx before starting mixxx).

A nice side effect of using QSettings is that all options in the settings-file are sorted by group. So every settings associated with the library for example will be at exactly the same place in the file and not spread across it.

[kain88-de]

As this is will lead to a huge change in mixxx i'm nut sure if you want to have it in master right now.
Since this adds mostly new code in new files the PR should age well until we decide to make the switch and replace m_pConfig with QSettings.

[sblaisot]

There is no capital on Windows filsystem. You should use another name.

[daschuer]

What is the original benefit of changing to QSettings? Which pending issues are solved by it?

QSettings is basically a wrapper around the native config systems of our target OSs.
The biggest benefit of it is that we are able to store the config data the native way.
Do we have a benefit if we switch to the native config systems? On one Hand we can rely on the OSs config handling throughout all derivatives, on the other hand we are not able to point the user to the same file on all our targets.
In the PR we register our own config format, do we still have benefit of QSystem in this case compared to our established system?

Please note that QSettings is reentrant but not thread-safe. Our current config system is not thread-safe as well, even though it is uses from multiple threads. https://bugs.launchpad.net/mixxx/+bug/1428500
This will be fixed after merging #501
It fixes also the sorting issue.

The spreading m_config can be easily fixed by tuning it into a singleton.

We have to be careful consider when is the right time to switch over to a new config system, since we have to provide a migration path. IMHO we should switch to QT 5 first since the config path handling has changed between QT5 and QT4.

There is also the pending user config dir bug:
https://bugs.launchpad.net/mixxx/+bug/1463273

[daschuer]

setFallbacksEnabled is non const, we should use the pointer notation.
A reference should be always const.

[kain88-de]

That will be solved when we use Qt5 where I can move QSettings.

[kain88-de]

What is the original benefit of changing to QSettings? Which pending issues are solved by it?

tl;dr

Less and better maintainable code.

longer

The current config-system is a pain to use. Accessing values is just plain strange. Compare the two access methods.

// Old
m_pConfig->getValueString(ConfigKey("[Controls]", "Tooltips", "1").toInt()
// new
settings.value("Controls/Tooltips", true).toBool();

I find the second easier to grasp and use. It relies on QVariant instead of QString as a backend to save settings which is more appropriate for this stuff. We sometimes go through weirds hops in our code an rely on automatic type conversion a lot to get the correct result. Finally it is super hard to get access to the config object in a class that does not already hold a pointer to it. You have to figure out a dependency chain to your target class and pass the pointer along the whole way.

We also have to manually save the settings (QSettings does this automatically). We have to be scared that we get mismatched files when we write from different threads. QSettings would fix all of these issues.

Please note that QSettings is reentrant but not thread-safe. Our current config system is not thread-safe as well, even though it is uses from multiple threads. https://bugs.launchpad.net/mixxx/+bug/1428500
This will be fixed after merging #501
It fixes also the sorting issue.

Yes QSettings is by itself not thread-safe. But it does not need to be, neither does it need to be a singleton. The idea is that you create a QSettings object when ever you need one and destroy it afterwards, thus we don't need to pass the object around and it will ALWAYS live in just one thread.
Two QSettings objects in different threads can point to the same data-file and changes are communicated between the two. See the cited docs here. This is not something that can be done with a simple singleton.

QSettings is reentrant. This means that you can use distinct QSettings object in different threads simultaneously. This guarantee stands even when the QSettings objects refer to the same files on disk (or to the same entries in the system registry). If a setting is modified through one QSettings object, the change will immediately be visible in any other QSettings objects that operate on the same location and that live in the same process.
QSettings can safely be used from different processes (which can be different instances of your application running at the same time or different applications altogether) to read and write to the same system locations. It uses advisory file locking and a smart merging algorithm to ensure data integrity. Note that sync() imports changes made by other processes (in addition to writing the changes from this QSettings).

`

QSettings is basically a wrapper around the native config systems of our target OSs.

Nope. It is a very generic interface to it. By default it allows to use target specific config but it can be customized to use the same formats on every platform as well as location. Which is what I have done here and also the reason why I had to create the MixxxSettings wrapper class.

Do we have a benefit if we switch to the native config systems? On one Hand we can rely on the OSs config handling throughout all derivatives, on the other hand we are not able to point the user to the same file on all our targets.

I setup QSettings in a way that we can use the same config-file names as before on all platforms. The filename differs right now to track changes while we port all settings to QSettings. The change can be done here

In the PR we register our own config format, do we still have benefit of QSystem in this case compared to our established system?

We bypass 'QSystem' completely in this PR anyway. The benefit we will get is a proper class to handle user settings inside of mixxx. It results in an easier to use settings interface and less code to maintain.

We have to be careful consider when is the right time to switch over to a new config system, since we have to provide a migration path. IMHO we should switch to QT 5 first since the config path handling has changed between QT5 and QT4.

Sure you can switch to QT5 first. That would actually be nice, because I can make some of this code nicer with movable Qt-types.

There is also the pending user config dir bug:
https://bugs.launchpad.net/mixxx/+bug/1463273

Using QSettings would actually make solving this bug easier.

[Be-ing]

Finally it is super hard to get access to the config object in a class that does not already hold a pointer to it. You have to figure out a dependency chain to your target class and pass the pointer along the whole way.

This was a big pain to attempt to deal with for #649.

[daschuer]

qsettings is unused.

[kain88-de]

This is a proof of concept. No setting has been switched so far. It should only show how qsettings can be used to replace the old system and still be backwards compatible.

[rryan]

I have two concerns here:

• From a software engineering perspective, this allows shortcuts that are "easier" at the time you bang out the code but more complicated since it relies on global state. With a little more effort you can arrive at a design that is explicit about its communication needs rather than shortcutting through the settings system. A lot of Mixxx code today already relies on user config as shared mutable state -- I'd like to see us get rid of it rather than make it easier. This makes code harder to maintain and harder to write tests for. This is the same argument I have against singletons.
• For a "future" settings format, I would like to move away from a key-value store and towards a structured representation of the configuration (a protocol buffer). This will reduce bugs since the compiler can statically verify that we area accessing a valid preference and make it easier to manage version upgrades and deprecation since we can bundle groups of associated properties into submessages and then unit test the conversion from one submessage type (the deprecated version) to another (the replacement).

[kain88-de]

From a software engineering perspective, this allows shortcuts that are "easier" at the time you bang out the code but more complicated since it relies on global state. With a little more effort you can arrive at a design that is explicit about its communication needs rather than shortcutting through the settings system. A lot of Mixxx code today already relies on user config as shared mutable state -- I'd like to see us get rid of it rather than make it easier. This makes code harder to maintain and harder to write tests for. This is the same argument I have against singletons.

Well these are two issues here.

• For me user configuration is a global state by definition. There can't be two different user-configurations in a running instance of mixxx. I also don't have that much of a problem with it because most likely we write to it rarely and from specialized settings dialogs and read often from it in different parts of mixxx. And reading values from the configuration should be easy. For me that should be possible from anywhere inside of mixxx without the need of passing objects along to the target class.
  Though I think it should be possible to design a system where only a few special classes can write to the user-config but all can read it.
• Abusing the current configuration system as a message passing system is not OK. I agree there with you. But I haven't looked into mixxx code for a while so I'm not sure where we are doing that.

I personally don't think we can get around the user-config being a shared state. There sure might be
ways to make it non mutable but I can't think of any right now. If you know of some publicly available examples it would be nice if you link them.

For a "future" settings format, I would like to move away from a key-value store and towards a structured representation of the configuration (a protocol buffer). This will reduce bugs since the compiler can statically verify that we area accessing a valid preference and make it easier to manage version upgrades and deprecation since we can bundle groups of associated properties into submessages and then unit test the conversion from one submessage type (the deprecated version) to another (the replacement).

I would still like that the configuration file is human readable. As this made it easier in the past to
help some users playing with the configuration to stop mixxx from crashing (mostly related to the waveforms). Defining all configuration options in a separate compilation file and having the compiler check that only those are accessed is a pretty good idea though. If that also includes type checking of the value then it's even better.

[ywwg]

human readable.

protocol buffers have a human-readable text format that is well-defined and parseable. It's a good choice for a configuration language.

[daschuer]

protobuf has human readable format, see http://stackoverflow.com/questions/18873924/what-does-the-protobuf-text-format-look-like
But IMHO an ini format is more established and continent as a human "writable" format.
If we want a more type save solution protobuf will help, but we may consider XML as well.
But I think we have no user complaining about our ini format, nor a pending bug because of missing tye safety, so there seams to be not a strong issue for a change.

[daschuer]

I have just debugged though the code and it looks like the proposed way of simplifying, comes with a lot of extra CPU load.

• Each new instance of a QSettings object reads and parses the whole ini file. You may argue that this is mostly done only during initializations, but the Mixxx initialization already takes quite long and should not take any longer.
• I have not found any sync mechanism inside Qt, of cause there is some syncing on the system level. But I am not sure if this prevents us from loosing config changes if we write the configs from different threads.
• The access via "AutoDJ/enabled" instead of "[AutoDJ], enabled" would be a new style of representing config values and will not match to the notation for ControlObjects and the final representation in the ini file. (As small disadvantage)
• The QVariant interface predicts a type safety we do not have using an *.ini file. All values are stored at Strings. Exposing this to the user code allows to special case some common errors.
  I agree that the QVariant interface comes handy with its implicit type casting.
  I also agree that the current interface can be tweaked.
  But a QVariant dispatcher is again extra CPU, and sometimes explicit is over implicit.
• The current pointer parsing solution helps use testing, but prevents us from using configs from everywhere. I am not sure which solution works best since there are many points to consider.
  A singleton solution will allow to use the config from everywhere, but since it will be finally locking, it should not be used from verywhere. Alternative: ControlObjects.
  I think I currently would prefer a testable singleton solution, where we are able to manipulate the control values for testing.

Some time ago I had the idea to merge Configs and control Objects, time has passed and we have no persistand ControlObjects. These are able to accessed from everywhere. but limited to a double type. I am sure this will carry us a while ;-)

[daschuer]

By the way: Can we merge #501 for 2.1.0 it fixes a race condition and other issues and does not prevent any changes that are proposed here.

[kain88-de]

I came across this post about a thread-safe configuration with compile time checks today.

http://jguegant.github.io/blogs/tech/thread-safe-multi-type-map.html

[Be-ing]

Any consensus on a way forward here? I'd like to get back to #649, but I don't know what the best way to overcome the obstacle of reading a configuration value there in a class that doesn't already do that.

[kain88-de]

@Be-ing I'm not going to finish this PR. It also looks like @rryan want's to refactor the internals of mixxx a lot with manager classes. A settings manager will likely fit into this scheme. http://mixxx.org/wiki/doku.php/mixxx_init_refactor

[Be-ing]

Cool, I'll get back to work on #649 after that's complete.