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

[James C. <cr...@in...>]

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.


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

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

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.

> 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.

Thanks for the info.

--James.