User:Edgar/Patch001Review

TimerRecord to remember the last schedule duration. by Lavanya Tekumalla

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 <DIR> . 02/08/2010 09:45 AM <DIR> .. 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 <retainRecordTimerDuration.patch patching file TimerRecordDialog.cpp Assertion failed: hunk, file ../patch-2.5.9-src/patch.c, line 354

This application has requested the Runtime to terminate it in an unusual way. Please contact the application's support team for more information.

D:\audio\Audacity\record timer duration retention>

---

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));