Re: [Tracker] REVIEW: api-cleanup branch

[Martyn Russell <martyn lanedo com>]

On 18/02/14 15:05, Carlos Garnacho wrote:
> Hey,

Hi,

> So the main focus here was libtracker-extract and libtracker-miner. On
> the former I outlined the caveats at
> https://mail.gnome.org/archives/tracker-list/2014-January/msg00033.html
>
> So in the branch I made libtracker-extract entirely private, if exposed
> as-is could hardly allow us to extend things in the future without
> API/ABI breaks (eg. changing to async apis, implementing in-module
> cancellation...).

I think that makes sense.

It's easy enough to add extractors anyway and in most cases, we would 
either take those on upstream OR people can package those privately.

> Wrt libtracker-miner, I've lately been thinking it has some feature
> creep, it gathers miner objects, helper objects that are already wrapped
> by some of those miner objects, and TrackerMinerManager, which is a
> facade to every miner DBus interface. So I attempted to make
> libtracker-miner just that, a library to implement miners. The utility
> objects can easily go private, TrackerMinerFiles wraps those in more
> convenient ways, plus it's weird to offer people ways to reimplement
> what you already offer tidily packed :)
>
> TrackerMinerManager went private too, I think the audience for this
> object is quite disconnected from the users of the rest of the
> libtracker-miner API, mostly limited to tracker-control or any other
> session-wide controller that might arise. If deemed important, I would
> propose making it public on a separated libtracker-control.

We could indeed do that. The only comment I have there is that it would 
likely be quite a small library and is it worth putting it somewhere else?

> Another thing worthwhile to mention is the TrackerMinerWeb deprecation,
> my investigation turned out with 0 implementations in the wild, and the

I actually think what you did here is good. I agree that it was very 
limited and it was really proof of concept and supposed to help miner 
developers wanting to integrate Flickr, etc web services.

Since this API, GOA has been coming on leaps and bounds and it really is 
outdated anyway, so I think what you did here makes sense.

> API it offers has been largely overtaken by *-online-accounts. It still

Right exactly :)

> makes sense to offer a basic miner for remote content, so I added
> TrackerMinerOnline that just controls miner state across network
> changes, in a way that is useful to the RSS miner, and could be
> encouraged for gnome-online-miners that could still be using g-o-a for
> the services themselves.

Precisely what I was thinking during review.

> > So comments:
> >
> > 3. The tracker-crawler.[ch] looks like a lift and shift to
> > libtracker-common, but I wonder why it's moved out of libtracker-miner.
> > It seems certainly to be a feature you would use from libtracker-miner?
> > What I don't understand is why the Makefile.am on master shows the
> > crawler sources as a separate variable to the other sources. Maybe
> > that's why you moved it? I would think it makes more sense to keep it in
> > libtracker-miner even if it was private and just keep the header
> > internal to that library? Logically, those functions are not useful
> > anywhere else in the code base and I would rather not bloat
> > libtracker-common.
> >
> >
> > 4. The tracker-storage.[ch] move made me laugh. This poor bastard has
> > been moved about, copied and pasted a bunch of times and there is even a
> > copy of it in libmediaart :) I think it was in libtracker-miner because
> > we need to know about storage for file system based miners and it makes
> > sense logically. Just wondering why you moved it out of
> > libtracker-miner? Saying that it looks like it was private anyway in
> > libtracker-miner. So moving it makes little sense and logically it
> > should be in libtracker-miner, given it's used for mining. No other
> > modules use that code IIRC, that's why we moved it out of
> > libtracker-common some years back.
>
> Moving TrackerCrawler and TrackerStorage was basically because
> tracker-control uses TrackerCrawler for some misc. things, and

I see, was your concern here that tracker-control shouldn't be linking 
to libtracker-miner or?

You mention below using GIO, would it be better to move TrackerCrawler 
back into libtracker-miner, make it private and use GIO for tracker-control?

API/ABI wise, we could do this AFTER a release anyway given it's all 
private. But I would like to resolve it before if we can.

> TrackerStorage was a bit too tied in to TrackerCrawler, so moving both
> around altogether seemed the best.

I think that's due to the volume foo in there. Did you have plans to 
improve this? libremovablemedia :P ?

On a serious note, if you moved the crawler back into libtracker-miner, 
would you do the same for TrackerStorage?

> Another option might have been using plain gio in tracker-control and
> keeping this in libtracker-miner, just private.

Yea, we could have done that, but I think I wanted to make use of 
filtering and some other features - and I was sure to "eat our own dog 
food" making sure this stuff works. That was my feeling at the time, 
it's not so important though. GIO is easily an alternative I agree.

Are you planning on switching to GIO in the future?

> > 5. I like the rename to tracker-miner-online.[ch] from
> > tracker-miner-web.[ch]. One concern I have is that it makes things

[...]

> So practically, potential users like gnome-online-accounts have just
> bypassed that on their own miner-alike objects. Further googling came up
> with 0 users of TrackerMinerWeb...

I have a suspicion the Flickr miner doesn't use the libtracker-miner 
APIs any longer.

> I agree TrackerMinerOnline is lower level, so I hope useful too as a
> base for the "web services" usecase that TrackerMinerWeb tried to cater
> for. I dislike gnome-online-miners not deriving from TrackerMinerObject
> at the very least (eg no progress report, pausing, etc), and I think
> this object could be a better fit for them.

Agree.

> > 6. I think moving tracker-miner-manager.h is possibly a mistake. It's
> > used by tracker-control and I have a feeling that others are making use
> > of that API (outside the community).
>
> I agree it can be useful to certain users, maybe a separate
> libtracker-control should be in place, so each library has a narrowed
> down audience. I'll hack that up later today.

Great. Thanks Carlos.

> Everything is needed to get 1.0 out of the door with relief :P.

:)

So to summarise, my wish list (assuming you agree):

1. Creation of libtracker-control for TrackerMinerManager APIs.
2. Move TrackerCrawler/TrackerStorage back into libtracker-miner and
   make them private (this was the case IIRC).
3. Switch to GIO for tracker-control (instead of TrackerCrawler APIs).

Thoughts?

Let me know when I can do a final review/merge.

--
Regards,
Martyn

Founder & Director @ Lanedo GmbH.
http://www.linkedin.com/in/martynrussell