Re: [Audacity-devel] Snap-To needs attention

[Al D. <bus...@gm...>]

On Sunday 14 February 2010 14:28:00 James Crook wrote:
> On 14/02/2010 20:16, Al Dimond wrote:
> > Having a time converter in AudacityProject would mean fewer
> > gPrefs lookups, so that sounds good. It looks like using
> > TimeConverter is similar to using the TTCs, where you have to set
> > a value (which sets some state) and then read it to get the
> > snapped value. So we'd have to be sure only one bit of code used
> > it at a time. That shouldn't be a problem now (as long as we
> > don't yield while using it and audio threads don't touch it) but
> > it seems kind of sloppy to have this time value sitting around in
> > AudacityProject being set to arbitrary values having nothing to
> > do with the Project.
> 
> Well...  I would argue that it is associated with the selection
>  rather than being associated with the project. We should be able
>  to make it so that we can guarantee only one thread ever changes
>  the selection at a time.  Of course you are pointing to a bigger
>  can of worms, that we haven't documented thread-safety or
>  otherwise of the project class and we do need to do that.  We have
>  warnings in the code around threading which are along the lines of
>  'here be dragons', and we could be a lot more specific about where
>  they are lurking and where is safe territory.
> 

Yeah. We definitely need to get a handle on that.

> > To me an ideal TimeConverter class for that purpose wouldn't
> > store the time in a member variable, it would merely store the
> > format info.
> >
> > wxString ValueToString(double rawTime);
> > double StringToValue(wxString valueString);
> >
> > Snapping would work like:
> >
> > t = stringToValue(valueToString(t));
> 
> Or even:
> t = mTimeConverter.Snap( t );
> 

It would be nice to have that as a shortcut :).

> > GridMove() needs something more from TimeConverter -- it needs a
> > class that holds a value and can do increments and decrements.
> > That logic is currently tied up in TTC (Increase(), Decrease()).
> 
> I would suggest we move those two functions from TimeTextCtrl to
> TimeConverter too...
> 
> The Array of standard formats should even be in yet another
>  different object since we do not want to keep creating that list
>  and tearing it down.  (certainly not on every mouse move).
> 

That would make sense. As long as we make sure it gets rebuilt when 
needed for translation purposes (to be honest I don't know just what 
we're doing with TTCs for translation).

> In a sense what I have done is to make TimeTextCtrl multiply
>  inherit from wxControl and TimeConverter, but being explicit about
>  it by having TimeTextConverter as a member variable, which is a
>  bit more verbose.  It is arguable that I should actually have used
>  multiple inheritance here.
> 

I guess the question is whether it's an "is a" relationship or a "has 
a" relationship. In my ideal scheme it's probably more "has a" -- but 
the way the code breaks down now it's closer to "is a", as so much 
state has to be shared.

> > So we'd want
> > something like a TimeString class for that stuff. You'd have to
> > give it a TimeConverter to construct it; it would need
> > GetString(), SetString() functions, plus stuff like....
> 
> Hmm.  I think we can postpone splitting a TimeString out of
> TimeConverter, though it does sound a very plausible further
>  breaking down of the class into components.
> 
> > If that basically sounds reasonable but you don't want to do it
> > (it's a lot of reorganization) I can take a hack at it later
> > today.
> >
> >   - Al
> 
> I'll do the change to avoid creating a TimeTextCtrl on a mouse
>  down, including moving the increase/decrease functions in, and
>  probably creation of a singleton class for that list of time
>  formats. I may or may not add a TimeConverter variable to the
>  project.  I mainly wanted to check that I hadn't missed some
>  reason not to, and you've reassured me on that.
> 
> I think beyond that everything is icing on the cake.  It will
>  ultimately make the code clearer and easier for people new to it
>  to follow, but it's a long term payoff rather than a near term
>  one.  The label track linking changes you're doing will have a
>  much sooner pay off in terms of users getting benefit, so is
>  better value for the time put in.
> 

Sounds good.

> Thanks for the info.
> 

Thanks for thinking about this and pulling TimeConverter out. That's 
definitely moving in the right direction.

 - Al

> --James.
> 
>