79865 - Offline: Get Msgs when offline doesn't display alert dialog.

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Backend, defect, P2)

Description

[laurel]

Using may09 commercial build
UI spec: http://www.mozilla.org/mailnews/specs/offline/

Getting new messages when in offline state doesn't display any alert dialog,
doesn't do anything. It should display a dialog "You are currently offline.
Would you like to go online to get your new messages?"

Comment 1

[David :Bienvenu]

this has to be done in js front end - I guess I'll be doing it, however.

Comment 2

[Mohan Bhamidipati]

Assigning myself. -mohan.

Comment 3

[Mohan Bhamidipati]

Attachment: proposed patch : (diff -uw + new files)

Comment 4

[Jussi-Pekka Mantere]

PDT+ per 6/12 mtg.

Comment 5

[Mohan Bhamidipati]

cc'ing bhuvan for review & sspitzer for super review;

Comment 6

[racham]

It will certainly be better to use nsIPromptService. That patch will be much 
simpler too.

BTW, please post screenshots of the popups/alerts. Have they been spec'ed out 
somewhere..If so, please do include URL to that place. If not, then those alert 
dialogs and text should be approved by Jennifer/Robin.

bhuvan 

Comment 7

[Mohan Bhamidipati]

Attachment: Latets Patch : included nsIPromptService

Comment 8

[Mohan Bhamidipati]

Attachment: screen shot for UI ( FOUR dialogs )

Comment 9

[Mohan Bhamidipati]

This patch covers usage of nsIPromptService for the following :

a) While going Online : "Send Unsent Messages" 
b) While going ofline : "Download Messages" 
c) File-> "Get Messages" : when offline 
d) File-> "Send Unsend Messages" : when offline 

Requesting Bhuvan for review and Seth for super review;

Comment 10

[jglick]

Increasing the space between the text message and the checkbox widget, and the 
checkbox widget and the buttons at the bottom like you've done in similar 
dialogs would be nice.  As well as wrapping the text message to the next line to 
keep the dialog from getting too big horizontally would be nice.

Comment 11

[Mohan Bhamidipati]

adding hwaara for review comments; 
hwaara, attached patch covers "nsIPromptService" for four dialogs;

Comment 12

[Håkan Waara]

Can you attach a patch with the change jglick requests and where you change the
"Yes" and "No" buttons to be "Cancel" and "Go Online"? 

