Falling out of bug 452281. This will add a new search entry in the the default mail toolbar intended to provide a better search experience than the existing quick search. The existing quick search could just be reused, however it will be easier to add new search entry code specific to the behaviour we want. Also this will leave the old quick search entry untouched such that people could bring it back. --- With the removal of several buttons from the default mail toolbar ( bug 474523 ) the new search entry should take up a larger amount of horizontal space. Likely twice the size of the current quick search entry. It's placement can be the same as the quick search entry. The emptytext should describe what the search entry is for. On the right hand side we should be showing a search icon, similar to what firefox uses for it's search input entry. $emptytext = "Search messages (" + keyboard_ accelerator + ")"; [ $emptytext # ] When a person enters some search text the search icon should change to be a |> "go" arrow button that is clickable to start the search, this button can be triggered from pressing enter as well. [ bugzilla |> ] In the same space as the go button we should provide a clear button that removes the search terms from the search entry after the search has been performed. [ bugzilla (x) ] When a search is started from the main mail tab it will always open a new tab. This the search entry should always be empty from the main tab. When in a search results tab changing the search input and searching again does not open the results in a new tab. (we may want to add in a ctrl-enter command that does this) If the tab is changed from one search result to another the search entry contents should change to match the tab search parameters.

This seems like an asuth kind of thing; handing off to him for now.

bienvenu, preliminary feedback about the extensive changes required for grouping logic is appreciated. (requesting review for tracking purposes, but I'm expecting an r- as a result no matter what.) This preliminary implementation adds a working, if sometimes slow, gloda-backed search whose "everything" facet results in queries based on subject, body, attachment names, and contact names. We add a "why" column that we group on by default for the everything facet. We cram things into a search view. There were a number of unhappy things about the search view, grouping, and custom columns that I have attempted to address. I have also attempted to clean-up potential logic deficiencies as I saw them. (I am primarily talking about cases where the code assumes that a message key alone is sufficient to identify things, for example in some removal cases. In some of those cases I may have improved things, but not actually made them correct.) I added documentation as I attempted to understand what code was doing; if the comment doesn't make sense, my revised logic probably doesn't either... Note: While I was touching things, I attempted to eliminate pre-processing from files where feasible. This allows use of "ac_add_options --enable-chrome-format=symlink" to actually do something, not to mention correct line error numbers and IDEs being able to construct outline views because of the resulting syntactically correct files. I may have done a stupid thing or two in the process; I will look into that more later. Things that this patch does not yet fully address: * l10n stuff; I started to do this, but some strings are hard-coded. * column persistence and other view-related state on tab changes. * faceting actually having a meaningful effect * having a node in the folder view be selected, being able to save searches, etc. Since we are not backed by an nsIMsgFolder (and I'm not sure we want to be), but the folder pane is very gung-ho about such things, this may be some amount of trouble. * dangerously inefficient queries in some cases. (the way the LIMITs and joins interact when fetching messages results in us doing the full-text join (which we don't even need) on all potential results, not just the results of the LIMIT.) Things that clarkbw needs to figure out: * What do we do about bounding search size? Right now I have an arbitrary LIMIT on each query result. The easiest solution is probably to cram something into the facet-bar, adding a "more" or "older" button that re-formulates the query or just inserts even more results. It gets a little tricky in the "everything" case since if we want to deal with time ranges, each result set will be have its own limit point on the timeline. It would be neat to do something clever in the dummy group header, but that's not really within our power unless we kludge some floating panels into location and snapping onto those lines and seeming like they are more than they really are. The great news is that this functionality is actually pretty sweet.

Comment on attachment 358798 [details] [diff] [review] v1 gloda search. preliminary infrastructure patch, many nuances and niceties to clean up Thx for working on this! You can have multiple custom columns in a view - is your curCustomColumn really curCustomSortByColumn? I had intended to allow more than two levels of sorting, but didn't finish implementing it - I'd rather go towards finishing it than encoding in the interface that we only do one level of secondary sort. That's why we have this: nsTArray <MsgViewSortColumnInfo> m_sortColumns; So the SecondarySortOrder and Type attributes could take an index param and use the m_sortColumns. I need to look at the rest of this more closely when I can apply the patch and run with it...

> Things that clarkbw needs to figure out: > * What do we do about bounding search size? Right now I have an arbitrary > LIMIT on each query result. The easiest solution is probably to cram something > into the facet-bar, adding a "more" or "older" button that re-formulates the > query or just inserts even more results. I guess it'd be good to know what are the performance limits of the search. How many results can we get before it becomes a bottleneck? And then what would the performance problems be with more/older buttons, just inserting rows or rerunning the query. I think the obvious move is to improve the facet system ( bug 474711 ) for search results such that large sets of searches are filtered down to smaller sets instead of all results scrolled and sorted. However the current mode of Thunderbird _is_ to sort and scroll through large sets of messages so it seems unclear what the best step forward would be.

(In reply to comment #3) > You can have multiple custom columns in a view - is your curCustomColumn really > curCustomSortByColumn? Yes. I introduced the simplest possible mechanism to workaround the lack of a database with the database info mechanism available. (Without an m_db, access to m_db of course falls down.) > I had intended to allow more than two levels of sorting, but didn't finish > implementing it - I'd rather go towards finishing it than encoding in the > interface that we only do one level of secondary sort. That's why we have this: > nsTArray <MsgViewSortColumnInfo> m_sortColumns; Yes, there is a lot of clean-up possible when it comes to sorting. My real goal was to simply have a public mechanism for nsMsgGroupThread to determine in which direction the date should be sorted. (I feel that it makes the most sense for this grouped search result case to have the date order be descending, whereas the default was ascending.) I am not opposed to normalizing/cleaning this up more, but it's not going to be particularly pretty and I am concerned at the potential level of fall-out. Although we could leave the current 'sort' mechanism and attributes intact and just have the sortColumns interface be sane.

Comment on attachment 358983 [details] [diff] [review] v2 gloda search. simply v1 updated for trunk and correcting a serious oversight in nsMsgDBView::FindHdr the FindHdr stuff makes sense - as a style nit, you want spaces around the "=" here: PRBool allowDummy=PR_FALSE. I'll try to get to the rest of this tomorrow. I suppose I should really try to fix the sorting so that n-level sorting is handled before insisting that the interface be generalized.

(In reply to comment #5) > Created an attachment (id=358983) [details] > v2 gloda search. simply v1 updated for trunk and correcting a serious > oversight in nsMsgDBView::FindHdr Just so we're clear, the "serious oversight" was in my changes to the code where I tried to subscript an array with nsMsgViewIndex_None-1; I changed that in the patch because it was rather crashy without it. The semantics of the dummy header and FindHdr and its upstream caller to find the first message in a thread is more an issue of confusion; I'm not sure what the intent of the various calls was and the 'dummy' header is certainly a sticky issue. (The pre-Andrew code also seemed confused as to what it was expecting.)

While you are adding this, are you going to fix some of the anomalies with existing search (TB's weakest aspect IMHO), specific areas of weakness that should ideally be addressed. I thought it might be useful to mention them now as it might be easier than trying to fix later ... Ability to control WHAT gets searched, usually I think you want to search everything - including multiple accounts, but sometimes people need to refine it - especially if the search term is potentially common EXCEPT in the area you want to search. Need to be able to narrow it down to Body or Headers, or search both at the same time (a huge weakness currently, is that you have to search multiple times, when you are looking for an email address that could be in the From header (if the message was to them), or in the To/Cc to find replies (i.e. you've deleted the original) or in body (if youv'e forwarded it). That requires three lines of search in current interface. Looking forward to being able to test this ....

line this up with the b2 release. this bug should always be following the release target of bug 464354

This revised patch has, I think, all of the UI and strings that will be in the final patch. clarkbw, I guess if you would like to give this a whirl, it would be good. The only potential string issue I can think of is that this patch does not even attempt to stub out a "saved search" in the folder pane (or elsewhere) and accordingly has no strings to that end. Nor does it actually do something with the 'save' button (which does have a string) that we display; presumably that process would involve a bunch of strings... feel free to spec that out and them implement it! :) I forget if I have already said this, but it might make sense to punt on saving in the b2 time-frame and instead log a bug for that targeted at b3. This patch makes a number of significant improvements in terms of tabbing and the nightmare that is tracking the state of collapsed things, whether things should be collapsed, etc. It's still not perfect (we also need to persist the splitter's offset I fear...) in those areas, and it still falls down quite spectacularly when it comes to table columns. Gloda queries should be faster; we ignore the 'deleted' index and avoid performing the gratuitous join, although that change is going to require a new bug to be spun-off to deal with the cache ramifications of letting people opt-out of that join. (We can efficiently guarantee all queries that want it get it, we just have to do it.)

Comment on attachment 360900 [details] [diff] [review] v3 gloda search; serious tab improvements (but more needed), faceting UI present but not hooked up. >--- comm-central.15568e9ec76d/mail/base/jar.mn 2009-02-06 05:36:01.000000000 -0800 >+++ comm-central.f5dee9dc071c/mail/base/jar.mn 2009-02-06 05:36:01.000000000 -0800 >-* content/messenger/mailWindowOverlay.js (content/mailWindowOverlay.js) >+ content/messenger/mailWindowOverlay.js (content/mailWindowOverlay.js) >-* content/messenger/extraCustomizeItems.xul (content/extraCustomizeItems.xul) >+ content/messenger/extraCustomizeItems.xul (content/extraCustomizeItems.xul) >-* content/messenger/msgMail3PaneWindow.js (content/msgMail3PaneWindow.js) >+ content/messenger/msgMail3PaneWindow.js (content/msgMail3PaneWindow.js) >-* content/messenger/search.xml (content/search.xml) >-* content/messenger/tabmail.xml (content/tabmail.xml) >+ content/messenger/search.xml (content/search.xml) >+ content/messenger/tabmail.xml (content/tabmail.xml) Is there a reason for removing the pre-processing here? From what I can see in your patch, tabmail.xml still has #ifndef statements in the code, so this patch will likely break tabmail.xml.

(In reply to comment #12) > Is there a reason for removing the pre-processing here? From what I can see in > your patch, tabmail.xml still has #ifndef statements in the code, so this patch > will likely break tabmail.xml. Yes, this needed a slight tweak. Just put the star back in front of tabmail in the jar.mn file and the patch works fine. There are two #ifndef's for the mac. Alternatively you could use andrew's runtime mac test from the search.xml file and replace the ifndef's. if (window.navigator.oscpu.substring(0, 3).toLowerCase() != "mac") Either way will work, testing the patch out now.

(In reply to comment #12) > Is there a reason for removing the pre-processing here? From what I can see in > your patch, tabmail.xml still has #ifndef statements in the code, so this patch > will likely break tabmail.xml. Yes, I glitched on that; I meant only to make the license block legal syntax but not remove the preprocessing. I'll post a revised patch that introduces a global and checks that instead. But more generally, from my comment #2: Note: While I was touching things, I attempted to eliminate pre-processing from files where feasible. This allows use of "ac_add_options --enable-chrome-format=symlink" to actually do something, not to mention correct line error numbers and IDEs being able to construct outline views because of the resulting syntactically correct files. I may have done a stupid thing or two in the process; I will look into that more later.

What is the ETA for this bug? Tomorrow at 23:59 PST is our string freeze and it would be great if this bug could be fixed by that time as mentioned in https://wiki.mozilla.org/Thunderbird/Driver_Meetings/2009-02-03

This has only the strings appropriate for what will work for beta 2. Namely, we are losing the "save search" button. The strings here are already in use in the implementation and should not require additional changes for the beta. The remaining work for this bug (for beta 2) is to: - Finish hooking up the facet bar to affect the current tab's search. - Rugged-ize the tab logic (in general) further, with a primary focus on displayed thread-pane columns. - Make sure the nsMsgDBView grouping logic is correct and does not break other things. - Make this search mode only work/appear when gloda is preferenced on. Since this feature does not work without gloda, it does not make sense for it to be visible without gloda. This will allow us to land this functionality, but if gloda does not bake sufficiently for beta 2 and we don't pref it on, this UI will stay hidden without a lot of legwork. (This bullet point comes frmo a discussion with dmose.)

r=dmose on the strings, with a two caveats: * fix the comment indenting * all the comments appear to be localization notes. please annotate them as such.

Entity in use changed to make sure L10N people pick up on it.

Comment on attachment 361594 [details] [diff] [review] [landed in comment 23] v2 strings; change the preferences dialog to tie global search with indexing r=dmose; good catch on the entity-in-use change

AFAICT, attachment 361594 [details] [diff] [review] needs landing ASAP. setting checkin-needed keyword.

Pushed http://hg.mozilla.org/comm-central/rev/b73c9b1c5710 with what I hope are vaguely workable l10n notes, the three dotses changed to ellipses, the trailing whitespace removed, and what I hope was the correct change, making the l10n note about glodaSearchFacet.to.label reflect the en-US string instead of saying to/from. But, we really need to think of this state of landing string-only patches as what it is: abject failure. We totally screwed up, with our planning and our scheduling and our scoping, and we're screwing our localizers, who will now try to guess what "short enough" is for a column header in a language where everything is much longer than English, with no idea whether they are using something that will overflow another element, or cause a scrollbar, or be cropped so that "Shithering" becomes "Shit...". Sometimes it's all you can do, but keep in mind: this is wrong.

This is not yet suitable for review, but it works, which is nice. I made a few last minute changes in the name of more satisfying usage. Namely, search is still ANDing all terms for now, and I boosted starred messages and messages involving people in your address book a lot. The (implied) AND had been "OR" for most of this patch... I suspect we really only want to fall back to "OR" when AND fails us or we are unsure as to whether we should attribute some terms to contact names or not. For example, it has been suggested that for "Andrew Sutherland gloda" we want to make sure we find all messages involving me and the term "gloda". Because we currently _do not_ put message headers or the names of people involved in the message in the full-text, we don't just get this for free. The options that jump out at me are to try and see if any of the terms match contacts (that we care about) and then subtract them off (or just leave them as ORed contacts), or to start indexing the people involved in the message. The latter may either trickier than it would seem, or just occasionally give poor results, because of the case where a person doesn't actually have a consistent (or any) full name in their email. Ex: we just get "<foo@example.com>" instead of "John Q Foo <foo@example.com>". Right now all the things I twiddled to play with importance are constants in msg_search.js, fundattr.js, and explattr.js. Anyone daring enough to test may be daring enough to play with those. Search on NOTABILITY or SCORE.

It strikes me that a Spotlight-style user experience might be a reasonable thing to shoot for. By that, I'm thinking of multiple kinds of searches, with the most-likely-to-be-relevant ones returning instantly and the less relevant ones filling in over time below them. e.g. AND results should return immediately, and then OR below them etc...

(In reply to comment #26) > It strikes me that a Spotlight-style user experience might be a reasonable > thing to shoot for. By that, I'm thinking of multiple kinds of searches, with > the most-likely-to-be-relevant ones returning instantly and the less relevant > ones filling in over time below them. e.g. AND results should return > immediately, and then OR below them etc... Yes, that sounds right, and perhaps we limit the number of hits so that if we get (e.g.,) 200 AND hits, we don't bother with the OR search.

(In reply to comment #26) > It strikes me that a Spotlight-style user experience might be a reasonable > thing to shoot for. By that, I'm thinking of multiple kinds of searches, with > the most-likely-to-be-relevant ones returning instantly and the less relevant > ones filling in over time below them. e.g. AND results should return > immediately, and then OR below them etc... How do we structure this given that we are dealing with an nsITreeView backed by a nsMsgDBView? I presume we want stable results; which is, we don't want the new results changing the order of the existing results. Otherwise the use might click and find they are clicking on a different message than the one they wanted and the one they wanted has scrolled itself off the screen. (It is conceivable the scoring heuristics would allow this to happen.) We could use the group-by-sort mechanism with primary sort on "why" and secondary sort on score. Unfortunately, although stable, if the secondary group is not visible on the screen, there's no reason for the user to suspect it's there. So maybe we don't want stability? Ugh. Bryan?

I had a talk with clarkbw, key points that I'm going to act on are: - We will full-text index at least the author of a message, and probably the to/cc/bcc parties as well. Building a clever back-tracking hypothesis engine to attribute search terms to contacts versus text is out of scope for this. - We do want stable results. - We probably only care about the "OR" results if we don't get enough "AND" results. - Because of stability concerns, probably the best way to show the existence of "OR" options is to grow the faceting bar at the top to say "did you want to try an 'OR' search? we got ### results" or something like that (my phrasing). The key thing is that we do not mutate the treeview contents until the user takes an action. - We can probably add some cheap facety re-weighting to quickly improve the sort order. For example "favor/don't favor people in my address book", "favor/don't favor newer messages", "this term is most important: blah|v", are examples of really simple things we can do without even looking at the messages. Performance was suspiciously good when I showed it to clarkbw compared to my own testing last night; I think I may need to take a day to look into the horrible memory/CPU problems people have going on before this patch is finished. Because we do not have a generational GC, the more gloda stuff we have in memory, the higher the GC cost, which I think was most of the performance problems I was seeing last night.

The above all sounds good to me. (In reply to comment #29) > - We probably only care about the "OR" results if we don't get enough "AND" > results. > - Because of stability concerns, probably the best way to show the existence > of "OR" options is to grow the faceting bar at the top to say "did you want to > try an 'OR' search? we got ### results" or something like that (my phrasing). What about just filling in the top OR results after all the AND results have come back, if we think we need to?

are you going to full text index the attachment names as well?

(In reply to comment #31) > are you going to full text index the attachment names as well? We already do this. Fulltext colums for each message are currently: subject, "whittled" body, and attachment names. "whittled" means that attribute providers that implement a contentWhittle method get a chance to process the body. The default whittler detects quoted text (which does not get indexed). The bugzilla plugin's whittler gets rid of the ASCII 'art' and useless header/footer messages. It does specially process the key/value key/oldvalue/newvalue things too, but I think those get emitted for indexing purposes.

Are these index choices going to be configurable by a user? For example .... as someone who throws away messages I've replied to, I definately want it searching the quoted text. I'm pretty sure I want it searching footers as well - e.g. who I do I know from XYZ company. (ASCII 'art' is rare enough not to be an issue either way) - Mitra

(In reply to comment #33) > Are these index choices going to be configurable by a user? For example .... > as someone who throws away messages I've replied to, I definately want it > searching the quoted text. I'm pretty sure I want it searching footers as well > - e.g. who I do I know from XYZ company. The current scheme with gloda is generally presuming that users will keep the messages in conversations they are indexing. If you are deleting the messages for neatness purposes, you might want to use the new "archive" button instead. If you are deleting them for resource reasons (disk space, etc.), you are probably not going to be happy with gloda in general... :( I think it would make sense to index quoted text when we don't have a copy of the original message for some reason (gloda should be able to detect this), but I think we would want to avoid making indexing quoted text a configuration option. The gloda extension points allow an extension to "out-bid" the default content whittler for its interpretation of the message. My rationale for this is that the quoted text would distort most of our options for sort ranking heuristics. If someone replies to a long message about 'hats' just to reply "me too! I love hats, can't get enough hats!" with a full message quote, and I search for "hats", I really don't want the me too message getting ranked above the original message. Not to mention when people reply back and forth with the quoting for the entire conversation building up. Massive distortion, that. By forcing the serious behavior change into an extension, it can also attempt to compensate for the impact on search (and perhaps elsewhere) in a relatively clean fashion. For example, the 'score' method could also be implemented on the attribute provider. But if we do that in core, the code is going to get confusing and unmaintainable quick. The footer ignoring I spoke of was specific to bugzilla bugspam with the bugzilla gloda plugin installed; it's not a general thing. It would be hard to get that logic right unless we analyzed multiple messages from a single sender to determine footer stuff. That's not in the cards for core gloda functionality at this time. (The ASCII 'art' I refer to is bugzilla specific too... also, the indexer doesn't actually tokenize that, so it's not like you can search on it anyways.)

Hi Andrew, your assumptions are valid, but show the danger of making assumptions about email - since I (as another example user) make a completely different set of assumptions. Just as illustration - not saying that this is the "right" way of looking at it. I delete the originals not because of space, but because of "neatness". If I get a message about XYZ and reply to it, then I delete the original, partly because if I do a seach on XYZ I always want to see the reply (including quoted original), i.e. I want to see the most recent part of the conversation. As I say, I'm not saying that's a BETTER way to use mail, but I guess its not uncommon, so we have to be careful about excluding quoted text. Also remember that for people who send emails back and forth, and forward etc then that quoted text is going to come back to you, or be forwarded from somewhere else. e.g. if Joe sends an email to Fred about XYZ, and Fred forwards it to me adding the line "Please act on this". Then if I search on XYZ I'm going to EXPECT the message to be found, and any other behavior would probably be considered a bug by that user. - Mitra

Slipping to m3. I have undertaken the task of refactoring much of the front-end code relating to database views (which is a lot) out of necessity. While this is a good thing, it is taking much more time than hoped owing to the complex way in which the code was "structured". I am not entirely confident that this will make m3. In order for this patch to land, we will need a fair number of mozmill tests. Because the general db view 'core' has a number of quasi-orthogonal feature sets grafted onto it (quick search, mailviews, virtual folders, weird special views, weird view flags, grouping, etc.) there is a lot of surface area to test. Also, I'm not done with the refactoring yet. I am confident the refactoring is the right thing to do, even if it induces a slip of this bug to 'm4' and requires us to create an 'm6'. The result will be a much better tested and engineered product, and most importantly something more maintainable. There is no way any human being understands the code as it exists in the tree. (If they do, they should be encouraged to instead focus their efforts on cold fusion; I expect that they would find it a walk in the park.) I will post a work-in-progress patch shortly for people to skim.

current refactoring, -w. I will attach the main new abstraction files as their own +only diff next.

right, so the FolderDisplayWidget is the key new abstraction, with SearchSpec being his helper friend. They attempt to unify the logic related to constructing nsMsgDBViews as well as much of the legwork related to make a FolderDisplayWidget 'active' when the tabs change. The patches as they currently exist do a pretty good job of have the mailviews stuff persisted, used, and changed on demand for views. They sorta work for multi-folder (XFVF) and single-folder (implemented as a quicksearch) saved searches, but each has a weird flaw (infinite loop for XFVF, headers are wrong for QS). The tab code has not been particularly brought up-to-date with this. I expect another day's work should have us back to our current level of working-ness and then another day to get the tab stuff in pretty good shape. Then we add the mozmill tests.

This basically just adds entirely new files and changes no existing files, so this should not break your tree. However, with the next patch 'layer', I will gut a lot of the code in the UI that is responsible for this, as I hook the UI up to the new 'folder display widget'.

Comment on attachment 375421 [details] [diff] [review] v1 view wrapper abstraction with many unit tests [Backout: Comment 46] in general, this looks good. I haven't run with it yet, but a few nits: typo: + * is fundamentally a UI issue. Additionally, because the MsgCraeteDBView Why did you remove the suite's version of CreateVirtualFolder but not mail/base/content/commandglue.js? Or is because that's part of a bigger patch? maybe expand on this comment a bit? + // XXX startuppage Is this to get more than a two-level sort? For very large folders, it would be much more efficient to tell the view about all the sort levels up front...this would probably mean a change to the way views are created/opened. And of course, the backend would need to change as well (though perhaps that's in a patch I haven't seen yet). + // clock through the rest of the sorts, if there are any + for (let iSort = this._sort.length - 2; iSort >=0; iSort--) { + [sortType, sortOrder] = this._sort[iSort]; + dbView.sort(sortType, sortOrder); + }

Comment on attachment 375421 [details] [diff] [review] v1 view wrapper abstraction with many unit tests [Backout: Comment 46] r/sr=me, modulo my prev comments.

(In reply to comment #41) > Why did you remove the suite's version of CreateVirtualFolder but not > mail/base/content/commandglue.js? Or is because that's part of a bigger patch? Yes, there are extensive removals from commandglue.js in another patch. That is one of them. > Is this to get more than a two-level sort? For very large folders, it would be > much more efficient to tell the view about all the sort levels up front...this > would probably mean a change to the way views are created/opened. And of > course, the backend would need to change as well (though perhaps that's in a > patch I haven't seen yet). > > + // clock through the rest of the sorts, if there are any > + for (let iSort = this._sort.length - 2; iSort >=0; iSort--) { > + [sortType, sortOrder] = this._sort[iSort]; > + dbView.sort(sortType, sortOrder); > + } In my group view patch you expressed interest in having a more explicit multi-level sort API along these lines. Since the goal of the wrapper is to create a sane abstraction layer over the views, it seemed reasonable to expose this now and have an under-the-hood kludge if anyone tries to use it for anything more complex than a primary sort. I have no improvements to the view sort logic in my queue.

pushed v1 view wrapper abstraction with many unit tests with changes: - removed XXX startuppage comment. that was detritus from when the db view wrapper was the folder display widget. (that got split out into a separate patch, improving factoring greatly). - typo fixed - minor change to DBViewWrapper.close() method to use searchSpec.dissociateView; had forgotten to do that when I introduced the method. (unit tests still pass and that code has intentional coverage. hooray unit tests!) - minor gloda fix for my fixIterator change which normalized the fixIterator result to be consistent over input types. (a unit test was broken without this. hooray unit tests!) http://hg.mozilla.org/comm-central/rev/b67f484152e4 Thanks for the quick review! Next up is finishing the folder display widget abstraction.

(In reply to comment #44) > http://hg.mozilla.org/comm-central/rev/b67f484152e4 The 3 tests are failing: can you fix or backout?

backed out: http://hg.mozilla.org/comm-central/rev/89b19fdacc36 http://hg.mozilla.org/comm-central/rev/9a87085b21c0 On 1.9.1 on OS X I'm not entirely sure what is up; the virtual folder is somehow failing to contain the header with offset 0, which might make sense if something was using the message key incorrectly in a conditional, but does not make sense cross-platform. It's conceivable that things other than linux are slow enough that they are causing a TimeSlice behavior that I have not observed. On the trunk, it looks like all the xpcshell instances are dumping core on exit, even though the tests passed. Fun. I was hoping that more builds would show up before I backed out to assist in tracking, but it wasn't happening...

make check on windows in mailnews/base succeeded for me w/ the patch.

This goes on top of the previous patch. Issues resolved (primarily unit test related): 1) Single-folder virtual folders would generate two message loaded notifications. In the case where the search did not complete without rescheduling itself, the first notification would come before the search completed, resulting in test failure. We now only generate one notification at the correct time. 2) The unit test was assuming that folder deletion was synchronous; but since we re-open the view when we get a moved or deleted notification, it was most definitely not. A related complication is that to really delete a folder (easily), we have to delete it to the trash and then empty it, which is two notifications. The test driver has some helper code with this. This point made me think of... 3) We used to generate all message loaded notifications even when the search was interrupted. This would at best generate busy-work for a DB view that was going away. So we no longer do this. In order to know when we are getting an interrupted notification we introduce a new success nsresult code that the search session passes to his notification friends. I believe these will resolve the 1.9.1 failures and most of the trunk failures. Unfortunately, these failures look like they are a result of xpcshell dumping core during XPCOM shutdown without backtraces: http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird/1241391217.1241392879.32164.gz I'm not entirely sure what to do about those without backtraces, although I did change the post-test cleanup code slightly in viewWrapperTestUtils.js, which might result in a healthier shutdown.

Comment on attachment 375576 [details] [diff] [review] view wrapper abstraction fixes v1 the asyncTestUtils part of this patch didn't apply on top of the old patch for me - I'll try to figure out why, but I thought I'd mention it.

the fixes patch didn't apply because the old timeout was 30, which you've changed to 600, but the diff thinks you've changed it from 60 to 600. My release build tests are still failing with these patches, on OS/X, but I'm not seeing any log files. I'll see if I can find log files with a debug build.

I had made minor changes to v1 for what I had tried to commit; this is that patch with the minor (noted) changes and one dump removal that I had pushed in a quicky follow-up commit. carrying forward r/sr=bienvenu because it was just nit-fixed. (providing for the revised next patch)

Revised fix on the previous patch. I wailed on things until the unit tests stopped showing memory leaks. The view abstraction was failing to close db view instances that it was replacing internally, which let to them hanging around, presumably for listener reasons. I also fixed up some unit tests that should have been using async refresh calls. It is my belief that this should no longer cause xpcshell tests to fail due to (silent) xpcshell crashes. If things still crash, please apply the fix on bug 483062 to your mozilla tree and see if you can get a backtrace.

Comment on attachment 375779 [details] [diff] [review] view wrapper abstraction fixes v2 [Checkin: See comment 55] Is this still needed? -DEFAULT_LONGEST_TEST_RUN_CONCEIVABLE_SECS = 60; +DEFAULT_LONGEST_TEST_RUN_CONCEIVABLE_SECS = 600;

(In reply to comment #53) > (From update of attachment 375779 [details] [diff] [review]) > Is this still needed? > > -DEFAULT_LONGEST_TEST_RUN_CONCEIVABLE_SECS = 60; > +DEFAULT_LONGEST_TEST_RUN_CONCEIVABLE_SECS = 600; Kinda. Sometimes I run my tests under chronicle-recorder which has approximately two orders of magnitude worth of overhead, which is why I bumped it to 10 minutes. What should ideally happen is that the xpcshell code itself is modified to time-out when the event-loop has nothing to do for a sufficient period of time. My event driver code could also do this itself, spinning an event loop and relying on all timeouts to have a 0 duration...

pushed v2 view wrapper abstraction: http://hg.mozilla.org/comm-central/rev/58ff5d78d1d4 pushed view wrapper abstraction fixes v2: http://hg.mozilla.org/comm-central/rev/6e3d055f9788 bustage fixes: - double event notification with unit test to verify resolved http://hg.mozilla.org/comm-central/rev/c6c0aba19ee9 - remove virtual folder UI changes due to URI string/nsIMsgFolder array mismatch: http://hg.mozilla.org/comm-central/rev/c3a34d1f5dd2 - disable unit test that was having issues on OS X http://hg.mozilla.org/comm-central/rev/51031c6c66f8 - restore test, resolving testing code issue that caused the problem http://hg.mozilla.org/comm-central/rev/023c29d6a511

The folder display layer exposes the DBViewWrapper of the previous layer to the UI. This patch does not handle and does not plan to handle: - Complex tab issues involving the folder pane. The folder pane issues should be dealt with by ditching the multiplexed tabs in some other bug. - Complex tab issues involving the thread pane columns. The set of visible columns should be properly updated by the patch, but dealing with re-ordered or user customized visible columns is also intended to be handled by ditching multiplexed tabs. This patch currently definitely regresses: - The next message to select after deletion logic. This patch definitely does not: - Remove all the dead code that has been mooted. - Convert the standalone message window. Oddly enough, the standalone message window still works, though. This patch definitely improves: - Quick searches in the face of tabs. This is not a final patch, but I am requesting review from bienvenu to get it on his radar so he can give it a good skim and point out any egregious oversights. I hope to provide an updated gloda-search patch against this shortly.

Comment on attachment 376759 [details] [diff] [review] folder display layer v1 I'll apply and run with this today. A couple comments: I think I might have just made this comment a lie, and not in a good way. I made SearchDBViews use the same m_deletingRows logic as nsMsgDBView, which avoids trying to select messages when items are removed from the view, until we're ready. And we need to get the OnDeleteCompleted notification so we can clear m_deletingRows. + /** + * Single-folder backed views inexplicably need help to know when their move / + * deletion operations complete. If we don't help them, they refuse to move + * / delete things the next time around. Thankfully, because only + * nsMsgDBView does this and only non-multi-folder variants use those calls, Does this method do anything anymore? function setTitleFromFolder(msgfolder, subject)

switching from my imap inbox to my pop3 inbox fails with this exception: --DOMWINDOW == 10 (06A8DCF8) [serial = 16] [outer = 03F8CF20] [url = imap://bien venu%40davidbienvenu%2Eorg@mail.davidbienvenu.org:143/fetch%3EUID%3E.INBOX%3E124 82] ``` folders length: 1 `````````` setting busy: true `````````` setting busy: false ``` EXCEPTION DURING NOTIFY: chrome://messenger/content/searchBar.js:204: TypeEr ror: gSearchInput.setSearchCriteriaText is not a function

(In reply to comment #57) > I think I might have just made this comment a lie, and not in a good way. I > made SearchDBViews use the same m_deletingRows logic as nsMsgDBView, which > avoids trying to select messages when items are removed from the view, until > we're ready. And we need to get the OnDeleteCompleted notification so we can > clear m_deletingRows. Yeah, that's not entirely desirable, but I expect we'll survive. The likelihood of a user attempting to perform multiple moves/deletions in multiple views that overlap in such a way that FolderDisplayWidget blindly passing the notification on confuses the state machines is quite low. I'll amend the code to just provide the notification. > Does this method do anything anymore? > > function setTitleFromFolder(msgfolder, subject) I think the standalone message window still uses it and needs it. I need to update that code.

This goes hand-in-hand with the about-to-be-posted "gloda search layer v2" (to avoid confusion I'm jumping to v2...) This is not final review-ready, but is pretty respectable. Marking for review just for tracking purposes.

Be forewarned, this bumps the gloda schema revision number. This will purge your global database and start a re-indexing. You will probably need to customize your toolbar to get the gloda search to show up. A neat trick is that you actually can use quick-search against the gloda search results! Woo! Gloda search results are sorted by the score column, but I haven't finished making dealing with columns pretty, so uh, the score column is hidden from the column picker and you can never ever see what the values are. (per davida, people are unlikely to care about the score, so the plan was to hide it by default.)

I'm building with these patches now, and I'll try them out today.

A couple things I noticed (apologies if these have been noted before): In a cross-folder saved search, next unread does a next unread, but it also asks me if I want to go to the next folder with unread. Indexing seemed to do the Trash and Spam folders awfully early in the process, before some if not all inboxes.

the next unread issue is not specific to cross-folder saved searches - I saw it in a newsgroup as well. If I add the gloda search widget to the toolbar and do a search, the folder picker that lets me pick a specific folder doesn't let me pick a parent folder (e.g., my Inbox has a sub-folder, and there's no equivalent of "file here" for the inbox). When I change what we're search over (e.g, involves, message text, etc), it doesn't seem to re-issue or change the search).

Very nearly there. This actually introduces a few shocking glitches, but once the minor causes are resolved, it should be good. Shocking glitches: - Open message in new tab is actually broken. I think I forgot to update the tab code to explicitly generate the selection event. - Switching between tabs will occasionally invert the set of displayed columns... Less shocking but annoying: - Standalone message window title doesn't get set. - Saving a virtual folder from the search view loses some (all?) predicate info. - Some of the history stuff is glitchy. Notable improvements: - Standalone message window pretty much works! - Search window pretty much works! - Spacebar navigation works! So, the point of this patch is not so much to run with it as to begin/continue initial review passes, since the amount of change after this point will be extremely small. I should have the glitches ironed out tomorrow and will spin a try server build then.

Comment on attachment 378809 [details] [diff] [review] folder display layer v3 How come you're removing the modes at the top of the files? Have we decided to kill those? re SearchDialog.js - FYI - people have asked for an optional message pane below the search results, with some little toggle widget to hide/show it. would be nice to use "let" instead of "var" here, and lots of other places: + var searchSubfolders = + document.getElementById("checkSearchSubFolders").checked; The comment indentation // Foo ... // Bar ... is not what we do everywhere else, but I'm not going to make you change it :-) Could you make these sentences, by capitalizing "get" and adding a period, same thing for "if"... + // get the msg folder we're moving messages into + // if the id (uri) is not set, use file-uri which is set for + // "File Here" and use let: + var destUri = destFolder.getAttribute('id'); + if (destUri.length == 0) + destUri = destFolder.getAttribute('file-uri'); - var destMsgFolder = GetMsgFolderFromUri(destUri).QueryInterface(Components.interfaces.nsIMsgFolder); + var destMsgFolder = GetMsgFolderFromUri(destUri).QueryInterface( + Components.interfaces.nsIMsgFolder); Make this a sentence: + // the collapsed state is the state after we released the mouse // so we take it as it is let + ? operator, unless it involves awkward line wrapping: + var folders = GetSelectedMsgFolders(); + if (folders.length == 1) + gFolderDisplay.show(folders[0]); + else + gFolderDisplay.show(null); If nsMsgViewFlagsType isn't used, just remove the commented out line: +var nsMsgFolderFlags = Components.interfaces.nsMsgFolderFlags; +//var nsMsgViewFlagsType = Components.interfaces.nsMsgViewFlagsType; typo: + // if we nuke received, put badck date... + receivedCol: ["dateCol"], Maybe just say: getDocumentElements() sets gSearchBundle? + // HACK string-bundle + getDocumentElements(); Either add a comment about the commented out code, or remove it, or uncomment it :-) + event.initEvent("MailViewChanged", false, false); + //event.setData("folderDisplay", this); "that's a thread" ? + * kick off the delete in the first place, but let's a thread I don't want to + * pull on right now. single blank lines should be sufficient here: + msgWindow.openFolder = this.view.displayedFolder; + + + this._active = true; + this._runNotificationsPendingActivation(); + + + // -- UI Archive should be the same as move. Is that not actually the case? + * The archive code tells us when it is starting to archive messages. This + * is different from hinting about deletion because it will also tell us + * when it has completed its mass move. can you append "have a dbview" to this comment? + // account central stuff when we don't So if the caller does something wrong, we'll select the first message? Why not not select anything? A comment would be helpful here. + // bounds-check the view index + if (aViewIndex < 0) + aViewIndex = 0; spurious blank lines: @@ -119,6 +118,8 @@ if (commandID) goUpdateCommand(commandID); } + + } undo and navigation history: + * The messenger object is the keeper of the 'undo' state, which is why we + * do this. and back/forward navigation history. + // Each tab gets its own messenger instance; I assume this is so each one + // gets its own undo/redo stack? let numMessages + let selectedMessages = gFolderDisplay.selectedMessages; + var numMessages = selectedMessages.length; pausing review at start of messageDisplay.js

(In reply to comment #67) > How come you're removing the modes at the top of the files? Have we decided to > kill those? I'd been removing them from JS code because they tried to make emacs use C++ mode or Java mode (for JS code). I use js2-mode[1] which is quite good, but does not ship with emacs. I figured trying to force emacs into a mode it does not ship with is probably not appropriate, but did not feel having it override default mode selection to an invalid mode was proper either. 1: http://code.google.com/p/js2-mode/

(In reply to comment #67) > Archive should be the same as move. Is that not actually the case? > > + * The archive code tells us when it is starting to archive messages. This > + * is different from hinting about deletion because it will also tell us > + * when it has completed its mass move. The archive logic tells us when it starts and stops its conceptual batch. The way the cross-folder manipulations work, we have a heads up before the conceptual batch starts (because the UI code pokes us before calling doCommand), but there is no conceptual batch end notification, at least under the current idiom. Our end notification is the notification that a delete completed in the folder, but that does not necessarily correspond to the cross-folder conceptual batch.

raw mq patch at: http://hg.mozilla.org/users/bugmail_asutherland.org/comm-central-patches/raw-file/47ebb3990b68/folder-display-layer.patch This is the real deal, no longer interim. I know this works on messages in their own window (from .eml and from real data), as well as messages in new tabs. I don't think message history navigation is perfect, but I'm not sure I actually regressed it. (If I did regress it, let's handle that elsewhere.) New and neat: - The tabs visible on each tab are persisted so that you can have a tab visible in one tab but not visible in another tab and switching between them does what would expect. This does not cover re-ordering of tabs. Reminders: - This patch is not intended to do anything to make the folder pane have better per-tab state. We continue (just as the current code does) to merely force the folder that is displayed on each tab to be selected in the folder pane when we switch to it. I will try and kick off some try-server builds of this...

this broke all my smart folders - the assertion with the following stack trace is probably related: > msgbase.dll!nsMsgSearchAdapter::EncodeImapTerm(nsIMsgSearchTerm * term=0x05eb7c90, int reallyDredd=0x00000000, const wchar_t * srcCharset=0x0020b8c8, const wchar_t * destCharset=0x0020b8c8, char * * ppOutTerm=0x0020b824) Line 479 + 0x25 bytes C++ msgbase.dll!nsMsgSearchAdapter::EncodeImap(char * * ppOutEncoding=0x0020b878, nsISupportsArray * searchTerms=0x05f7c1d8, const wchar_t * srcCharset=0x0020b8c8, const wchar_t * destCharset=0x0020b8c8, int reallyDredd=0x00000000) Line 725 + 0x1e bytes C++ msgbase.dll!nsMsgSearchOnlineMail::Encode(nsCString & pEncoding={...}, nsISupportsArray * searchTerms=0x05f7c1d8, const wchar_t * destCharset=0x0020ba2c) Line 162 + 0x78 bytes C++ msgbase.dll!nsMsgSearchOnlineMail::ValidateTerms() Line 71 + 0x24 bytes C++ msgbase.dll!nsMsgSearchScopeTerm::InitializeAdapter(nsISupportsArray * termList=0x05f7c1d8) Line 1835 + 0x1c bytes C++ msgbase.dll!nsMsgSearchSession::Initialize() Line 428 + 0x14 bytes C++ msgbase.dll!nsMsgSearchSession::Search(nsIMsgWindow * aWindow=0x05f9eb18) Line 258 + 0x8 bytes C++ An other cross-folder saved search loaded OK, but deleting with multiple selection (e.g., two messages at a time) didn't cause the next message to get loaded (the next msg was selected, however).

with this patch, quick search in an imap folder happens online - that's not desirable :-)

(In reply to comment #72) > with this patch, quick search in an imap folder happens online - that's not > desirable :-) So, it doesn't actually happen in all cases, just IMAP folders that are not marked for offline and whose server's searchScope is online. I think I over-simplified the logic in searchSpec.js' SearchSpec.updateSession... It appears that the original logic in searchBar.js that powers both virtual folders and quick searches against them would _only_ use online search if quick-search was configured to perform a body search or one of the search terms was against the body attribute. That (original) logic has the flaw of always making body searches online, even if the folder is available offline. It appears the original logic in command.glue.js' setupXFVirtualFolderSearch would use offline search unless the 'searchOnline' property was set on the virtual folder, and in that case it would defer to folder.server.searchScope. The current SearchSpec.updateSession logic, which is used for both of the above cases, is: if ((folder instanceof nsIMsgLocalMailFolder) || (folder.flags & nsMsgFolderFlags.Offline) || ioService.offline) scope = nsMsgSearchScope.offlineMail; else scope = folder.server.searchScope; Although SearchSpec has an onlineSearch property, I never actually make use of it... It feels like the right course of action is to modify SearchSpec.updateSession to alter its behaviour based on whether it only has virtualFolderTerms. If so, then we use the 'searchOnline' property to determine whether we ever check folder.server.searchScope, with the amended behaviour that if the folder is nsMsgFolderFlags.Offline, we force it offline. If there are viewTerms (from mail views) or userTerms (from quick search), then we only consider an online search for the message body, with the amended behaviour that if the folder is nsMsgFolderFlags.Offline, we leave the scope offline. (So only a body search on an IMAP folder that is not offline would ever be an online search.) Does that sound right?

(In reply to comment #71) > this broke all my smart folders - the assertion with the following stack trace > is probably related: > > > msgbase.dll!nsMsgSearchAdapter::EncodeImapTerm(nsIMsgSearchTerm * term=0x05eb7c90, int reallyDredd=0x00000000, const wchar_t * srcCharset=0x0020b8c8, const wchar_t * destCharset=0x0020b8c8, char * * ppOutTerm=0x0020b824) Line 479 + 0x25 bytes C++ This is exploding on the sole 'matchAll' ("ALL") criterion. For me this only happens on the "Archives" smart folder; my Inbox works, etc. I think there was buggy behavior in the old code which may cause different behaviour. Namely, commandglue.js' FolderPaneSelectionChange() method would create a filter to parse the 'searchStr' property. It would then pass these terms through CreateGroupedSearchTerms. CreateGroupedSearchTerms, however, fails to propagate the 'matchAll' attribute's value. The specific assertion seems to be anger over nsIMsgSearchTerm.attribute == nsMsgSearchAttrib.Default, but presumably the code should never have reached that point. It appears nsMsgImapSearch is entirely ignorant of the matchAll situation, which I presume is willfull, but without much explanation in the code. I'll see if any of the old wiki or web docs on the search subsystem shed some light, but otherwise your input would be greatly appreciated.

I think the smart folder issue was ultimately the same as the quick search issue - since I had turned off indexing for my imap account in order to test some drag drop code, we ended up trying to do the search online. Now that I turned indexing back on, my smart folders work, as does quick search. Your proposal for searchSpec.updateSession does sound right...

I believe the code would still be capable of generating an online IMAP search with a single matchAll constraint which is still at risk of causing this problem. The scenario would be: opening an XFVF folder whose: a) only constraint is "ALL", b) with 'searchOnline' enabled and c) which contains at least one IMAP folder that is c.i) not marked for offline storage and c.ii) whose server is not marked for offline 'searchScope' This should be addressed. Also, it raises a question as to whether I am wrong about using the folder's offline flag to force the initial XFVF open to offline... I am concerned that 'searchOnline' conflates two issues: 1) Making sure that we have up-to-date headers for a folder, and 2) Making the IMAP server do the legwork of the search. I would suspect that most of the time, case 1 is all that we care about, and by moving to an offline search if the folder is marked for Offline, I might be compromising that somewhat. Although autosync will keep the folders up-to-date, the fact that (AFAIK) it only does things when you aren't looking could prove problematic.

(In reply to comment #76) > I believe the code would still be capable of generating an online IMAP search > with a single matchAll constraint which is still at risk of causing this ... > This should be addressed. I added the following patch snippet which will be included in my next patch iteration, and this made the explosion go away. It results in the production of a query of the form "SEARCH UNDELETED". Not sure if that is sufficiently legit not to terminate. diff --git a/mailnews/base/search/src/nsMsgSearchAdapter.cpp b/mailnews/base/search/src/nsMsgSearchAdapter.cpp --- a/mailnews/base/search/src/nsMsgSearchAdapter.cpp +++ b/mailnews/base/search/src/nsMsgSearchAdapter.cpp @@ -714,19 +714,23 @@ nsresult nsMsgSearchAdapter::EncodeImap // create our expression nsMsgSearchBoolExpression * expression = new nsMsgSearchBoolExpression(); if (!expression) return NS_ERROR_OUT_OF_MEMORY; for (i = 0; i < termCount && NS_SUCCEEDED(err); i++) { char *termEncoding; + PRBool matchAll; nsCOMPtr<nsIMsgSearchTerm> pTerm; searchTerms->QueryElementAt(i, NS_GET_IID(nsIMsgSearchTerm), (void **)getter_AddRefs(pTerm)); + pTerm->GetMatchAll(&matchAll); + if (matchAll) + continue; err = EncodeImapTerm (pTerm, reallyDredd, srcCharset, destCharset, &termEncoding); if (NS_SUCCEEDED(err) && nsnull != termEncoding) { expression = nsMsgSearchBoolExpression::AddSearchTerm(expression, pTerm, termEncoding); delete [] termEncoding; } }

raw patch at: http://hg.mozilla.org/users/bugmail_asutherland.org/comm-central-patches/raw-file/597956fdeb46/folder-display-layer.patch This resolves the issues bienvenu raised: - comment #71, deletion with multiple selection. We now generate a synthetic SelectionChanged notification to the nsMsgDBView when the selection is already where we want it to be following a delete. (We do not suppress NoteChange notifications, in keeping with the previous code's behaviour.) - comment #72, improperly having searches be online for quick search. I implemented this per my comment of #73 and have not done anything raised by my issue in the latter half of comment #76. (The concern being that we do not guarantee that the headers are completely up-to-date in the case of folders that are marked offline.) - comment #71, assertion involving online searches of IMAP folders with an "ALL" constraint. Although mitigated by the previous change, I have included the patch I listed in comment #77 so that the IMAP code does not explode (although it may do something ridiculously inefficient). This is to address the legal situation in the first half of comment #76 where our logic will produce an online search scope. Additionally, I made a few message display related changes due to my own testing: - messageDisplay.js has been modified to not update its visible state when it is inactive. This was just paranoia while addressing the next issue... - I was inverting the visible attribute of MessageDisplayWidget instances if the splitter changed state. This made the view suppress messages when it shouldn't. - If you collapsed the message pane then un-collapsed it without changing messages, nsMsgDBView's suppression logic stopped us from re-displaying the message. We work-around this. (We immediately clear the pane when you collapse the pane both for garbage collection reasons as well as to avoid confusing the user; if they change tabs the multiplexed tabbing implementation might make the old message visible to them otherwise, which would be confusing at best.)

I'm still seeing cases where I delete multiple messages in a normal threaded view, and the next message gets selected but not displayed. It doesn't happen all the time, but it has happened about half the time. I'll try to figure out if there's a pattern. Actually, this is what I think it is - if I have messages: 1 2 3 and I click on 2, then extend the selection to 1, and delete, 3 gets selected but not loaded, If I select 1, then extend to 2, and press delete, 3 gets selected and loaded. Re imap online match all searches, we could either construct protocol that does a match all search (bad for large folders), or somehow turn them into an update of the folder, followed by an offline list of all the msgs in the folder - the latter is probably tricky but it should be doable.

Ugh, new messages are automatically selected, loaded and marked read when I enter a folder (I have newest messages at the top). We don't want that (for one thing, we won't see the new folder summary page :-) )

Comment on attachment 379640 [details] [diff] [review] folder display layer v5 (qdiff -b -p -U8) + if (gMessageDisplay.isDummy) + gMessageDisplay.onDisplayingMessage(messageHeaderSink.dummyMsgHeader); + UpdateMailToolbar("I hate bottles"); :-) - Maybe refer to "eml file finished loading" or something like that. + * the user selecting a message in another window to spawn us or through + * some indirection like displaying a message by message-id. (I think + * that's for newsgroups?) I believe this also happens when we're launched by an indexer like spotlight or vista search - so we should make sure that gets tested w/ this patch :-) + * We also have efficiency concerns about creating new nsIMsgDBViews from + * whole cloth when all we want to do is display a message from them. One reason we do this is because you can do view navigation from a single displayed message, which requires a backing view. + if (fromToolbar) { + // if news, don't delete + if (gDisplayFolder.view.isNewsFolder) + return; } You were just reformatting this, but you can make it just one If statement while you're here. + // respect for random global variables such as the one that + // controlled this. + /* + var el = document.getElementById("msgHeaderView"); + el.setAttribute("style", el.getAttribute("style")); + */ when you're commenting out lines like this, it's nicer to use // so that it's obvious to greppers that the line is commented out. + // Do this before LoadPostAccountWizard since that code selects the first + // folder for display, and we want the tabs prepared. + let tabmail = document.getElementById('tabmail'); + if (tabmail) Doesn't it also potentially display the what's new tab? It never hurts to mention that :-) + if (iTab >= 0) + tabNode = this.tabContainer.childNodes[iTab]; + else + tabNode = null; ? operator, if it doesn't make the line too long. can use ? operator here: + if (sortTypeType == "string") + sortCustomColumn = sortType; + else + sortCustomColumn = sortType.id; and here: + if (searchTerms.length == 0) + return null; + else + return searchTerms;

Two more testing issues: Grouping doesn't show the expand/collapse widgets on the groups, perhaps because of the games the view wrapper plays with the threaded view flag when grouping is turned on. This was in my local inbox. Switching views (e.g., from grouped to threaded) loaded a previously unselected message (I probably didn't have anything selected in the grouped view). Then, trying to switch back to grouped produced this exception: JavaScript error: , line 0: uncaught exception: [Exception... "Component returne d failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIMsgDBView.open]" nsresu lt: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)" location: "JS frame :: file:///C:/moz illa-build/msys/tbirdhq/objdir-tb/mozilla/dist/bin/modules/dbViewWrapper.js :: D BViewWrapper__createView :: line 986" data: no] ++DOMWINDOW == 19 (0B669AD0) [serial = 124] [outer = 04723C18] And now I can't read any non saved-search folder - I just see the account central page in the thread+msg pane area.

I would have thought a restart would fix my problem of not being able to read folders, but not so much :-( Switching back to a build w/o the folder-display patch, and restoring the sort order to unthreaded, newest messages at the top, allowed me to go back to a gloda build. I'll try to get STR.

I've finished reviewing the patch, and modulo the comments I've made so far, and the regressions, it looks good.

Comment on attachment 379640 [details] [diff] [review] folder display layer v5 (qdiff -b -p -U8) minusing since new patch is coming.

raw patch at: http://hg.mozilla.org/users/bugmail_asutherland.org/comm-central-patches/raw-file/4a5bfd63b2f5/folder-display-layer.patch (In reply to comment #80) > Ugh, new messages are automatically selected, loaded and marked read when I > enter a folder (I have newest messages at the top). We don't want that (for one > thing, we won't see the new folder summary page :-) ) mailnews.scroll_to_new_message was using navigate with selection, now it doesn't select. (In reply to comment #79) > I'm still seeing cases where I delete multiple messages in a normal threaded > view, and the next message gets selected but not displayed. It doesn't happen ... > and I click on 2, then extend the selection to 1, and delete, 3 gets selected > but not loaded, If I select 1, then extend to 2, and press delete, 3 gets > selected and loaded. Thanks for the repro. This situation leaves us with a currentIndex of -1, but the selection already contains the index. So my previous workaround did not cover this case. This is testcase fodder... > Re imap online match all searches, we could either construct protocol that does > a match all search (bad for large folders), or somehow turn them into an update > of the folder, followed by an offline list of all the msgs in the folder - the > latter is probably tricky but it should be doable. Would having DBViewWrapper have a command that causes it to walks all of the underlying folders backing a view and calling updateFolder (in sequence, one at a time) be simpler but equivalent? (In reply to comment #82) > Grouping doesn't show the expand/collapse widgets on the groups, perhaps > because of the games the view wrapper plays with the threaded view flag when > grouping is turned on. This was in my local inbox. I think this was mainly a failure to call UpdateSortIndicators when we updated the view and some wackiness in that function, now resolved. However, I did also update the code to pass in the thread bit along with the grouping bit. I'm pretty sure the C++ code would set it itself, but there's no harm in passing it. > Switching views (e.g., from grouped to threaded) loaded a previously unselected > message (I probably didn't have anything selected in the grouped view). Then, > trying to switch back to grouped produced this exception: ... > And now I can't read any non saved-search folder - I just see the account > central page in the thread+msg pane area. Yeah, nsMsgDBView gets upset if it is provided what it considers an illegal sort. Once you got into the state for a folder, it's not surprising it wedged itself. I'm a bit more surprised that no other folders worked; perhaps the view update depth became permanently non-zero? In any event, the initial and follow-on problems has been resolved by making sure we validate the search against the 'mode' at 1) folder opening time, 2) changes to sorts, 3) changing to grouped-by-sort 'mode'.

the delete problem is still there, though it requires one more step to reproduce: After the steps in #c79 (modified to include a 4th message), if I then delete the message that was auto-selected after the first two messages were deleted, nothing gets selected or loaded after. I'm not seeing scrolling to new messages when I open a folder with new mail, but I'm not sure exactly what it did before. But the good news is that it's not auto-selecting a message for me. Grouping is working, and I haven't managed to get things confused again... Deleting the last message in the view doesn't cause the next to last (now new last) message to get loaded.

raw: http://hg.mozilla.org/users/bugmail_asutherland.org/comm-central-patches/raw-file/b8f4f8ef2ac7/folder-display-layer.patch (In reply to comment #87) > the delete problem is still there, though it requires one more step to > reproduce: > > After the steps in #c79 (modified to include a 4th message), if I then delete > the message that was auto-selected after the first two messages were deleted, > nothing gets selected or loaded after. resolved. We now ensure that currentIndex is set to the view index in selectViewIndex in its compensation logic. > I'm not seeing scrolling to new messages when I open a folder with new mail, > but I'm not sure exactly what it did before. But the good news is that it's not > auto-selecting a message for me. I just verified that it works for me (and that our logic is consistent with the old code). I sorted the folder by name so that no other heuristics would randomly happen on the right message. I sent myself a message from another account, and when I switched in the message was displayed with the correct buffer padding of our ensure logic. If it continues to not work for you, the most likely explanation I can think of is that the folder you are testing with is causing FolderDisplayWidget.onAllMessagesLoaded to not trigger in a reasonable time-frame (but is displaying cached results). This would likely occur in a case where DBViewWrapper.shouldShowMessagesForFolderImmediately does not tell us to enter a folder immediately, and instead we wait on the kFolderLoaded notification or search completion (if a search is active). I think this could happen for an IMAP folder... For your convenience, the body of that method is: return (this.isVirtual || !(this.displayedFolder.manyHeadersToDownload || this.listener.shouldDeferMessageDisplayUntilAfterServerConnect)); Where shouldDeferMessageDisplayUntilAfterServerConnect is just a generic hook created for the mail.password_protect_local_cache. (And it's good I just looked that up because I had a syntax error that would cause a runtime error if that pref was enabled... good 'ole E4X making "foo..bar" legal syntax iff foo is an E4X-y thing. > Deleting the last message in the view doesn't cause the next to last (now new > last) message to get loaded. I fixed this. I think this regressed when I responded to your concern about selectViewIndex compelling out-of-bound values into bounds by making out-of-bounds values clear the selection. The next message to delete was == row count (after deletion), so it never did what we would want.

deletion and selection are working better - but when I delete the last message in the view, I do see this assertion: xpcom_core.dll!NS_DebugBreak_P(unsigned int aSeverity=0x00000001, const char * aStr=0x6800be94, const char * aExpr=0x6800be78, const char * aFile=0x6800be38, int aLine=0x00000477) Line 364 + 0xc bytes C++ msgbase.dll!nsMsgDBView::GetSelectedIndices(nsAutoTArray<unsigned int,1> & selection={...}) Line 1143 + 0x2a bytes C++ msgbase.dll!nsMsgDBView::GetNumSelected(unsigned int * aNumSelected=0x002ae114) Line 6580 C++ I'm not seeing the scrolling to new, and it is an imap folder I've been testing with. Actually, I think it fails when selecting the folder is the thing that caused the new message to be discovered and downloaded, which does happen after the view is created. So it might be a notification order thing, like you say. If I get new mail using the drop down button, before clicking on the imap inbox, scrolling does work.

it's getting harder to break, but I have managed to get into situations where discontinuous selections followed by delete causes a message to get selected but not loaded - it might also involve toggling part of the selection before deletion. But I don't quite have steps to reproduce, and it's not a big deal.

When I was trying to eliminate some of the horror of gNextMessageViewIndexAfterDelete I went too far. SetNextMessageAfterDelete in msgMail3PaneWindow.js would not leave without setting gNextMessageViewIndexAfterDelete to some value (even if the choice was dubious), so after deletion, something would always be displayed. Whereas FolderDisplayWidget.hintAboutToDeleteMessages (also triggered by the updateNextMessageAfterDelete notification) in my patch only would set things only if it thinks it is not dealing with a right-click. And it was easily tricked by having the current index not be part of the selection (using one of the cases cribbed from SetNextMessageAfterDelete). This patch (for illustration purposes only, I will provide a v8 patch shortly) corrects that logic by being smarter (if slightly more evil) about determining when the selection has changed due to a right-click. Also, it turns out I had completely missed the fact that RestoreSelectionWithoutContentLoad was also used for the folder pane. I had broken it by taking away stuff the folder pane probably wanted, so I have put that back and specialized dudes.

(In reply to comment #89) > deletion and selection are working better - but when I delete the last message > in the view, I do see this assertion: > > xpcom_core.dll!NS_DebugBreak_P(unsigned int aSeverity=0x00000001, const > char * aStr=0x6800be94, const char * aExpr=0x6800be78, const char * > aFile=0x6800be38, int aLine=0x00000477) Line 364 + 0xc bytes C++ > msgbase.dll!nsMsgDBView::GetSelectedIndices(nsAutoTArray<unsigned int,1> & > selection={...}) Line 1143 + 0x2a bytes C++ > msgbase.dll!nsMsgDBView::GetNumSelected(unsigned int * > aNumSelected=0x002ae114) Line 6580 C++ I think this assertion should only trigger if the selection somehow extends beyond the bounds of the actual rows in the tree. I might have mitigated this, but I find that a disturbing situation to be in. If you can provide a deeper backtrace next time if you see it again, it would be appreciated. > I'm not seeing the scrolling to new, and it is an imap folder I've been testing > with. Actually, I think it fails when selecting the folder is the thing that > caused the new message to be discovered and downloaded, which does happen after > the view is created. So it might be a notification order thing, like you say. > If I get new mail using the drop down button, before clicking on the imap > inbox, scrolling does work. Are you sure that the old code handled this better?

raw: http://hg.mozilla.org/users/bugmail_asutherland.org/comm-central-patches/raw-file/46a9eaaa0875/folder-display-layer.patch This is basically just v7 with the other patch I just put up for selection. I think this makes selection logic much cleaner. I just kicked off a try build, don't know if the try server will explode or not, but people can check here: http://tinderbox.mozilla.org/showbuilds.cgi?tree=ThunderbirdTry It looks like my try server cert might have some issues affecting naming; hopefully that does not make builds unusable.

(In reply to comment #92) > I think this assertion should only trigger if the selection somehow extends > beyond the bounds of the actual rows in the tree. I might have mitigated this, > but I find that a disturbing situation to be in. If you can provide a deeper > backtrace next time if you see it again, it would be appreciated. The rest of the stack is js - my guess is that we're trying to update some command enabled status. I'll see if it happens with the new patch. > > Are you sure that the old code handled this better? Yeah, I went back and tried the old code and it did work just fine. I'll look into it tomorrow.

Ah, there was some c++ on the stack - OnItemIntPropertyChanged then went into js, and ended back up with the previous stack I attached. msgbase.dll!nsMsgMailSession::OnItemIntPropertyChanged(nsIMsgFolder * aItem=0x047172ec, nsIAtom * aProperty=0x03ceef04, int aOldValue=0x000005da, int aNewValue=0x000005d9) Line 161 + 0x5a bytes C++ msgbsutl.dll!nsMsgDBFolder::NotifyIntPropertyChanged(nsIAtom * aProperty=0x03ceef04, int aOldValue=0x000005da, int aNewValue=0x000005d9) Line 4639 + 0x40 bytes C++ msgbsutl.dll!nsMsgDBFolder::UpdateSummaryTotals(int force=0x00000001) Line 3827 C++ msgimap.dll!nsImapMailFolder::UpdateSummaryTotals(int force=0x00000001) Line 1618 + 0x23 bytes C++ msgbsutl.dll!nsMsgDBFolder::OnHdrAddedOrDeleted(nsIMsgDBHdr * aHdrChanged=0x0a3b5b40, int added=0x00000000) Line 1083 C++ msgbsutl.dll!nsMsgDBFolder::OnHdrDeleted(nsIMsgDBHdr * aHdrChanged=0x0a3b5b40, unsigned int aParentKey=0xffffffff, int aFlags=0x00000089, nsIDBChangeListener * aInstigator=0x00000000) Line 1065 C++ msgdb.dll!nsMsgDatabase::NotifyHdrDeletedAll(nsIMsgDBHdr * aHdrDeleted=0x0a3b5b40, unsigned int aParentKey=0xffffffff, int aFlags=0x00000089, nsIDBChangeListener * aInstigator=0x00000000) Line 676 + 0x5d bytes C++ msgdb.dll!nsMsgDatabase::DeleteHeader(nsIMsgDBHdr * msg=0x0a3b5b40, nsIDBChangeListener * instigator=0x00000000, int commit=0x00000001, int notify=0x00000001) Line 1754 C++ msgdb.dll!nsMsgDatabase::DeleteMessages(nsTArray<unsigned int> * nsMsgKeys=0x0035b014, nsIDBChangeListener * instigator=0x00000000) Line 1697 + 0x34 bytes C++ msgdb.dll!nsImapMailDatabase::DeleteMessages(nsTArray<unsigned int> * nsMsgKeys=0x0035b014, nsIDBChangeListener * instigator=0x00000000) Line 93 C++ msgimap.dll!nsImapMailFolder::CopyMessagesOffline(nsIMsgFolder * srcFolder=0x047172ec, nsIArray * messages=0x0a52a4e8, int isMove=0x00000001, nsIMsgWindow * msgWindow=0x05b53fb8, nsIMsgCopyServiceListener * listener=0x00000000) Line 6655 C++ msgimap.dll!nsImapMailFolder::CopyMessages(nsIMsgFolder * srcFolder=0x047172ec, nsIArray * messages=0x0a52a4e8, int isMove=0x00000001, nsIMsgWindow * msgWindow=0x05b53fb8, nsIMsgCopyServiceListener * listener=0x00000000, int isFolder=0x00000000, int allowUndo=0x00000001) Line 6815 + 0x1f bytes C++ msgbase.dll!nsMsgCopyService::DoNextCopy() Line 321 + 0x5c bytes C++ msgbase.dll!nsMsgCopyService::DoCopy(nsCopyRequest * aRequest=0x0a6156f0) Line 263 + 0x8 bytes C++ msgbase.dll!nsMsgCopyService::CopyMessages(nsIMsgFolder * srcFolder=0x047172ec, nsIArray * messages=0x0a327f88, nsIMsgFolder * dstFolder=0x0470bf4c, int isMove=0x00000001, nsIMsgCopyServiceListener * listener=0x00000000, nsIMsgWindow * window=0x05b53fb8, int allowUndo=0x00000001) Line 524 + 0xc bytes C++ msgimap.dll!nsImapMailFolder::DeleteMessages(nsIArray * messages=0x0a327f88, nsIMsgWindow * msgWindow=0x05b53fb8, int deleteStorage=0x00000000, int isMove=0x00000000, nsIMsgCopyServiceListener * listener=0x00000000, int allowUndo=0x00000001) Line 2197 + 0x4f bytes C++ msgbase.dll!nsMsgDBView::DeleteMessages(nsIMsgWindow * window=0x05b53fb8, unsigned int * indices=0x0035b7b8, int numIndices=0x00000001, int deleteStorage=0x00000000) Line 2883 + 0x42 bytes C++ msgbase.dll!nsMsgDBView::ApplyCommandToIndices(int command=0x00000007, unsigned int * indices=0x0035b7b8, int numIndices=0x00000001) Line 2627 + 0x29 bytes C++

Trying to save an advanced search as a saved search fails silently - clicking OK doesn't dismiss the dialog, or create a saved search, or give any errors/warnings or debug output. I added the view picker to the toolbar. We seem to have lost the logic that shows the "Not Deleted" choice only for imap folders on servers where we're using the imap delete model. I opened a second folder in a tab - both folders were inboxes, one pop3, one IMAP. We're not restoring the selected message correctly. Sometimes we leave a message displayed from the prev tab in the message pane when switching back to a tab; sometimes we select and focus a message in the thread pane, but don't load it in the message pane, etc.

Picking the "people I know" custom view, and then picking "save view as folder" doesn't populate the saved search dialog with the search criteria, nor does it create the default saved search name correctly (e.g., Inbox-People I know)

at this point with this patch, I'm having to go look for trouble, instead of having trouble find me, though people with other usage models might have a different experience. I think it's worth throwing up a try server build, perhaps after fixing the most recent issues. I'll look into the scrolling to new imap messages issue and get back to you.

I'm going to try and quickly whip up some mozmill tests; all the ingredients should be there now. Although I think my most recent (v8) selection logic is much mo better, I fear it probably regressed the tab switching you see. We're playing whack-a-mole now and it's not a good use of your time.

I'm running with both patches applied right now, and I was able to get my profile into a state where it crashed due to a null pointer dereference as soon as it logged in to gmail, at startup. The crash was because m_imapMailFolderSink was null here: <http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapProtocol.cpp#7983> I was able to work around this by going offline immediately at startup, and then going online again. I don't see the crash happen now. No idea how I got into that state in the first place, though.

Typing something into the gloda search box, then clicking the search icon and choosing "Save Search as a Folder..." doesn't work. It errors out with JavaScript error: chrome://messenger/content/searchBar.js, line 241: gSearchSession is not defined

Just noticed something else -- STR: 1. Type something in the search box. 2. Receive new mail in that folder, and make sure it causes an alert to appear. Also make sure that mail doesn't match the search text. 3. Click on the alert. The message should show up in the message pane, but it doesn't. We should probably exit the search view at this point.

STR: 1. Open a UTF-8 quoted-printable email with non-ASCII characters in the message pane. (IMAP folder) 2. Middle click on the thread pane message to open it in a tab. 3. Switch back to the 3pane tab. The UTF-8 characters aren't treated as UTF-8 any more. There's an "Overwriting an existing document channel!" assertion that happens there, which I suspect may be the issue.

"The UTF-8 characters aren't treated as UTF-8 any more." s/UTF-8/non-ASCII/g

This is getting into über nitpick territory, but: STR: 1. Select a message in the thread pane. 2. Middle click on the message in the thread pane to open it in a new tab. 3. Switch back to the first tab. 4. De-select that message, selecting nothing at all. Note that the message pane is blank. 5. Switch to the second tab. 6. Switch back to the first tab. The message pane should be blank right now, but it actually contains the contents of the second tab.

OK, testing with your latest iteration (a494b661e829). STR: 1. Open a message in a new window (double click). 2. Delete it from within the window. The delete fails with: An error occurred executing the cmd_delete command [Exception... "'[JavaScript Error: "gRightMouseButtonDown is not defined" {file: "chrome://messenger/content/folderDisplay.js" line: 961}]' when calling method: [nsIController::doCommand]" nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" location: "JS frame :: chrome://global/content/globalOverlay.js :: goDoCommand :: line 86" data: yes] Although delete works from within a message tab, we should either advance to the next message or close the tab, depending on the pref in bug 274628.

STR: 1. Select multiple messages to get the summary to show up. 2. Middle click on a message so that it opens in a new tab. The new tab keeps displaying the summary, and doesn't switch to the message. If you switch away from the message tab, then back to it, it corrects itself.

Comment on attachment 379817 [details] [diff] [review] folder display layer v8 (qdiff -b -p -U8) a new patch is coming, I believe.

raw patch: http://hg.mozilla.org/users/bugmail_asutherland.org/comm-central-patches/raw-file/8115962839eb/folder-display-layer.patch I have some mozmill tests working: TEST test_selection_on_entry PASSED TEST test_selection_extension PASSED TEST test_selection_last_message_deleted PASSED TEST test_selection_persists_through_threading_changes PASSED TEST test_no_selection_persists_through_threading_changes PASSED TEST test_selection_persists_through_folder_tab_changes PASSED TEST test_enter_scroll_to_new PASSED in comm-central/mail/test/mozmill: python runtest.py --showall -t folderdisplay/test_real_inbox.js This patch does not address some of the concerns from comments 96 onwards, which is why I am not posting a prettier version for review. This patch is based off revision e2d3efb4e7df (Sat May 30 01:33:34 2009 +0200), and has not yet been re-based to compensate for changes landed since then. bienvenu, if you could set up an environment in which you can run mozmill so that you can verify the tests run on your machine too, that would be ideal. This primarily entails installing mozrunner, jsbridge, and mozmill on your system so that when you invoke the above script, the python modules are on your sys.path. mail/test/mozmill/runtest.py may need some love if you are on windows. sid0, since you're also aggressively tracking this, it'd be great if you could give them a whirl too. Especially I think given that you also have an active windows dev setup? Implementing tests for the remainder of the points raised should go much faster now that the groundwork for mozmill tests of this functionality has been laid. Also, I think the selection logic should be much improved, although I haven't written the unit tests for the more esoteric cases, so who knows.

sid0, if you get mozmill tests running on vista64, I'd be very interested in finding out what you had to do...

Running the patch under mozmill on Mac (today's build), I see: TEST setupModule PASSED TEST test_selection_on_entry PASSED TEST test_selection_extension FAILED EXCEPTION: Desired selection is: 1 but actual selection is: 1,2 at: test_real_inbox.js line 289 Error("Desired selection is: 1 but actual selection is: 1,2") 0 assert_selected_and_displayed(1) test_real_inbox.js 289 test_selection_extension() test_real_inbox.js 474 frame.js 401 frame.js 447 frame.js 489 frame.js 358 frame.js 500 server.js 164 server.js 168 TEST test_selection_last_message_deleted PASSED TEST test_selection_persists_through_threading_changes PASSED TEST test_no_selection_persists_through_threading_changes PASSED TEST test_selection_persists_through_folder_tab_changes PASSED TEST test_enter_scroll_to_new PASSED There's also a few assertions (of the same type) that I've not seen before: -1608014048[70a980]: ###!!! ASSERTION: wrong entry: 'e->mObject == owner', file ../../dist/include/xpcom/nsISupportsImpl.h, line 145

Mark, the assertion bug is bug 492324, wherein you can find a fix. One indeed does get a lot of them if running without it. Thanks for trying the tests out on OS X!

raw patch: http://hg.mozilla.org/users/bugmail_asutherland.org/comm-central-patches/raw-file/7df3d60a5ff1/folder-display-layer.patch Notable change to the nsITreeView interface to surface the selected message headers so that we don't have to go through the URIs when we really want the headers. This patch includes a JS tracing utility that is active on the new abstractions, dump()ing to console. They use ANSI colors, but standard 16-colors and it should work on any system. I will take it out for the final patch. It is my hope that it assists in the review process and in figuring out what is happening in any regressions observed in the try build. try-server builds are kicked off, will newsgroup post when they are up and if they are good: http://tinderbox.mozilla.org/showbuilds.cgi?tree=ThunderbirdTry

Overall, this looks pretty solid on first run. It took me a while to find an issue - we don't seem to be persisting view flags/sort changes (or we're not restoring the persisted state). STR: 1. Start with an threaded inbox. 2. Turn off threading by clicking on the thread column. 3. Select a different folder and return to inbox. The inbox is threaded again. I also tried turning off threading and then sorting by subject, w/ the same result - back to threaded mode when I returned.

Comment on attachment 382355 [details] [diff] [review] folder display layer v9 (qdiff -b -p -U8) silly nit - decl of rv should be moved to just before where it's used. And I don't think you need to initialize it to NS_OK. +NS_IMETHODIMP nsMsgDBView::GetMsgHdrsForSelection(nsIMutableArray **aResult) +{ + nsresult rv = NS_OK; + + nsMsgViewIndexArray selection; + GetSelectedIndices(selection); + PRUint32 numIndices = selection.Length(); + + nsCOMPtr<nsIMutableArray> messages(do_CreateInstance(NS_ARRAY_CONTRACTID, &rv)); + NS_ENSURE_SUCCESS(rv, rv);

Bug 497199 filed to track regressions found after this lands (or regressions we're not going to fix before landing).

From IRC: [14:40] <bienvenu> asuth, hmm, I've gotten your patch confused, such that selecting folders doesn't load the view... [14:40] <bienvenu> folderDisplay - line 1389 - typeError - treeSelection is null [14:40] <bienvenu> maybe coincidental, but I tried to update my non-working rss feeds for fun right before [14:41] <asuth> bienvenu: does it keep displaying the account central pane? [14:41] <bienvenu> asuth - yes [14:42] <bienvenu> asuth, is that the folder pane's tree selection? [14:43] <asuth> bienvenu: no, that's the thread pane [14:43] <bienvenu> oh, we're trying to navigate to the new msg? [14:43] <asuth> it must be trying to do that too early [14:43] <bienvenu> asuth, anything you want me to look at before I restart? [14:43] <asuth> it must have the pending navigation persisted [14:43] <bienvenu> asuth, that sounds like it could be... [14:44] <asuth> (for cross-folder navigation, we 'push' the navigation request so that we perform it when we enter the folder)

Comment on attachment 382355 [details] [diff] [review] folder display layer v9 (qdiff -b -p -U8) >diff --git a/mail/base/content/tabmail.xml b/mail/base/content/tabmail.xml >--- a/mail/base/content/tabmail.xml >+++ b/mail/base/content/tabmail.xml [...] > <method name="openTab"> > <parameter name="aTabModeName"/> > <body> > <![CDATA[ >@@ -339,17 +412,17 @@ > this.selectTabByIndex(null, tabIndex); > return; > } > } > > // we need to save the state before it gets corrupted > this.saveCurrentTabState(); > >- let tab = {mode: tabMode, canClose: true}; >+ let tab = {mode: tabMode, busy: false, canClose: true}; > tabMode.tabs.push(tab); > > var t = document.createElementNS( > "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul", > "tab"); > t.setAttribute("crop", "end"); > t.maxWidth = this.tabContainer.mTabMaxWidth; > t.minWidth = this.tabContainer.mTabMinWidth; >@@ -359,17 +432,17 @@ > t.className = "tabmail-tab"; > this.tabContainer.appendChild(t); > this.tabContainer.appendChild(t); I may not be understanding this correctly but figured the double appendChild(t) could be a double copy / paste error.

I'm getting a lot of Error: menuItems[0] is undefined Source File: chrome://messenger/content/search.xml Line: 99 in the error console .

With the latest rev: 1. Select multiple messages 2. Middle click on a message to open a new tab 3. Switch back to the first tab. Multiple messages aren't selected any more.

(In reply to comment #114) > 1. Start with an threaded inbox. > 2. Turn off threading by clicking on the thread column. > 3. Select a different folder and return to inbox. > > The inbox is threaded again. I also tried turning off threading and then > sorting by subject, w/ the same result - back to threaded mode when I returned. Done; added test_real_folder_threading_persistence unit test to test_viewWrapper_realFolder.js for verification.

STR: 1. Open a new message tab. 2. Switch back to the first tab. 3. Select multiple messages in this tab so that a summary shows up. 4. Switch to the second tab. We keep showing the summary. According to asuth it's because we've picked the wrong pane.

STR: 1. Open a new message tab. 2. Switch back to the first tab. 3. Delete the message open in the message tab. The title doesn't change, either at this point or when we switch to the message tab.

(In reply to comment #107) > STR: > 1. Select multiple messages to get the summary to show up. > 2. Middle click on a message so that it opens in a new tab. > > The new tab keeps displaying the summary, and doesn't switch to the message. If > you switch away from the message tab, then back to it, it corrects itself. (In reply to comment #122) > STR: > > 1. Open a new message tab. > 2. Switch back to the first tab. > 3. Select multiple messages in this tab so that a summary shows up. > 4. Switch to the second tab. > > We keep showing the summary. According to asuth it's because we've picked the > wrong pane. folder-display/test-summarization.js added

(In reply to comment #123) > STR: > > 1. Open a new message tab. > 2. Switch back to the first tab. > 3. Delete the message open in the message tab. > > The title doesn't change, either at this point or when we switch to the message > tab. I have fixed this and augmented the folder-display/test-deletion-with-multiple-displays.js mozmill test to verify the message tab updates its title both when it is in the foreground and background.

(In reply to comment #119) > I'm getting a lot of Error: menuItems[0] is undefined > Source File: chrome://messenger/content/search.xml > Line: 99 in the error console . I have stopped the code from performing this access pattern and one other. The other was some code that assumes it could use ".value" to access something that was in a DOM attribute with the name "value" rather than an actual JS property. I have not built-up a unit test on this. The quick search 'widget' could still use some refactoring (which I'm not doing for this patch), and that would be the time to consider additional unit testing of UI implementation nuances.

This is probably covered by one of the reported issues with the columns, but I lost my "from" column at some point running with this patch...

For the external observer this bug seems to have a different scope ("folder display layer"?) than indicated by the title. David, Andrew, would you mind updating it?

Comment on attachment 382355 [details] [diff] [review] folder display layer v9 (qdiff -b -p -U8) >+JSTreeSelection.prototype = { >+ /** >+ * The current nsITreeBoxObject, appropriately QueryInterfaced. May be null. >+ */ >+ _treeBoxObject: null, ... >+ get tree JSTreeSelection_get_treeBoxObject() { >+ return this._treeBoxObject; >+ }, >+ set tree JSTreeSelection_set_treeBoxObject(aTreeBoxObject) { >+ this._treeBoxObject = aTreeBoxObject; >+ if (aTreeBoxObject) >+ this._treeBoxObject.QueryInterface(Ci.nsITreeBoxObject); >+ }, The IDL for this property is attribute nsITreeBoxObject tree; So you can simply implement this as tree: null,

(In reply to comment #129) > The IDL for this property is > attribute nsITreeBoxObject tree; > So you can simply implement this as > tree: null, Good point. My concern was that the class might be provided a reference to a treeBoxObject that had not been properly QueryInterfaced by JS code (so there would not be an automatic QI by the wrapper) and so XPConnect might get angry when a nsITreeBoxObject interface method/attribute was used. But further research shows the only way to get a real XPCOM tree box interface ends up retrieving it already QI'd anyways. The lingering issue is that I find the attribute name 'tree' to be ambiguous so preferred a more descriptive name for internal use. I'll lose the QI's but keep the custom attribute.

(In reply to comment #117) > From IRC: > > [14:40] <bienvenu> asuth, hmm, I've gotten your patch confused, such that > selecting folders doesn't load the view... > [14:40] <bienvenu> folderDisplay - line 1389 - typeError - treeSelection > is null > [14:40] <bienvenu> maybe coincidental, but I tried to update my > non-working rss feeds for fun right before The most likely explanation for this problem is that some other error happened which left the code in a 'wedged' state. I have made the following changes to FolderDisplayWidget in an attempt to avoid this eterna-wedge: - _pendingNavigation is cleared when it is accessed and before the call to navigate. If navigate explodes, _pendingNavigation will be cleared. (Because it is intended to be 'pushed' before folder navigation happens, it isn't something that should be cleared as part of the folder or view life-cycles.) - _runNotificationsPendingActivation clears _notificationsPendingActivation when it retrieves the list prior to executing the calls. If an error occurs, a function won't be left stuck in there. - onDestroyingView clears _notificationsPendingActivation so that a new view doesn't get misleading events that belonged to the previous view. This would only be a real (testable) problem if we could tell background tabs to do things.

Comment on attachment 382355 [details] [diff] [review] folder display layer v9 (qdiff -b -p -U8) minusing based on bugs encountered with this patch - but we're very close, and I think asuth has fixed the "blocker" issues already - we just need a new patch w/ those fixed.

(In reply to comment #120) > With the latest rev: > > 1. Select multiple messages > 2. Middle click on a message to open a new tab > 3. Switch back to the first tab. > > Multiple messages aren't selected any more. Overkill fixed and unit tested which should cover all right-click and middle-click potential issues. mozmill test is folder-display/test-right-click-middle-click.js. (Thanks to bwinton for the moral support and sid0 for the technical discussion.) Overkill fix in this case means that we create a transient nsITreeSelection object to replace the 'real' (C++) nsTreeSelection object when the user performs a right-click or a middle-click. Not only that, but the JSTreeSelection logs all calls to adjustSelection on it. This allows us to play back the adjustSelection calls to the real selection when we go to put it back. This neatly handles updating the selection in cases of deletion as well as any changes to the view while the pop-up menu is popped up while also being extremely performant. (Previously, we used FolderDisplayWidget's ability to save its selection to a list of message-id's. Although resilient in the face of mutations to the view, it is potentially egregiously expensive. The new solution can handle the case where the user selected-all without exploding.) The one lingering ugly bit is that cmd_delete in the 3-pane needs to know that a right-click transient selection is in effect so it can _not_ tell the FolderDisplayWidget that a deletion is about to happen. We want to avoid telling it because that would cause us to try and save off the 'next message to select'. But unless the user right-clicked on an existing selection (which we handle in the call to ChangeSelectionWithoutContentLoad by not creating a transient selection), that is entirely the wrong thing to do for the situation. I should also note that the folder pane benefits from these enhancements too, although I did not write any tests for it.

(In reply to comment #127) > This is probably covered by one of the reported issues with the columns, but I > lost my "from" column at some point running with this patch... The logic currently replaces senderCol with recipientCol and recipientCol with senderCol when their legality changes because it's an incoming/outgoing folder. The most likely explanation is that you turned off the recipientCol when you were an outgoing folder and so we didn't try and convert it back when you went into an incoming folder. If that is not the case, then this needs STR or the exception/error output to figure out what is happening, given that the window of opportunity for us to turn a column off without turning its counterpart on is small. In general, the column issue needs to be dealt with, and as mentioned on bug 497272, the simplest solution is probably just to persist column states on a per-folder basis with sane defaults for first-entry.

(In reply to comment #128) > For the external observer this bug seems to have a different scope ("folder > display layer"?) than indicated by the title. David, Andrew, would you mind > updating it? Here you go, good sir.

raw patch: http://hg.mozilla.org/users/bugmail_asutherland.org/comm-central-patches/raw-file/eefd67de8c87/folder-display-layer.patch This resolves all known issues that I haven't explicitly disclaimed on this bug or its helper tracking bug on bug 497199. I will kick off a set of try builds, but it is my understanding there is basically no chance of the OS X build succeeding without gozer to watch over it.

In a normal (imap) folder, switching back to threaded mode, I don't see the twisties anymore. STR: 1. In threaded mode, toggle out of threaded mode. 2. Switch folders 3. Switch back to the original folder, and toggle back into threaded mode - no twisties.

(In reply to comment #137) > In a normal (imap) folder, switching back to threaded mode, I don't see the > twisties anymore. STR: > > 1. In threaded mode, toggle out of threaded mode. > 2. Switch folders > 3. Switch back to the original folder, and toggle back into threaded mode - no > twisties. Whoops. This is obviously the result of the different way I encode _viewFlags and that I did not remember to perform the fix-up prior to updating viewFlags on the view. Given the number of times I have messed this up, it is clear that having nsMsgDBView encode 3 states in 2-bits and then having DBViewWrapper encode those 3 states differently in the same 2 bits is foolish. (My cause was noble, however. :) I will make this consistent and update the DBViewWrapper unit tests to: - blackbox it by checking that IsContainer or whatever makes the twisties appear is returning the correct values. - whitebox it by verifying the bits are set as expected

(In reply to comment #136) > I will kick off a set of try builds, but it is my understanding there is > basically no chance of the OS X build succeeding without gozer to watch over > it. The mac build indeed failed - but I don't see any win32 builds either or did you pushed semonkey builds only ?

In terms of columns being recipient or sender (see Comment #134) please note that there is a long outstanding bug 259270 requesting a column that shows Sender or Recipient depending on the message. I don't know if this work will be effected by having a column that could be either Sender or Recipient?

(In reply to comment #140) > In terms of columns being recipient or sender (see Comment #134) please note > that there is a long outstanding bug 259270 requesting a column that shows > Sender or Recipient depending on the message. I don't know if this work will be > effected by having a column that could be either Sender or Recipient? Having a magic column that shows the right thing based on the message would neatly simplify the issue with the use case mentioned in that comment. But that is indeed a task for another bug, like the one you cited.

(In reply to comment #138) > (In reply to comment #137) > > In a normal (imap) folder, switching back to threaded mode, I don't see the > > twisties anymore. STR: > > Whoops. This is obviously the result of the different way I encode _viewFlags > and that I did not remember to perform the fix-up prior to updating viewFlags > on the view. Given the number of times I have messed this up, it is clear that Double whoops. That analysis was wrong. The problem was we weren't generating a notification that ends up calling UpdateSortIndicators. However, I did clean up our use of _viewFlags before realizing this was not the case. And then I went a step further and stopped us maintaining our own copy of the view flags except when we are about to clobber the view (and don't want it to do wasted work) or there simply is no view. While fixing things, a failing unit test turned me on to the fact that grouping does not actually work so painlessly in the world of just changing the flags. As per legacy code, we were creating an nsMsgGroupView when in grouped mode for a single folder, which obviously cannot just have its viewFlags bits changed to turn it into an nsMsgThreadedDBView subclass. But when I just changed us to always use nsMsgThreadedDBView, it turned out that he does not actually notice changes to the kGroupBySort bit. (nsMsgXFVirtualFolderDBView is the first sub-class to do so.) I am going to chase this down and expand the test_viewWrapper_realFolder.js unit test to do a more thorough verification that changing the bits actually results in threaded/unthreaded/grouped-by-sort views. I had just been verifying the bits flipped in that test and rationalizing that it was the problem of the (currently nonexistent) view unit tests to handle.

raw patch: http://hg.mozilla.org/users/bugmail_asutherland.org/comm-central-patches/raw-file/2700020f70fc/folder-display-layer.patch (In reply to comment #142) The C++ single folder implementation turned out to be a lot more work and unit tests to address. So we are now back to strategy-parity with the pre-patch code rather than trying to fix the back-end. For single folder cases we re-populate the view when transitioning to/from a grouped view. This is all that is required for not losing messages from XFVF folders, which was part of bienvenu's original concern that led to trying to use the viewFlags bits in all cases. The unit tests pass, although it would not hurt if we had more of them. I believe I experienced the situation bienvenu experienced where the folder display became wedged and changing folders did not fix things. Unfortunately, I am unable to reproduce and the information on the error itself was regrettably insufficient. I have, however, attempted to blunt the impact of the situation by having all code in FolderDisplayWidget guard against inexplicably not having a selection object.

Comment on attachment 382997 [details] [diff] [review] folder display layer v11 (qdiff -b -p -U8) [Checkin: Comment 145] I think it's time to release this on an unsuspecting public.

pushed: http://hg.mozilla.org/comm-central/rev/b0e37b312b54

I've tested the try-server build asutherl...@asutherland.org-folder-display-layer-0609-mail-try-mac.dmg a few days ago. But I had a annoying behavior with this build. And now I see the same behavior in a recent build (20090613). So I think this has to do with this bug. Normally I have the message pane closed and read my mails in the standalone window. But if I now close the message pane (drag the splitter down) and switch to another folder (e.g. from inbox to draft) than the message pane is open again. Than I close it, switch to another folder, its open...

(In reply to comment #146) Nomis, I have logged bug 498106 for that problem. If you encounter any other problems with this (now-landed) patch, please check the tracking bug, bug 497199, and if you do not see them there, please file a new bug dependent on bug 497199. Sorry for the regression. We'll have it fixed soon! The easiest way to see what known regressions exist is the dependency tree for 497199: https://bugzilla.mozilla.org/showdependencytree.cgi?id=497199&hide_resolved=0

OK, thanks. If I will encounter another issue I will have a look at bug 497199 :-)

http://mxr.mozilla.org/comm-central/source/mail/base/content/folderDisplay.js#1040 Are those lines supposed to be commented out?

(In reply to comment #149) > http://mxr.mozilla.org/comm-central/source/mail/base/content/folderDisplay.js#1040 > > Are those lines supposed to be commented out? They should have been removed and the commments updated to better reflect the new reality made possible by the transient selection we create.

(In reply to comment #150) > (In reply to comment #149) > > http://mxr.mozilla.org/comm-central/source/mail/base/content/folderDisplay.js#1040 > > > > Are those lines supposed to be commented out? > > They should have been removed and the commments updated to better reflect the > new reality made possible by the transient selection we create. Shall we file a bug to remember this ?

Would someone be so kind to post a screenshot(s) of the current state? TIA

Peter, there is not yet any change to the UI

The code to detect RSS / feed messages is IMHO buggy. The old function IsFeedItem() was able to detect feed items in non-rss-accounts because of the 'content-base' part (it's possible to copy feed items into other accounts). The new function isn't able to detect these feed items anymore. Is this "regression" intended? I was using the old one for an Add-on. New code -------------------------- /** * @return true if there is a selected message and it's an RSS feed message. */ get selectedMessageIsFeed FolderDisplayWidget_get_selectedMessageIsFeed() { let message = this.selectedMessage; return Boolean(message && message.folder && message.folder.server.type == 'rss'); }, Old function ----------------- function IsFeedItem() { return (GetFirstSelectedMessage() && ((gMsgFolderSelected && gMsgFolderSelected.server.type == 'rss') || 'content-base' in currentHeaderData)); }

BTW: Another bug seems to be non-persistent columns in thread pane most times, Shredder is restarted.

(In reply to comment #154) > The code to detect RSS / feed messages is IMHO buggy. The old function > IsFeedItem() was able to detect feed items in non-rss-accounts because of the > 'content-base' part (it's possible to copy feed items into other accounts). The > new function isn't able to detect these feed items anymore. Is this > "regression" intended? I was using the old one for an Add-on. Please create a new bug dependent on bug 497199, the tracking bug. Unfortunately, I don't see an easy solution to the problem. The code can't use currentHeaderData because that assumes the message has been streamed and the headers are available. I don't see any obvious properties on the header that would allow us to determine that the message originally came from an RSS. The message-id is tempting, but abusing an opaque identifier in such a fashion seems even more dangerous. If you want to mention what your add-on's use case is on the bug, we can determine whether we need to be propagating information on the RSS 'message', whether MessageDisplayWidget should be exposing some information, or whether a specific check in your add-on's code would be sufficient.

Please CC me on the "detect an RSS feed item" bug. I wrote some code as part of iterating on the "disable JavaScript in messages" that's likely to be useful in solving in exactly that problem. It didn't end up landing in the final version of the patch, but shouldn't be too hard to extract and use, I suspect.

This patch is basically the same as gloda search layer v2 with the following list of changes. The intent is to try and land this patch as quickly as possible as an _optional search that does not break/replace quick-search_ so that we can iterate on the gloda ramifications and search interface. The hope is that this also helps keep the patch size manageable. - Its FolderDisplayWidget-related code has been updated; it no longer fights said code. - The gloda search facet bar has been hidden. Since it wasn't really hooked up, this results in less confusion. The XUL innards and strings are still there and the intent is to hook them up shortly. - The quick-search and mail-view combo-box stay as customizable toolbar items; the defaults have not been changed. Gloda search needs to be manually customized onto the toolbar. This is a change from the v2 patch (which was a change from the v1 patch) where I was moving quick-search and the mail-view combo-box back to a bar "inside" the thread pane (basically, just above) like they were in Thunderbird 1.5. - I implemented folder indexing priorities for the indexing sweep. Since applying this patch nukes your gloda database, this is a nice improvement that helps you be able to actually test things without going crazy. Sent folders come first, then inboxes, then favorite/checknew folders, then everybody else. It also is told not to go into junk or trash folders, but this does not obviate the bug about gloda not indexing junk. (Gloda will still event-driven index things in the trash/junk folders, or messages marked as junk in non-junk folders. Additionally, deletion logic is still potentially an issue.) The indexing sweep logic also becomes a little bit cleaner as a result. - The search tab title is now properly localized. - The gloda search tab monitor resets the search string when you change the text in it when a gloda search tab, resulting in a new gloda search tab being opened. This is less confusing, but it may not actually be the behavior we want for the text-field. I forget if we want to modify the existing search rather than open a new one. I need to re-read this bug/my notes/bother clarkbw. I just submitted try builds with the designator "gsl-v3". Be aware that your gloda database will be blown away _every time_ you make the transition from running with a TB build without this patch to running with this patch. ex: nightly -> with this patch -> gloda database blown away! -> nightly -> db not blown away -> with this patch -> gloda database blown away! There isn't much harm in it, but I'm sure it could be annoying. There is no practical alternative to this state of things.

I've run with this for a bit. The main issue I've seen so far is that delete from search results does not cause the view to get updated, and leaves things in a generally not so good state.

The same thing happens in advanced search results. Perhaps this is fallout from the refactoring patch that no one noticed...ugh, I really hate having to go back to build w/o this patch to see my gloda db blown away...

I have seen non-responsive UI when doing a search - whenever I break into the debugger, we're in the address book code looking for a card. Granted, it takes me a couple seconds to break into the debugger, but I would look at what we're doing with the address book. At first glance, I think that find card stuff is using a hash table lookup, so it's surprising that I can catch it in the debugger fairly reliably.

as wsmwk points out, double clicking a search result brings up an empty stand alone msg window, along with this error on the js console: JavaScript error: file:///C:/mozilla-build/msys/tbirdhq/objdir-tb/mozilla/dist/b in/modules/dbViewWrapper.js, line 143: aFolders is null

My guess is that the refactoring patch doesn't deal with sending notifications for search results views - it looks like the view wrapper FolderNotificationHelper needs to know about every folder that gets a search hit, and add that folder+wrapper combo to _interestedWrappers. I'd say we could do this up front w/ the search scope except that gloda searches have a global scope so we'd be adding every folder. Or we could tell the FolderNotificationHelper that search results view wrappers are interested in notifications for all folders. That might be easier...it's probably not a big deal to send notifications about folders that aren't in the search results, since they'll be ignored.

(In reply to comment #161) > I have seen non-responsive UI when doing a search - whenever I break into the > debugger, we're in the address book code looking for a card. Granted, it takes > me a couple seconds to break into the debugger, but I would look at what we're > doing with the address book. At first glance, I think that find card stuff is > using a hash table lookup, so it's surprising that I can catch it in the > debugger fairly reliably. What I recall from IRC was that the primary e-mail address lookup uses a hash table, but that we have to walk for the secondary e-mail address in the event the hash lookup fails? I have not looked at the code myself, so I couldn't say. It would not be surprising if gloda's treatment of address book cards is taking a bad situation and making it worse. Gloda caches positive indications of the user having an address book card, but does not cache negative indications. If my previous assertion about the code is true, the negative case is the expensive one. My concern about caching negative indications was that we have no life-cycle maintenance when it comes to address book cards, so we want to be careful about using an out-of-date value. Ways to deal with this, assuming it truly is a problem are: 1) Fix the AB code proper... 2) Make gloda have some concept of 'sessions' so that we can cache that information for the duration of computing the search, but then drop the cache after that. 3) Expand the current address book indexer logic; some of that code may be in my rotted patch on tagging contacts. In a world where gloda listens for address book changes, it can ensure that in-memory representations are up-to-date when things change. The dangerous case is something like LDAP that happens outside our world view, but I vaguely suspect we're not properly handling that case right now anyways, and if we are, it's a horrible thing.

Could be - for the primary address, we force it to lower case so we can use the hash table. I don't remember if the secondary address uses the same trick, but if it doesn't, this is a general problem that we should solve in the AB, since more and more code is looking up contacts. CC'ing Standard8 for input.

(In reply to comment #165) > Could be - for the primary address, we force it to lower case so we can use the > hash table. I don't remember if the secondary address uses the same trick, but > if it doesn't, this is a general problem that we should solve in the AB, since > more and more code is looking up contacts. CC'ing Standard8 for input. The primary email address is a hash table lookup, the secondary email address is search through the table. However, last time I tried this, searching 5000 cards with getCardForEmail was something less than 500ms (and probably much less than that, i.e. 100ms or somthing - I can't remember the actual value). So unless David has extremely large address books, or we're doing a search for every email address in every email stored in gloda, would we really be hitting the address book that hard? I haven't been following what is going on closely enough to know.

Comment on attachment 384591 [details] [diff] [review] gloda search layer v3 (-b) nothing remotely faceted [Checkin: Comment 168] I think we should get this in and let nightly users bang on it. I'd really like to get the delete fix in into the same build but that's waiting for an sr from standard8. A couple nits: Is some code going to get added to the if clause? Also, there are some dump statements after this code... + if (widgetNode == this._facetTypeNode) { + + } + // -- Location Facet this can be all on one line: + let card; + card = GlodaUtils.getCardForEmail(this._value);

pushed: http://hg.mozilla.org/comm-central/rev/87e6de24a564 Marking FIXED. Faceting enhancements and changes related to making gloda search the default search on the toolbar are going to be tracked on bug 474711. Regressions should continue to be tracked against bug 497199. I fixed the 'let card' nit. The widgetNode/dump nit is on inert code that will be modified by bug 474711 (and made active).

(In reply to comment #163) > My guess is that the refactoring patch doesn't deal with sending notifications > for search results views - it looks like the view wrapper > FolderNotificationHelper needs to know about every folder that gets a search > hit, and add that folder+wrapper combo to _interestedWrappers. I'd say we could > do this up front w/ the search scope except that gloda searches have a global > scope so we'd be adding every folder. > > Or we could tell the FolderNotificationHelper that search results view wrappers > are interested in notifications for all folders. That might be easier...it's > probably not a big deal to send notifications about folders that aren't in the > search results, since they'll be ignored. apologies for bringing up old stuff - Is this resolved (like in reference to comment 162), and if not do we have a bug for it?

This global search cannot be used if the mail toolbar is disabled. Control K does nothing. Should I bring this up elsewhere?