All users were logged out of Bugzilla on October 13th, 2018

Del msg from QuickSearch does undo in folder, but doesn't reappear in QS view

VERIFIED FIXED in mozilla1.0

Status

P2
normal
VERIFIED FIXED
17 years ago
10 years ago

People

(Reporter: laurel, Assigned: naving)

Tracking

(Blocks: 1 bug)

Trunk
mozilla1.0
All
Windows 98
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

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

Comment 1

17 years ago
I forgot about undo, will try to fix it. 
(Assignee)

Comment 2

17 years ago
Well, undo is just like adding new header/ getting new mail, that is 
disabled in the search view.
(Reporter)

Updated

17 years ago
Blocks: 106943
(Reporter)

Comment 3

17 years ago
still exists dec 14 commercial trunk
Keywords: nsbeta1

Updated

17 years ago
Status: NEW → ASSIGNED
Keywords: nsbeta1 → nsbeta1+
Priority: -- → P2
(Assignee)

Comment 4

17 years ago
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. 
Depends on: 120583

Updated

17 years ago
Target Milestone: --- → mozilla1.0
(Assignee)

Comment 5

17 years ago
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?
(Assignee)

Comment 6

17 years ago
Created attachment 72140 [details] [diff] [review]
proposed fix

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.

Comment 7

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

Comment 8

17 years ago
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. 

Comment 9

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

Comment 10

17 years ago
Created attachment 72461 [details] [diff] [review]
patch

removed matchHdr from nsIMsgSearchAdapter.
Attachment #72140 - Attachment is obsolete: true

Comment 11

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

Comment 12

17 years ago
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 13

17 years ago
Comment on attachment 72461 [details] [diff] [review]
patch

r=bienvenu, assuming those three issues are resolved.
Attachment #72461 - Flags: review+

Comment 14

17 years ago
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!
(Assignee)

Comment 15

17 years ago
Created attachment 72637 [details] [diff] [review]
proposed fix

new patch
Attachment #72461 - Attachment is obsolete: true

Comment 16

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

Comment 17

17 years ago
Created attachment 72713 [details] [diff] [review]
patch as per mscott comments

Made it to use wstring PRUnichar ** native types, I don't know how I left it
as nsString, nice catch.
Attachment #72637 - Attachment is obsolete: true
(Assignee)

Comment 18

17 years ago
Created attachment 72714 [details] [diff] [review]
right patch

Made it to use wstring PRUnichar ** native types, I don't know how I left it
as nsString, nice catch.
Attachment #72713 - Attachment is obsolete: true

Comment 19

17 years ago
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
Attachment #72714 - Flags: superreview+

Comment 20

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

Comment 21

17 years ago
Comment on attachment 72714 [details] [diff] [review]
right patch

carrying fwd r=bienvenu
Attachment #72714 - Flags: review+

Comment 22

17 years ago
Comment on attachment 72714 [details] [diff] [review]
right patch

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #72714 - Flags: approval+
(Assignee)

Comment 23

17 years ago
fixed
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
(Assignee)

Comment 24

17 years ago
thanks for noscript clarification
(Reporter)

Comment 25

17 years ago
OK using mar18 commercial trunk build: win98, linux rh6.2, mac OS 9.2
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey

Updated

10 years ago
Component: MailNews: Search → MailNews: Message Display
QA Contact: laurel → search
You need to log in before you can comment on or make changes to this bug.