56670 - add error handling ui to movemail

Status: RESOLVED FIXED

(MailNews Core :: Movemail, defect, P3)

Description

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

see http://lxr.mozilla.org/seamonkey/source/mailnews/movemail/src/movemail.c for
all the error handling ui we had in 4.x

Comment 1

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

component -> movemail

Comment 2

[Axel Pauli]

It's annoying when movemail reports no errors e.g. mail directory access right
not 1777. I pressed multiple times the button get mail and nothing happened
although the mail file was not empty. Starting netscape mailer 4.78, I got
immediately
an error box informing about this fact.

Axel Pauli

Comment 3

[Adam D. Moss]

FWIW movemail spits various messages to stdout, though
that is clearly not useful if you do not run from a
console.

Adding helpwanted keyword and -> NEW.

Comment 4

[Philip K. Warren]

I am going to implement this for Movemail.

Comment 5

[Philip K. Warren]

Attachment: Patch v1 (cvs diff -wu)

This patch does the following:

1) Adds new error messages to localMsgs.properties and nsLocalStringBundle.h
for Movemail.
2) Adds a new Error function which will be used to display the error messages.
3) Removes some unnecessary code and greatly simplifies the GetNewMail
function. It is currently several levels deep in some places. I have removed a
few of these unnecessary levels and reformatted the loops. This is why I
provided a diff -wu patch because the majority of these changes are whitespace.


After this patch is reviewed and checked in I will add the code which calls the
Error function. I would prefer to implement this in small steps otherwise the
patch file becomes too large to review.

Comment 6

[Philip K. Warren]

I realize that my localMsgs.properties implements message #4030 which conflicts
with an existing message on the trunk. I will bump up the numbers on the
movemail messages when I check this in to make sure there are no conflicts.

Comment 7

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

Comment on attachment 126051 [details] [diff] [review]
Patch v1 (cvs diff -wu)

looks good.  I'm not using movemail, so if things still work for you after this
change,
that's good enough for me.

some minor suggestions / questions:

1)

+    nsresult rv;
+    nsCOMPtr<nsIPrompt> dialog;
+
+    rv = mMsgWindow->GetPromptDialog(getter_AddRefs(dialog));
+    if (NS_SUCCEEDED(rv) && dialog) {

instead:

+    nsCOMPtr<nsIPrompt> dialog;
+    nsresult rv = mMsgWindow->GetPromptDialog(getter_AddRefs(dialog));
+    if (NS_FAILED(rv))
+      return;


this way you don't have to move all the code into the if block.

You don't have to check both rv and dialog, checking rv should be enough,
looking at
http://lxr.mozilla.org/mozilla/source/mailnews/base/src/nsMsgWindow.cpp#504

2)

+	 PRUnichar *errStr = nsnull;

consider using a nsXPIDLString instead.

nsXPIDLString errStr;

+
+	 // Format the error string if necessary
+	 if (params) {
+	     nsCOMPtr<nsIStringBundle> bundle;
+	     rv = mStringService->GetBundle(getter_AddRefs(bundle));
+	     if (NS_SUCCEEDED(rv))
+		 bundle->FormatStringFromID(errorCode, params, length,
getter_Copies(errStr));
+	 }
+	 else {
+	     mStringService->GetStringByID(errorCode, getter_Copies(errStr));
+	 }
+
+	 if (!errStr.IsEmpty()) {
+	     dialog->Alert(nsnull, errStr.get());
+	 }
+    }

3)

was this code removed because it was unneeded?

-    // Create nsFileSpec and nsILocalFile for the spool file
-    nsFileSpec spoollocspec(spoolnameStr);
-    nsCOMPtr<nsILocalFile> spoollocfile;
-    rv = NS_FileSpecToIFile(&spoollocspec, getter_AddRefs(spoollocfile));
-    if (NS_FAILED(rv))
-	 return PR_FALSE;
     // Create nsFileSpec and nsILocalFile for the spool.mozlock file
     nsFileSpec tmplocspec(mozlockstr.get());
     nsCOMPtr<nsILocalFile> tmplocfile;
     rv = NS_FileSpecToIFile(&tmplocspec, getter_AddRefs(tmplocfile));
     if (NS_FAILED(rv))
	 return PR_FALSE;
