143708 - Can't switch online/offline mode from MailNews anymore

Status: VERIFIED FIXED

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

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

Linux 2002051108 and on current CVS...

The little plugging wires icon to switch online/offline mode in the lower right
corner of MailNews doesn't work anymore.

However it does still work from within the Browser.

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

Caused by checkin of bug 102517.

Comment 2

[Mike Gebis]

I see this on W2K 2002051104.  Additionally, I can't even change back to
online from the menu in the MailNews window.  (But as the original reporter
mentioned, using a browser window works either with the icon or the menu.)

Someone so empowered should change the OS to "ALL".

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

great. I always like functions who behave contrary do documentation.

ok, the problem is: in mailnews/base/resources/content/mail-offline.js, these
two functions are called:
gOfflineManager.synchronizeForOffline and
gOfflineManager.goOnline

Now, these functions already change the offline state themselves. So if I return
true, line 74 of utilityOverlay.js is reached:
  ioService.offline = !ioService.offline;

Which changes offline state again, and therefor the state is not changed at all.

Comment 4

[Christopher Aillon (sabbatical, not receiving bugmail)]

Ok, on a hunch I had biesi look into this thinking that the native mail
functions changed state, then we we returned true, we reversed the online state
to back where we started from!

So, as partial penalty to him for not testing the patch to 102517, I'm assigning
to him.  :-)

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

this patch does two things:
in mail-offline.js, calls synchronizeForOffline to be called with
goOfflineWhenDone set to false; and it adds an argument to goOnline
(goOnlineWhenDone) (and fixes all callers, using true everywhere except in
mail-offline.js).

Comment 6

[Håkan Waara]

Comment on attachment 83261 [details] [diff] [review]
patch

r=hwaara

Let's get this in ASAP.

Comment 7

[Alec Flett]

Comment on attachment 83261 [details] [diff] [review]
patch

the patch looks reasonable to me. Sorry if I sr'ed a bad warnings patch. I'd
like to see an r= from the guy who made the original warnings patch, to confirm
he knows what he busted :)

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Alecf, I already condemned the guy who made the warnings patch to fix this bug
in comment #4.  He's the one that wrote this patch, too.  :-)

Comment 9

[R.K.Aa.]

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

Comment 10

[grylchan]

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

Comment 11

[David :Bienvenu]

It seems to me like it would be much cleaner and straightforward to have these
functions return false so utilityOverlay.js wouldn't diddle the offline state.
goOnline SHOULD go online. It also seems like the first patch should be backed
out, and the warning fixed correctly, instead of trying to recover from the
first patch by changing all this code. Is there a reason we're not trying that
instead?

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

>It seems to me like it would be much cleaner and straightforward to have these
>functions return false so utilityOverlay.js wouldn't diddle the offline state.

that would be possible too... not sure if that's cleaner; definitely affects
less code, though.

>goOnline SHOULD go online.

ok, if you say that; so in this case it probably is the best to simply return false

> It also seems like the first patch should be backed
>out, and the warning fixed correctly

Well, I tried directly fixing it correctly; since I need r= and sr= anyway, I
thought I could fix it directly, instead of backing out and fixing afterwards.

I'll attach a patch to return false in a second

Comment 13

[Christopher Aillon (sabbatical, not receiving bugmail)]

> It seems to me like it would be much cleaner and straightforward to have these
functions return false

It seems odd to have a function always return false.  If we go this route, just
have it |return;| in all such places and remove the comment which says to return
false if we fail.

Please add a comment at the start of the function which says that the function
always goes online or offline.

Still better, do we have any other functions that would be called by the eval in
utilityOverlay.js?  Should we just let all eval'd functions there handle their
own online/offline state, and not even worry about what the eval's return value
is?  I think that would be the optimal solution...

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch to change return true->return false

ok, can I get r= and sr= for this patch?

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

>It seems odd to have a function always return false.  If we go this route, just
>have it |return;| in all such places and remove the comment which says to return
>false if we fail.

well no... that sounds bad... since the return value is used, we should return
false, imho

btw, I forgot to remove the comment at the top of the function, I'll do that
before checking in

Comment 16

[David :Bienvenu]

> ok, if you say that; so in this case it probably is the best to simply return
false
I was agreeing with you that the method should do what its name promises :-)

>It seems odd to have a function always return false

this function returns false so that toggleOfflineStatus in utilityOverlay.js
won't toggle the offline state, because we're taking care of it! We're not the
only checkFunc possible - if you look at that code, it might make more sense why
we need to return false. It's not a success or failure code; it's an indication
to toggleOfflineStatus that it should NOT change the offline state.

Comment 17

[David :Bienvenu]

thank you, that's much better. I'm sorry I didn't get involved in the earlier
warning fix, but I wasn't aware of the semantics of this return value so I
didn't know how much mischief could be caused by changing it. What we need is a
comment at the beginning of this routine saying that the return value (true or
false) controls whether the caller will toggle the offline state. Could you
remove the old comment and add a new comment, and then I'll sr that? thx.

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: change the comment

Comment 19

[David :Bienvenu]

Comment on attachment 83602 [details] [diff] [review]
change the comment

sr=bienvenu

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

ok, can somebody r= this patch? hwaara or sspitzer maybe?

Comment 21

[Navin Gupta]

Comment on attachment 83602 [details] [diff] [review]
change the comment

r=naving

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

checked in

Comment 23

[Christopher Aillon (sabbatical, not receiving bugmail)]

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

Comment 24

[Frederic Bezies]

Cannot see this bug anymore with build 2002051504 - WinXP. Thanks !

Comment 25

[Rene ALBERT]

I work to day with 20020515 (Win95 - talkback/zip)
The situation is better, but not satisfactory.

If you click dircetly on the 2-plug icon, OK, now it turns off-line.
But if you click before elsewhere (newsgroup, or mail), it becomes impossible
now to turn off-line (no result when clicking on the icon).

At this moment it is necessary to lauch the remote connexion (RTC here), which
paradoxically turns now the icon off-line !!!

Comment 26

[David :Bienvenu]

do you have it configured to automatically download messages for offline use
(Edit | Preferences | Offline & Disk Space)? Are you physically connected to the
network or not when you click the offline button?

Comment 27

[grylchan]

Using
commercial trunk on linux 2.2
 Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.0.0+) Gecko/20020516 
mozilla trunk on mac os 10.1.4
 Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.0.0+) Gecko/20020516

This initially looks fixed. Clicking offline icon in (browser,
messenger, composer, addbook, stand alone mail mesg) results in going
offline, icon dissconnected. Clicking again results in going online.

if you click the offline icon in messenger/seperate mail window, it will result
in the download mesg prompt. And when I said download at the prompt,
both my mail folders/newsgroups were downloaded. 

Comment 28

[grylchan]

Using commercial trunk

2002-05-24-08-trunk nt 4.0
2002-05-24-07-trun linux 2.2
2002-05-24-03-trunk mac 9.2.2
2002-05-24-08-trunk mac 10.1.4

Verified clicking icon in any of the windows (browser,
messenger,addbook,composer, stand alone mesg) results in
icon being disconnected/connected. And clicking the icon
in stand alone mail/news mesg or messenger results in the
mail download prompt. Also tested going offline via file
menus if all windows and the icon matches the state of
switching on/offline in file menu

marking as verified.

Comment 29

[R.K.Aa.]

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