FS#10123 - Rbutil: rework RbSettings class

Attached to Project: Rockbox
Opened by Dominik Riebeling (bluebrother) - Saturday, 11 April 2009, 17:11 GMT
Last edited by Dominik Riebeling (bluebrother) - Wednesday, 29 April 2009, 20:59 GMT
Task Type Patches
Category Rbutil
Status Closed
Assigned To Dominik Riebeling (bluebrother)
Operating System All players
Severity Low
Priority Normal
Reported Version Version 3.2
Due in Version Future release
Due Date Undecided
Percent Complete 100%
Votes 0
Private No


This is the current state of reworking the RbSettings class to use a list-based approach rather than a separate member function for each setting. This has the major advantage that the API doesn't change on every new setting that gets added and it's much cleaner IMO.

Some remarks:
- the handling could get simplified further if we drop returning the correct type but always use a QVariant as QSettings does.
- the handling of subsections could be merged with the main access functions by changing the argument order and using default values. Not sure if I like a different (less forward IMO) argument order
- another option would be to require the subsection to match another enum, but that would require more changes to the code using it (mainly tts and encoder classes). Not sure if I like that.

The current patch works fine as far as I could test it. Feedback welcome :)
This task depends upon

Closed by  Dominik Riebeling (bluebrother)
Wednesday, 29 April 2009, 20:59 GMT
Reason for closing:  Accepted
Additional comments about closing:  updated version committed as r20823.
Comment by Dominik Wenger (Domonoky) - Saturday, 25 April 2009, 19:48 GMT
I just took a short look a this, a few remarks:

- I think using QVariant as return and set values and using just one long list of settings would make the code much cleaner.
- Perhaps even combine Device and Usersettings into one list, by using another entry in the struct to differentiate them. (And Device settings should be renamed while we are at it.).
- About the subsections: why not use a overloaded variant of the normal "value()" function, or default parameters ?
- If you use a QMap or alike you dont need to search for the setting entry, but then runtime initialisation (in constructor ) is needed.
- While we are reworking the settings, it might be good to make static "value()" and "setValue()" functions and initialise on access. Then we dont need to pass the settings pointer everywhere.

Apart from this it looks like the right way :-)

Comment by Dominik Riebeling (bluebrother) - Sunday, 26 April 2009, 09:07 GMT
Updated diff:
- use a QVariant as return type for access functions
- merge all sublists to device and user list (merging those two remining lists is on my todo)
- use references for subValue / setSubValue.

As for the other comments:
- the subsections intentionally don't use a default value as default values need to be the trailing ones -- and having the section after the value looks like a quite weird (and error-prone) interface to me.
- I don't use a QMap so I can make the tables const static. A QMap would need to do the lookup too, and as it's now only a few places remaining for the lookup I don't see any real benefit from using a QMap
- using static access functions sounds like a good idea.
Comment by Dominik Riebeling (bluebrother) - Wednesday, 29 April 2009, 18:57 GMT
Updated version that merges User and Device list. IMO we should try to get this in rather soon -- once the switch from the old API to this one is done making the value() / setValue() functions static and removing the settings pointer passing is a separate thing and much less complicated.