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?"

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

Assigning myself. -mohan.

PDT+ per 6/12 mtg.

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

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

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;

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.

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

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.

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

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.

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?

-------------- 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);

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.

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...

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.

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.

------------------------ 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;

requesting hwaara & bhuvan for r & seth for sr.

I will look into this. Just give me a second

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! :)

-------------- 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. --------------

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.

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.

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;

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.

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

Would like to check in before UI freeze;

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

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.)

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;

requesting seth for sr=;

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.

----------------- 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; ---------

requesting bhuvan for review & seth for super review;

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)

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.

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

requesting seth for super review;

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.

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

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.

requesting seth for super review;

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.

Fix checked in on behalf on mohanb.

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

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

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

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.

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

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?

david is right. marking fixed.

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