add error handling ui to movemail

RESOLVED FIXED

Status

MailNews Core
Movemail
P3
normal
RESOLVED FIXED
18 years ago
10 years ago

People

(Reporter: (not reading, please use seth@sspitzer.org instead), Assigned: Philip K. Warren)

Tracking

({helpwanted})

Trunk
x86
Linux
helpwanted

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

12.09 KB, patch
(not reading, please use seth@sspitzer.org instead)
: review+
(not reading, please use seth@sspitzer.org instead)
: superreview+
Details | Diff | Splinter Review
17.94 KB, patch
(not reading, please use seth@sspitzer.org instead)
: review+
(not reading, please use seth@sspitzer.org instead)
: superreview+
Details | Diff | Splinter Review

Updated

18 years ago
Status: NEW → ASSIGNED
QA Contact: esther → stephend
component -> movemail
Component: Mail Back End → Movemail

Comment 2

17 years ago
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

16 years ago
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.
Status: ASSIGNED → NEW
Keywords: helpwanted
QA Contact: stephend → gayatri
QA Contact: gayatri → stephend
(Assignee)

Comment 4

15 years ago
I am going to implement this for Movemail.
Assignee: adam → pkw
(Assignee)

Comment 5

15 years ago
Created attachment 126051 [details] [diff] [review]
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.
(Assignee)

Comment 6

15 years ago
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.
(Assignee)

Updated

15 years ago
Attachment #126051 - Flags: review?(bienvenu)
(Assignee)

Updated

15 years ago
Attachment #126051 - Flags: review?(bienvenu) → review?(sspitzer)
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;
Attachment #126051 - Flags: review?(sspitzer) → review+
(Assignee)

Comment 8

15 years ago
Created attachment 126774 [details] [diff] [review]
Patch v2 (cvs diff -wu)

Fixes reviewer comments. Changes Movemail error message ids to not conflict
with latest messages added on the trunk.
Attachment #126051 - Attachment is obsolete: true
(Assignee)

Comment 9

15 years ago
> 3)
> 
> was this code removed because it was unneeded?

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

Updated

15 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

15 years ago
Attachment #126774 - Flags: superreview?(sspitzer)
Attachment #126774 - Flags: review?(sspitzer)
Comment on attachment 126774 [details] [diff] [review]
Patch v2 (cvs diff -wu)

r/sr=sspitzer, thanks philip
Attachment #126774 - Flags: superreview?(sspitzer)
Attachment #126774 - Flags: superreview+
Attachment #126774 - Flags: review?(sspitzer)
Attachment #126774 - Flags: review+
(Assignee)

Comment 11

15 years ago
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
(Assignee)

Comment 12

15 years ago
Created attachment 126963 [details] [diff] [review]
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.
(Assignee)

Updated

15 years ago
Attachment #126963 - Flags: superreview?(sspitzer)
Attachment #126963 - Flags: review?(sspitzer)
Comment on attachment 126963 [details] [diff] [review]
Part 2 - Patch v1

sorry for the delay philip.

r/sr=sspitzer@mozilla.org
Attachment #126963 - Flags: superreview?(sspitzer)
Attachment #126963 - Flags: superreview+
Attachment #126963 - Flags: review?(sspitzer)
Attachment #126963 - Flags: review+
(Assignee)

Comment 14

15 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.