65704 - offline status indicators not changing

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect, P2)

Description

[Alec Flett]

When you go offline/online by clicking on the offline/online indicator, only the
current window's offline indicator changes - all the other windows indicate the
old state.

I've got an idea for a fix, and it requires a slight addition to necko to
broadcast the offline state when it changes.

Basically, when the state changes, we use the nsIObserverService to send out a
notification that the offline state has changed. Then we implement an observer
in each window that updates the respective observer, which in turn updates the
UI. I'll implement this all in utilityOverlay.js

Comment 1

[Alec Flett]

ok, I have a fix ready in my tree... I can't attach a patch until the tree opens
(long story..)

Comment 2

[Blake Ross]

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

Comment 3

[Alec Flett]

Update on my patch, before I finally attach it: I've made the offline status
indicator into an XBL widget, the only trick is updating the menuitem in the
file menu to switch back and forth between Work Offline and Work Online. 

Comment 4

[Alec Flett]

moving key 0.8 bugs that didn't make it to 0.9
(yes, these are important enough to keep, yadda yadda)

Comment 5

[Paul Chen]

Sorry, don't see any traction, bumping off mozilla0.9 train, resetting target
milestone

Comment 6

[laurel]

So this is going to apply for all windows, both browser AND mail windows?  Just
making sure, just checking... or else we need to get a separate mail bug
targeted. It'd be great to have this for beta.

Comment 7

[Viswanath Ramachandran]

Don't think this is required for Navigator nsbeta1 or rtm. Putterman - does mail 
need this? 

Comment 8

[scottputterman]

Yeah, I don't think it makes sense to have an offline icon in the windows if it
doesn't reflect the correct state. cc'ing jpm in case he wants to take it for
his team.

Comment 9

[Hong Kwon]

This should be fixed for rtm, as it would be very confusing for users with 
multiple windows open.  Isn't there a patch for this?

Comment 10

[David :Bienvenu]

Mail needs this - it's a dogfood bug for offline users (never mind catfood). It
needs to be fixed for beta. I'm talking to Seth and Alec about solving this somehow.

Comment 11

[Alec Flett]

right, they should all be changing simultaneously, like they did in 4.x
but N6 went out in the current condition, I think we can survive beta without
it.. definite for rtm though

Comment 12

[laurel]

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

Comment 13

[David :Bienvenu]

Seth's going to help with this. Thanks, Seth!

Comment 14

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

I've got the fix for multiple browser windows, just need to fix some minor 
issues with some mailnews windows.

the fix is to utilityOverlay.js

when we get cycles, we can redo this as an XBL widget.

when I get this fixed, I'll log a new bug on the XBL widget.

Comment 15

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

Attachment: first draft of the fix.

Comment 16

[David :Bienvenu]

looks good to me, r or sr = bienvenu

Comment 17

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

before I check in, I'm going to polish up that fix a little more and try to get 
all the offline indicators playing nice.  (search, compose, mail.)

I hope to have something today.

Comment 18

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

(editor, history, addrbook, too.) 

Comment 19

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

so far so good.  mail and browser UI (indicator, tooltips, and menu item) do 
the right thing.  I'm fixing the mail .js so we observe the offline state and 
do the appropriate things when the online state changes.

this makes MsgToggleWorkOffline() in commandglue.js much simpler and easier to 
read since the UI elements are already handled by utilityOverlay.js

patch coming soon.  (bienvenu is going to like it, I tell you what.)

Comment 20

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

bienvenu:  now, if mail is up, and we go offline (or online) from any window, 
the proper UI will pop up.

rough draft of this patch coming soon...

Comment 21

[David :Bienvenu]

actually, I was hoping that clicking on the offline ui icon wouldn't bring up
any ui - only the menu would do that, the way 4.x worked. I wanted clicking the
icon to be the minimalistic way of going online/offline.

Comment 22

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

Attachment: second draft, still a work in progress.

Comment 23

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

