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

On 28/04/10 16:10, Adrien Bustany wrote:
On Wed, 28 Apr 2010 11:22:49 +0000, Martyn Russell<martyn lanedo com>
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

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! :)


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
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
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
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
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


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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]