-    // Create nsFileSpec and nsILocalFile for the spool.lock file
-    nsFileSpec locklocspec(lockstr.get());
-    nsCOMPtr<nsILocalFile> locklocfile;
-    rv = NS_FileSpecToIFile(&locklocspec, getter_AddRefs(locklocfile));
-    if (NS_FAILED(rv))
-	 return PR_FALSE;

Comment 8

[Philip K. Warren]

Attachment: Patch v2 (cvs diff -wu)

Fixes reviewer comments. Changes Movemail error message ids to not conflict
with latest messages added on the trunk.

Comment 9

[Philip K. Warren]

> 3)
> 
> was this code removed because it was unneeded?

Yep. The spoollocspec, spoollocfile, locklocspec and locklocfile variables were
never used in the ObtainSpoolLock function.

Comment 10

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

Comment on attachment 126774 [details] [diff] [review]
Patch v2 (cvs diff -wu)

r/sr=sspitzer, thanks philip

Comment 11

[Philip K. Warren]

First set of changes checked in.

Checking in resources/locale/en-US/localMsgs.properties;
/cvsroot/mozilla/mailnews/local/resources/locale/en-US/localMsgs.properties,v 
<--  localMsgs.properties
new revision: 1.25; previous revision: 1.24
done
Checking in src/nsLocalStringBundle.h;
/cvsroot/mozilla/mailnews/local/src/nsLocalStringBundle.h,v  <-- 
nsLocalStringBundle.h
new revision: 1.18; previous revision: 1.17
done
Checking in src/nsMovemailService.cpp;
/cvsroot/mozilla/mailnews/local/src/nsMovemailService.cpp,v  <--
nsMovemailService.cpp
new revision: 1.35; previous revision: 1.34
done
Checking in src/nsMovemailService.h;
/cvsroot/mozilla/mailnews/local/src/nsMovemailService.h,v  <--  nsMovemailService.h
new revision: 1.3; previous revision: 1.2
done

Comment 12

[Philip K. Warren]

Attachment: Part 2 - Patch v1

1) Cleans up whitespace and removes unused code from
nsMovemailIncomingServer.cpp.
2) Clean up code which searches for spool file.
3) Allocate file streams on the stack instead of the heap. There were several
instances where they would leak in certain instances in the previous code.
4) Make sure to Addref/Release the nsParseNewMailState class. It was previously
not being addref'd so it was going away at unexpected times.
5) Added error conditions for the following:
     - MOVEMAIL_SPOOL_FILE_NOT_FOUND
     - MOVEMAIL_CANT_OPEN_SPOOL_FILE
     - MOVEMAIL_CANT_CREATE_LOCK
     - MOVEMAIL_CANT_TRUNCATE_SPOOL_FILE
     - MOVEMAIL_CANT_DELETE_LOCK

I still need to implement an error for MOVEMAIL_SPOOL_FILE_LOCKED. I will do
this in another bug.

Comment 13

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

Comment on attachment 126963 [details] [diff] [review]
Part 2 - Patch v1

sorry for the delay philip.

r/sr=[e-mail]

Comment 14

[Philip K. Warren]

Fixed.

Checking in nsMovemailIncomingServer.cpp;
/cvsroot/mozilla/mailnews/local/src/nsMovemailIncomingServer.cpp,v  <-- 
nsMovemailIncomingServer.cpp
new revision: 1.11; previous revision: 1.10
done
Checking in nsMovemailService.cpp;
/cvsroot/mozilla/mailnews/local/src/nsMovemailService.cpp,v  <-- 
nsMovemailService.cpp
new revision: 1.36; previous revision: 1.35
done