103734 - Implement "Quick Search" in 3 pane.

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect, P2)

Description

[Navin Gupta]

Comment 1

[Navin Gupta]

here is the specs http://www.mozilla.org/mailnews/specs/qksearch/

Comment 2

[Navin Gupta]

The backend part will involve most work. For the backend to work, I think the 
following interfaces need to be moved around/added. 
1) Move nsIMsgSearchNotify interface from nsMsgSearchDBView to nsMsgDBView. 
2) Add nsIMsgSearchSession, nsIUrlListener to mailSession

We may need to implement a very small subset of the function in these interface
but they should allow us to add stuff in future, pretty easily.

These two changes will allow us to issue search from the 3pane i.e 
maiSession.search() and build up the search results in nsMsgDBView. 

Rest of all the functionality should work (example - move/copy/delete) 
but we will have to decide whether to show new messages in the search view. 
I don't see any reason, why we should not. 

This is a very rough sketch but should be the base of all the things to come.

Comment 3

[scottputterman]

marking nsbeta1+

Comment 4

[Navin Gupta]

The spec says the user will be able to change the search criteria to 
either sender or subject or both and this will be controlled by pref.
Will this be a hidden pref or something ? jennifer, can you provide 
some details. 

Comment 5

[scottputterman]

my opinion is that we could just do "subject or sender" and be correct most of
the time.

Comment 6

[jglick]

Should this be a dup of bug 63573?

Comment 7

[jglick]

"Subject or Sender contains:" should be the default. 

The original thought was to create UI for a pref so users could change this if 
they wanted (to either "Subject contains:", "Sender contains:" or "Subject or 
Sender contains:").

As Scott suggested, why don't we go with the "Subject or Sender contains:" for 
now. No pref. And file the Pref to change this as a separate RFE bug? 

Thoughts?

Comment 8

[Navin Gupta]

jennifer, that sounds good to me. 

Comment 9

[Navin Gupta]

Attachment: proposed fix, v1

Comment 10

[Navin Gupta]

Quick Search implementation:

The user enters some text in the search text
box and if he hits enter/pause, we take the input, create a search session
if there is not one, add listeners, then create searchTerms  and issue the 
search. There is javascipt SearchNotify listener that updates the status
bar and the other listener is the dbview which adds all the keys to level 0
on a search hit. Before we issue a search, we cache all the keys, flags and 
levels, if the previous view was a folder view. This is detected w/ a boolean
isSearchView. Once the search is done we try to restore the selection if 
possible, and set focus to thread-pane.
 
If the input is "" we reload the folder. On reloading we use cache keys as
long as there has been no deletion or addition of msgs to the folder. 


I have moved nsIMsgSearchNotify interface from nsMsgSearchDBView to nsMsgDBView 
the base class because all children (nsMsgSearchDBView and nsMsgThreadedView) 
use it. 

changed long to typedefs name in nsMsgSearchCore.idl as it was originally 
intended to be.

disabled :
 sortByThread in the searchView (all keys are added at level 0).
 Everything else is allowed in searchView. 

The searchBar is present under View | Show/Hide. It is a toolbar so the 
grippy is there to collapse/uncollapse.

Sender changes to Recipient on folder being sent, also the search term criteria
changes from Sender to ToOrCC. On folder switch, the search input is reset.

This works for 3Pane and alt3-pane. 

I have added comments, wherever possible.

Comment 11

[Navin Gupta]

seth and david, please review.

Comment 12

[jglick]

>The searchBar is present under View | Show/Hide. It is a toolbar so the 
>grippy is there to collapse/uncollapse.

Should it have the grippy so it can be collapsed? Or should it be like the 
status bar and only have an on/off state? My original thinking was on/off only 
to avoid the potential complexity of a collapsing toolbar in the middle of the 
mail window. Do folks think being able to collapse it but leave it turned On is 
valuable?

Comment 13

[Scott MacGregor]

I think it should just be show hide like the sidebar and the status bar, etc.
Wire it up to a key to quickly bring it back (like F9 for the sidebar). That's
my two cents.....I think a collapsed quick search bar would look weird too.

Comment 14

[scottputterman]

I agree with mscott.  Just show/hide.

Comment 15

[Navin Gupta]

The toolbar by default has a grippy and I see no reason to remove it. Also
this is a very minor issue. I will attach a screen-shot of how the search 
bar looks. 

Comment 16

[Scott MacGregor]

>The toolbar by default has a grippy and I see no reason to remove it

I think we're giving you a reason =). The spec says no grippy and I think most
of us are in agreement that we don't want the grippy. What do you think about it?

Comment 17

[Navin Gupta]

Attachment: screenshot

Comment 18

[scottputterman]

Yeah, I still think it's better without the grippy.  Can you try it without it
and post that screenshot?  And, sorry to nitpick, but could you center the
"subject or sender" text vertically with respect to the search edit field?

Comment 19

[Navin Gupta]

Attachment: new screen-shot w/o grippy

Comment 20

[(not reading, please use [e-mail] instead)]

navin, can you attach the patch for the version that doesn't have the grippy?

Comment 21

[(not reading, please use [e-mail] instead)]

also, in your local tree, do a "cvs add" of the new files (but don't commit)

then, do "cvs -q diff -uwN > diff.txt", which will include the new files you've 
created.

Comment 22

[jglick]

Navin, attachment id=54154 looks great! :-)

Comment 23

[Navin Gupta]

Attachment: proposed fix,v2

Comment 24

[(not reading, please use [e-mail] instead)]

we don't want to use a normal textbox and timer for this, as we'd have to re-
implement all of the timer and alternate input logic (ctrl + v paste, middle 
mouse button paste on linux, etc) that we've already got for autocomplete 
widget.

of course, for this widget we don't need autocomplete, we just need a simple 
timed textbox.  I've sent mail to hewitt, asking how we could use a subset of 
the autocomplete widget.

I need this same simple timed textbox for #83023 (adding quick search to 
address book) and #87924 (timed quick search for news subscribe dialog)

