- Status Closed
- Percent Complete
- Task Type Patches
- Category User Interface
- Assigned To No-one
- Operating System All players
- Severity Low
- Priority Very Low
- Reported Version Version 3.3
- Due in Version Undecided
-
Due Date
Undecided
- Votes
- Private
FS#10356 - Merge the "Replaygain Off" option into the replaygain type; eliminate the "On/Off" setting
This patch adds a new replaygain type “off” and eliminates the menu setting “Enable replaygain”. I hope I got it right
Closed by fml2
2009-06-21 09:02
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
2009-06-21 09:02
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
Committed in r21414. Close the task to
not forget to do it later; reopen or
submit bug reports if some are found.
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 think the mailing list result was pretty clear. Just go for it.
Minor nitpicking: REPLAYGAIN_OFF could be == 0 as it makes slightly more sense, but that doesn’t actually change anything.
But adding the new option in the beginning (rather than at the end) would break the existing configurations (if somebody has set a value different from the default). Besides, I’d like that somebody take a look at the patch, I might have screwed something (but hopefully not).
I don’t think it breaks existing configs, but as I said, it doesn’t matter.
Looking at the patch, it looks fine.
I don’t see why REPLAYGAIN_OFF needs to be or even should at any specific position – it’s an enum, and the actual value of the enum doesn’t matter as you (at least should) only use the symbolic name anyway. It doesn’t change anything for existing configurations (i.e. cfg files) either as cfg files save the string representation, not any numerical value. Thus the enum value is simply an internal representation, so it’s perfectly safe to change that any time.
There’s a slightly different thing that caught my attention (but isn’t an issue of / should addressed with this patch): why is the type enum unnamed and the replaygain_type in the settings struct declared as int? Using a named enum instead would allow the compiler to warn if you use a value outside of the enum. Or did I miss any reason for using an int here?
@Dominik: having OFF=0 would allow to write something like “if (!replaygain_type)”. But I agree that this specifical case should be tested for by a dedicated comparison (which is done in the patch).
As to the usage of the enum: IIUC, it’s used just to defined some int values. Some #define’s could have been used as well. And I believe to have seen some code that uses a negative values as an indicator of some special condition which is then translated to some other value.