Can't switch online/offline mode from MailNews anymore

VERIFIED FIXED in mozilla1.1alpha

Status

SeaMonkey
MailNews: Message Display
P1
normal
VERIFIED FIXED
16 years ago
14 years ago

People

(Reporter: Christopher Aillon (sabbatical, not receiving bugmail), Assigned: Biesinger)

Tracking

({regression})

Trunk
mozilla1.1alpha
x86
All
regression

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Caused by checkin of bug 102517.
Keywords: regression

Comment 2

16 years ago
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".

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.
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.  :-)
Assignee: sspitzer → cbiesinger
OS: Linux → All
Created attachment 83261 [details] [diff] [review]
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

16 years ago
Comment on attachment 83261 [details] [diff] [review]
patch

r=hwaara

Let's get this in ASAP.
Attachment #83261 - Flags: review+

Comment 7

16 years ago
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 :)
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

16 years ago
*** Bug 144304 has been marked as a duplicate of this bug. ***
Priority: -- → P1
Target Milestone: --- → mozilla1.1alpha

Comment 10

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

Updated

16 years ago
QA Contact: olgam → gchan

Comment 11

16 years ago
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?
>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
> 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...
Created attachment 83597 [details] [diff] [review]
patch to change return true->return false

ok, can I get r= and sr= for this patch?
Attachment #83261 - Attachment is obsolete: true
>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

16 years ago
> 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

16 years ago
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.
Created attachment 83602 [details] [diff] [review]
change the comment
Attachment #83597 - Attachment is obsolete: true

Comment 19

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

sr=bienvenu
Attachment #83602 - Flags: superreview+
ok, can somebody r= this patch? hwaara or sspitzer maybe?
Status: NEW → ASSIGNED

Updated

16 years ago
Attachment #83602 - Flags: review+

Comment 21

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

r=naving
checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
*** Bug 144742 has been marked as a duplicate of this bug. ***

Comment 24

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

Comment 25

16 years ago
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

16 years ago
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

16 years ago
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

16 years ago
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.
Status: RESOLVED → VERIFIED

Comment 29

16 years ago
*** Bug 148421 has been marked as a duplicate of this bug. ***
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.