Re: [Audacity-devel] TimerRecordDialog - Remember Last Duration

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

Rupinder Singh wrote:
> Hi Vaughan,
>
> Thanks for the feedback. I have made the following changes:
>
>     > Storing the duration:
>     >
>     >    * Your hr, min, sec vars are used only within that
>     conditional, so
>     >      no reason to give them scope of the whole method.
>
>
> The variables have now been brought out of the conditional.
Um, no, I meant that they should be declared in that conditional, since 
they were used only in that conditional. Previously, you declared them 
in the scope of the method, but they didn't need scope outside the 
conditional. What you did took apart the (better) structure of the 
conditional.

---

>
>     >    * dTime is used later, but my preference is to declare it just
>     >      before its first use.
>
>
> dTime is now declared just before its use.
Yes, but the others (hr, min, sec) are still declared far away from 
their use.

---
>
>
>     >    * Your "else" clause duplicates the line before the
>     conditional. You
>     >      should have gotten rid of the original because in your code
>     it has
>     >      no effect other than wasting cycles, and...
>
>
> Fixed this. Now only one statement sets the initial duration.
True, but now m_DateTime_Start is set many lines before it's used. 
Rather than keep giving you hints that are causing you to move further 
from what I think is good form, here's how I'd write the first section:

       double dTime;
       if (gPrefs->Read(wxT("/TimerRecord/Last"), &dTime))
       {
          // Use the stored value.
          long hr = (long)(dTime / 3600.0);
          long min = (long)((dTime - (hr * 3600.0)) / 60.0);
          long sec = (long)(dTime - (hr * 3600.0) - (min * 60.0));
          m_TimeSpan_Duration = wxTimeSpan(hr, min, sec);
       }
       else
          m_TimeSpan_Duration = wxTimeSpan::Minutes(5); // default 5
    minute duration

       m_DateTime_Start = wxDateTime::UNow();
       m_DateTime_End = m_DateTime_Start + m_TimeSpan_Duration;


---

>  
>
>
>     >    * You changed the comment on the one in the "else" for no reason,
>     >      e.g., "default 5 minute duration" to "default Duration of 5
>     >      minutes". This gives no more information and simply adds
>     work for
>     >      anyone comparing the two versions. And there is no Duration
>     >      variable/method/etc, so capitalizing it makes no sense (in
>     English).
>
>
> Thanks for pointing this out, I've kept the original text intact.
>
>
>
>     >    * And overall, I don't consider this a very useful
>     improvement and
>     >      would prefer to not put *every* parameter in Prefs. I welcome
>     >      other comments about that.
>
>     >
>     > Your change for a zero duration comes after after WaitForStart.
>     > There's no reason to wait for start if the duration is zero,
>     e.g., if
>     > start time is in the future, the wait-for-start progress dialog will
>     > chug along, then the operation will be canceled without the user
>     > knowing why (if he or she accidentally set it to zero). The
>     proper way
>     > to do this is with a wxValidator, and to disallow setting it to zero
>     > and/or disable OK if they set a zero duration.
>
>
> Yes, I didn't look into that case.
>  
>
>     >
>     > Take a look into doing it that way, please. In the meantime, I'll
>     > submit a simpler fix so it doesn't crash.
>     >
>     Actually, a validator is not the right way to go, because there are
>     intermediate states where the control can be zero. E.g., if I want 30
>     seconds instead of 5 minutes, I might zero the 5 and I'd get the error
>     before I could type in the 3.
>
>     So, I think the fix I committed is okay. I'd prefer to just
>     disable the
>     OK button when duration is zero, but our convention has become, rather
>     than prevent it, to let the error happen and then explain the obvious,
>     so I put up a wxMessageBox.
>
>  
> This looks great. Thanks! But another bug seemed to have crept in. 
> After the wxMessageBox is done "OK" , the Timers stopped progressing. 
> I have tried to fix this in the patch. I hope the patch is fine now.
>
Good job catching the bug. But the better fix is to move the first line, 
the call to m_timer.Stop() to later, so you don't have to restart it.

Take a look at what I've committed. I still haven't added the Prefs 
entry, don't think it's a useful idea.

- V