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

On Thu, 29 Apr 2010 10:30:23 +0100, 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>
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
where this comes from, how it was generated, etc.
Those are used by Flickr to identify the app. I added a comment for

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

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

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

Just for the record: I used to do things more asynchronous, but librest
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
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
We wanted to see with rishi how we could unify credential storage, but
meanwhile an
ad-hoc solution will do.



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