extragear/multimedia/amarok/src

[Martin Aumueller]

On Fri July 14 2006 02:17, Jeff Mitchell wrote:
> Guys, please don't get overzealous in doing ATF checking.  If you see a
> function that doesn't do a check, it's usually either specifically by
> design, or because it will be checked for soon down the line anyways.  The
> core functions all check for it before anything (files or the database) is
> modified, so at best you save maybe a millisecond or two of processing time
> and at worst you change the functionality in unintended ways.  And
> functions that use the unique ids that don't do a check will just get
> QString::null anyways, and will just revert to normal behavior.

Wrt intention: if ATF is off, then my intention is that nothing ATF related is 
happening. Most importantly, I don't want my files changed. I don't want 
either ATF tags added nor removed.
>
> That having been said, most of this commit is incorrect or not needed, so
> I'm going to revert a lot of it.
>
> Btw, sorry about all the ;s, either it was compiling fine for me anyways,
> or I made some last minute changes without realizing I hadn't
> compile-tested.

Compiling was not the problem: the problem was that the empty statement ; was 
executed conditionally instead of setUniqueId - which was called 
unconditionally!
>
> >  void MetaBundle::setUniqueId( TagLib::FileRef &fileref, bool recreate,
> > bool strip ) {
> > +    if( !AmarokConfig::advancedTagFeatures() )
> > +        return;
>
> This was right before, and now will behave incorrectly.  ATF being turned
> on is checked for below, and parts of this function must still be able to
> run even when ATF is off (see the "strip" bool).
>
> > +
> > +    if( !(url().protocol() == "file") )
> > +    {
> > +        debug() << "no file protocol url" << endl;
> > +        return;
> > +    }
>
> It checks for it being a stream below, etc.  What if there are non "file"
> protocols that can handle metadata?  We check to see if TagLib can get a
> fileref and if the file isWritable according to TagLib anyways, so let
> TagLib figure it out.

Checking for protocol=="file" is clearly necessary. Imagine the following:
You add remote file, say fish://nowhere.com/mp3/tune.mp3 to your playlist.
You also happen to have a file /mp3/tune.mp3 locally on your computer.
What will happen is: url().path() returns /mp3/tune.mp3 - and this is what 
gets passed to taglib. taglib will happily modify the local file, whereas the 
correct behaviour would have been to modify the remote file. (But this is 
beyond taglib's capabilities.)

So, whenever taglib is called with url().path(), then a check for 
url().protocol() is necessary, as taglib is not able to figure out 
ambiguities like these.
>
> >  void
> >  MetaBundle::newUniqueId()
> >  {
> > +    if( !AmarokConfig::advancedTagFeatures() )
> > +        return;
> > +
>
> Again, not necessary.  I'll leave it for now but at some point it may go
> away in the future so people can assign unique IDs even when ATF isn't
> actually enabled.
>
> >  void MetaBundle::removeUniqueId()
> >  {
> > +    if( !AmarokConfig::advancedTagFeatures() )
> > +        return;
> > +
>
> This defeats the functionality, since we want people to be able to remove
> UIDs even if ATF is off.  This is by design.  See the strip bool that is
> passed to setUniqueId.

The additional checks for ATF might not be necessary in all cases, but as the 
mishap with the semicolons showed: defensive programming can't hurt. And this 
is a case where valuable user data is at stake - an mp3 collection that could 
have taken years to assemble, so that it's in order to be cautious.