GNOME Bugzilla – Bug 640659
Adding Zeitgeist Searchproviders to GNOME Shell
Last modified: 2011-07-24 19:32:10 UTC
This patch has 3 main changes: 1) Add the option of doing asynchronous searching with the Searchproviders by allowing them to add items to the results at any time 2) Add JS wrapper around the Zeitgeist API §) Add ZeitgeistAsyncSearchProvider Interface and the relevant Searchproviders that inherit from it: Music, Video, Documents, Pictures, Other
Created attachment 179387 [details] Add zeitgeist.js which is a wrapper around the zeitgeist dbus api
Created attachment 179388 [details] Add Asyc Searchproviders (thanks to magcius) by allowing searchproviders to add items at any time into their results Add new interface ZeitgeistAsyncSearchProvider Add new Searchproviders for Pictures, Movies, Music, Documents
Created attachment 179389 [details] add "Other" categories and allow launching results
Created attachment 179390 [details] clean up patch clean up docInfo
Review of attachment 179388 [details]: Please split this patch into two parts: "Add support for asynchronous search providers" "Add zeitgeist search providers" Please merge correctly, and please squash the patches. I will help you do this on IRC.
Created attachment 179401 [details] [review] Add support for asynchronous search providers
Created attachment 179402 [details] [review] Add Zeitgeist JS bindings
Created attachment 179403 [details] [review] Add support for Zeitgeist search providers
Review of attachment 179403 [details] [review]: You need to use semicolons, and have some misaligned lines. Additionally, see the Style Guide: http://live.gnome.org/GnomeShell/StyleGuide ::: js/ui/docDisplay.js @@ +58,3 @@ + +function ZeitgeistAsyncSearchProvider() { + this._init(interpretations); Where does 'interpretations' come from? @@ +63,3 @@ +ZeitgeistAsyncSearchProvider.prototype = { + __proto__: Search.SearchProvider.prototype, + _init: function(title ,interpretations) { Mismatch with constructor above. style nit: should be (title, interpretations) @@ +109,3 @@ + this.tryCancelAsync(); + var items = []; + log("--- Results for " + this._name) No. @@ +134,3 @@ + __proto__: ZeitgeistAsyncSearchProvider.prototype, + _init: function() { + interpretations = ["http://www.semanticdesktop.org/ontologies/2007/03/22/nfo#Document", Ah, this is where "interpretations" comes from. It's probably unintentional, but the lack of "var" or "let" makes it a global. Please don't do this. ::: js/ui/overview.js @@ +166,3 @@ this.viewSelector.addSearchProvider(new AppDisplay.PrefsSearchProvider()); this.viewSelector.addSearchProvider(new PlaceDisplay.PlaceSearchProvider()); + //this.viewSelector.addSearchProvider(new DocDisplay.DocSearchProvider()); Don't comment things out. Just remove it.
Review of attachment 179402 [details] [review]: Not sure if there's a better way to do the objectification or simplification of the D-Bus messages, like having an alternate constructor like "Subject.fromSimplified();" and "new Subject().toSimplified();" ::: js/misc/zeitgeist.js @@ +2,3 @@ + + +function objectifyEvents(rawEvents){ style nit: missing space between close parent and brace. @@ +3,3 @@ + +function objectifyEvents(rawEvents){ + var events = []; Please use "let" instead of "var". @@ +97,3 @@ + this.manifestation = manifestation; + this.actor = actor; + this.paylaod = payload; 'paylaod'? @@ +225,3 @@ +FTS.prototype = { + _init: function() { + log("Zeitgeist: Created new FTS module"); No. @@ +245,3 @@ +DBus.proxifyPrototype(FTS.prototype, ZeitgeistFTSIface); + +// Testing Why is this section here and commented out?
Review of attachment 179401 [details] [review]: This looks to be the same as my patch, so I'll tackle any issues with it.
Created attachment 179532 [details] [review] Add Zeitgeist JS bindings
Review of attachment 179532 [details] [review]: Overall, it's fine, but a couple of missing semicolons and mis-indented things. ::: js/misc/zeitgeist.js @@ +25,3 @@ + +const SIG_EVENT = 'asaasay' +const MAX_TIMESTAMP = 9999999999999 Missing semicolons. @@ +52,3 @@ + rawSubject[4], // mimetype + rawSubject[5], // text + rawSubject[6]) // storage Missing semicolons. @@ +59,3 @@ + rawSubject[0] = subject.uri; + rawSubject[1] = subject.interpretation; + rawSubject[2] = subject.manifestation Missing semicolons. @@ +69,3 @@ +/* Zeitgeist Events */ + +function Event(interpretation, manifestation, actor, subjects, payload) { Is there a reason 'id' and 'timestamp' aren't in the constructor? It looks like you use 'new Event' and then set 'id' and 'timestamp' down below? @@ +109,3 @@ + rawEvent[2] = event.payload; + return rawEvent; +} Missing semicolons. @@ +177,3 @@ + function handler(results, error) { + if (error != null) + log("Error querying Zeitgeist for events: "+error); Indented by five spaces. @@ +180,3 @@ + else + callback(results.map(Event.fromPlain)); + } Missing semicolons. @@ +217,3 @@ + function handler(results, error) { + if (error != null) + log("Error searching with Zeitgeist FTS: "+error); Indented by five spaces. @@ +220,3 @@ + else + callback(results[0].map(Event.fromPlain)); + } Missing semicolons.
Created attachment 179557 [details] [review] Add Zeitgeist JS bindings
Created attachment 179558 [details] [review] 179401: Add support for asynchronous search providers
Review of attachment 179557 [details] [review]: Looks fine to me.
Review of attachment 179558 [details] [review]: I'm not exactly sure what happened with the attachment name... are you using git-bz? ::: js/misc/docInfo.js @@ +63,3 @@ + this.timestamp = event.timestamp; + this.name = event.subjects[0].text; + this._lowerName = event.subjects[0].text.toLowerCase(); The private member looks a little ugly here, but personal preference. @@ +68,3 @@ + this.interpretation = this.subject.interpretation; + this.category = ""; + this._icon = null; Doesn't look like "category" and "_icon" are used. ::: js/ui/docDisplay.js @@ +9,3 @@ +const Lang = imports.lang; +const Mainloop = imports.mainloop; +const St = imports.gi.St; St and Mainloop aren't being used. @@ +16,2 @@ +// FIXME: The subject cache is never being emptied. +var ZeitgeistSubjectCache = {}; Use 'let' although 'const' will work here as well. @@ +59,3 @@ +function ZeitgeistAsyncSearchProvider() { + this._init(interpretations); +} This should look like: function ZeitgeistAsyncSearchProvider(title, interpretations) { this._init(title, interpretations); } Although you never use the constructor itself. @@ +67,3 @@ + Search.SearchProvider.prototype._init.call(this, title); + this._ZGFTS = new Zeitgeist.FTS(); + this.templates = [] Missing semicolons. @@ +111,3 @@ + uri = GLib.uri_unescape_string(uri, ''); + if (GLib.file_test(uri, GLib.FileTest.EXISTS)) { + if (ZeitgeistSubjectCache[subject.uri] == undefined) { if (ZeitgeistSubjectCache.hasOwnProperty(subject.uri)) { @@ +115,3 @@ + ZeitgeistSubjectCache[info.uri] = info; + } + items.push(event.subjects[0].uri); items.push(subject); @@ +131,3 @@ + _init: function() { + let interpretations = ['http://www.semanticdesktop.org/ontologies/2007/03/22/nfo#Document'] + ZeitgeistAsyncSearchProvider.prototype._init.call(this, "Documents", interpretations); Missing semicolons, additionally you probably want to use gettext for "Documents" here. @@ +144,3 @@ + _init: function() { + let interpretations = ['http://www.semanticdesktop.org/ontologies/2007/03/22/nfo#Video'] + ZeitgeistAsyncSearchProvider.prototype._init.call(this, "Videos", interpretations); Ditto. @@ +159,3 @@ + 'http://www.semanticdesktop.org/ontologies/2007/03/22/nfo#Audio', + 'http://www.semanticdesktop.org/ontologies/2009/02/19/nmm#MusicPiece'] + ZeitgeistAsyncSearchProvider.prototype._init.call(this, "Music", interpretations); Ditto. @@ +172,3 @@ + _init: function() { + let interpretations = ['http://www.semanticdesktop.org/ontologies/2007/03/22/nfo#Image'] + ZeitgeistAsyncSearchProvider.prototype._init.call(this, "Pictures", interpretations); Ditto. @@ +190,3 @@ + '!http://www.semanticdesktop.org/ontologies/2007/03/22/nfo#Audio', + '!http://www.semanticdesktop.org/ontologies/2009/02/19/nmm#MusicPiece'] + ZeitgeistAsyncSearchProvider.prototype._init.call(this, "Other", interpretations); Semis, gettext. This is also confusing, because you call the parent's init and then override the template behavior below. Not sure if it would be better to have a '_createTemplates' function in the parent prototype that you override here. @@ +194,3 @@ + // Here we construct an AND-like query instead of OR-like + this.templates = [] + let subjects = [] Watch out for those semicolons.
Created attachment 179584 [details] [review] Add support for asynchronous search providers Thank you for your reviews. > + this.name = event.subjects[0].text; > + this._lowerName = event.subjects[0].text.toLowerCase(); > The private member looks a little ugly here, but personal preference. It's just like in DocInfo. I think it makes sense to keep them together since they both are about the name, but I can move _lowerName to the end if you prefer. > +var ZeitgeistSubjectCache = {}; > Use 'let' although 'const' will work here as well. Changed it to const. Is there any information on when to use which of them? I've only found http://svn.gnome.org/viewvc/gjs/trunk/doc/Style_Guide.txt?view=markup, which says «in particular we think you need "var" to define module variables»; is a «module» something else than a file? Also, some files are using var at the top (eg. js/ui/extensionSystem).
Created attachment 179585 [details] [review] Add support for Zeitgeist search providers (Argh, replaced the wrong patch.)
Review of attachment 179557 [details] [review]: ::: js/misc/zeitgeist.js @@ +197,3 @@ + ], +}; + Just use the name "FullTextSearch" instead of FTS here. @@ +222,3 @@ + } + this.SearchRemote(query, [0, MAX_TIMESTAMP], + eventTemplates.map(Event.toPlain), 0, 130, 4, handler); What are the numbers "0, 130, 4"?
Review of attachment 179585 [details] [review]: Most of these comments are due to me not knowing about Zeitgeist, and I trust you on Zeitgeist more than me, but I'd like a bit more documentation. ::: js/misc/docInfo.js @@ +60,3 @@ + _init : function(event) { + this.event = event; + this.subject = event.subjects[0]; What happens when the event has more than one subject? ::: js/ui/docDisplay.js @@ +6,1 @@ const DocInfo = imports.misc.docInfo; You remove here "_" but use it down below. @@ +15,3 @@ +const ZeitgeistSubjectCache = {}; + +// FIXME: Superseded by Zeitgeist. Can this be deleted? Delete it if you don't use it or replace it. It's in git if we need it again. @@ +69,3 @@ + + _buildTemplates: function(interpretations) { + this.templates = [] Semicolon. @@ +72,3 @@ + for (let i = 0; i < interpretations.length; i++) { + let subject = new Zeitgeist.Subject('', interpretations[i], '', '', '', '', ''); + let event = new Zeitgeist.Event('', '', '', [subject], []); This pattern of filling in half the parameters with blanks seems a bit odd. Doing something like: Zeitgeist.Event.templateFromInterpretation(interpretation); would make this become this.templates = interpretations.map(Zeitgeist.Event.templateFromInterpretation); But I trust that you know this a bit better than I. @@ +82,3 @@ + + _asyncCancelled: function() { + // FIXME: Cancel pending D-Bus calls. This needs to be filled in so that we don't rack up four or five D-Bus calls on every search. @@ +197,3 @@ + _buildTemplates: function(interpretations) { + // For this one we need to AND the interpretations instead + // of OR'ing them. Why? The comment doesn't explain. @@ +205,3 @@ + } + let event = new Zeitgeist.Event('', '', '', subjects, []); + this.templates.push(event); Similarly, something like: this.templates = [Zeitgeist.Event.fromSubject(interpretations.map(Zeitgeist.Subject.templateFromInterpretation))];
For the "const" question, the style guide doesn't say... the most recent copy is at http://git.gnome.org/browse/gjs/tree/doc/Style_Guide.txt and I edited the GnomeShell/StyleGuide page to link there. I'm very sure the var limitation is gone, though that's a question for Colin Walters. "const" is technically correct because the variable itself is never bound to another object, but it may be confusing because the cache itself is mutable. One more thing: can you upload the newest "asynchronous" patch, because it's marked as obsolete here on git-bz.
> What happens when the event has more than one subject? There isn't currently any data-source doing this. This could be changed to add them all, but for now I'd just take the first one, and reconsider this once events with more than one subject exist (either having it take all subjects or changing the query to avoid getting such results -eg. if they only happen for some particular type of event-). Afaik this is what all existing GUIs do. > This pattern of filling in half the parameters with blanks seems a bit odd. > Doing something like: > Zeitgeist.Event.templateFromInterpretation(interpretation); Could be done (although in any case that'd have to be Event.templateFromSubjectInterpretation), but personally I'd see such a function as pretty arbitrary. Who says we won't be adding a condition other than the subject interpretations? I agree that the constructors aren't that readable, though. Would you prefer having the constructor take no arguments and assign the values directly afterwards (as in "subject.interpretation = 'foo';")? > This needs to be filled in so that we don't rack up four or five D-Bus calls on > every search. Yeah. Is there an example somewhere on how to do this from JavaScript? I couldn't figure it out. > + _buildTemplates: function(interpretations) { > + // For this one we need to AND the interpretations instead > + // of OR'ing them. > Why? The comment doesn't explain. Most queries are like "get all sound AND music" (therefor we want stuff matching any of the templats), however this one asks for everything not matched by the queries above, so it's "get everything that *isn't* music NOR image NOR document..." (that is, it must match ALL templates -which are negated-). > + [...] eventTemplates.map(Event.toPlain), 0, 130, 4, handler); > What are the numbers "0, 130, 4"? I'm not familiar with FTS, so I'll leave this one for Seif.
Created attachment 179842 [details] [review] Add support for asynchronous search providers
Created attachment 179843 [details] [review] Add Zeitgeist JS bindings
Created attachment 179844 [details] [review] Add support for Zeitgeist search providers
Review of attachment 179844 [details] [review]: ::: js/misc/docInfo.js @@ +52,3 @@ }; + No blank line. @@ +62,3 @@ + this.subject = event.subjects[0]; + this.timestamp = event.timestamp; + this.name = event.subjects[0].text; this.name = this.subject.text; @@ +94,3 @@ + } + return mtype; + } Comma or no comma? @@ +99,1 @@ var docManagerInstance = null; You removed all usage of DocItemInfo and DocManager in docSearch.js Remove DocItemInfo and DocManager here too. ::: js/ui/docDisplay.js @@ +10,3 @@ +const Gettext = imports.gettext.domain('gnome-shell'); +const _ = Gettext.gettext; No. @@ +42,3 @@ + + _asyncCancelled: function() { + // FIXME: Cancel pending D-Bus calls. This needs to be fixed. @@ +93,3 @@ + _init: function() { + let interpretations = ['http://www.semanticdesktop.org/ontologies/2007/03/22/nfo#Document']; + ZeitgeistAsyncSearchProvider.prototype._init.call(this, _("Documents"), interpretations); Other search section titles are uppercase: use DOCUMENTS, MUSIC.
Review of attachment 179843 [details] [review]: Mostly comments relating to style and cleanup. ::: js/misc/zeitgeist.js @@ +24,3 @@ +const DBus = imports.dbus; + +const SIG_EVENT = 'asaasay'; Might make sense to include the "(" and ")" that you seem to be using every time below. @@ +117,3 @@ +/* Zeitgeist D-Bus Interface */ + +const ZeitgeistLogIface = { Drop the "Zeitgeist" prefix. Module namespaces are enough. @@ +174,3 @@ + }, + + findEvents: function(timeRange, eventTemplates, storageState, numEvents, resultType, callback) { Drop this class, use DBus.makeProxyClass, and make this a wrapper function: Zeitgeist.findEvents @@ +189,3 @@ +/* Zeitgeist Full-Text-Search extension D-Bus Interface */ + +const ZeitgeistFTSIface = { FullTextSearchIface or IndexIface (because of org.gnome.zeitgeist.Index) Drop the "Zeitgeist" prefix. Module namespaces are enough. @@ +214,3 @@ + * searches. + */ + search: function(query, eventTemplates, callback) { Drop this class, use DBus.makeProxyClass, and make this a wrapper function: Zeitgeist.fullTextSearch. Docstring needs improvement: /** * fullTextSearch: * * Asynchronously search Zeitgeist's index for events relating to the query. * * @param query The query string, using asterisks for wildcards. Wildcards must * be used at the start and/or end of a string to get relevant information. * @param eventTemplates Zeitgeist event templates, see * http://zeitgeist-project.com/unexisting-doc/event-templates.html for more information * @param callback The callback, takes a list containing Zeitgeist.Event objects */ @@ +222,3 @@ + } + this.SearchRemote(query, [0, MAX_TIMESTAMP], + eventTemplates.map(Event.toPlain), 0, 130, 4, handler); As explained on IRC, the magic values are: 0 - offset into the search result 130 - maximum number of results, which is 130 due to a Zeitgeist and SQLite bug 4 - sort by 'most used' The 130 should definitely have a comment like // FIXME: we can only get 130 results due to http://bugs.launchpad.net/zeitgeist/12345678 and should maybe be a constant like const MAX_RESULTS = 130; with the aforementioned comment. '4' should be handled with a more complete enum, but http://zeitgeist-project.com/docs/0.6/datamodel.html#resulttype seem a bit on the crazy side, so something like const ResultType = { MOST_POPULAR_SUBJECTS: 4, }; should be fine for a stub, adding more as need be.
Created attachment 180575 [details] [review] Add Zeitgeist JS bindings
Created attachment 180576 [details] [review] Add support for Zeitgeist search providers
Created attachment 180585 [details] [review] Add support for asynchronous search providers Fix two space errors.
Created attachment 180586 [details] [review] Add Zeitgeist JS bindings Fix a couple whitespace mistakes.
Created attachment 180588 [details] [review] Add support for asynchronous search providers (Forgot to regenerate the patch). By the way, magcius, are you taking care of this one? Because that's what I understood from your last comment on it (so I haven't really looked at the contents of this patch).
Created attachment 180590 [details] [review] Add Zeitgeist JS bindings
Created attachment 180595 [details] [review] Add support for Zeitgeist search providers Updated to work apply correctly against HEAD.
Review of attachment 180588 [details] [review]: Yep, I'll take care of any problems with this patch.
Review of attachment 180590 [details] [review]: You need to add js/misc/zeitgeist.js to Makefile.am Otherwise, good, and I'm going to ask for some more testing of these patches. ::: js/misc/zeitgeist.js @@ +28,3 @@ + +// FIXME: We can only get 130 results due to the 132 argument limit +// set by Python's SQLite, workaround under development. Bug link if possible. @@ +32,3 @@ + +const ResultType = { + MOST_POPULAR_SUBJECTS: 4, Should have a comment explaining where this comes from // http://zeitgeist-project.com/docs/0.6/datamodel.html#resulttype // We only currently use MOST_POPULAR_SUBJECTS @@ +176,3 @@ +const Log = DBus.makeProxyClass(LogIface); + +/* Currently not being used: */ I assume you're going to use it in the overview tabs, so it's best to keep this uncommented. @@ +216,3 @@ + * be used at the start and/or end of a string to get relevant information. + * @param eventTemplates Zeitgeist event templates, see + * http://zeitgeist-project.com/unexisting-doc/event-templates.html for more Needs a real link. @@ +229,3 @@ + } + _index.SearchRemote(query, [0, MAX_TIMESTAMP], + eventTemplates.map(Event.toPlain), Align these with "query".
Review of attachment 180595 [details] [review]: Looks good to me. ::: js/misc/docInfo.js @@ +16,2 @@ this._lowerName = this.name.toLowerCase(); + this.uri = event.subjects[0].uri; this.uri = this.subject.uri; ::: js/misc/zeitgeistSearch.js @@ +1,1 @@ +/* -*- mode: js2; js2-basic-offset: 4; indent-tabs-mode: nil -*- This file goes in js/ui.
Created attachment 180599 [details] [review] Add Zeitgeist JS bindings
Created attachment 180600 [details] [review] Add support for Zeitgeist search providers
Created attachment 180650 [details] [review] Add Zeitgeist JS bindings Merged some changes by Federico into my patch.
Created attachment 180651 [details] [review] Add support for Zeitgeist search providers
Created attachment 182302 [details] [review] Add support for asynchronous search providers
Comment on attachment 180588 [details] [review] Add support for asynchronous search providers This new version is against the latest master from today; it handles the little change in updateSearch() to return a [] instead of an undefined value.
Siegfried's patches are settling down nicely, so I fixed up the last nits and pushed a "zeitgeist" branch to git.gnome.org. We should take care of merging master periodically into zeitgeist so they stay in sync. I can take care of this. I'll start pushing the "Recent Activities" journal tab for the overview into that same branch - right now my commits are still unpolished; I'll push them up as they become clean.
I guess one remaining problem is that if Zeitgeist's full-text-search extension is not installed, then searching doesn't work at all. You get an exception about a missing D-Bus method. In that case, we should probably fall back to searching just by filename.
Ping? Even if the Zg stuff is rejected, it would be good to have async support for search providers - can someone please review attachment 182302 [details] [review]
Review of attachment 182302 [details] [review]: I wrote the original version of this patch, and it was quite a while ago: I expect it would need at least a bit of effort put towards it to get it to apply properly. John, do you want to clean this up? Here's some things to start with. Additionally, I'd like to get it to a place where all items could be asynchronous: a new application gets installed as you're staring at the results, a new contact comes online, etc. ::: js/ui/search.js @@ +145,3 @@ + * to the current search. + */ + addItems: function( items) { Remove space in front of " items". @@ +262,3 @@ Signals.addSignalMethods(SearchProvider.prototype); + No blank line. @@ +398,3 @@ + return; + + results.push.apply(results, items); I'm a horrible person -- this relies on modifying the list in-place. @@ +399,3 @@ + + results.push.apply(results, items); + this.emit('results-updated', this._previousResults); I think it might be better to only cause a re-display for one provider. Splitting this into two signals: 'search-updated' and 'search-completed' might be a bit less ugly. The former would have (provider, newItems) as arguments, and the latter would have (results). @@ +425,3 @@ } + } else { + terms = this._previousTerms; We should probably bail out if this is the case: I doubt we should cancel and re-search with the same items. @@ +430,3 @@ let results = []; if (isSubSearch) { + for (let i = 0; i < this._providers.length; i++) { Might be worth a comment why we loop through all providers. If I remember correctly, that could happen if results don't come back yet.
Created attachment 192553 [details] [review] search: support for asynchronous search providers Original patch by Jasper St. Pierre and Seif Lofty. I reworked this a bit to apply on master and did some adaptations based on Jasper's review.
(In reply to comment #50) > Additionally, I'd like to get it to a place where all items could be > asynchronous: a new application gets installed as you're staring at the > results, a new contact comes online, etc. > Can this be done in a follow-up patch?
Given that the Zeitgeist search providers aren't going in the shell and Federico, Seif and such are working on an extension as a replacement, I'm going to close this bug. Can you file a new bug with the async patch?
See Bug 655220