Comment 25

[(not reading, please use [e-mail] instead)]

I'm wrong, <textbox oninput=""> already does the right thing with paste.

but I'm looking into if we can use type="autocomplete" (so we don't have to 
duplicate the timer code).

I'm hoping by setting some attributes on the textbox, we can turn an 
autocomplete into a simple timed textbox.

Comment 26

[[e-mail]]

[From discussion on n.p.m.ui]
IMHO The search bar is not in the normal toolbar location and is therefore not a
toolbar but a grippyless box and therefore the v2 fix is correct.

Comment 27

[(not reading, please use [e-mail] instead)]

I talked with hewitt, and re-using the existing autocomplete widget isn't the
way to go.

he suggests we extend the textbox in XBL and create a timedtextbox in
mailWidgets.xml (if others find it useful, we can move it to global).

I've logged bug #106049 for that new widget.

we can proceed with this patch, and then later switch 3 pane quick search over
to it once I fix #106049.

Comment 28

[(not reading, please use [e-mail] instead)]

some comments and questions:

1)

var searchBox = null;

can you do

var gSearchBox = null;

to continue our style of doing gFoo for globals?

2)

+ statusFeedback.showProgress(100);  

does that mean that when a search finishes, we show the progress meter as maxed
out?  If so, that's not consistent with our current behavior (like when loading
a page or loading a mail message.)  I think you want to send it with (0).  (I've
got a bug on me to fix this in subscribe.)

3) restore selection

a) what does restore selection do if the selected message isn't in the search
results?
b) if the selected message is in the search results, does restore selection
cause us to reload the message again
c) what happens in those scenarios if multiple items are selected?

4) are you disabling sort by thread when in search mode, or just ignoring the
command when the user tries it?  

5) if the user was sorted by thread, and they do a quick search, how do you
sort?  if the user switches sort order on a search results, and they clear the
result, do you restore the sort order?

