518718 - [Faceted Search] Could facet by account

Status: RESOLVED FIXED

(Thunderbird :: Search, defect)

Description

[Andrew Sutherland [:asuth] (he/him)]

There have been requests for faceting by account.  We originally did not support this because the gloda data model currently does not expose the account (although the use case has been desired).

The changes made to support grouping of mime types by their category has made it feasible to implement the account as a (synthetic) extra facet which we are aliasing to a non-existent "account" attribute for the purposes of localization.

The provided patch's major change is to create the concept of facet definitions, allowing support for multiple facets per attribute.  The extra facets live in "extraFacets" which is a bit ugly, but livable and provides us with the concept of a canonical facet which I think is desirable.

This patch does make it possible to issue gloda queries like: "messageQuery.folderAccount(someGlodaFolder)" to query on all the folders associated with the account associated with someGlodaFolder.  If we went a little further we could allow someone to pass-in an incoming server instead of a Gloda folder.  This would make more sense, but I'm not sure we really want to be pushing this use-case right now given that the resulting SQLite query is pretty ugly.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

Attachment: facet account patch v1 [checked in]

Comment 2

[David Ascher (:davida)]

Comment on attachment 402706 [details] [diff] [review]
facet account patch v1 [checked in]

This works great, and solves an obvious faceting need.  It's a bit hard to review line by line because so many of the faceting code is touched, and I don't claim to understand it all.

I have a few nits, which I'm fine with addressing post-landing this, but before we close this bug.


>diff --git a/mailnews/db/gloda/modules/datamodel.js b/mailnews/db/gloda/modules/datamodel.js
>--- a/mailnews/db/gloda/modules/datamodel.js
>+++ b/mailnews/db/gloda/modules/datamodel.js
>@@ -390,16 +390,24 @@ GlodaFolder.prototype = {
>       this._xpcomFolder = null;
>       // since the last retrieval time tracks whether we have marked live or
>       //  not, this needs to be reset to 0 too.
>       this._activeHeaderRetrievalLastStamp = 0;
>     }
> 
>     return true;
>   },
>+
>+  /**
>+   * Return the string associated with this account.
>+   */
>+  get accountLabel() {
>+    let msgFolder = this.getXPCOMFolder(this.kActivityFolderOnlyNoData);
>+    return msgFolder.server.prettyName;
>+  }

The paranoid in me wonders whether we need to worry about exceptions along the way, or empty prettyNames.

(gloda.js)

>     // -- L10n.
>     // If the provider has a string bundle, populate a "strings" attribute with
>     //  our standard attribute strings that can be UI exposed.
>-    if ("strings" in aAttrDef.provider) {
>+    if (("strings" in aAttrDef.provider) && (aAttrDef.facet)) {
>       let bundle = aAttrDef.provider.strings;
>-      let attrStrings = aAttrDef.strings = {};
>+
>+      function gatherLocalizedStrings(aPropRoot, aStickIn) {
>+        for each (let [propName, attrName] in
>+                  Iterator(Gloda._ATTR_LOCALIZED_STRINGS)) {
>+          try {
>+            aStickIn[attrName] = bundle.get(aPropRoot + propName);
>+          }
>+          catch (ex) {
>+            // do nothing.  nsIStringBundle throws exceptions because it is a
>+            //  standard nsresult type of API and our helper buddy does nothing
>+            //  to help us.  (StringBundle.js, that is.)
>+          }
>+        }

I realize you just moved that code, but I think it'd be good to add a comment here pointing to a bug against nsIStringBundle or StringBundle.js which justifies the use of a blanket exception handler.  I presume that bug asks/would ask for a "has()" method that we could use instead.

I appreciate & like that the l10n system you have here is generic and will work with extensions, though!

r=davida with those addressed in a subsequent checkin.  

(to whoever checks this in -- don't close the bug)

Comment 3

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/comm-central/rev/1b8533bb0bd3