81720 - Bogus error messages when a save operation failed

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect)

Description

[Jean-Francois Ducarroz]

This bug report is the message compose side of bug 81487.

Currently, when we failed during a save as draft or save as template, we are displaying twice an alert that say 
"Send failed" Also, internally we are not correctly propagating the error which lead to the crash describe in bug 
81487 or will try to delete the draft message from the server if it have been already saved as draft!

The correct behavior should be to displain only one error message that say either that the save draft failed or the 
save template failed. Save as File use a totally different patch and is not concerned by this bug.

Comment 1

[Jean-Francois Ducarroz]

Mark is as nsbeta1 as the other part of this bug as been already marked and approved nsbeta1. 
Also due to the fact we will try to delete the draft message from the server when save draft failed, mark it as 
dataloss.

Comment 2

[scottputterman]

marking nsbeta1+

Comment 3

[Jean-Francois Ducarroz]

Attachment: proposed fix, v1

Comment 4

[Jean-Francois Ducarroz]

oops, wrong patch, please ignore!

Comment 5

[Jean-Francois Ducarroz]

Attachment: this time is the right patch, ignore the previous one!

Comment 6

[Cavin Song]

Looks good and it's so nice to have distinct error messages for different 
operations. r=cavin.

Comment 7

[Jean-Francois Ducarroz]

R=varada, SR=bienvenu

Comment 8

[Jean-Francois Ducarroz]

Fixed and checked in

Comment 9

[Blake Ross]

From <http://mozilla.org/hacking/reviewers.html>:

20.When reviewing code, verify that the prevailing module style, if any, has 
been observed ("When in Rome...") in the patch.

Now, a quick glance at nsMsgSend.cpp will reveal that there is no dominate 
style, particularly because of checkins like these.  So I feel compelled once 
again to reopen this and ask that -- at the very least -- you maintain 
consistency with yourself when checking in. Ducarroz, you seem to have taken 
some ownership of this file over the past six months or so.  How about 
establishing a consistent style now and enforcing it in the future? 

Comment 10

[David :Bienvenu]

Reopening this bug is absolutely the wrong thing to do - Is this bug fixed? Yes
it is. Why would we want the whole world to think it wasn't fixed? Open a new
bug if you want to the coding style changed.

Comment 11

[Blake Ross]

Was this bug fixed in the right way?  No, I don't think so.  I'm asking simply 
that the code checked in in this patch, as part of this bug, be consistent with 
itself.

+         if ( m_deliver_mode != nsMsgSaveAsDraft && m_deliver_mode != 
nsMsgSaveAsTemplate ) 
+    {
+      PRBool oopsGiveMeBackTheComposeWindow = PR_FALSE;

+      if (mGUINotificationEnabled)
+      {
+        nsMsgAskBooleanQuestionByString(prompt, eMsg, 
&oopsGiveMeBackTheComposeWindow);
+        if (!oopsGiveMeBackTheComposeWindow)
+           aStatus = NS_OK;

Why did this make it through three reviewers?  Perhaps the mail team is 
accustomed to looking at such code, but it's a big inhibitor (and a common 
complaint) from contributors who are trying to help out.

If you'd truly prefer that this go in a separate bug report, then fine.  But the 
issue will not be cast aside.

Comment 12

[Jean-Francois Ducarroz]

Blacke, what are you speaking about? the tabs? the architecture? Please explain, 
thanks.

If it's about the tabs, the mess come from the beginning of the project when we 
did not have a strict rule about it. The original code comes from 4.5 and Rich 
and myself were working on it, both using a different tab settings. Now, all new 
code use 2 spaces per tab. You are right, we haven't corrected the whole file to 
be consistent which make diffs looks ugly but it looks ok on my editor. I think 
we have way more important stuff to do than westing our precious time fixing old 
tabbing issue.

If you really want to see that fixed, please file a new bug but do not reopen a 
bug that has been fixed and is still working correctrly. As far I can tell, tabs 
doesn't not affect the way the code is executed!

Now, if you are speaking about the architecture, especially the way we manage 
errors during a send/save message, we know it's a big mess and it's on my plate 
to totally rewite it, probably right after the next Netscape Release.

Comment 13

[Blake Ross]

Okay, with this patch specifically I am referring to: 
            
+         if ( m_deliver_mode != nsMsgSaveAsDraft && m_deliver_mode != 
nsMsgSaveAsTemplate )  
+    { 
+      PRBool oopsGiveMeBackTheComposeWindow = PR_FALSE; 
            
The |if| is indented too far, and the conditions should not be wrapped in spaces 
(according to most of the rest of the file, and other instances in the same 
patch). 

            
+      if (mGUINotificationEnabled) 
+      { 
+        nsMsgAskBooleanQuestionByString(prompt, eMsg, 
&oopsGiveMeBackTheComposeWindow); 
+        if (!oopsGiveMeBackTheComposeWindow) 
+           aStatus = NS_OK; 
            
Inconsistent indentation following the |if|s.

And of course, aStatus is renamed, but that's not the fault of this patch.

So I'm not just talking about this (horrendous) file, but inconsistency within 
the patch itself.  I don't understand why we keep adding to the problem, saying 
we'll fix it all later at some fuzzy, undefined point.  That isn't the way we 
work, or should be working.

If the new code uses 2 spaces, why do some places in the patch use 4?  And why 
does the modeline say 4?

Comment 14

[Blake Ross]

I am not only speaking from personal experience when I say that the current 
state of all the mailnews code -- even the FE, not just the parts from the old 
codebase -- has kept quite a few would-be contributors out.  It is a mess.  It 
is ridden with tabs and hellish indentation, and other inconsistent styling 
abound.  Conversion from the old codebase is a minor excuse (how long has it 
been? what's being waited for?), but it's not a panacea, and it's certainly not 
an excuse for introducing new problems.

No "precious time" would be "wasted" if patches such as this (or worst offenders 
that I've seen go in over the past months) were done correctly the first time.  
Tabs are now a known no-no; there's no excuse for them to be in new patches 
anymore.

I've filed bug 81827 to handle issues that this patch introduced, as well as 
others in this file.

Comment 15

[Jean-Francois Ducarroz]

Personally, I am developping either on Mac using CW or on Window using VC++, in 
both case my tabs setting are set to 2 spaces. As far I know, the modeline stuff 
is not supported by those editors. If I have two fix few line in a 4 spaces 
indented function, I generally let it this way to be consistent and looks nice. 
But if I have to do more or if the indentation is not consistent, I normally 
correct it. About letting code with tabs instead of spaces, they are not visible 
as the editor correctly display tabs as n spaces. In this particular case the 
indentation problem you see in the diff come to the fact the function 
nsMsgComposeAndSend::NotifyListenerOnStopCopy is written using mostly 2 spaces 
and few tabs (old code), which display correclty under VC++ but in the diff, 
tabs are converted to 4 spaces making it looks wrong!!

If you had complains about that before I checked in, I would have fixed them no 
problem (it's very easy to do it with VC++) but after the fact, no! It would be 
a waist of time to go again though the whole checkin process!

Comment 16

[esther]

using build 2001-05-23 on win, mac and linux if you go offline and then do a 
Save as draft you will get a more correct error.  "Unable to save your messge as 
draft.  Please verify that your mail preferences are correct and try again."  
and you don't get 2 error messages.  So per Summary this bug is fixed. However,  
if you still have the compose window up and change to online status via 3-pane 
window (the icon in the New Msg window now indicates your online) and you try to 
save the draft you still get the error.  Laurel will log a new bug for that.  
This one is verified.