(please remember to never use Yes and No buttons. If you really want to know why
(it's a FAQ :) ) email mpt.

Comment 13

[Mohan Bhamidipati]

Attachment: new screen shot v1.0

Comment 14

[Mohan Bhamidipati]

Attachment: screen shot - only new ones : v1.1

Comment 15

[Mohan Bhamidipati]

Attachment: latest patch v2.0

Comment 16

[Håkan Waara]

Mohan, I read through your patch and have lots of comments -- however, you
included a fix to remove sendMsgs and downloadMsgs which is a different bug. It
makes it hard to review if you merge all your patches like that.

Can you please post a separate patch with only the code additons/removal that is
needed to fix /this/ bug?

Thanks

Comment 17

[Håkan Waara]

If you intend to fix all these bugs at once, I don't think it's appropriate for
0.9.2 trunk since it can easily introduce regressions.

Mega comment coming soon with comments for this patch.

Comment 18

[Håkan Waara]

Here comes the first batch of review comments. Enjoy!

+function promptSendMessages()

Please be consistent in how you name your functions. I think you should (as you
did with CheckForUnsentMsgs() ) have it capitalized. PromptSendMessages().
Remember to fix any usages of this name elsewhere to use the same name.

On a related note, the other function is CheckForUnsentMsgs(). That's also
consistent: Msgs -- Messages. Please use the full word there (and replace any
usages...)

+ if(CheckForUnsentMsgs())
offlineManager.goOnline(true, true, msgWindow);
+ else
+ offlineManager.goOnline(false, true, msgWindow);

How about something like this instead:
offlineManager.goOnline(CheckForUnsentMsgs(), true, msgWindow);

+function CheckForUnsentMsgs()
+{
+  return true;

You didn't mean to do this, did you? This will break CheckForUnsentMsgs() and
make it so it never works!

+  var msgFolder = new Object();
I think doing var msgFolder = { }; is a more clear way to do this. However, it's
optional whether you want to change it.

+ (promptService.BUTTON_TITLE_IS_STRING * promptService.BUTTON_POS_0) +
+ gOfflinePromptsBundle.getString('sendMessagesCancelButtonLabel'), 
Don't fetch a custom cancel string like this. BUTTON_TITLE_IS_STRING is only to
be used when you want to specify your own string -- when there are no predefined
strings available for your purpose. 

Look in nsIPromptService.idl and you will find some default strings you can use
(like OK, Cancel, Don't Save etc.). Please use that in all of your prompt
functions. (This is true generally too, so please also do it in the future.)

+ if(checkValue.value) gPrefs.SetIntPref("offline.send.unsent_messages", 0);
+ else gPrefs.SetIntPref("offline.send.unsent_messages", 1);

With some indenting, these two lines are hard to read since they wrap and all
that. Can you please use the (more standard) form

if blah
  blah()
else
  blah()

?

+ gOfflinePromptsBundle.getString('downloadMessagesYesButtonLabel'),
+ gOfflinePromptsBundle.getString('downloadMessagesCancelButtonLabel'), 
+ gOfflinePromptsBundle.getString('downloadMessagesNoButtonLabel'), 

Ok, this comment also applies to all your prompt calling functions. Please
rename these bundle's string names to match their content. If the string
downloadMessagesYesButtonLabel indeed is not "Yes", developers who read this
code will get confused.

-    if(folder) {
-        SendUnsentMessages(folder);
+    if(folder) SendUnsentMessages(folder); 

Not good. See my comment about if() clauses above.

+function CheckOnline()
+{
+  var ioService =
nsJSComponentManager.getServiceByID("{9ac9e770-18bc-11d3-9337-00104ba0fd40}",
"nsIIOService");
+  if(ioService.offline) return false;
+  else return true;	
+}

The last if clause can be optimized to be:

return ioService.offline;

Index: messageWindow.xul

It's something in strange in your patch. The patch to messageWindow.xul appears
several times. Hmm...

downloadMessagesLabel=Do you want to download messages \nfor offline use before
you go offline?\n\n

Remove all \n you have inserted. These are not meant to be inserted by hand. The
dialog will appear in various sizes depending on the system its being viewed in,
so don't "hard-wrap" like this unless you're making a new paragraph.

BAD usage: Hey\n thereGOOD usage: Hey there. \n\n Bye
Fix this for all new strings you are adding!
Other things that are important:

* Remove all tabs you have (accidently?) inserted. They are not welcome in our
source files
* Get jglick or mpt to look through the wordings and optimize them. We want as
clear and concise wordings as possible. Right?
* I think you should try and share code between your prompt functions. They are
very similar looking many of them. Maybe the part that initializes a
nsIPromptService can be made a little helper function, and then the part that
actually calls confirmEx() can be left as-is?

Comment 19

[Mohan Bhamidipati]

Attachment: new patch : v2.1

Comment 20

[Mohan Bhamidipati]

--------------
1.
downloadMessagesLabel=Do you want to download messages \nfor offline use before
you go offline?\n\n

Remove all \n you have inserted. These are not meant to be inserted by hand. The
dialog will appear in various sizes depending on the system its being viewed in,
so don't "hard-wrap" like this unless you're making a new paragraph.

-- '\n's are added to create extra space needed between 'label' & 'check box'
-- also for splitting into two lines
-- & to make the prompt look small.
--------------
2. 
Would remove multiple diff's for messageWindow.xul in the next patch.
--------------
3.
if we optimize PromptDownloadMessages() etc. for common code it may be little 
confusing. Please let me know if I should go ahead and optimise it to add a 
helper function;
-------------
4.
used 'downloadMessagesCancelButtonLabel' etc. for consistency.
-------------

thanks for the review (initial); 

Comment 21

[Mohan Bhamidipati]

would like to do all cleanup and minor changes related to this by opening 
another bug for m0.9.3 and want add this functionality to the tree first, if it 
is ok with you. 

Comment 22

[Håkan Waara]

I don't think you should - considering the large amount of code being added to a
central piece of mailnews - risk regressions so close to mozilla0.9.2 release.
This is very good work from you Mohan, but at the same time it's a risky checkin.

I'll re-review the new patch you added now...

Comment 23

[Håkan Waara]

To answer your questions/answers:

1. "Splitting into two lines" is ok, assuming you mean to make a new paragraph.
But don't hard-wrap like that just to adjust the dialogs' sizes, it's a hack.

3. Ok, confusing code is never good. How about a InitPrompts() function and a
global nsIPromptService variable (together with the existing
gOfflinePromptsBundle variable), so you don't need to reload for that each time?
The InitPrompts() would probably check if the global was null and init it, if it
was...

4. The name of the string is still not consistent if it doesn't reflect its content.

And onto the re-review. I noticed that you ignored some of my
questions/requests. Which forces me to re-add them here. :(

+function CheckOnline()

Can you please optimize this function as I suggested in my earlier review? Or do
you have any objection?

switch(buttonPressed.value) {

No reason to have a switch-case if there are only two values to check. Please
use if() instead here as usual. This applies to all your prompt functions.

sendMessagesCancelButtonLabel=Cancel
getMessagesOfflineCancelButtonLabel=Cancel
sendMessagesOfflineCancelButtonLabel=Cancel

Can you use the predefined Cancel string (available in nsIPromptService.idl), or
is there any reason you ignored this nit of mine from the last review?

+ var identitiesCount;
+ var allIdentities;
+ var currentIdentity;
+ var numMessages;

... would be more plain as |var identitiesCount, allIdentities, currentIdentity,
numMessages;|

+function PromptSendMessages()

You're still adding tabs, as I mentioned yesterday. Many of them being in this
new function but there are many of them in other functions as well. Please
remove all new tabs you've added.

--- New file : msgOfflinePrompts.properties ---

We have filter.properties, messenger.properties, subscribe.properties (and
similar). I think it would be fair to have offline.properties also.

Comment 24

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

If you make this change to nsIMsgSendLater, you can simplify 
CheckForUnsentMessages() 

Index: public/nsIMsgSendLater.idl
===================================================================
RCS file: /cvsroot/mozilla/mailnews/compose/public/nsIMsgSendLater.idl,v
retrieving revision 1.9
diff -u -w -r1.9 nsIMsgSendLater.idl
--- nsIMsgSendLater.idl 2001/03/17 01:58:14     1.9
+++ nsIMsgSendLater.idl 2001/06/22 20:41:42
@@ -40,5 +40,5 @@
   attribute nsIMsgWindow msgWindow;
   void      RemoveListener(in nsIMsgSendLaterListener aListener);
   void      AddListener(in  nsIMsgSendLaterListener aListener);
-  void      GetUnsentMessagesFolder(in nsIMsgIdentity userIdentity, out nsIMsgF
older folder);
+  nsIMsgFolder getUnsentMessagesFolder(in nsIMsgIdentity userIdentity);
 };

that will generate the same thing (for C++) but your JS can be simpler.

Comment 25

[Mohan Bhamidipati]

------------------------
I guess there is some problem with nsIPromptService, when used with 3 buttons, 
especially when custom strings are combined with standard button values as 
defined in nsIPromptService.idl, I have observed atleast one problem :
only 2 buttons were getting displayed.

So keeping "custom cancel strings" as they are.
 for example : sendMessagesCancelButtonLabel=Cancel
I guess this is ok.
------------------------
"Splitting into two lines" is ok, assuming you mean to make a new paragraph.
But don't hard-wrap like that just to adjust the dialogs' sizes, it's a hack.

we need this for now until there is a provision in nsIPromptService to accept 
new paragraphs.
------------------------
All other changes suggested by hwaara are part of this new patch now, let me 
know if they are missing;
------------------------
added seth's modification for nsIMsgSendLater.idl and changed the call in 
commandglue.js to include hwaara's suggestion about nsIMsgFolder (new object); 
out parameter of nsIMsgSendLater.
------------------------

adding the patch soon;

Comment 26

[Mohan Bhamidipati]

Attachment: new patch : included hwaara 's suggestions

Comment 27

[Mohan Bhamidipati]

requesting hwaara & bhuvan for r & seth for sr.

Comment 28

[Håkan Waara]

I will look into this. Just give me a second

Comment 29

[Håkan Waara]

Review of latest patch:

+ (gPromptService.BUTTON_TITLE_CANCEL * gPromptService.BUTTON_POS_1),

Earlier you wrote that it didn't work, nonetheless, you still use it?? This is
in all of your prompt functions.

\ No newline at end of file

Fix this.

+startupMode.dtd
[...]
+      .\startupMode.dtd \
[...]
+startupMode.xul
+startupMode.js
(and more)

What is this? If it's a part from your other patch - then you must land this
together with that. Otherwise stuff will break.

# Send Mesages Prompt
[...]
# Send Mesages Offline Prompt

Fix spelling.

Not much left till you get a r= from me! :)

Comment 30

[Mohan Bhamidipati]

Attachment: Latest Patch : with hwaara's suggestions.

Comment 31

[Mohan Bhamidipati]

--------------
1. cancel button string had problem when 3 buttons are used.
   when 2 buttons are used, I found it to be ok.
2. fixed "no newline"
3. "startupMode.*" : yes, I will land that also along with this.
4. fixed spelling.
--------------

Comment 32

[Matthew T (active 1999-2002)]

Unfortunately, there are a number of UI problems here.

     (?) Do you want something that looks misleadingly like help? ( Yes ) ( No )

The beginning of the snowball is that you use the question mark icon for these
alerts. Please don't. It's never existed on Mac OS (and Mozilla's one looks
awfully alien there), and Microsoft deprecated its use on Windows years ago. If
you are asking the user a question, you should use the exclamation mark icon, or
not use an alert at all. (There's a bug on removing the question mark icon from
Mozilla completely.)

                                     /!\ Are you sure you want to show an alert?

Now, using the exclamation mark icon implicitly means that you are only supposed
to use an alert when the user is in danger of some sort of loss -- loss of data,
loss of money, loss of time, loss of face, whatever. Checking with the user
before going online falls into that category, since it will usually involve the
time- and/or money-consuming establishment of an Internet connection.

                                                   [/] Always annoy me immensely

However, downloading new messages, or sending unsent messages, are not risky
activities, and as such a confirmation alert is completely inappropriate. (Yes,
I know 4.x had alerts for these cases, but that doesn't make them right.) There
is already a perfectly good way of going online and then sending unsent messages
(the `Get New Messages' command), or downloading new messages and then going
offline (the `Download/Sync Now' command), so adding alerts for the basic `Work
Online'/`Work Offline' commands to do exactly the same thing is just annoying.
(Yes, I know they have checkboxes to turn them off, but that *still* doesn't
make them right.)

If the `Download/Sync Now' menu item isn't obvious enough (it probably needs
merging with `Get Msg [sic]' so as to have its own toolbar button), then file a
separate bug for it. Please don't just spray alert boxes all over the place to
make up for UI deficiencies elsewhere.

Comment 33

[Håkan Waara]

File two bugs:

* One on the issue that one button disappear if you use the predefined cancel
string (include some sample code there).
* And one on yourself to make these prompts use the predefined string once you
understand how it is done, or someone fixes the nsIPrompt bug.

Then: test your fix thoroughly! I've noticed that some of your fixes included
code that would make your fix not work if you run it, which leads me to think
that you  are not (always?) testing the patches you produce.

Please do this to avoid regressions that you or someone else will have to clean
up after.

when you have done all of the above: r=hwaara.  Good work.

Comment 34

[Mohan Bhamidipati]

filed a bug : 87688 for button disappeared using nsIPromptService.
filed a bug : 87692 for using "pre defined cancel button".
tested the patch;
--requesting seth for sr;

Comment 35

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

1)  CheckOnline()

instead of using the ioservice CID (9ac9e770-18bc-11d3-9337-00104ba0fd40)

use the contract id:

"@mozilla.org/network/io-service;1";


2)  in mail3PaneWindowCommands.js, we have

1127 function CheckOnline()
1128 {
1129   var ioService =
nsJSComponentManager.getServiceByID("{9ac9e770-18bc-11d3-9337-00104ba0fd40}",
"nsIIOService");
1130   if(ioService.offline) return false;
1131   else return true;
1132         
1133 }

let's not have this code in two places.  please add your implementation to a
proper place (maybe commandglue.js, try and see if that works) and make it so we
only implement CheckOnline() once.

3)  PromptGetMessagesOffline()

why does PromptGetMessagesOffline() have a return value?  it doesn't look like
it should have one.

4) localization notes:

in offline.properties, I think you'll need to add a note not to localize "\n"
It's a little tricker than that.  you really want the localizer to use newlines
to make
the text fit nicely in the dialog.

5)  remember to "cvs remove" the files you removed from the tree.

Comment 36

[Håkan Waara]

(... and cvs add the new ones.)

Comment 37

[Mohan Bhamidipati]

Attachment: new patch : with seth's suggestions.

Comment 38

[Mohan Bhamidipati]

Would like to check in before UI freeze;

Comment 39

[Mohan Bhamidipati]

1. CheckOnline() function is in commandglue.js;
   ioServiceId replaced with contractId
2. removed from CheckOnline() from mail3paneWindowCommands.js
3. PromptGetMessagesOffline() : removed return value;
4. added localization notes for offline.properties;
5. removed files : a) downloadMsgs.js  b) downloadMsgs.xul c) sendMsgs.js d) 
sendMsgs.xul
6. added files : a) startupMode.js b) startupMode.xul c) offline.properties

