The current search from the chat tab shows only IM conversation results, and searching from another tab shows only email results. As we aren't very satisfied of this behavior, I'm trying to make the search global and the same in these tabs. Andrew explained to me that gloda collections can't contain results of mixed types, so the only solution seems to be to merge the results just before they are displayed. The patch I'm attaching is still a work-in-progress, but it seems to work. I would like some feedback on the WIP patch I currently have before pushing this further. The next steps are: - remove the existing IM-specific search (trivial; just some cleanup) - attempt to add some filters. I think a "Source" filter would be nice, with the network that messages are coming from ("Twitter", "IRC", "email", ...). Not sure yet how difficult that would be.

Comment on attachment 612888 [details] [diff] [review] WIP I think the "!val"'s you are introducing might need to be more specific, namely null based on the in check preceding it. (We do have some boolean facets.) None of your changes seem unreasonable to me. Specifically, it seems quite reasonable to add specific special cases for IM's rather than try and generalize to include other noun types too at this point. Do you have a plan for what "open as list" will do in the face of mixed search results? (You might want to bring Blake in on that.) As mentioned in the e-mail thread, I think the 'account' facet that's derived from the folder facet could cover the source. (It should only show up when the results include more than one account (I think), so you might not be seeing it right now.)

- Handled Andrew's comment about !val - Removed the existing chat-only search. I'm not sure of what to do with the source/account facet; it doesn't seem easy to handle as the account facet is derived from the folder facet and IM conversations aren't in folders.

My suggestion for using the folder comes from a half-implemented prototype some time ago to expose twitter to (only) gloda. The idea was that when defining a non-standard message source, gloda would define a (fake) folder that the messages could be said to live in. Here's the patch-set I was using to support the extension at the line in question for the extra API call: http://hg.mozilla.org/users/bugmail_asutherland.org/comm-central-patches/file/6d032fc4d180/gloda-actions#l396 And here's the extension at the line that makes that call to define the folder: http://hg.mozilla.org/users/bugmail_asutherland.org/gp-snitter/file/555f8100703b/modules/index_twitter.js#l74 The "account" facet is using the "accountLabel" getter on GlodaFolder. Now, that does assume that it is an XPCOM folder, but that getter can be changed. It's probably key to note that the twitter plugin actually was creating messages in the same noun-space as normal messages, so that bit is slightly different, but I think the idea still holds, although there may be a few other pieces of code making assumptions that will need to be patched.

In terms of your out-of-band question about exposing "account" directly on the messages, I think I'm neutral. It would want to be a magic-getter on the datamodel that just looks things up internally. I don't think it would want to be an attribute we can query on, or if it were, it probably would want to expand into a query for all the folders belonging to the account. I'm confident the reason I went with creating a synthetic folder was because I had to do that to reuse the message noun type and its database table. Since you aren't doing that, account can work just as well. protz, do you have any opinion either way?