working on:

1) removing the offline observer when a browser window goes away
2) making sure when new windows open, they get the correct offline state
3) fixing compose, addressbook, etc..

Comment 24

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

> actually, I was hoping that clicking on the offline ui icon wouldn't bring up
> any ui - only the menu would do that, the way 4.x worked. 
> I wanted clicking the icon to be the minimalistic way of going online/offline.

ok, I'll fix it.

Comment 25

[Alec Flett]

your 2nd draft looks like the wrong patch file, but the 1st draft is looking
good.. but please make sure the code wraps at 80 columns :)

Comment 26

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

I'm need to bite the bullet and finish alecf's XBL for the offline indicator.

there doesn't seem to be a way to do onload and onunload for the overlay.

Comment 27

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

Attachment: second draft, include some mailnews goop.

Comment 28

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

Attachment: wrong diff again.

Comment 29

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

maybe not, I think I can do this:

addEventListener("load",foo(),false);
addEventListener("unload",bar(),false);

Comment 30

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

> but please make sure the code wraps at 80 columns :)

oops, sorry.  I'll fix that.

Comment 31

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

patch coming.  please review.  I'll continue to work on the mailnews part of 
this in another bug.

Comment 32

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

Attachment: fix, please review.

Comment 33

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

Attachment: fix, please review.

Comment 34

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

Attachment: fix, please review.

Comment 35

[David :Bienvenu]

r=bienvenu, thanks, Seth!

Comment 36

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

slight revision, we don't need to call setOfflineStatus() anymore in 
navigator.js and mailWindow.js, as the load hander for the utility overlay
will set the correct state.

new patch on the way

Comment 37

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

Attachment: new patch

Comment 38

[Alec Flett]

looks good!
my only nit is that the IOService now has a contract id (maybe it didn't when
this was first written:
"@mozilla.org/network/io-service;1" (from nsNetCID.h)

Comment 39

[Alec Flett]

oh, and sr=alecf with that switch. Thanks for taking care of this seth

Comment 40

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

thanks for the info about the progid.  there are few other bugs in bugzilla
about switching to it, once it was added.

I'll make the fix and then land.  then, I'll go finish up all the mailnews
offline issues that I started.  (and make sure offline indicator is doing the
right thing in the rest of the tree.) 

Comment 41

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

Attachment: final draft.

Comment 42

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

anyone know why history has an offline indicator?

Comment 43

[Alec Flett]

I hate to say this, but there are a LOT of people bringing in utilityOverlay.xul
- could you fix the onload handler to make sure there is an offline widget, and
bail early if there isn't one? should just be a one line addition..

http://lxr.mozilla.org/seamonkey/search?string=utilityOverlay.xul

Comment 44

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

good catch.  I'll work on that now.

Comment 45

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

Attachment: director's cut.

Comment 46

[Alec Flett]

I like it.
sr=alecf

Comment 47

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

new version of the patch coming.  comments (and remaining issues) to come next.

Comment 48

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

Attachment: new patch

Comment 49

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

the last patch gets browser, compose, 3 pane, stand alone msg window all doing 
the right thing.  when the offline state changes, all those windows will update.
when going offline in 3 pane or std alone msg window, the user gets prompted 
before going offline and can cancel the offline state chagne.

comments / issues

1)  utitlityOverlay is getting loaded multiple times.  four times for the 3 
pane, and twice for the std alone msg window.  that causes me to add the same 
observer multiple times.  when the window goes away, we only remove the 
observer once.  so if the offline state changes after the window goes away, we 
assert.

I added a call to remove the current observer, before adding it.  that prevents 
the assertion because before we add the observer, we remove the previous one.
I could have written it so I enumerated over the observers, and didn't
add again if a duplicate was found.  but this was simpler.  of course, I need 
to figure out why this is happening to those two mail windows.  

[this multiple overlay problem might indication of a performance problem?]

