Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#10356 - Merge the "Replaygain Off" option into the replaygain type; eliminate the "On/Off" setting

Attached to Project: Rockbox
Opened by Alexander Levin (fml2) - Saturday, 20 June 2009, 19:25 GMT+2
Last edited by Alexander Levin (fml2) - Sunday, 21 June 2009, 11:02 GMT+2
Task Type Patches
Category User Interface
Status Closed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version Version 3.3
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

This patch adds a new replaygain type "off" and eliminates the menu setting "Enable replaygain". I hope I got it right :-)
   replaygain-menu-merge.diff (4.3 KiB) 
 apps/gui/gwps-common.c     |    2 +-
 apps/settings.h            |    6 +++---
 apps/menus/playback_menu.c |    4 +---
 apps/dsp.c                 |    5 +++--
 apps/settings_list.c       |    6 ++----
 5 files changed, 10 insertions(+), 13 deletions(-)

This task depends upon

Closed by  Alexander Levin (fml2)
Sunday, 21 June 2009, 11:02 GMT+2
Reason for closing:  Accepted
Additional comments about closing:  Committed in r21414. Close the task to not forget to do it later; reopen or submit bug reports if some are found.
Comment by Thomas Martitz (kugel.) - Saturday, 20 June 2009, 21:21 GMT+2
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.
Comment by Alexander Levin (fml2) - Saturday, 20 June 2009, 21:25 GMT+2
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).
Comment by Thomas Martitz (kugel.) - Saturday, 20 June 2009, 22:48 GMT+2
I don't think it breaks existing configs, but as I said, it doesn't matter.

Looking at the patch, it looks fine.
Comment by Dominik Riebeling (bluebrother) - Saturday, 20 June 2009, 23:37 GMT+2
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?
Comment by Alexander Levin (fml2) - Sunday, 21 June 2009, 00:02 GMT+2
@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.

Loading...