Re: [Tracker] miner-flickr-review branch review

[Martyn Russell <martyn lanedo com>]

On 28/04/10 16:10, Adrien Bustany wrote:
> On Wed, 28 Apr 2010 11:22:49 +0000, Martyn Russell<martyn lanedo com>
> wrote:
> > 5. Do we writeback nao:description? I guess this is something Flickr
> > puts next to each picture?
> I don't get this one... I store pictures descriptions as nie:comment.
> And AFAIK Flickr doesn't allow me to update a picture's description.

Looks like we don't writeback nao:description, but your branch changes 
the ontology so we signal it...

+ tracker:writeback true

Which looks like it isn't needed?

> > 6. What are the API_KEY/SHARED_SECRET? They are defined but look like
> > some sort of random string. We should at least put a comment describing
> > where this comes from, how it was generated, etc.
> Those are used by Flickr to identify the app. I added a comment for that.

Great thanks.

> > 8. Shouldn't progress be 0.0 initially and status "Initializing" ?
> Technically the miner won't do anything until you call authenticate,
> so it's idle.

That's true actually. Instead of "Idle" though, perhaps we should use 
"Not authenticated with service" or something to that effect, otherwise 
people will assume it is working and all up to date?

> > 10. Won't the init_datasource() error on every subsequent construct()
> > other than the first (since it tries to insert without checking)?
> It doesn't error here

Then why have the try{} ?

> > 11. What does this do:
> > â+       delete_query = ("delete {<%1$s>  dc:title ?title }"
> > +                    +  "where  {<%1$s>  dc:title ?title }")
> > +                    .printf (photoset_urn);
> I remove the stored title, since it's going to be imported again (useful
> if the photoset title changed remotely, to refresh it)

I meant, what does the <%1$s> mean here? :)

> > 13. Please space things properly:
> >
> > + (1.0*indexed_photosets)/n_photosets;
> >
> > Also, why 1.0 * ? Do you mean to just cast this to a gdouble?
> Added cast and spaced

Great! :)

> > 15. Exif stuff should probably be added to libtracker-extract? I see a
> > lot of duplicate code here where we check whitespace values, etc with
> > constants in SPARQL. I would prefer it if we simplified all this and had

Yea :/ we can improve that later I think.

> > 16. How does yield work?
> I invite you to look at
> http://blogs.gnome.org/juergbi/2009/09/18/closures-and-asynchronous-methods-in-vala/

OK great, still learning bits ;)

I am quite interested in how this works under the hood. Decided to read 
the generated code:

  valac -C --pkg gio-2.0 vala-yield.vala

Very cool stuff! :)

Thanks

> > 19. I would prefer it if writeback was done in a different vala file so
> > we keep the mining and writeback clearly apart.
> Hmm yeah I understand that, but it's pretty hard to split the class
> without
> having to duplicate a lot of code :/ Or I can make the run_call public in
> MinerFlickr and pass it to a MinerFlickrWriteback object maybe...

OK, I thought this might be the case, that's fine then.

> > 20. I see we use nao:identifier? why?
> I need to keep a persistent mapping between urns and flickr identifiers. I
> chose to use nao:identifier for that, but that could be stored in a
> separate
> file/DB too.

I see, that makes sense.

> > 21. This update_tripple_* API feels like it is duplicating code, I
> > wonder if we could improve that in the lower APIs
> Updating values is a real pain with SPARQL. So yeah, might make sense to
> have
> that in libtracker-client, at least until I do my summer of code ;)

Yea, remind me what you're doing for that?

> > 22. Can we instead use #ifndef G_OS_WIN32 or put a #error in here? â
> >
> > +#if G_OS_WIN32
> > +#else
> I don't think so. But yeah that looks awkward as it is :)

Can you use ifndef then please.

> > 24. Generally we need more status updates I feel.
> I have to find how I can emit them, since I'm doing a lot of things in a
> synchronous
> way.

OK.

Let me know when you've changed the #ifndef things and we can merge this 
to master for today's release! :)

--
Regards,
Martyn