Comment 40

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

I don't know if it is considered good practice to have a localization note like
that at the top of the file that applies to several strings.  I'm used to seeing
a note above every line that requires it.

instead, please add above every place it is appropriate.

+#   LOCALIZATION NOTE :
+#   do not localize "\n".  use "\n" to make the text fit nicely in the dialog.

> 2. removed from CheckOnline() from mail3paneWindowCommands.js

I didn't see that in the patch, please include that as part of your diff.

Also, there is more duplicated code we can combine.

these two blocks of code appear twice:

a)

+    var folders = GetSelectedMsgFolders();
+    var compositeDataSource = GetCompositeDataSource("GetNewMessages");
+    GetNewMessages(folders, compositeDataSource);

b)

+    var folder = GetFirstSelectedMsgFolder();
+    if(folder) 
+       SendUnsentMessages(folder); 

put those blocks in seperate functions, please.

(I'm glad the nsIMsgSendLater change worked for you, that makes your .js much
cleaner.)

Comment 41

[Mohan Bhamidipati]

1. localization note added before every line that needed it;
2. removed CheckOnline() : mail3paneWindowCommands.js : added diff's to the 
patch.
3. added two functions GetFolderMessages() & SendFolderUnsentMessages()
   changed the blocks and their calls;
patch follows;

