- Status Closed
- Percent Complete
- Task Type Patches
- Category Rbutil
-
Assigned To
bluebrother - Operating System All players
- Severity Low
- Priority Very Low
- Reported Version Version 3.2
- Due in Version Future release
-
Due Date
Undecided
- Votes
- Private
Opened by bluebrother - 2009-04-11
Last edited by bluebrother - 2009-04-29
FS#10123 - Rbutil: rework RbSettings class
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 :)
rework-rbsettings-01.diff
(102.4 KiB)
2009-04-29 20:59
Reason for closing: Accepted
Additional comments about closing: Warning: Undefined array key "typography" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 371 Warning: Undefined array key "camelcase" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 407
updated version committed as r20823.
Loading...
Available keyboard shortcuts
- Alt + ⇧ Shift + l Login Dialog / Logout
- Alt + ⇧ Shift + a Add new task
- Alt + ⇧ Shift + m My searches
- Alt + ⇧ Shift + t focus taskid search
Tasklist
- o open selected task
- j move cursor down
- k move cursor up
Task Details
- n Next task
- p Previous task
- Alt + ⇧ Shift + e ↵ Enter Edit this task
- Alt + ⇧ Shift + w watch task
- Alt + ⇧ Shift + y Close Task
Task Editing
- Alt + ⇧ Shift + s save task
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
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.
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.