6)

 NS_IMPL_ADDREF_INHERITED(nsMsgSearchDBView, nsMsgDBView)
 NS_IMPL_RELEASE_INHERITED(nsMsgSearchDBView, nsMsgDBView)
 NS_IMPL_QUERY_HEAD(nsMsgSearchDBView)
-    NS_IMPL_QUERY_BODY(nsIMsgSearchNotify)
 NS_IMPL_QUERY_TAIL_INHERITING(nsMsgDBView)

I think you can replace all that with

NS_IMPL_ISUPPORTS_INHERITED(nsMsgSearchDBView, nsMsgDBView)

7)

-  NS_ENSURE_TRUE(mBundle, NS_ERROR_NULL_POINTER);
+  //NS_ENSURE_TRUE(mBundle, NS_ERROR_NULL_POINTER);
+  if (!mBundle) return NS_ERROR_NULL_POINTER;

why'd you remove that assert?  are you hitting that code without at a bundle?
It's there to help us track down bugs where we're not updating the status text.

Comment 29

[Navin Gupta]

>var searchBox = null;
>can you do
>var gSearchBox = null;
>to continue our style of doing gFoo for globals?

I don't know how religiously we follow this 
we have messagesBox that is also global. Anyway i will change it.

>3) restore selection

>a) what does restore selection do if the selected message isn't in the search
results?
  doesn't do anything because viewIndex = -1

>b) if the selected message is in the search results, does restore selection
>cause us to reload the message again
  yes

>c) what happens in those scenarios if multiple items are selected?

  it doesn't preserve multiple selection. 

>4) are you disabling sort by thread when in search mode, or just ignoring the
>command when the user tries it?  

 ignoring the command when the user tries it. 

>if the user was sorted by thread, and they do a quick search, how do you
>sort?

  looks like it does by date, sort order is not changed, i mean it is still
threaded per say (from menus, sort indicators) but since keys are added at level 
0, no threading is seen.

>if the user switches sort order on a search results, and they clear the
>result, do you restore the sort order?

yes


>7)

>-  NS_ENSURE_TRUE(mBundle, NS_ERROR_NULL_POINTER);
>+  //NS_ENSURE_TRUE(mBundle, NS_ERROR_NULL_POINTER);
>+  if (!mBundle) return NS_ERROR_NULL_POINTER;

>why'd you remove that assert?  are you hitting that code without at a bundle?
>It's there to help us track down bugs where we're not updating the status text.

this part of patch is not part of the fix. 

Comment 30

[(not reading, please use [e-mail] instead)]

>>b) if the selected message is in the search results, does restore selection
>>cause us to reload the message again
> yes

so, every time I do a quick search, we reload the current message?  
That will hurt peformance.  Can you explain why we need to reload the message?

> >c) what happens in those scenarios if multiple items are selected?
> it doesn't preserve multiple selection.

notice if you select 3 items and change the sort (or switch to or from threaded
mode) we restore the selection properly.  

I think we're going to want to make that work for quick search.  after entering
into quick search mode, we should attempt to restore our thread pane selection,
selecting those message if they are in our results.  unless the user alters the
selection, we should restore the selection back if we jump out of quick search mode.

comments?

Comment 31

[(not reading, please use [e-mail] instead)]

don't we want to remove the timer when the user hits enter?

looking at onSearchInput(), it looks like we might fire onEnterInSearchBar() 
twice if I start typing (so the timer gets created) and then hit enter before 
the timer fires.

how about this:

function onSearchInput(event)
{
  if (gSearchTimer) {
    clearTimeout(gSearchTimer); 
    gSearchTimer=null;
  }

  if (event.keyCode == 13) {
    onEnterInSearchBar();
  }
  else {
    gSearchTimer = setTimeout("onEnterInSearchBar();", 800);
  }
}

Comment 32

[Navin Gupta]

>so, every time I do a quick search, we reload the current message?  
>That will hurt peformance.  Can you explain why we need to reload the message?