Comment 42

[Mohan Bhamidipati]

Attachment: patch with suggested modifications

Comment 43

[Mohan Bhamidipati]

requesting seth for sr=;

Comment 44

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

two comments:

1)  you have:

+function MsgGetMessage()
+{
+  if(CheckOnline())
+    GetFolderMessages();
+  else 
+    PromptGetMessagesOffline();
+} 

+function GetFolderMessages()
+{
+  var folders = GetSelectedMsgFolders();
+  var compositeDataSource = GetCompositeDataSource("GetNewMessages");
+  GetNewMessages(folders, compositeDataSource);
+}

That code assumes that all calls go through MsgGetMessage().  they don't.  For 
example, see MsgGetMessagesForAccount().

2)  don't forget about news.  news has MsgGetNextNMessages() / GetNextNMessages
().

3)  don't forget about MsgGetMessagesForAllAuthenticatedAccounts()

4) [optional]

+function SendFolderUnsentMessages()
+{
+  var folder = GetFirstSelectedMsgFolder();
+  if(folder) 
+    SendUnsentMessages(folder); 
+}

this is not a new problem, but you implementation of CheckForUnsentMessages() 
pointed it out.  In your implementation of CheckForUnsentMessages() you check 
all the identities, but SendFolderUnsentMessages() only uses the identity of 
the first selected folder.  (keep in mind, not all account have identities.  
for example, "Local Folders").  we need to fix SendUnsentMessages() to be more 
like CheckForUnsentMessages(), and iterate over all the unsent msg folders, 
ignoring the current selected folder.  the only time we care about the selected 
folder would be if the user right clicks on a particular "Unsent Messages" 
folder and does "Send Unsent Messages".