(In reply to Florian Quèze from comment #2) > Created attachment 613569 [details] [diff] [review] > WIP 2 I pushed this patch to the try server, here's the resulting build: https://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/florian@queze.net-4c7137fce9b1/ Jb and Blake may want to try this.

Okay, initial thoughts: When I click on an IM search result (in my case, Twitter), it would be nice to highlight the message that matched somehow. (Email only shows one message, so there's less ambiguity as to which one you should be looking at.) "Open as list" only opens some of the messages. We should perhaps make that label more clear. "Open email as list"? And as you know, we kind of need a way of filtering on type (IM/email). :) Thanks, Blake.

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #6) > Okay, initial thoughts: Thanks for the feedback. > When I click on an IM search result (in my case, Twitter), it would be nice > to highlight the message that matched somehow. Yes, it would be nice! :) At some point I wanted to open a findbar on the displayed IM conversation automatically and to fill the input box of the findbar automatically with the searched term. That wouldn't really work however, as if I search "testing" in gloda, a message containing "test" or "tested" will be returned as a result. I think I should somehow prefill the input box of the search bar with the text of the first gloda match, but that doesn't seem as easy to do. > "Open as list" only opens some of the messages. We should perhaps make that > label more clear. "Open email as list"? If we can take string changes, sure. > And as you know, we kind of need a way of filtering on type (IM/email). :) I would like to filter either by network (email/Twitter/Google Talk/IRC/...) or by account, but only filtering IM vs email may be easier.

(In reply to Florian Quèze from comment #7) > That wouldn't really work however, as if I search "testing" in gloda, a > message containing "test" or "tested" will be returned as a result. > I think I should somehow prefill the input box of the search bar with the > text of the first gloda match, but that doesn't seem as easy to do. You wrote the patch that does this for messages! Just copy and paste your code :) http://hg.mozilla.org/comm-central/rev/916cf00f9afa

I don't think I have any strong opinion on this :)

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #6) > Okay, initial thoughts: > > When I click on an IM search result (in my case, Twitter), it would be nice > to highlight the message that matched somehow. (Email only shows one > message, so there's less ambiguity as to which one you should be looking at.) I would like to handle this in a separate bug, as it's not related to unifying email and IM results. > "Open as list" only opens some of the messages. We should perhaps make that > label more clear. "Open email as list"? Once we are sure of the wording, this change is trivial to make. We probably need to also change the string id to ensure translators will see the new string. While looking in the code where that string is set, I noticed that there's also this string; which doesn't seem to be used at all: glodaFacetView.results.message.openAsList.tooltip=Show all of the messages in the active set in a new tab, closing this tab. Do we want to actually use it in a tooltip, or should we just remove it? Seems like another bug though.

This new iteration of the patch fixes the account facet by making "account" an attribute of messages. I pushed this to try, I'll paste the link to the builds once they are finished.

Try server builds with attachment 616148 [details] [diff] [review]: Mac and Linux: https://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/florian@queze.net-d8d3fd377883/ Window: https://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/john.hopkins@mozillamessaging.com-0a52e770ccad/try-comm-central-win32/

Comment on attachment 616148 [details] [diff] [review] Patch v3 Review of attachment 616148 [details] [diff] [review]: ----------------------------------------------------------------- This all seems pretty reasonable. I'm r-minusing because I'm still pretty concerned about the changes to facet.js logic without us having any unit tests and I want to review the fallout of whatever form the goal you are trying to accomplish takes on. I am only reviewing this on a core gloda basis, not so much the faceted UI basis. I definitely have not run with this, so some other party needs to do that aspect of the review. I'm requesting it from Blake since I think regressions fall on his head anwyays, and he ideally will run with the patch in his testing. Since any regressions are going to have to do with facet.js, it may make sense for Blake to defer said review until the next patch. ::: mail/components/im/modules/index_im.js @@ +105,3 @@ > get subject() this._title, > get date() new Date(this._time * 1000), > + get recipients() { From a performance perspective, it's better if we have a consistent shape for the object and avoid the in check. (I do realize the message class does some "in" checking that is not so great.) Specifically, how about we set _recipients to null initially and then populate on demand? ::: mailnews/db/gloda/modules/datamodel.js @@ +573,5 @@ > catch (ex) { > } > return null; > }, > + get account() { Creating a new GlodaAccount instance for every message (and not caching it) seems fairly expensive. How about we defer to the GlodaFolder instance since we already need to ask it for the XPCOM folder anyways? ::: mailnews/db/gloda/modules/facet.js @@ +214,5 @@ > let groups = this.groups = {}; > this.groupCount = 0; > > for each (let [, item] in Iterator(aItems)) { > + if (!(attrKey in item)) This and all the other skip-if-no-attribute constitutes a change in semantics that is concerning. Unfortunately, due to concerns about disk space and memory usage (because of the potential for sparse attributes), I believe the lack of an attribute can still have meaning and bailing out of the loop will break things. Obviously, if we had unit tests, this might not be a concern. But so I either need you to write a bunch of unit tests for the existing faceting logic, do an audit of all attributes and the faceting in use, or not change the semantics of this logic. What is the purpose of this change, anyways? Are you trying to stop instant messages from showing up in "negative evidence" facets where you don't think IM's should show up at all? ::: mailnews/db/gloda/modules/fundattr.js @@ -128,5 @@ > - // sort the groups by string using magic convenience value > - groupComparator: function(a, b) { > - return a.accountLabel.localeCompare(b.accountLabel); > - }, > - queryHelper: "Account", This query helper (it lives under the defineNoun for "folder") needs to be removed or migrated. I think the grander idea behind supporting this was that every set of faceted constraints in the faceting UI would also be structured as a valid gloda query. This had the enhancement upside of allowing us to say "don't see your message in here? get more messages like this!" in a very straightforward fashion. I rather doubt there is any code anywhere that actually uses this logic as it stands, including unit tests. Since that enhancement is not going to happen and since I think the resulting query would be arguably pretty inefficent as things are currently structured, I think it makes sense to just remove that query helper. @@ +126,5 @@ > }); // tested-by: test_attributes_fundamental > this._attrFolder = Gloda.defineAttribute({ > provider: this, > extensionName: Gloda.BUILT_IN, > + attributeType: Gloda.kAttrDerived, You need: canQuery: false It's defaulting to true because facet is true. And unless we migrate that query helper logic, we can't actually query on this attribute. ::: mailnews/db/gloda/modules/gloda.js @@ +1260,5 @@ > + return -1; > + } > + return a.name.localeCompare(b.name); > + }, > + toParamAndValue: function(aGlodaAccount) { toParamAndValue isn't required since this is a non-queryable type of thing.

(In reply to Andrew Sutherland (:asuth) from comment #13) > ::: mailnews/db/gloda/modules/facet.js > @@ +214,5 @@ > > let groups = this.groups = {}; > > this.groupCount = 0; > > > > for each (let [, item] in Iterator(aItems)) { > > + if (!(attrKey in item)) > > This and all the other skip-if-no-attribute constitutes a change in > semantics that is concerning. Unfortunately, due to concerns about disk > space and memory usage (because of the potential for sparse attributes), I > believe the lack of an attribute can still have meaning and bailing out of > the loop will break things. > > Obviously, if we had unit tests, this might not be a concern. But so I > either need you to write a bunch of unit tests for the existing faceting > logic, do an audit of all attributes and the faceting in use, or not change > the semantics of this logic. > > What is the purpose of this change, anyways? Are you trying to stop instant > messages from showing up in "negative evidence" facets where you don't think > IM's should show up at all? The obvious case I remember was preventing the "folder" facet from breaking horribly when some search results didn't have a "folder" attribute. I think another facet of a different type had a similar problem, and then I ported this change to all other instances of similar looking code. > ::: mailnews/db/gloda/modules/fundattr.js > @@ -128,5 @@ > > - // sort the groups by string using magic convenience value > > - groupComparator: function(a, b) { > > - return a.accountLabel.localeCompare(b.accountLabel); > > - }, > > - queryHelper: "Account", > > This query helper (it lives under the defineNoun for "folder") needs to be > removed or migrated. It's already removed in the current patch, isn't it? Or are there parts of it that I forgot? > @@ +126,5 @@ > > }); // tested-by: test_attributes_fundamental > > this._attrFolder = Gloda.defineAttribute({ > > provider: this, > > extensionName: Gloda.BUILT_IN, > > + attributeType: Gloda.kAttrDerived, > > You need: > canQuery: false > > It's defaulting to true because facet is true. And unless we migrate that > query helper logic, we can't actually query on this attribute. > > ::: mailnews/db/gloda/modules/gloda.js > @@ +1260,5 @@ > > + return -1; > > + } > > + return a.name.localeCompare(b.name); > > + }, > > + toParamAndValue: function(aGlodaAccount) { > > toParamAndValue isn't required since this is a non-queryable type of thing. It was definitely called during my testing when adding a constraint, but maybe it's because I forgot the |canQuery: false|?

(In reply to Florian Quèze from comment #14) > > What is the purpose of this change, anyways? Are you trying to stop instant > > messages from showing up in "negative evidence" facets where you don't think > > IM's should show up at all? > > The obvious case I remember was preventing the "folder" facet from breaking > horribly when some search results didn't have a "folder" attribute. I think > another facet of a different type had a similar problem, and then I ported > this change to all other instances of similar looking code. The simplest fix I can think of for this is for us to introduce a sentinel object like Gloda.IGNORE_FACET: {}, and have that loop bail if it sees that. Then your noun instance can just put, folder: Gloda.IGNORE_FACET on its prototype and be done with it. In a perfect world, I'd probably rather have well-defined/documented behaviour about the impact of missing attributes and unit tests to back that up. Failing that, a very explicit hack such as IGNORE_FACET has a lot to commend it (namely there are no new subtle failure cases.) > > This query helper (it lives under the defineNoun for "folder") needs to be > > removed or migrated. > > It's already removed in the current patch, isn't it? Or are there parts of > it that I forgot? http://mxr.mozilla.org/comm-central/source/mailnews/db/gloda/modules/gloda.js#1171 Unless that was removed in the patch and I missed it. > It was definitely called during my testing when adding a constraint, but > maybe it's because I forgot the |canQuery: false|? Yeah. It would have been inserting rows into messageAttributes without that.

note that for dependency/load ordering reasons, you might need to put IGNORE_FACET in a more root dependency like datastore.js and just have gloda.js export it, possibly via having public.js add it on, or the like. (Assuming you take that route.)

Comment on attachment 616148 [details] [diff] [review] Patch v3 UI-Review: Something seems a little odd when I select "Twitter", and have all the other facets entirely vanish, but hopefuly that'll be fixed when we get Address Book integration, and are able to show the people for a Twitter stream. So I guess ui-r=me, even though it still feels unpolished. >> When I click on an IM search result (in my case, Twitter), it would be nice >> to highlight the message that matched somehow. (Email only shows one >> message, so there's less ambiguity as to which one you should be looking at.) >I would like to handle this in a separate bug, as it's not related to unifying email and IM results. So, I think I disagree, since this is getting IM results to work the same as email results, which seems important if we propose to unify them. I guess I might accept it as a separate bug that this one depends on, though. Have you opened that new bug yet? I'm concerned about this getting lost in the shuffle. >> "Open as list" only opens some of the messages. We should perhaps make that >> label more clear. "Open email as list"? >Once we are sure of the wording, this change is trivial to make. We probably need to also change the string id to ensure translators will see the new >string. I think, if there's no way to get the IM conversations into the list, then "Open email as list" will work. And we should also hide it when there are no messages shown, because opening up an empty list isn't useful to anyone. >While looking in the code where that string is set, I noticed that there's also this string; which doesn't seem to be used at all: >glodaFacetView.results.message.openAsList.tooltip=Show all of the messages in the active set in a new tab, closing this tab. > >Do we want to actually use it in a tooltip, or should we just remove it? Seems like another bug though. I don't see why we shouldn't roll the fix for that into this patch, too. It seems like a small change. I also agree that we should definitely have tests for this piece of code, particularly since we're about to change the behaviour. >+++ b/mail/base/content/glodaFacetView.js >@@ -912,31 +901,37 @@ function reachOutAndTouchFrame() { > if ("searcher" in aTab) { > FacetContext.searcher = aTab.searcher; > aTab.searcher.listener = FacetContext; >+ if ("IMSearcher" in aTab) { >+ FacetContext.IMSearcher = aTab.IMSearcher; >+ aTab.IMSearcher.listener = FacetContext; >+ } So, why aren't we just always hooking up the IMSearcher? Other than that, I don't have any particular problems with the front-end bits of this version of the code, but if they get a lot of changes, feel free to ask for a re-review. Thanks, Blake.

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #17) > Comment on attachment 616148 [details] [diff] [review] > Patch v3 > >+++ b/mail/base/content/glodaFacetView.js > >@@ -912,31 +901,37 @@ function reachOutAndTouchFrame() { > > if ("searcher" in aTab) { > > FacetContext.searcher = aTab.searcher; > > aTab.searcher.listener = FacetContext; > >+ if ("IMSearcher" in aTab) { > >+ FacetContext.IMSearcher = aTab.IMSearcher; > >+ aTab.IMSearcher.listener = FacetContext; > >+ } > > So, why aren't we just always hooking up the IMSearcher? Because we don't want (or at least I assume we don't want) IM results if the IM feature has been turned off with the global mail.chat.enabled preference.

(In reply to Florian Quèze from comment #18) > > >+ if ("IMSearcher" in aTab) { > > >+ FacetContext.IMSearcher = aTab.IMSearcher; > > >+ aTab.IMSearcher.listener = FacetContext; > > >+ } > > So, why aren't we just always hooking up the IMSearcher? > Because we don't want (or at least I assume we don't want) IM results if the > IM feature has been turned off with the global mail.chat.enabled preference. And we don't want to (or can't) check the pref? (I'm okay with that, if that's the reason.) Thanks, Blake.

Comment on attachment 616148 [details] [diff] [review] Patch v3 (In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #19) [...] > And we don't want to (or can't) check the pref? (I'm okay with that, if > that's the reason.) We do check the pref, here: >diff --git a/mail/base/content/search.xml b/mail/base/content/search.xml >+ let prefBranch = >+ Components.classes['@mozilla.org/preferences-service;1']. >+ getService(Components.interfaces.nsIPrefBranch); >+ if (prefBranch.getBoolPref("mail.chat.enabled")) { >+ if (!("GlodaIMSearcher" in window)) >+ Cu.import("resource:///modules/search_im.js"); >+ args.IMSearcher = new GlodaIMSearcher(null, searchString); >+ } >+ tabmail.openTab("glodaFacet", args); I think checking the pref in several places would make the code more fragile.

(In reply to Andrew Sutherland (:asuth) from comment #13) > ::: mail/components/im/modules/index_im.js > @@ +105,3 @@ > > get subject() this._title, > > get date() new Date(this._time * 1000), > > + get recipients() { > > From a performance perspective, it's better if we have a consistent shape > for the object and avoid the in check. I don't think I understand how this affects performances (I thought consistent object shapes was important only for good tracing, and but jstracer has been disabled (bug 693815) and removed (bug 698201)), but I think I've done the changes you wanted to this getter anyway. > ::: mailnews/db/gloda/modules/facet.js > @@ +214,5 @@ > > let groups = this.groups = {}; > > this.groupCount = 0; > > > > for each (let [, item] in Iterator(aItems)) { > > + if (!(attrKey in item)) > > This and all the other skip-if-no-attribute constitutes a change in > semantics that is concerning. Unfortunately, due to concerns about disk > space and memory usage (because of the potential for sparse attributes), I > believe the lack of an attribute can still have meaning and bailing out of > the loop will break things. > > Obviously, if we had unit tests, this might not be a concern. But so I > either need you to write a bunch of unit tests for the existing faceting > logic, do an audit of all attributes and the faceting in use, or not change > the semantics of this logic. After a quick audit of attributes and facets I now think that change caused a problem with the tags, attachmentTypes and mailingLists attributes. The new patch I'm attaching uses your IGNORE_FACET sentinel value suggestion. > @@ +126,5 @@ > > }); // tested-by: test_attributes_fundamental > > this._attrFolder = Gloda.defineAttribute({ > > provider: this, > > extensionName: Gloda.BUILT_IN, > > + attributeType: Gloda.kAttrDerived, > > You need: > canQuery: false After adding this, I had to rename my 'account' attribute to '_account' to still pass the test at http://hg.mozilla.org/comm-central/annotate/c603ee7bcda3/mailnews/db/gloda/modules/gloda.js#l1516 Because of this, I had to add the _ in the ids of the localizable strings too, which doesn't seem great as it will force all localizers to "retranslate" them. I would have preferred finding another solution, but I don't understand the code around this test well enough to change it. Maybe you or protz will have a better solution to offer. (In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #17) > >> When I click on an IM search result (in my case, Twitter), it would be nice > >> to highlight the message that matched somehow.[...] > I guess I might accept it as a separate bug that this one depends on, > though. Have you opened that new bug yet? No, but I will before marking this bug as resolved. > >> "Open as list" only opens some of the messages. We should perhaps make that > >> label more clear. "Open email as list"? Done. > And we should also hide it when there are no messages shown [...] Done too. > >While looking in the code where that string is set, I noticed that there's also this string; which doesn't seem to be used at all: > >glodaFacetView.results.message.openAsList.tooltip=Show all of the messages in the active set in a new tab, closing this tab. > > > >Do we want to actually use it in a tooltip, or should we just remove it? Seems like another bug though. > > I don't see why we shouldn't roll the fix for that into this patch, too. It > seems like a small change. I changed the code to set the title attribute to the tooltip text (I adapted the wording a bit). However the tooltip isn't displayed for at least 2 different reasons: first the results are in a <browser> that's in an <iframe>, neither of which has the "tooltip" attribute set correctly; second it seems the code to fill the tooltips with the text in the title (html) or tooltiptext (xul) attribute doesn't work for anonymous (XBLgenerated) nodes. I don't have a trivial fix for these problems, and I still think that seems like something for another bug.

protz, there's a question in here I'd like you to weigh in on if you have the free brain cycles; it's okay if you're too busy though. (In reply to Florian Quèze from comment #21) > > From a performance perspective, it's better if we have a consistent shape > > for the object and avoid the in check. > > I don't think I understand how this affects performances (I thought > consistent object shapes was important only for good tracing, and but > jstracer has been disabled (bug 693815) and removed (bug 698201)), but I > think I've done the changes you wanted to this getter anyway. The JM/TI polymorphic inline caches (PICs) still operate based on shape. As far as I understand, shape is all we have for fast-pathing property lookup or determining 'type' of instances. And it's still very much dependent on the order in which properties are added (although dherman recently proposed making it less sensitive to that in the js-engine list, although the property would always need to be present.) > After adding this, I had to rename my 'account' attribute to '_account' to > still pass the test at > http://hg.mozilla.org/comm-central/annotate/c603ee7bcda3/mailnews/db/gloda/ > modules/gloda.js#l1516 That test would definitely benefit from a comment. It is trying to make sure that we generate a query constraint for '_deleted' which is the only '_' prefixed attribute. It is prefixed because only gloda internals should be aware of deleted messages. I don't think the specialized check is actually necessary at all; the attribute for '_deleted' should just have its canQuery set to true in defineAttribute. There should be no bad fallout from that because Gloda.grokNounItem will a) Gloda.grokNounItem doesn't process attributes stored with underscores, and b) Gloda.grokNounItem doesn't get called on deleted messages. So I just refreshed my understanding of how glodaFacetView.js works, and realize that yes, we are using the queries to do the filtering. Which does make sense and is a good idea, I say. But it does mean that we need a constraint on the query object, as you discovered. So, yes, we either want that test to pass or we want another query to be added somehow. There are a few ways I can think of to do this: - Make 'canQuery' be an enumerated-ish type that takes on the values 'database', 'memory', and false. The current value of true would be akin to 'database' and mean to grokNounItem that yes, it should create rows in messageAttributes. Any truthy value, however, would make the check in Gloda._bindAttribute pass. There would be the unfortunate side effect that the query could still hit the database where it would simply return no results. - Add another property to the attribute definition to cover the 'memory' case of the previous bullet. - Add a property to the noun definition that conveys that the noun is a synthetic concept and have _bindAttribute check for that when it's dealing with the noun type. - Use the queryHelpers mechanism instead. Modify _bindAttribute to stop producing the exploding constraint methods by just removing that last else case so it won't fight the queryHelpers. If people try to query on something they can't query on, there just won't be a method and that will stop them. Don't get rid of the queryHelper on folder after all. Do rename your account attribute to "folderAccount" for consistency with the query name. Have your IMConvesationNoun definition create a query helper that converts the "account" query into a query on your path attribute. This would require a minor change to defineAttribute to also have a concept of queryHelpers where it copies the helper onto the query instance. (The current queryHelpers mechanism is special in that it allows an object noun (aka the thing in the attribute) to contribute a query constraint function back to the subject noun (aka the thing the attribute lives on)'s query class. The queryHelpers mechanism, or something like it that is equally capable of generating disk queries is the most featureful, but still awfully complicated. (There might also be less confusing ways of accomplishing that goal...) So, I'd be inclined to say do the 'disk'/'memory'/false thing for canQuery and making sure to change the defaulting logic in defineAttribute to set the value to "disk" instead of true when populating it based on the value of facet. It's reasonably simple and is fairly self-descriptive. Too much copy-and-paste for "this._attrFolder =". One of those should be this._attrAccount.

Sorry for the delay, I was on vacations until Monday and I must confess this fell under my radar. I tried really hard to remember why I added the extra test, i.e. the one that tests whether the first character is _. The only attribute that starts with a _ is _deleted, and I don't even think we want to query on that. Maybe there were more attributes in the past? I'm unsure. If I understood properly (sorry for skimming over this bug's contents), you want to be able to facet over an attribute even though you don't want to query on it? If then, I'm not sure I understand why you still want to go into the codepath that tests for '_'. Can you sum up the situation? I may even have something helpful to say :). Thanks and sorry again for the delay, jonathan

(In reply to Jonathan Protzenko [:protz] from comment #23) > If I understood properly (sorry for skimming over this bug's contents), you > want to be able to facet over an attribute even though you don't want to > query on it? I want to facet on an attribute ("account") that isn't stored in the database, but generated from the GlodaMessage and GlodaIMConversation instances. I think whether this means canQuery needs to be true, false, or another string value is the question.

In this case I guess that means that you need canQuery: false, since you're not going to generate SQLite queries. canQuery is for attributes that generate entries in the messageAttributes or whatever tables, which allows one to do: let q = new Gloda.query(Gloda.NOUN_MESSAGE); q.attr(val); After which, the constraint is added that attribute attr must have value val. Since you're definitely not going to query on the account type, since there's no underlying sqlite representation for that attribute, I would say that canQuery: false is the right answer, with facet: true of course. Andrew can correct me if I'm wrong, of course :)

Andrew requested in comment 13 that I add canQuery: false, and the consequence of that (an issue with the constrainer in _bindAttribute) was what we discussed in comments 21 and 22. Andrew suggested in comment 22 that maybe canQuery could have 3 different values: "disk", "memory" and false.

I don't want to suck protz back into the world of gloda needlessly; I'm not a cruel man ;). I just wanted to see if he had any opinion based on what is fresh in his mind. (I certainly had forgotten most of what was going on, and I had written much of it!) I think the way I phrased things I made it seem like a blocking question; I had not meant it to be so. My apologies. Let's go with disk/memory/false since it's easiest and explicit. Florian, if you want to come up with a better solution or use one of the other options I proposed, that's also fine.

Okay, the three-valued canQuery attribute seems like the right thing to do, then :). No worries about sucking me back in, I wrote this code!

I moved forward with the 3-values-for-canQuery approach. I kept the current meaning of true and false, and used "memory" for the "account" attribute. For a truthy but not exactly true value the code will behave like for true but won't store anything in the database. I noticed that the "account" attribute was still being added to the attributeDefinitions table, a behavior which didn't seem wanted. I've added a test before the GlodaDatastore._createAttributeDef to prevent this. Not adding any of the attributes that didn't have canQuery === true caused issues, so I also tested for aAttrDef.attributeType != Gloda.kAttrDerived. The result is that the following attributes aren't added to the attributeDefinitions table any more: built-in subjectMatches built-in account built-in fulltextMatches built-in bodyMatches built-in attachmentNamesMatch built-in authorMatches built-in recipientsMatch built-in identities im fulltextMatches I don't think any of these had a good reason to be in that table, but I may be misunderstanding the meaning of that table, so please double check that this is ok.

(In reply to Florian Quèze from comment #29) > I noticed that the "account" attribute was still being added to the > attributeDefinitions table, a behavior which didn't seem wanted. > I've added a test before the GlodaDatastore._createAttributeDef to prevent > this. Not adding any of the attributes that didn't have canQuery === true > caused issues, so I also tested for aAttrDef.attributeType != > Gloda.kAttrDerived. The main benefit of them existing in that table is then they get (persistent) attribute id's. Since it is only things that aren't persisted (to JSON or messageAttributes), the loss of the attribute id probably doesn't break anything. But if you look a few lines after that change, you'll see we do have a dictionary where we key based on the attribute ID, so I would feel better if you take that extra check back out. I think the benefit of having an attribute ID outweighs the small disk usage. Also, if we end up persisting queries to disk (we have an enhancement on that), we will probably want to use the attribute id's, in which case we will want them.

Comment on attachment 621057 [details] [diff] [review] Patch v5 Review of attachment 621057 [details] [diff] [review]: ----------------------------------------------------------------- r=asuth with this change and that other change from my previous comment. This should be pretty awesome! Woo! (And BTW, I've been finding your previous enhancement to the faceted search UI to highlight the matching text super awesome as well. Thanks again for that!) And of course bwinton's r and ui-r should still hold for this patch. ::: mailnews/db/gloda/modules/query.js @@ +177,5 @@ > iConstraint++) { > let constraint = curQuery._constraints[iConstraint]; > let [constraintType, attrDef] = constraint; > let boundName = attrDef ? attrDef.boundName : "id"; > + if (!("Gloda" in Cu.getGlobalForObject(this))) Please change this so that GlodaDatastore in datastore.js defines IGNORE_FACET. query.js already imports that file, so that should be okay and least likely to result in other awkward dependency hacks in the future. Then, have Gloda in gloda.js just re-expose IGNORE_FACET.

Patch taking into account comments 30 and 31.

https://hg.mozilla.org/comm-central/rev/f31b8ad31c08 There are string changes in the attachment that was checked-in, so it wouldn't directly apply to aurora, but it would be very easy to create a version of the patch that doesn't require any string change. I'm not doing this now, because I'm not sure we want to take such a large change on aurora, and even if we do, I would definitely want it to bake for a few days on trunk before requesting approval for aurora.

in which version Instant messaging will be implemented? and will there be a funtionality to import all contacts from pidgin into thunderbird? jabber, icq, aim, etc...???

i see: https://wiki.mozilla.org/Features/Thunderbird/Instant_messaging_in_Thunderbird says: "Landed for Thunderbird 13 but pref'ed off for Tb 13 and Tb 14. We are polishing this feature, and expect to ship it pref'ed on for Tb 15. Feedback welcome."

(In reply to Ruben from comment #34) > in which version Instant messaging will be implemented? I see (comment 35) that you already found the correct answer. > and will there be a funtionality to import all contacts from pidgin into > thunderbird? > jabber, icq, aim, etc...??? This isn't planned, but you can file an enhancement request on bugzilla for this feature, or create an add-on for it. Also, this bug is about searching emails and IM conversations at once; please avoid talking here about things that aren't directly related to this bug. Thanks.

ok, here: https://bugzilla.mozilla.org/show_bug.cgi?id=779049 import all contacts from pidgin into thunderbird IM chat contacts