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 _______________________________________________ tracker-list mailing list tracker-list@gnome.org https://mail.gnome.org/mailman/listinfo/tracker-list
