Re: [Tracker] RFC: UPnP mediaserver (ContentDirectory) miner

[Martyn Russell <martyn lanedo com>]

On 10/02/11 12:37, Jussi Kukkonen wrote:
> Hi all,

Hi,

> I'm writing a UPnP ContentDirectory (mediaserver) miner for tracker. See
> code at
>     git://gitorious.org/tracker-upnp/tracker-upnp.git
>     http://gitorious.org/tracker-upnp/tracker-upnp
> I'm currently working on it as a independently installable component (as
> I need it for tracker 0.10 in MeeGo and assumed I'm way too late to get
> it in the release). It would still be nice to get this into tracker git
> (better sooner than later), which is why I'm writing this email. Let me
> know if you have comments on the idea / implementation.

Thanks for writing this miner :) my comments:

1. Why use upnp:available when tracker:available exists and a lot of 
queries already use that?


2. The miner object needs to be GInitiable, see how we do this with the 
flickr miner if you need some hints:

http://git.gnome.org/browse/tracker/tree/src/miners/flickr/tracker-miner-flickr.vala


3. Coding style, for vala files, we keep the initial function bracket on 
the same line as the function parameters. We also don't have parameters 
on separate lines as a general rule (unless we breach 80 columns). There 
are cases where there is no space before/after {} for some property 
get/sets. There are also situations where the if () { } else if () { } 
else { } brackets are not all in place. For details see:

  http://live.gnome.org/Tracker/Documentation/CodingStyle#Vala


4. We should handle Ctrl+C gracefully (which the Flickr miner seems to 
do too).


5. The state changes should be debug messages I think so identify what's 
going on when running it more clearly.


6. There is an indentation issue here, but also I think this should be 
an nmm:MusicPiece AND nfo:Audio...

                        else if (item.upnp_class.has_prefix ("object.item.audioItem"))
                        nmm_type = "nmm:MusicPiece";


7. There are some TODOs which would need fixing, like:

/* TODO add a nmm:Artist */
/* TODO add nmm:MusicAlbum and link to that */

We don't allow extractors to insert with links to missing resources 
generally.

--

If you can amend these issues, I will do a final test and sanity check 
the SPARQL used before migrating it into the tracker code base.

Thanks for all the hard work put in here guys,

--
Regards,
Martyn