[Tracker] REVIEW: api-cleanup branch

[Martyn Russell <martyn lanedo com>]

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

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


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.


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?


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


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

--
Regards,
Martyn

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