<html><head><meta name="qrichtext" content="1" /></head><body style="font-size:9pt;font-family:DejaVu Sans Condensed">
<p>On Thu August 17 2006 14:44, Jeff Mitchell wrote:</p>
<p>[...]</p>
<p><span style="color:#007000">> > - and please try to make the code easier to understand more safe</span></p>
<p><span style="color:#008000">></span></p>
<p><span style="color:#008000">> ? I don't think the code is that hard to understand. The process in</span></p>
<p><span style="color:#008000">> collectiondb for instance is more well documented than the much of the code</span></p>
<p><span style="color:#008000">> in amarok (see playlist.cpp, for instance), and the stuff in MetaBundle is</span></p>
<p><span style="color:#008000">> fairly straighforward. It's the algorithms that are hard to understand. </span></p>
<p><span style="color:#008000">> And we're always trying to make Amarok code more safe, aren't we...don't</span></p>
<p><span style="color:#008000">> think I haven't seen all your commits with the DeepCopys :-)</span></p>
<p><span style="color:#008000">></span></p>
<p><span style="color:#007000">> > * we should properly lock files that any Amarok process writes to</span></p>
<p><span style="color:#007000">> > (imagine a writable collection shared by several users - this is not only</span></p>
<p><span style="color:#007000">> > ATF related)</span></p>
<p><span style="color:#008000">></span></p>
<p><span style="color:#008000">> You can't do this, really. Scott and I had a very long discussion about</span></p>
<p><span style="color:#008000">> it. The problem is that flock and fcntl either don't work at all or are</span></p>
<p><span style="color:#008000">> very broken with files on network file systems, including NFS and Samba --</span></p>
<p><span style="color:#008000">> and many, many people fall into this realm. So you have to get around</span></p>
<p><span style="color:#008000">> this. You can either try per-file lockfiles, which have lots of problems,</span></p>
<p><span style="color:#008000">> or a global lockfile, which also has problems, and doesn't allow for tag</span></p>
<p><span style="color:#008000">> edits to be done during a scan. I've discussed these options at length</span></p>
<p><span style="color:#008000">> with Ian.</span></p>
<p></p>
<p>I didn't see anybody try flock or <span style="color:#ff0000">fcntl</span>. Both of these mechanisms were created to handle cases as ours. So I think it's worth trying before resorting to other mechanisms that involves changes in many places.</p>
<p><span style="color:#008000">></span></p>
<p><span style="color:#008000">> If you've seen my recent commits, you'll see I'm taking another approach --</span></p>
<p><span style="color:#008000">> adding the ability to a) find the instance of a running ScanController, and</span></p>
<p><span style="color:#008000">> b) pause the collectionscanner. So if you want to do tag editing, instead</span></p>
<p><span style="color:#008000">> of calling save() on the TagLib::File, you pass it into a function which</span></p>
<p><span style="color:#008000">> checks to see if a scanner is running. If so, you pause it, *wait for</span></p>
<p><span style="color:#008000">> confirmation*, do the write, then unpause it. If confirmation never comes,</span></p>
<p><span style="color:#008000">> you abort the tag writing. In addition, make sure when</span></p>
<p><span style="color:#008000">> amarokcollectionscanner is starting that no other processes are still</span></p>
<p><span style="color:#008000">> existing/running -- if so kill them, and if you can't (owned by another</span></p>
<p><span style="color:#008000">> user, or something else) then abort the scany. Crashing amarokapp with a</span></p>
<p><span style="color:#008000">> running scanner, then starting again was one reason the earlier problem</span></p>
<p><span style="color:#008000">> could occur.</span></p>
<p><span style="color:#008000">></span></p>
<p><span style="color:#008000">> It is true that this won't handle the case of a networked filesystem with</span></p>
<p><span style="color:#008000">> multiple users all scanning at the same time, all with ATF turned on. </span></p>
<p><span style="color:#008000">> There really isn't a good way to handle this that I've seen, and it's not</span></p>
<p><span style="color:#008000">> even limited to ATF: you can run into the same problem if you and someone</span></p>
<p><span style="color:#008000">> else both try to update the tags of a file at the same time, even with ATF</span></p>
<p><span style="color:#008000">> off. Again, it's just a casle where ATF makes it more likely because it's</span></p>
<p><span style="color:#008000">> dealing with so many files that first scan. So perhaps putting a warning</span></p>
<p><span style="color:#008000">> in the Information dialog about this case in the big red PLEASE NOTE</span></p>
<p><span style="color:#008000">> section would be good, telling users to scan from a single computer first</span></p>
<p><span style="color:#008000">> (and then possibly make the collection read-only for most users -- this is</span></p>
<p><span style="color:#008000">> a good idea even without ATF), or use a single computer's database, or etc.</span></p>
<p><span style="color:#008000">></span></p>
<p><span style="color:#007000">> > * do not make the behaviour of MetaBundle dependent on a</span></p>
<p><span style="color:#007000">> > process name (this is not very transparent to the caller, and some day,</span></p>
<p><span style="color:#007000">> > the process names might change and ATF will stop working)</span></p>
<p><span style="color:#008000">></span></p>
<p><span style="color:#008000">> And you suggest what, instead?</span></p>
<p></p>
<p>Please see below.</p>
<p>[...]</p>
<p><span style="color:#007000">> > * limit the locations where meta data writes are triggered to as few</span></p>
<p><span style="color:#007000">> > places as possible and make it more clear when this happens</span></p>
<p><span style="color:#008000">></span></p>
<p><span style="color:#008000">> It already only happens in a few places. We just need to update all the</span></p>
<p><span style="color:#008000">> file->save() commands to use the new function.</span></p>
<p><span style="color:#008000">></span></p>
<p><span style="color:#007000">> > * make the ATF related parts of the MetaBundle API behave just as the</span></p>
<p><span style="color:#007000">> > rest of MetaBundle - Amarok developers are already familiar with this</span></p>
<p><span style="color:#007000">> > pattern: + a setUniqueId(QString) call that sets the unique id *only in</span></p>
<p><span style="color:#007000">> > memory* + a uniqueId() call for querying the unique id</span></p>
<p><span style="color:#008000">></span></p>
<p><span style="color:#008000">> That's already the current behavior.</span></p>
<p></p>
<p>There is still this <span style="color:#ff0000">setUniqueId</span> call that does the actual writing. Which is confusing, as this call has the same name as the others, and it behaves differently. But I see, the situation has improved a bit: the other <span style="color:#ff0000">setUniqueId</span> calls don't call it. But the one w/o parameters is also not in line with the general <span style="color:#ff0000">MetaBundle</span> <span style="color:#ff0000">API</span> pattern: all other setters accept a parameter.</p>
<p><span style="color:#008000">></span></p>
<p><span style="color:#007000">> > + write the unique id together with the other tags in</span></p>
<p><span style="color:#007000">> > MetaBundle::save() + perhaps provide a saveUniqueId() as an optimization</span></p>
<p><span style="color:#007000">> > for only writing the unique id to the file</span></p>
<p><span style="color:#008000">></span></p>
<p><span style="color:#008000">> Won't be necessary once the new save function is implemented -- everything</span></p>
<p><span style="color:#008000">> will use that.</span></p>
<p><span style="color:#008000">></span></p>
<p><span style="color:#007000">> > * with this changed API, the collection scanner could just call</span></p>
<p><span style="color:#007000">> > bundle->setUniqueId(somerandomid); bundle->saveUniqueId() - no need for</span></p>
<p><span style="color:#007000">> > special casing MetaBundle for processes having a certain name</span></p>
<p><span style="color:#008000">></span></p>
<p><span style="color:#008000">> No, you can't do that, first because the scanner has to use an ID if it</span></p>
<p><span style="color:#008000">> already exists, which requires a lot of various logic and taglib stuff, and</span></p>
<p>> second because there are other times when UIDs can be set (or unset -- you</p>
<p>> have to allow for stripping as well). These functions are implemented</p>
<p>> outside the scanner, for instance through DCOP.</p>
<p></p>
<p>I still think you can: of course my pseudo code was not complete. Do something like this when scanning a file:</p>
<p>- read the tags, including a unique id if available (but don't yet do anything except for just reading the tags)</p>
<p>- if no unique id is there, then invent a new one and write it to the file</p>
<p></p>
<p>This would mean changing the current code from</p>
<p></p>
<p><span style="color:#ff0000">MetaBundle</span> bundle( path ); // this is scary, might write to the file if no unique id is there!</p>
<p></p>
<p>to</p>
<p></p>
<p><span style="color:#ff0000">MetaBundle</span> bundle( path ); // would just read tags</p>
<p>if( bundle.<span style="color:#ff0000">uniqueId</span>.<span style="color:#ff0000">isEmpty</span>() )</p>
<p>{</p>
<p> bundle.<span style="color:#ff0000">setUniqueId</span>( <span style="color:#ff0000">createUniqueId</span>() );</p>
<p> bundle.save();</p>
<p>}</p>
<p></p>
<p>The advantage of this is:</p>
<p>- it's obvious when the file gets written</p>
<p>- <span style="color:#ff0000">MetaBundle</span> remains a general purpose class with no magic <span style="color:#ff0000">behaviour</span> depending on the <span style="color:#ff0000">processs</span> name</p>
<p>- only one <span style="color:#ff0000">setUniqueId</span> method in <span style="color:#ff0000">MetaBundle</span> should be sufficient then - just as with e.g. <span style="color:#ff0000">setArtist</span></p>
<p>- less logic in <span style="color:#ff0000">MetaBundle</span>: no need for <span style="color:#ff0000">processesing</span> 'recreate' or 'strip' options - just do a <span style="color:#ff0000">setUnqueId</span>( <span style="color:#ff0000">createUniqueId</span>() ) or <span style="color:#ff0000">setUniqueId</span>( <span style="color:#ff0000">QString</span>::null ) followed by saving the bundle - the other cases you mentioned would work with this scheme easily</p>
<p></p>
<p>In my opinion, removing the special logic for unque ids from MetaBundle and doing it where it is necessary, would really make it easier to understand what's going on (e.g. I just had a look at collectionscanner.cpp for writing this mail and I was thinking, where the hell do the unique ids get written, until I found that it happens in readTags!)</p>
<p></p>
</body></html>