User:Edgar/Patch001Review

General Comments
Reviewer: Ed Musgrove +1

I find this patch useful in two situations. As a normal user I almost always want to record at the same duration as that used last time. As a beta tester investigating a bug I always want to record for a very brief duration; I might perform this timed recording task 50 or more times in an hour trying to pin down a specific problem, the reduction in keystrokes and mouse movements obtained by using this patch is significant. In fact while exploring a bug with timed recording last fall I wrote this exact same code and have had it in "My Audacity" ever since.

I think this patch is stale, it would not apply (I will try to reference a fresh batch via nabble): - D:\audio\Audacity\record timer duration retention>dir Volume in drive D is RAIDdata Volume Serial Number is EA04-0A51

Directory of D:\audio\Audacity\record timer duration retention

02/08/2010 09:45 AM    . 02/08/2010 09:45 AM              .. 02/06/2010 10:22 AM             1,670 retainRecordTimerDuration.patch 02/08/2010 09:37 AM            17,057 TimerRecordDialog.cpp 2 File(s)        18,727 bytes 2 Dir(s) 712,987,291,648 bytes free

D:\audio\Audacity\record timer duration retention>patch -p1  - Probably because the patch is stale in re. SVN TRUNK; patch: - @@ -209,6 +218,9 @@   m_timer.Stop; // Don't need to keep updating m_DateTime_Start to prevent backdating. +  double dTime = m_TimeSpan_Duration.GetSeconds.ToDouble; +  gPrefs->Write(wxT("/TimerRecord/LastDurationInSeconds"), wxString::Format(wxT("%ld"),(long) dTime)); +   int updateResult = eProgressSuccess; if (m_DateTime_Start > wxDateTime::UNow) updateResult = this->WaitForStart; - TRUNK: -  m_timer.Stop; // Don't need to keep updating m_DateTime_Start to prevent backdating. this->EndModal(wxID_OK); }

///Runs the wait for start dialog. Returns false if the user clicks stop while we are recording ///so that the high bool TimerRecordDialog::RunWaitDialog {  int updateResult = eProgressSuccess; if (m_DateTime_Start > wxDateTime::UNow) updateResult = this->WaitForStart; -

Code Specific Comments
The following two lines are from the patch: +  minutes = (long) ((seconds - hours * 3600 ) / 60); +  seconds = (long) (seconds - ( hours * 3600 + minutes * 60 )); I would replace them with the functionally identical: minutes = (long) ((seconds - (hours * 3600)) / 60); seconds = (long) (seconds - ((hours * 3600) + (minutes * 60))); Two tiny changes -- I remove the superfluous spaces (i.e. after the 60 in "minutes * 60 ));" on the second line). In both lines I added superfluous parentheses around the multiplication [i.e. (hours * 3600)]; I know that multiplication takes precedence over addition, I just find that it makes the code easier to read. -- Because the original patch was stale it was not clear exactly where Lavanya intended to perform the preferences write. I have chosen to put it after EndModal on the (probably erroneous) assumption that while the operating system is doing housework closing a window Audacity can be doing the disk write. m_timer.Stop; // Don't need to keep updating m_DateTime_Start to prevent backdating. this->EndModal(wxID_OK); double dTime = m_TimeSpan_Duration.GetSeconds.ToDouble; gPrefs->Write(wxT("/TimerRecord/LastDurationInSeconds"), wxString::Format(wxT("%ld"),(long) dTime));