We need to reload the message because we want to reload the 
currentlyDisplayedMessage again. There is code that does not loadMessageByKey
if key is the same as m_currentlyDisplayedMsgKey or something. it doesn't hurt
any performance. 

>I think we're going to want to make that work for quick search.  after entering
>into quick search mode, we should attempt to restore our thread pane selection,
>selecting those message if they are in our results.  unless the user alters the
>selection, we should restore the selection back if we jump out of quick search 
>mode.

I tried this but it does not work for some reason, the problem is that sort 
results are different than search result, I mean the messages will always be
there for sort but for search it will depend on the search criteria so it 
ends up selecting wrong stuff. we can log a bug about this, I did not want 
to spend lot of time on this. 

Comment 33

[(not reading, please use [e-mail] instead)]

>>so, every time I do a quick search, we reload the current message?  
>>That will hurt peformance.  Can you explain why we need to reload the message?
>
>We need to reload the message because we want to reload the 
>currentlyDisplayedMessage again. There is code that does not loadMessageByKey
>if key is the same as m_currentlyDisplayedMsgKey or something. it doesn't hurt
>any performance. 

naving and I debugged, and calling ReloadMessage() will reload the message if
the selected message is in the search results.

this brings up some UI issues.

what do we do with the message pane and the selection going into and after
returing from quick search?

I'd argue that going into quick search, we should blank out the message pane,
and not restore the selection that the user had in the thread pane.

my reasoning is that doing a quick search is the same as loading a virtual
folder.  The current folder loading blanks out the message pane and clears the
selection.

here's the user experience:

the user has loaded their inbox and is reading a message from scott putterman. 
they go to quick search, and they type in a name, say "scott", we clear the
message pane (if not already clear) and we clear the thread pane.  

they get back a lot of hits, including the message they were reading.  

the message is selected (because we restore selection) and the message is
reloaded, perf hit, since the message is reloaded.  (I'm arguing to skip this step.)

because they got back a lot of hits, they extend their quick search to "scott
putterman".  we clear the message pane (if not already clear), and we clear the
thread pane.  

they get back fewer hits.
 
the message is again selected (because we restore selection) and the message is
reloaded, perf hit, since the message is reloaded.  (I'm arguing to skip this step.)

now they select the message they were searching for and peform an action on the
desired message (read it, reply, delete, etc). 

now they jump back to their inbox, by clearing the quick search text area.

I'm not sure if it is best at this point to restore the original selection and
select the original message.

comments?

Comment 34

[(not reading, please use [e-mail] instead)]

one possibility for what to do on coming back to the inbox, in addition to
restoring selection, is to restore the scroll position. 

there's an existing bug on this for persisting scroll position after folder loading.

Comment 35

[scottputterman]

I'd say for the case where they are searching, clearing the message pane is the
correct thing to do.  

When they reselect the folder or clear it, I think we should just let the folder
load like normal where it scrolls to the first new message.  If we ever
implement Remember Last Selected Message then it will do the right thing.

Comment 36

[Navin Gupta]

It always makes more sense to reselect the message if it is in the search
results or in the folder view. I say this because the user will anyway
want to do something in the thread-pane after having issued a search or coming
out of it. I have also made it so that it scrolls to the message in the view. 

Also clearing message-pane while issuing search seems more correct to me 
as a user from ui perspective. 

>When they reselect the folder or clear it, I think we should just let the 
>folder
>load like normal where it scrolls to the first new message.  If we ever
>implement Remember Last Selected Message then it will do the right thing.

This would still affect the performance just as reloading currently selected 
message. Remember Last Selected message has nothing to do here because search 
uses the same view as the folder. 

Comment 37

[David :Bienvenu]

Unless I'm misunderstanding the argument, I'm completely with Navin on this one
- very often I'm trying to find a thread, so I do a quick search to find one of
the messages, select it, then go back to the normal view. I want my message
still selected and loaded. Scrolling to it is better than nothing, but I think
I'd like it to stay selected and loaded.

Comment 38

[jglick]

I agree with Scott.

Comment 39

[scottputterman]

You're right.  It would be like implementing Go To Folder.  So, we probably
should keep the selection when the user clicks on the folder.  I do think we
should clear the currently selected message when doing a search, though.

Comment 40

[(not reading, please use [e-mail] instead)]

to summarize:

1) on performing a quick search, clear the message pane and clear the selection.

2) coming out of quick search (either by clicking the current folder, or by
clearing the quick search text) we restore the selection that the user had in
quick search.  ("go to folder" is a good analogy.)

if I'm wrong, please correct me.  

naving, can you get that going and attach a new patch?

Comment 41

[Navin Gupta]

why complicate it more and not preserve the selection during both ways
going into search or coming out of it. Also clicking on current folder again
is not yet implemented, that was never dicussed.

Comment 42

[scottputterman]

The spec mentions clicking on the same folder reloading the folder.

Here are some suggestions:

1.  Clicking on the same folder after a search shows the whole folder and
scrolls to and keeps selected the current selection assuming it's still in the
view (not sure if it's possible to not be in the view).

2.  Clicking on a different folder after a search clears the message pane.

3.  Blank the message pane while searching.  I don't think the user should be
able to take any action while a search is going on.

4.  I'm not sure what to do if the previously selected message is in the search
results.  Jennifer tells me that Outlooks selects the first message (and this
sounds similar to the way the AB is going to look).  But, I'm against selecting
the first message if it's going to mark it as read.  This is something I think
we have to play with.  Right now I still think we shouldn't select anything.

Any disagreements with these?

Comment 43

[Navin Gupta]

> The spec mentions clicking on the same folder reloading the folder.
ok, putterman was thinking switching to a different folder and coming back, 
while I was thinking clicking on already selected folder again. 

Clicking on already selected folder raises another problem i.e which selection
to restore "a message selected in search selection" or "Remember last selected
message for the folder". 

Comment 44

[scottputterman]

I actually was thinking about the currently selected folder.  My comment about
it being in the spec was actually the switching folder example.

I think with Remember Last Selected Message, we obey the pref.  If they have it
set then we don't act like Go To Folder.  If they don't have it set then we do
act like Go To Folder.  Since we haven't implemented Remember Last Selected
message yet, we act like Go To Folder.

Comment 45

[(not reading, please use [e-mail] instead)]

> why complicate it more and not preserve the selection during both ways
> going into search or coming out of it.

preserving selection and multiple selection going into quick search doesn't make
sense, as your selection may not be there.  since we aren't going to load the
message, preserving selection spells trouble.  having the user click on the
already selected message sounds bad to me.

as I said earlier, doing a quick search is like loading a folder.

coming out of quick search, you'll always find your selection.  (the quick
search results is a subset of the folder). 

implementing selection preservation when going out of quick search gives us "go
to folder" functionality, without having to open the search dialog.

> Also clicking on current folder again is not yet implemented, that was never
discussed.

I agree with putterman that we need this.  the one potential usuability problem
I can see with quick search:  how to get out of it.  

having multiple ways to get out of quick search (clear text, select another
folder, re-select folder) is a good thing.

Comment 46

[Navin Gupta]

Attachment: revised patch

Comment 47

[Navin Gupta]

So I have added another thing ie ability to click on the currently loaded
folder to get out of search view. made changes suggested by seth. Also made it
so that we are not restoring selection when we go into search view.

 NS_IMPL_ADDREF_INHERITED(nsMsgSearchDBView, nsMsgDBView)
 NS_IMPL_RELEASE_INHERITED(nsMsgSearchDBView, nsMsgDBView)
 NS_IMPL_QUERY_HEAD(nsMsgSearchDBView)
-    NS_IMPL_QUERY_BODY(nsIMsgSearchNotify)
 NS_IMPL_QUERY_TAIL_INHERITING(nsMsgDBView)

>I think you can replace all that with

