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

[Adrien Bustany <abustany gnome org>]

On Thu, 29 Apr 2010 10:30:23 +0100, Martyn Russell <martyn lanedo com>
wrote:
> 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?
Uuuh actually it's for nao:hasTag, I didn't read that line right :) So yes
it's 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?
True. Fixed that.

> > > 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{} ?
What I meant is that it doesn't fail on the SPARQL side (triple is ignore
if already inserted). But the DBus call can fail...

> > > 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? :)
Oh, that's standard printf syntax: it allows you to specify the position
of
the argument you want to pick
printf ("%2$s %1$s", "rocks", "Tracker") prints "Tracker rocks"

> > > 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
Yep, it's some sugar around GAsyncReadyCallback and friends... That's why
I ported the
libtracker-miner to GAsyncReadyCallback actually :) To be able to use
yield from Vala.

> > > 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?
The idea is to make an ORM for Tracker. So I guess with a good ORM I'd
just do my_object.nie_title = "Foo"...

> > > 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.
I said "I don't think so" :) I tried actually, and it doesn't work. I
could
probably patch Vala for that, but that's low priority.

> > > 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.
Just for the record: I used to do things more asynchronous, but librest
suffers
from weird crashes in that case. So I have to go synchronous for now...
I'll see
if librest stays alive, if it disappears I'll have to port to libsoup, and
will
try to make things more async again. Or I could use threads meanwhile...

> Let me know when you've changed the #ifndef things and we can merge this

> to master for today's release! :)

Well so #ifndef is not fixable, but I fixed the initial status. If it's OK
to merge,
let's merge :) This weekend I'll try to hack up a quick gtk interface for
association.
We wanted to see with rishi how we could unify credential storage, but
meanwhile an
ad-hoc solution will do.

Cheers

Adrien