327269 - UI lacks proper progress indication for lookups in address book

Status: NEW

(Thunderbird :: Address Book, defect)

Description

[Anton Bobrov]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1

this almost has no impact on local address book lookups since
they are usually pretty fast but same lookups against ldap
directory can take emough time to confuse end users that their 
lookups do not work or that there are no matched entries. real
problem is that there are no visual feedbacks from UI on when
the search is started and when it is finished. status text is
always shows "no matches found" by default even when lookup is
in progress and total entries found only reflect the total #
found so far so it cannot be used as an indicator of search
completion. there are several places where UI should indicate:

- Address Book itself, the throbber in the top right corner.
  should start spining on lookup and stop when its done.

- Advaced Address Book search, the statusbar in the bottom
  right corner. should show undetermined while search is in
  progress and get back to normal when search is done.

- Compose New Mail window -> Contacts -> Address Book panel
  on the left. Additional throbber is required right next
  to the search box to indicate search progress.

- Compose New Mail window, autocomplete feature should spin
  the throbber to indicate autocompletion lookup progress
  and stop it when its done.


Reproducible: Always

Comment 1

[Anton Bobrov]

Attachment: suggested fix

Comment 2

[David :Bienvenu]

Comment on attachment 211968 [details] [diff] [review]
suggested fix

overall, I think this kind of feedback is good but I'll let Scott have the final say about that.

One question: making this global seems like a potentially dangerous thing:

+nsCOMPtr<nsIAbViewListener> mAbViewListener;
+

why did you do that?

Comment 3

[Anton Bobrov]

i'm not very familiar with the code at this point so i simply 
choosen an easy way to expose it so it is accessible. please 
feel to fix it as you see fit so it cannot make any potential 
danger to the rest of the code there.

Comment 4

[David :Bienvenu]

Comment on attachment 211968 [details] [diff] [review]
suggested fix

ah, that's a no-no...I'll cc standard8 and see if he has an idea about how else to do this. I'm not familiar with the relationship between these classes.

Comment 5

[Mark Banner (:standard8)]

