Rockbox

  • Status Closed
  • Percent Complete
    100%
  • 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
Attached to Project: Rockbox
Opened by fml2 - 2009-06-20
Last edited by fml2 - 2009-06-21

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

Committed in r21414. Close the task to not forget to do it later; reopen or submit bug reports if some are found.

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.

fml2 commented on 2009-06-20 19:25

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?

fml2 commented on 2009-06-20 22:02

@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...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing