Using oct26 commercial trunk build When you delete a message which in the 3-pane QuickSearch view then Undo Deleted Message, it does indeed undo and is back in the regular folder view but doesn't reappear in the existing QuickSearch view. Steps: 1. With QuickSearch bar shown (View|Show/Hide|Search Bar), select a folder in the three pane mail window. 2. Type a string in the QS bar which will yield some matches. 3. Select a result in the QS view of the thread pane and delete it (used Delete toolbar button). 4. Leaving QS view as is, Edit|Undo Deleted Message. Result: nothing appears to happen, message is not placed into the QS view although it really has been undeleted. 5. Look in the folder view or a re-initiated QS for that message and you'll find it is present; the undelete did occur. Result: Undo did not place the message back in the Quick Search view. Side issue/kinda related: Undo related to main/advanced search window - bug 102255

I forgot about undo, will try to fix it.

Well, undo is just like adding new header/ getting new mail, that is disabled in the search view.

still exists dec 14 commercial trunk

this is like adding new headers to QS view- same as showing new mail in QS view. This will get fixed when that one gets fixed.

I have thought about this bug and one way I like is to have the view keep a copy of searchString and on addition of new header - OnNewHeader() if it is in search view check if matches the search criteria and if it does allow it to be added to the view. david, what do you think?

The fix is to have the view hold a weak reference to the searchSession so that we can match the search criteria with the new hdr. I have added a MatchHdr function to the sesssion and adpater so that we can match just one hdr. Also we are *not* supporting threaded search views so just add the hdr to the view. Both undo & new arrival of mail gets added to the search view if they meet the criteria. david, can you review.

why is this noscript? it looks scriptable to me. + [noscript] void matchHdr(in nsIMsgDBHdr aMsgHdr, out boolean aResult); and it should really be boolean matchHdr(in nsIMsgDBHdr aMsgHdr); the comment here is a little cryptic: /*for quick search there is only one scope. If you want to use this routine +make sure you handle multiple scopes */ +nsMsgSearchSession::MatchHdr seems to me that you're implying that the caller needs to handle multiple scopes, but I don't see how that's possible; it would really be up to this method to handle multiple scopes. In any case, perhaps you could add an assertion that there's ever only one scope when this method gets called. I'm not sure that adding a general method on the search session is the way to go here - that seems to imply that it might be possible for various kinds of adapters to implement this, but it's not possible, as is evident by the fact that all the adapters other than offline mail just error out. It might be cleaner just to call: nsresult nsMsgSearchOfflineMail::MatchTermsForSearch(nsIMsgDBHdr *msgToMatch, nsISupportsArray* termList, const char *defaultCharset, nsIMsgSearchScopeTerm *scope, nsIMsgDatabase *db, PRBool *pResult) which is a static method that determines if the passed in msg hdr matches the passed in search criteria.

The reason I added matchHdr was because it is lot cleaner we don't have to redo all the search stuff , like getting charset, scope etc in the view code so adding a method to search session looks ok to me. I see matchHdr as something very generic that other scopes can make use of in future. I made it no script because i don't see any use of this method from js.

that's exactly my point - no other adapters can EVER possibly implement matchhdr - NNTP or IMAP, for example, don't have any protocol which allows you to pass in an nsIMsgDBHdr and match them against a search term, and I can guarantee that they never will. It's a method that's completely specific to the offline mail scope. You shouldn't make things [noscript] unless they can't be scripted, or shouldn't be scripted.

removed matchHdr from nsIMsgSearchAdapter.

do you need the makefile.win and Makefile.in changes to export nsMsgLocalSearch.h or were they just hanging out in your tree? why the timer code change? - m_backgroundTimer->Init(TimerCallback, (void *) this, 0, NS_PRIORITY_NORMAL, NS_TYPE_REPEATING_SLACK); + m_backgroundTimer->Init(TimerCallback, (void *) this, 100, NS_PRIORITY_NORMAL, NS_TYPE_REPEATING_SLACK); If you're going to leave this unimplemented, you should assert as well: +NS_IMETHODIMP +nsMsgDBView::GetSearchSession(nsIMsgSearchSession* *aSession) +{ + return NS_ERROR_NOT_IMPLEMENTED; +} + or you could implement it.

do you need the makefile.win and Makefile.in changes to export nsMsgLocalSearch.h or were they just hanging out in your tree? why the timer code change? - m_backgroundTimer->Init(TimerCallback, (void *) this, 0, NS_PRIORITY_NORMAL, NS_TYPE_REPEATING_SLACK); + m_backgroundTimer->Init(TimerCallback, (void *) this, 100, NS_PRIORITY_NORMAL, NS_TYPE_REPEATING_SLACK); yes, all this was left hanging out, I will remove them before checking in. Also I will add the assertion. Can you r this patch.

Comment on attachment 72461 [details] [diff] [review] patch r=bienvenu, assuming those three issues are resolved.

can you post a new patch for me that has the changes David suggested including taking out the code you said wasnn't part of the patch? Thanks!

new patch

Comment on attachment 72637 [details] [diff] [review] proposed fix hmmm do we really need the arguments in getSearchCharsets to be nsString's as opposed to the more conventional PRUnichar ** aDestCharset? your nsCRT::strcmp useage (as opposed to using strcmp) looks correct since your strings are unicode and we still use the nsCRT versions for unicode. Nice. you added a couple of nsString variables. Can they be nsAutoStrings instead? (i.e. nullCharset, folderCharset)

Made it to use wstring PRUnichar ** native types, I don't know how I left it as nsString, nice catch.

Made it to use wstring PRUnichar ** native types, I don't know how I left it as nsString, nice catch.

Comment on attachment 72714 [details] [diff] [review] right patch remove: +interface nsString; + from nsIMsgSearchAdapter.idl and the noscript tag since we aren't using nsString anymore. sr=mscott

anticipating Navin's possible response, we should make this scriptable - I can imagine this being useful in js code, and according to jband, there's no space advantage to making things noscript.

Comment on attachment 72714 [details] [diff] [review] right patch carrying fwd r=bienvenu

Comment on attachment 72714 [details] [diff] [review] right patch a=asa (on behalf of drivers) for checkin to the 1.0 trunk

fixed

thanks for noscript clarification

OK using mar18 commercial trunk build: win98, linux rh6.2, mac OS 9.2