[Faceted Search] Could facet by account

RESOLVED FIXED in Thunderbird 3.0rc1

Status

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: asuth, Assigned: asuth)

Tracking

Trunk
Thunderbird 3.0rc1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

9 years ago
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.
(Assignee)

Comment 1

9 years ago
Created attachment 402706 [details] [diff] [review]
facet account patch v1 [checked in]
Attachment #402706 - Flags: review?(david.ascher)
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)
Attachment #402706 - Flags: review?(david.ascher)
Attachment #402706 - Flags: review+
Attachment #402706 - Flags: approval-thunderbird3+
Keywords: checkin-needed
http://hg.mozilla.org/comm-central/rev/1b8533bb0bd3
Keywords: checkin-needed
Target Milestone: --- → Thunderbird 3.0rc1
Attachment #402706 - Attachment description: facet account patch v1 → facet account patch v1 [checked in]
(Assignee)

Updated

9 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.