2)  mail has a special requirement.  when the user tries to change their 
offline state, we want to jump in and ask them if they want to do some work 
before changing state.  they can also cancel (and decide not to change their 
offline state.)  I extended utitlityOverlay.js to look for the "offline-status" 
element, and get the "checkfunc" attribute.  if that succeeded, we eval() that 
func and if that returns false, we bail on the offline state change.  otherwise 
we proceed.  right now, only the 3 pane and stand alone message window have 
this behavior.  if you change state in other windows, you will not run the 
mail "pre-offline" state change func.  any suggestions on another way to do 
this?  

3)  I was able to remove the onfocus hander for msg compose, and clean up the 
offline handling.  but, to get the utitlityOverlay to work I needed to have 
something observe the "Communicator:WorkMode" broadcaster.  in nav, 3 pane, and 
std alone msg window, we have a "File | Go Offline..." menuitem.  msg compose 
doesn't have this, so I added it, but made it hidden by default.
I need to figure out a no hacky way to get the overlay to overlay without 
needing a menuitem like that.  I think the statusbar overlay would be enough, 
but I'm too tired to figure that out right now.

4)  history, search and subscribe offline indicators are not working properly 
yet.  for similar reasons to #3.  if there are more windows that have offline 
indicators (status bars), we'll need to fix them

5)  we're not using it, but I've left in the observer for the mail windows..
If there is any UI that needs to be disabled when offline, that's the place to 
do this.  if none of the mail window UI needs to change, we can remove the 
observer code.

6)  when the msg compose window comes up (in offline mode) the send button 
doesn't say "Send Later".  It will change to that if you change state after the 
window is open.
  
all that said, I think it's worth landing this before addressing all the 
issues.  we are better off than before, and this will make it better in the 
common case.  [and the offline UI owner can address the other issues]

Comment 50

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

whoops, I need to fix something minor with msg compose, new patch coming soon.

Comment 51

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

moving to 0.9.1

Comment 52

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

Attachment: patch

Comment 53

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

new patch coming that fixes #6:

> 6)  when the msg compose window comes up (in offline mode) the send button 
> doesn't say "Send Later".  It will change to that if you change state after 
> the window is open.
  

Comment 54

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

Attachment: latest version of the patch, include "send later" fix.

Comment 55

[David :Bienvenu]

I agree that it's worth landing this - it will be much better than what we have now.

Comment 56

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

new patch on the way.  it will be up to date with the trunk.

Comment 57

[David :Bienvenu]

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

Comment 58

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

Attachment: up to date fix.

Comment 59

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

darn, recent changes on trunk will require more updates.  new patch coming...

Comment 60

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

Attachment: revised fix.

Comment 61

[Ben Goodger (use ben at mozilla dot org for email)]

sr=[e-mail]. 

Comment 62

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

fix landed.  now to go log bugs on the open issues.  this should help users 
(and qa) with offline.

Comment 63

[Alberto J. Rodriguez]

Wonderful team work.
Please take a look at bug 79952 , which has operational characteristics similar 
to the one you have solved.

Comment 64

[grylchan]

looks like one more bug got assigned to me..

Looks very similar to bug 78920

Not sure which build is the official 'beta build'. I tested with this build:
2001-06-07-13-0.9.1/ - win nt 4.0
2001-06-07-03-0.9.1/ - mac os 9.0.4
2001-06-07-13-0.9.1/ -linux 2.2, red hat 7.0

Verifying that clicking the offline indicator in one of these windows results in
the other windows displaying the same state:
a.Browser 
b.Composer/Editor page 
c.Mail/news compose window 
d.Messenger window 
e.Separate message window 
f.Separate newsgroup window
g.search messages window
h.address book window
i.history window
j.subscribe to newsgroups window

It works as expected in all windows except for history, search messaages, and
subscribe to newsgroup window (as the indicators don't work at all which is a 
known bug 80970)

No need to test trunk builds as this fix landed before we began branching.

Marking as Verified.