Re: [Tracker] REVIEW: api-cleanup branch

[Carlos Garnacho <carlosg gnome org>]

Hey,

On jue, 2014-02-13 at 18:25 +0000, Martyn Russell wrote:
> Hello all,
>
> So Carlos recently finished the api-cleanup branch¹. I am reviewing it 
> here because it is important people understand the changes going on AND 
> to give some feedback before merging.
>
> ¹ https://git.gnome.org/browse/tracker/log/?h=api-cleanup

First off, I think this branch deserves some further explanations. If
1.0 is going to get out of the door, I thought it would be great to
thing ahead in backwards compat, and commit ourselves only to the API
that's proven strong and/or successful. If this could be used to
constrain the numerous entry points that Tracker has gained, the best.

One thing to note is that libtracker-sparql is untouched, IMO the API is
concise and future wise can cope with additions/deprecations. The
objects there aren't meant to be derived either, so no padding pointers
are necessary in these classes.

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

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.

Another thing worthwhile to mention is the TrackerMinerWeb deprecation,
my investigation turned out with 0 implementations in the wild, and the
API it offers has been largely overtaken by *-online-accounts. It still
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.

With these changes, libtracker-miner object hierarchy would be:

-- TrackerIndexingTree (helper object for TrackerMinerFS)
-- TrackerMinerObject
 |- TrackerMinerFS
 |- TrackerMinerOnline
 \- TrackerDecorator
   \- TrackerDecoratorFS

> So comments:
>
> 1. First, thank you Carlos for clearing up the documentation aspect of 
> libtracker-miner. There was some left over crap which we've moved out to 
> libmediaart and now we should have less warnings when compiling AFAICS :)
>
> 2. The tracker-accumulators.h has:
>
> +#ifndef __LIBTRACKER_MINER_UTILS_H__
> +#define __LIBTRACKER_MINER_UTILS_H__
> ...
> +#endif /* __LIBTRACKER_MINER_UTILS_H__ */
>
> Which seems wrong, given it's in libtracker-common? :)

Oops, yes, I didn't change #defines when moving files around.

> 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
TrackerStorage was a bit too tied in to TrackerCrawler, so moving both
around altogether seemed the best.

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

> 5. I like the rename to tracker-miner-online.[ch] from 
> tracker-miner-web.[ch]. One concern I have is that it makes things 
> simpler (which is good), but also seems to remove any capability for 
> online services. Previously with miners like the Flickr miner, we needed 
> a way to authenticate and get an OK from the end user to index that 
> data. What we have in master currently is a basic version of the GOA 
> stuff that has come on leaps and bounds AFAICS. So are you assuming 
> those miners using the old API will need to figure all that out 
> themselves now? I should add, I think the use of this API is pretty 
> limited to one miner right now, maybe 2?

I think TrackerMinerWeb was a good idea coming around at the wrong
time :), it doesn't offer much more than a hashtable to store
credentials, the users of this object still need to:

1) Fetch the account information somehow (probably from GOA/UOA) and
store it as TrackerMinerWeb info
2) Getting that account info from the hashtable and using it in
implementation-dependent ways

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

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

> 7. Nice catch with tracker-writeback/ if we really were getting the 
> MinerManager and doing nothing with it :)
>
>
> ****
>
> So in general, I like the changes, I think some of the removals (like 
> password provider and some of the authentication web miner stuff) really 
> only affect one miner or so, most don't need that framework.
>
> However, the miner-manager removal does concern me because there is no 
> way for people interested in the indexing processes to get an overall 
> state or idea of what is going on now without checking each miner. Also, 
> there is no way to control ALL miners collectively which is quite 
> important for some systems (IIRC Nokia paused all miners when a call 
> came in or was made). It seems there is no way to do this now easily.
>
> Other than that, great work Carlos! :)

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

Cheers,
  Carlos