Some API comments and questions

[Scott Wheeler]

Finally have home internet again -- so starting in on some of the questions 
from the last month or so:

On Tuesday 17 January 2006 12:54, nerochiaro wrote:
> I'm writing Ruby bindings for TagLib 1.4 (full API) and so far I got
> most of Xiph and APE tags to work and I'm moving on to wrap the ID3
> tags handling. I have some questions and comments I would like to
> share before finishing this.
> Keep in mind that my C++ is somewhat rusty at the moment and that most
> of the work is being done in SWIG and Ruby, so pardon me if something
> I say doesn't make much sense.
>
> 1) Files get opened in the constructor and get closed in the
> destructor. While this is fine for C++ use, it does become
> inconvenient when used from languages that rely on garbage collection.
> With GC objects can be deleted at any point in time after going out of
> scope, thus potentially leaving files open much longer than needed.
> In my bindings i have handled this with an hack on the Ruby GC, but
> having a File::close function to just close the underlying file handle
> (fclose) would be much more cleaner.

Why not just use a proxy object?  i.e. Just create a class with the File 
methods and wrap those with (just using C++ here since I don't know Ruby):

RubyFile::read()
{
  if(!closed())
    theRealFile->read(...);
}

> 2) I see that in many places Map-based objects are returned by
> reference (e.g. Ogg::XiphComment::fieldListMap or
> APE::Tag::itemListMap), while List-based objects are always returned
> by value (e.g. ID3v2::TextIdentificationFrame::fieldList or
> APE::Item::toStringList).
>
> This seems to have the drawback that we're accessing a different copy
> of the List each time, while it would be more convenient to be
> accessing the same List instance across calls.

Well, technically it's not accessing a different copy of the containers each 
time since all of the TagLib containers use copy-on-write semantics.  (i.e. 
copying one list to another list is just a pointer copy -- they're not 
separated until you change one of them).

To my knowledge the only place that references are returned they are const 
references, which means that you can't (or at least really shouldn't) modify 
them either.

While I can see your point that it at times would be easier to work with the 
underlying data structures directly, it's often either not practical (i.e. 
the structure in the API is not the same one used internally) or not good 
design to expose underlying data structures (since that prevents you from 
later modifying the underlying data structures).  Also, if users of the API 
are working with those structures directly it means that from the libraries 
perspective we don't know when they might be modifying them (i.e. we return a 
reference, do something to the list, the user modifies it and leaves it in a 
strange state, further processing didn't account for that, etc.).  Using 
accessor / mutator methods keeps the boundary between the library and the 
application more clear.

> It even probably resulted in a bug already, in APE::Tag::setValue (see
> KDE bug #120256), and in general makes working with the APE tags more
> cumbersome (especially from bindings).

That one wouldn't have specifically been avoided since the toStringList() call 
that's used would need to create a temporary object anyway.

>
> 3) Finally, a minor nitpick: the docs for TagLib::Map< Key, T
>
> >::insert(const Key & key,const T &  value) state that if the key
>
> exists, the value gets overwritten. Instead, if the key exists,
> nothing is done, at least the STL implementation i use (GNU).

That's easy enough to fix.  :-)

-Scott

-- 
I mean, if 10 years from now, when you are doing something quick and dirty, 
you suddenly visualize that I am looking over your shoulders and say to 
yourself, "Dijkstra would not have liked this", well that would be enough 
immortality for me.
-E. W. Dijkstra