something to note, the back end has it hard coded right so that for all 
identities we use the same unsent messages folder.  (it is pointed to by the 
pref "mail.default_sendlater_uri").  but that is just how it is currently 
implemented.  we should not write code that assumes that is how the back end 
works, as it may change.

#4 is optional, as your fixes make it no less broken than it already is.  I'll 
log a bug to clean this up, but if you feel abitious, you can clean it up in 
one fell swoop.

cc'ing racham.  he should also review.

Comment 45

[Mohan Bhamidipati]

-----------------
Modified the following functions : 
 a) PromptGetMessagesOffline()
 b) PromptSendMessagesOffline()
to return the button selected; 
and made it generic so that these prompts can be called from other get/send
functions;

added new functions :
 a) GetFolderMessages()
 b) GetMessagesForAllAuthenticatedAccounts()
 c) GetMessagesForAccount(aEvent)

modified the following functions :
 a) MsgGetMessage()
 b) MsgGetMessagesForAllAuthenticatedAccounts()
 c) MsgGetNextNMessages()
 d) MsgSendUnsentMsg()

checked MsgGetNextNMessages()/GetNextNMessages for news;

modified the function :
 a) SendFolderUnsentMessages() : 
    (1) to  change existing "SendUnsentMessages" for the Identity 
        of a Selected folder";  
    (2) and to iterate through all identiites and checking
        SendUnsentMessagesFolders similar to "CheckForUnsentMessages()"

