Closed
Bug 79865
Opened 23 years ago
Closed 23 years ago
Offline: Get Msgs when offline doesn't display alert dialog.
Categories
(SeaMonkey :: MailNews: Backend, defect, P2)
SeaMonkey
MailNews: Backend
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla0.9.3
People
(Reporter: laurel, Assigned: mohanb)
References
Details
Attachments
(17 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
image/jpeg
|
Details | |
(deleted),
image/jpeg
|
Details | |
(deleted),
image/jpeg
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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•23 years ago
|
||
this has to be done in js front end - I guess I'll be doing it, however.
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•23 years ago
|
||
Assigning myself. -mohan.
Assignee: bienvenu → mohanb
Status: ASSIGNED → NEW
Assignee | ||
Updated•23 years ago
|
Priority: -- → P2
Target Milestone: --- → mozilla0.9.2
Assignee | ||
Comment 3•23 years ago
|
||
Assignee | ||
Updated•23 years ago
|
Whiteboard: [PDT+] → [PDT+] requesting r & sr
Assignee | ||
Comment 5•23 years ago
|
||
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
Assignee | ||
Comment 7•23 years ago
|
||
Assignee | ||
Comment 8•23 years ago
|
||
Assignee | ||
Comment 9•23 years ago
|
||
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;
Assignee | ||
Updated•23 years ago
|
Whiteboard: [PDT+] requesting r & sr → [PDT+] requesting r & sr Planning for checkin by FRI : 06/22
Comment 10•23 years ago
|
||
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.
Assignee | ||
Comment 11•23 years ago
|
||
adding hwaara for review comments;
hwaara, attached patch covers "nsIPromptService" for four dialogs;
Comment 12•23 years ago
|
||
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.
Assignee | ||
Comment 13•23 years ago
|
||
Assignee | ||
Comment 14•23 years ago
|
||
Assignee | ||
Comment 15•23 years ago
|
||
Comment 16•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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?
Assignee | ||
Comment 19•23 years ago
|
||
Assignee | ||
Comment 20•23 years ago
|
||
--------------
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);
Assignee | ||
Comment 21•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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.
Assignee | ||
Updated•23 years ago
|
Whiteboard: [PDT+] requesting r & sr Planning for checkin by FRI : 06/22 → [PDT+]
Updated•23 years ago
|
Whiteboard: [PDT+] → [PDT+] patch in progress
Comment 24•23 years ago
|
||
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.
Assignee | ||
Comment 25•23 years ago
|
||
------------------------
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;
Assignee | ||
Comment 26•23 years ago
|
||
Assignee | ||
Comment 27•23 years ago
|
||
requesting hwaara & bhuvan for r & seth for sr.
Whiteboard: [PDT+] patch in progress → [PDT+] patch ready; requesting r & sr
Comment 28•23 years ago
|
||
I will look into this. Just give me a second
Comment 29•23 years ago
|
||
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! :)
Assignee | ||
Comment 30•23 years ago
|
||
Assignee | ||
Comment 31•23 years ago
|
||
--------------
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•23 years ago
|
||
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•23 years ago
|
||
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.
Assignee | ||
Comment 34•23 years ago
|
||
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;
Assignee | ||
Updated•23 years ago
|
Whiteboard: [PDT+] patch ready; requesting r & sr → [PDT+] patch ready; r=hwaara; requesting sr
Comment 35•23 years ago
|
||
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•23 years ago
|
||
(... and cvs add the new ones.)
Assignee | ||
Comment 37•23 years ago
|
||
Assignee | ||
Comment 38•23 years ago
|
||
Would like to check in before UI freeze;
Assignee | ||
Comment 39•23 years ago
|
||
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•23 years ago
|
||
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.)
Assignee | ||
Comment 41•23 years ago
|
||
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;
Assignee | ||
Comment 42•23 years ago
|
||
Assignee | ||
Comment 43•23 years ago
|
||
requesting seth for sr=;
Comment 44•23 years ago
|
||
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.
Updated•23 years ago
|
Whiteboard: [PDT+] patch ready; r=hwaara; requesting sr → [PDT+]
Assignee | ||
Comment 45•23 years ago
|
||
-----------------
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;
---------
Assignee | ||
Comment 46•23 years ago
|
||
Assignee | ||
Comment 47•23 years ago
|
||
requesting bhuvan for review & seth for super review;
Whiteboard: [PDT+] → [PDT+] r=hwaara; requesting sr;
Comment 48•23 years ago
|
||
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)
Target Milestone: mozilla0.9.2 → mozilla0.9.3
Comment 49•23 years ago
|
||
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•23 years ago
|
||
please attach a new patch (after addressing racham's comments) for me to super
review.
Assignee | ||
Comment 51•23 years ago
|
||
Assignee | ||
Comment 52•23 years ago
|
||
requesting seth for super review;
Comment 53•23 years ago
|
||
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.
Assignee | ||
Comment 54•23 years ago
|
||
Comment 55•23 years ago
|
||
Removing PDT+, marking nsbranch; should be checked into the 0.9.3 trunk ASAP,
tested & verified, then submitted for limbo builds.
Keywords: nsBranch
Whiteboard: [PDT+] r=hwaara; requesting sr; → r=hwaara; requesting sr;
Assignee | ||
Comment 56•23 years ago
|
||
Comment 57•23 years ago
|
||
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.
Assignee | ||
Comment 58•23 years ago
|
||
Assignee | ||
Comment 59•23 years ago
|
||
requesting seth for super review;
Comment 60•23 years ago
|
||
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.
Assignee | ||
Comment 61•23 years ago
|
||
Comment 62•23 years ago
|
||
Fix checked in on behalf on mohanb.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 63•23 years ago
|
||
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
Whiteboard: r=hwaara; requesting sr; → do not check this into the branch until #89150 is fixed.
Comment 64•23 years ago
|
||
Adding vtrunk; bugs need to be verified in the trunk for eventual limbo checkins.
Status: RESOLVED → ASSIGNED
Keywords: vtrunk
Comment 65•23 years ago
|
||
Clearing resolution, nsBranch bugs should be closed only after checkins into
limbo builds.
Resolution: FIXED → ---
Comment 66•23 years ago
|
||
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.
Keywords: vtrunk
Whiteboard: do not check this into the branch until #89150 is fixed. → do not check this into the branch until #89150 is fixed.,verified on trunk
Comment 67•23 years ago
|
||
removing nsBranch. too big of a patch for the branch, and we've already found
one regression.
Keywords: nsBranch
Comment 68•23 years ago
|
||
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•23 years ago
|
||
david is right. marking fixed.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago → 23 years ago
Depends on: 89150
Resolution: --- → FIXED
Whiteboard: do not check this into the branch until #89150 is fixed.,verified on trunk
Comment 70•23 years ago
|
||
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
Status: RESOLVED → VERIFIED
Updated•20 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•