Re: [Audacity-quality] Removal of CleanSpeech from our code

[Vaughan J. <va...@au...>]

Thanks for doing this, Martyn. I've done a code review of the patch.

Summary: I think it does no harm, and you should commit it, so more
people can easily test it using nightlies.


Some details:

* In ControlToolBar::EnableDisableButtons, you can get rid of the local
var cleaningSpeech. It's always false, so doesn't affect any of the
conditionals it's in. Don't bother to CLEANSPEECH it, because it's not
in CLEANSPEECH-only lines.

* For the "CLEANSPEECH remnant" cases, please add "end CLEANSPEECH
remnant" comments.

* BatchCommands::ApplyMenuCommand can be removed. It's not being called.

* I'd get rid of the comment at LoadEffects.cpp line 240. Irrelevant.

* In BatchPrefs::Apply(), your comment asks "is the rest of this fn
needed?" -- but it's only the "return true;", which is obviously
necessary. Maybe I don't know what you're referring to.

* Need to remove the commented-out line in
ControlToolBar::RegenerateToolsTooltips().


If you'd like to commit your changes, I'll do these additional changes.


>From my perspective, it would have been easier to review if you hadn't
worried about the indentations, and just limited the build conditionals
to the actual lines that need to be deleted, then fixed the indentations
when you actually delete it. The way you did it meant lots more lines to
review, and confusing nesting (e.g., #ifdef CLEANSPEECH inside a #else
to a #ifdef CLEANSPEECH). This was worst in
AudacityProject::CreateMenusAndCommands, because it already has a
complicatedly nested bunch of them. Easier to review just the functional
changes.

Great work. Thanks!

More comments inline...


On 4/19/2012 4:47 PM, Martyn Shaw wrote:
> Summary:
> I'm being cautious here.  

Good idea. Lots of changes.

>[...] 
> 
> Issues that I see remaining are:
> 
> I have left in ADVANCED_EFFECT and additionalEffects, in case they are
> useful in the future (for batch, or whatever).
> 
> BatchPrefs are commented out in PrefsDialog::PrefsDialog so Batch/Debug
> is never set/used.  The whole of BatchPrefs could be removed (or might
> be useful in the future).  Not a CS issue.
> 
> HIDDEN_EFFECT is a mystery to me.

Used only in EffectNyquist. Roger?


> 
> SIMPLE_EFFECT appears to be unused, but, as with HIDDEN_EFFECT, may be
> useful in future.

Neither of them appears to be tested anywhere (unless using their values
rather than names), just set.

SIMPLE_EFFECT is useful only as a shorthand for BUILTIN_EFFECT |
PROCESS_EFFECT. I'd actually get rid of the whole comment about "most
basic of menus" -- we don't use it that way.


> 
> The whole 'SpecialCommands' thing is a bit of a bodge, which I have left
> in, since we need to decide if we need it or not (or have another
> plan).  There are comments to that effect in the code above
> BatchCommands::ApplySpecialCommand.  BuildCleanFileName comes into that.
> 
> The RestoreChain methodology is flawed, with a new method needing to be
> added for each default chain.  I have left it in for the single default
> chain 'MP3Conversion' that is left.  The method used in EQ for default
> curves is much better, I feel.
> 

I think both these are separate from CleanSpeech purge, and should be
done, but after CleanSpeech is gone.

Thanks!

- V