posting the patch soon;
---------

Comment 46

[Mohan Bhamidipati]

Attachment: new patch with suggested changes

Comment 47

[Mohan Bhamidipati]

requesting bhuvan for review & seth for super review;

Comment 48

[Asa Dotzler [:asa]]

pushing out. 0.9.2 is done. (querying for this string will get you the list of
the 0.9.2 bugs I moved to 0.9.3)

Comment 49

[racham]

Here are some comments :

1. There is one more entry point to GetMessagesForInboxOnServer from
AccountCentral. Take care of the following case with online check.
http://lxr.mozilla.org/seamonkey/source/mailnews/base/resources/content/mailWindow.js#502

2. commandglue.js :
Please add some comments above statements like these (at least in one place).
Input boolean params (false, true) do not convey anything unless there are
supported by some comments.

+        offlineManager.goOnline(false, true, msgWindow);
+        numMessages = msgFolder.getTotalMessages(false);

3. Optimize services calls too (like InitPrompts())
var gOfflineManager;
InitServices()
{
  if (!gPrefs) {
    GetPrefsService();
  }
  if (!gOfflineManager) {
    gOfflineManager =
Components.classes["@mozilla.org/messenger/offline-manager;1"]                 
        .getService(Components.interfaces.nsIMsgOfflineManager);
  }
}

4. mailwindowoverlay.js :

This is done several times. 
+      var offlineManager =
Components.classes["@mozilla.org/messenger/offline-manager;1"]
+                             
.getService(Components.interfaces.nsIMsgOfflineManager);

get them into something like 

if (!gOfflineManager) 
  GetOfflineMgrService();

Cover the points mentioned above, r=bhuvan.

Comment 50

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