(In reply to comment #4)
> (From update of attachment 211968 [details] [diff] [review] [edit])
> ah, that's a no-no...I'll cc standard8 and see if he has an idea about how else
> to do this. I'm not familiar with the relationship between these classes.
> 

Just to let you know I've seen this and I'll need a couple of days to think about where it needs to go - I also need to check out the interaction between nsIAbView and the search classes to see if we can improve that interface.

Comment 6

[David :Bienvenu]

thx a lot, Mark.

Comment 7

[Mark Banner (:standard8)]

I've started to look at this patch. Just some general comments to start off with.

Firstly I think the general idea of the functionality is good. Thanks for putting the time in. Hopefully we can work together to improve it a bit.

(In reply to comment #0)
> - Address Book itself, the throbber in the top right corner.
>   should start spining on lookup and stop when its done.

Looks good. Would it be possible to additionally change the status bar so that at the start of/during the search it shows "Searching...found x so far" (or something along those lines) and then at the end it gives the total matches found? This would provide additional clarity to the user.

> - Advaced Address Book search, the statusbar in the bottom
>   right corner. should show undetermined while search is in
>   progress and get back to normal when search is done.

This seems to behave strangely - it flickers to a full bar, and then shows up to about 1/4 of the bar. I don't know enough about that one to see what's happening.

> - Compose New Mail window -> Contacts -> Address Book panel
>   on the left. Additional throbber is required right next
>   to the search box to indicate search progress.

Why not use the throbber on the compose new mail window itself? If you re-use the one on the compose mail window then its a simpler UI for the user.

> - Compose New Mail window, autocomplete feature should spin
>   the throbber to indicate autocompletion lookup progress
>   and stop it when its done.

I couldn't see this working, but perhaps my directory isn't big/slow enough.

Just a couple of comments on the code itself for now:

Can you do the patch against the trunk code, at least initially please? Generally we get things implemented working on the trunk and then transfer them to the branch if deemed to be working correctly.

In javascript, rather than use item.setAttribute("x", y) you should normally (I believe) be able to use item.x = y; This makes the code easier to read (yes I know some of that code is existing, but we tend to try and clean it up as new code is added). It is still best to use .removeAttribute though.


I'm still looking at what to do about the nsIAbViewListener, I think I know a possible solution, but I still need to think about it a bit more to be sure.

Comment 8

[Anton Bobrov]

Mark, thanx for getting involved here!

> Looks good. Would it be possible to additionally change the status bar so that
> at the start of/during the search it shows "Searching...found x so far" (or
> something along those lines) and then at the end it gives the total matches
> found? This would provide additional clarity to the user.

altho onCountChanged is currently not aware of when the search is 
started and when its done it can be changed and perhaps invoked 
with additional argument that would indicate current search state 
so it can spit appropriate messages as statustext but i personally 
think its a wee bit of an overkill & trashing UI. imho one visual 
feedback indicator is enough to show users there is a search going 
on and two indicators would simply split users attention therefore 
can be confusing. one idea of further improvement i have is to 
stop showing "no matches found" if the user did not search for 
anything or the search box is clear. you can have a look at Apple
MacOSX builtin AddressBook, i believe they have some pretty fierce
rules regarding keep it simple UI. the other solution would be to
ditch throbbers/statusbars and use only statustext to indicate but
i think its less visible and less user friendly in this case.  

> This seems to behave strangely - it flickers to a full bar, and then shows up
> to about 1/4 of the bar. I don't know enough about that one to see what's
> happening.

what OS/GUI you have tried it on? the reason i ask is because the
statusbar seems to be have few glitches today. i put myself on cc
list for this bug : 304147 "progressmeter in undetermined mode 
does not work in Mac OS X" maybe there are similar bugs for other
OS/GUI pairs. i cannot try it on anything else but MacOSX Panther
today however what i did observe on Panther is the following :

- progress bar never oscillates when activated. this is exactly
  the same for the latest off the shelf Firefox and Thunderbird
  on Panther so it cannot be my build issue. 

- for some weird reason activating/deactivating that progressbar 
  in address search doesnt work if you do it according to the
  docs and code references inside mailnews. ie normally what one
  would do to activate it is gStatusBar.mode = "undetermined";
  and then use gStatusBar.mode = "normal"; to deactivate it when
  done however doing just that seems to be not enough. it would
  only change its state the window is resized after state change
  so after struggling with it for awhile i came up with solution
  you see in my patch. cant explain it tho, triggers something.

in any case it would be real nice if somebody can try it on 
Win and Linux KDE/GNOME just to see whats the score there.

> Why not use the throbber on the compose new mail window itself? If you re-use
> the one on the compose mail window then its a simpler UI for the user.

good point. couldnt reach it :( can you tell me how to do that?!
that pane lives as a separate object of some sort so i had to
add a new throbber to it because i dunno how to get to the 
throbber in the main window from that pane. there is more to it.
another annoying thing i did notice is whenever i try to make an
object like throbber hidden any neighbor object in close range
that is willing to stretch would stretch over it. in this case 
its the searchtext box left to the throbber. again, pardon me
for bringing Apple here second time in the row but according to
their UI guidelines deactivated throbbers should be hidden. can
do that but all the flickering with neighbor objects resizing 
that surrounds it during hide/show doesnt look nice so i gave
up on it. is there any way to make it work as expected ? maybe
useless for this particular case but just so i know for future
reference so if you know let me know please.

> I couldn't see this working, but perhaps my directory isn't big/slow enough.

try searching for something that would give you a very long list
of matches eg "jo" or similar. i did test it on pretty large dir
here and it was over vpn link as well, kinda gives more lag for
the thingie to spin i guess.

> Can you do the patch against the trunk code, at least initially please?
> Generally we get things implemented working on the trunk and then transfer them
> to the branch if deemed to be working correctly.

sure. couldnt find proper checkout instructions for the current
Thunderbird. what do i do to checkout the HEAD ? i only need
modules that are necessary to build Thunderbird not all of it..
[ the srcs i have right now are the ones from tarball off ftp ] 

> In javascript, rather than use item.setAttribute("x", y) you should normally (I
> believe) be able to use item.x = y; This makes the code easier to read (yes I
> know some of that code is existing, but we tend to try and clean it up as new
> code is added). It is still best to use .removeAttribute though.

ok, i gonna redo it once we sort out all the rest of it.

> I'm still looking at what to do about the nsIAbViewListener, I think I know a
> possible solution, but I still need to think about it a bit more to be sure.

thanx, i'm sorry for a cheap hack there but since i did not
know any better i figured that what reviews are for right?!

Comment 9

[Mark Banner (:standard8)]

(In reply to comment #8)
> Mark, thanx for getting involved here!
no problem.

> altho onCountChanged is currently not aware of when the search is 
> started and when its done it can be changed and perhaps invoked 
> with additional argument that would indicate current search state 
> so it can spit appropriate messages as statustext but i personally 
> think its a wee bit of an overkill & trashing UI.

Yeah, I missed that (see below though), I think I agree with your other comments though.

> > Why not use the throbber on the compose new mail window itself? If you re-use
> > the one on the compose mail window then its a simpler UI for the user.
> good point. couldnt reach it :( can you tell me how to do that?!

I'll have a look, it may need opening up some more.

> sure. couldnt find proper checkout instructions for the current
> Thunderbird. what do i do to checkout the HEAD ? i only need
> modules that are necessary to build Thunderbird not all of it..

See http://developer.mozilla.org/en/docs/Download_Mozilla_Source_Code in particular the page about getting it from CVS 

> > I'm still looking at what to do about the nsIAbViewListener, I think I know a
> > possible solution, but I still need to think about it a bit more to be sure.
> thanx, i'm sorry for a cheap hack there but since i did not
> know any better i figured that what reviews are for right?!

Well, maybe initially ;-)

It looks like the solution isn't that easy so I'm not surprised you had problems. My main concern is that the address book code doesn't have enough of a direct link between directory searches/lookups (i.e. nsIAbDirectory, nsIAbLDAPDirectory) and the views (nsIAbView). Bug 135231 is a good example of this. At the moment the views don't know about asynchronus searches that are happening and I think that is wrong.

I think I've come up with a possible plan that I'll be discussing on irc (irc.mozilla.org on #maildev) sometime this week (evening UTC ;-) ), and I'll post it somewhere once I've discussed it.

Comment 10

[Anton Bobrov]

Attachment: new patch against HEAD

Comment 11

[Anton Bobrov]

Mark, new patch per yr suggestions. sorry it took sometime coz
i been quite busy lately. did you have a chance to figure out
lookup/view issue btw ?

Comment 12

[Mark Banner (:standard8)]

(In reply to comment #11)
> Mark, new patch per yr suggestions. sorry it took sometime coz
> i been quite busy lately. did you have a chance to figure out
> lookup/view issue btw ?
> 

Ok, I've just been able to spend some time looking at it, and I believe there may be a solution. I've added a brief commentary to bug 135231 comment 8. I've got a part patch for it, but I'll need a week or two to finialise it a bit more.

Comment 13

[Mark Banner (:standard8)]

*** Bug 337589 has been marked as a duplicate of this bug. ***

Comment 14

[Mark Banner (:standard8)]

*** Bug 338565 has been marked as a duplicate of this bug. ***

Comment 15

[Mark Banner (:standard8)]

I've just added a WIP patch on bug 135231 which will provide better linkage between directory searches and views. It still needs work, but gives the basic idea.

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 215875 [details] [diff] [review]
new patch against HEAD

Please don't review this patch (I know it wasn't assigned to anyone - I'd forgotten about it).

As per comment 12 this still needs some changes by bug 135231, and also uses a static nsCOMPtr<> which isn't allowed.

Comment 17

[Mark Banner (:standard8)]

Anton, do you mind if I take this patch and update and include it with one that I'm currently working on?

Comment 18

[Anton Bobrov]

Mark, please do, absolutely.

Comment 19

[Wayne Mery (:wsmwk)]

(In reply to comment #17)
> Anton, do you mind if I take this patch and update and include it with one that
> I'm currently working on?

Mark, is this addressed in another bug?

Comment 20

[:aceman]

So what is the state of this patch? Was it included in the other patch?

Comment 21

[Ludovic Hirlimann [:Usul]]

(In reply to :aceman (away 27.-2.) from comment #20)
> So what is the state of this patch? Was it included in the other patch?

Waiting for standard8's input

Comment 22

[Mark Banner (:standard8)]

Unfortunately I can't remember which patch I was intending on including this on. I think at this stage, enabling good UX for progress etc is something that we should be checking for in the new address book UI as we work on the modern address book feature.

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Noted.