NS_IMPL_ISUPPORTS_INHERITED(nsMsgSearchDBView, nsMsgDBView)

This doesn't work and why change something that is already working and is not 
wrong?

Comment 48

[David :Bienvenu]

looks like my review comments got lost. The method reloadFolder looks to me like
it should really be called "ReloadFolderAfterQuickSearch" or something like that
because it's not a generic reloadFolder method; it assumes you've done a quick
search and want to go back to the full folder view.

And just a nit, but here:
+  if (mIsSearchView)
+  {
+    OnHeaderAddedOrDeleted();  //db has changed and we are not adding hdr to
view so clear the cached info..
+    return rv; // do not add a new message to search view.
+  }

you want to return NS_OK, not rv, since you're not setting rv (the code up above
is, and someone could add code above you and break you).

Comment 49

[(not reading, please use [e-mail] instead)]

> This doesn't work and why change something that is already working and is not 
> wrong?

try this:

NS_IMPL_ISUPPORTS_INHERITED1(nsMsgSearchDBView, nsMsgDBView, nsIMsgDBView)

Comment 50

[jglick]

I think we should try it as Seth and Scott propose and see how it feels. If it 
doesn't seem like the right behavior, we can revisit. As Seth mentioned, we 
might need to make it more clear how to get out of search mode, maybe a "Clear" 
button (clicking on a folder or removing text from search field might not be 
obvious to some).

Comment 51

[Navin Gupta]

Is the review over ? do you want a new patch ? 
Frankly, I am tired of fixing this kind of minor nit picking, which comes in
small chunks, rather than being a one time thing.

Comment 52

[David :Bienvenu]

You haven't fixed the name of reloadFolder. I'm sorry if you think that's a nit,
but it's very important that the method names reflect what the methods do. Sure,
you know what it does, but someone else coming to the code will be confused for
a while. ReloadFolder sure sounds like a generic method to reload a folder. Am I
misreading the code?

Comment 53

[Navin Gupta]

Attachment: new patch w/ comments incorporated

Comment 54

[David :Bienvenu]

Comment on attachment 54936 [details] [diff] [review]
new patch w/ comments incorporated

thanks, sr=bienvenu

Comment 55

[(not reading, please use [e-mail] instead)]

Comment on attachment 54936 [details] [diff] [review]
new patch w/ comments incorporated

r=sspitzer

Comment 56

[Navin Gupta]

feature landed

Comment 57

[Navin Gupta]

I want to thank bienvenu and sspitzer for helping me.

Comment 58

[Peter Lairo]

I've read MOST of the posts here, so I apologize if I'm overlooking something.
Looking at the second screenshot: 

Since the search bar takes up so much space, it would be better to give it some
*more functionality*. 

Assuming the search *hides* all messages that do NOT meet the search criterion: 
-> What if a user wants to see the replies to his posts and therefore filters on
his name (this is what I might use it for most)? Then the user would have to
type his name, highlight one of his posts, remove the filter, read the reply,
reenter his name in the search, highlight his next post, remove the filter ...
repeat ad nauseum.

Therefore, it would be useful to have some buttons in the quicksearch bar:
1. Filter (or "Go"?): hides all messages except those that match the search
criterion (current behaviour?).
2. go next: doesn't hide anything. Jumps to the next message that matches the
filter.
3. go previous: doesn't hide anything. Jumps to the previous message that
matches the filter.
Note: 2 and 3 could just be icon arrows (<-/->) to save space.

Of course Prev. and Next should respect the collumn sort order the user has set.

PS. How can this bug be fixed when all bugs it depends on are not fixed? Just
seems odd.

Comment 59

[scottputterman]

I created a different Quick Search tracking bug in
http://bugzilla.mozilla.org/show_bug.cgi?id=106943.  Removing dependencies from
this one.

Comment 60

[laurel]

Basically implemented and functioning. Other issues have been/will be logged
separately and one can also refer to the tracking bug.
OK using oct29 commercial trunk build: win98, mac OS X, linux rh6.2