please attach a new patch (after addressing racham's comments) for me to super
review.

Comment 51

[Mohan Bhamidipati]

Attachment: new patch with bhuvan's comments;

Comment 52

[Mohan Bhamidipati]

requesting seth for super review;

Comment 53

[racham]

Mohan,

You seemed to have missed #1 comment item in the patch. In mailWindow.js, we
need CheckOnline() coverage in OpenInboxForServer() routine.

Also, it will be useful to add some comments above offline function calls both
in commandglue.js and mailwindowoverlay.js (some of the most common js files we
all use).

thanks,
bhuvan.

Comment 54

[Mohan Bhamidipati]

Attachment: new patch with (1)

Comment 55

[Jussi-Pekka Mantere]

Removing PDT+, marking nsbranch; should be checked into the 0.9.3 trunk ASAP,
tested & verified, then submitted for limbo builds.

Comment 56

[Mohan Bhamidipati]

Attachment: new patch with added comments

Comment 57

[racham]

You seemed to have added comments above each function you modified. That's good.
It will be nice if you can add something similar to the following at the first
occurrence of goOnline function call also.

/** 
 * Time to go online. call goOnline interface :
 * some details :
 * API : void goOnline (in boolean sendUnsentMessages, in boolean
playbackOfflineImapOperations, in nsIMsgWindow aMsgWindow);
 * Param 1 : sendUnsentMessages - desc ?
 * Param 2 : playbackOfflineImapOperations - desc ?
 * Param 3 : aMsgWindow - desc ?
 */
+      gOfflineManager.goOnline(false, false, msgWindow);

do that. r=bhuvan.

Comment 58

[Mohan Bhamidipati]

Attachment: new patch with bhuvan's comments

Comment 59

[Mohan Bhamidipati]

requesting seth for super review;

Comment 60

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

sr=sspitzer

bhuvan brings up a good point about commenting arguments that aren't clear.

here's how I'd suggest doing the comments:

gOfflineManager.goOnline(false /* sendUnsentMessages */, 
  false /* playbackOfflineImapOperations */, msgWindow);
  
note, it is not necessary to comment msgWindow, since it is clear what the 
argument is supposed to be.       

Comment 61

[Mohan Bhamidipati]

Attachment: latest patch with seth's comment.

Comment 62

[Diane Sun]

Fix checked in on behalf on mohanb.

Comment 63

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

don't check this into the branch yet.

there's a problem.  now when you send unsent messages, we send each unsent 
message multiple times, one for each identity.

see bug #89150 

Comment 64

[Jussi-Pekka Mantere]

Adding vtrunk; bugs need to be verified in the trunk for eventual limbo checkins.

Comment 65

[Jussi-Pekka Mantere]

Clearing resolution, nsBranch bugs should be closed only after checkins into
limbo builds.

Comment 66

[grylchan]

Commercial builds
2001-07-05-09-trunk/ win nt 4.0
2001-07-05-08-trunk/ linux 2.2, red hat 7.0
2001-07-05-08-trunk/ mac os 9.0.4

Looking at the comments I only need to verify
whether the "get message" alert dialog works.

As the other fixes
>a) While going Online : "Send Unsent Messages" 
>b) While going ofline : "Download Messages" 
 Covered under bug 82568
>d) File-> "Send Unsend Messages" : when offline 
 Covered under bug 79245

So when offline and clicking the "get messages" button results in
the following:
  -A "Get message" window pops up with a question mark
  -the text says 'You are currently offline. Would you like
                  to go online to get your new messages?'
  -Two buttons: Go online (with this set as the default) and
                Cancel button
  -clicking 'Go online' results in going online 
     - if you have a unsent message in the unsent folder,
       it does not get sent (that's good)
     - no 'send unsent messages' prompt pops up 
  -clicking 'Cancel' results in staying offline

Note: Didn't see Seth's regression about sending multiple unsent
    messages, bug #89150. But I will ask for details in that bug.

Verified Get Messages alert dialog works as expected.
Verified on trunk.

Removing Keyword Vtrunk.

adding 'verified on trunk' to status whiteboard.

Leaving status as Assigned per instructions from Jussi.

Comment 67

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

removing nsBranch.  too big of a patch for the branch, and we've already found 
one regression.

Comment 68

[David :Bienvenu]

this is fixed on the trunk, and not going onto the branch - why don't we just
mark it fixed and get it off the .9.3 radar?

Comment 69

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

david is right.  marking fixed.

Comment 70

[grylchan]

Commercial builds:
2001-08-15-09-trunk/ - nt 4.0
2001-08-15-14-trunk- linux 2.2, mac 9.0.4

Verified the following (in both themes):

So when offline and clicking the "get messages" button results in
the following:
  -A "Get message" window pops up with a question mark
  -the text says 'You are currently offline. Would you like
                  to go online to get your new messages?'
  -Two buttons: 'Go online' (with this set as the default) and
                Cancel button
  -clicking 'Go online' results in going online 
     - if you have a unsent message in the unsent folder,
       it does not get sent (that's good)
     - no 'send unsent messages' prompt pops up 
  -clicking 'Cancel' results in staying offline