95116 - Send document feature in Simple MAPI

Status: VERIFIED FIXED

(MailNews Core :: Simple MAPI, defect, P1)

Description

[Tiantian Kong]

This is to track the implementation for the send document feature in Simple MAPI.

Comment 1

[Hong Kwon]

mass qa assigning all simple MAPI bugs to olgac 

Comment 2

[Rodney Velasco]

QA over to trix.

Comment 3

[Tiantian Kong]

nsbranch+

Comment 4

[Jaime Rodriguez, Jr.]

Marking PDT per PDT.

Comment 5

[Rajiv Dayal]

Attachment: patch for the SendDocuments feature of MAPI

Comment 6

[Rajiv Dayal]

the send document feature parses the data passed for the files to be sent. A 
list of file URLs delimited by comma is created to be specified for the 
nsIMsgCompFields 'attachments'. It then displays the mailnews compose window to 
allow the user to specify the recipients and a note for the files to be sent.

Please find the patch (v1) for this feature implementation attached above.

JF, can u please review this patch.

thanks, - rajiv.

Comment 7

[Jean-Francois Ducarroz]

Comment on attachment 49677 [details] [diff] [review]
patch for the SendDocuments feature of MAPI

1) pFile->GetURL (&pURL) return a copy of the url therefore you need to free it
(2 occurances in the code). The best way would be to declare pURL as a nsXPIDLCString,
that will take care or freeing the menory for you:

        nsXPIDLCString pURL ;
        pFile->GetURL (getter_Copies(pURL)) ;

2) Please add a new line at the end of files.

Comment 8

[Tiantian Kong]

Added PDT status, per PDT meeting today. Once we got all r and sr, will change
it into PDT+. 

All r ans sr, please try your best to give comments within 24 hours. If no
reply, then our assumption will be within 24 hours. If you cannot do it within
24 hours, please attend the PDT meeting at 12 noon on 9/18 at the pit of bldg 21
to explain why. Many thanks.
=======================
Subject: Urgent: your review and super review comments.
Date: Mon, 17 Sep 2001 13:18:36 -0700
From: Tiantian Kong <[e-mail]>
Reply-To: [e-mail]
To: [e-mail], [e-mail],	Doug Turner <[e-mail]>, [e-mail],	[e-mail],
Rajiv Dayal <[e-mail]>
CC: Elaine King <[e-mail]>

Hi:

You got this mail because you are the r or sr of features in simple MAPI.

Pathes for these has been up on the bug. 

On behalf on PDT, I'm requesting that you provide immediate feeback. If you
cannot be back to us today, please reply by 
email, and cc Elaine King, as to when you'll be able to get back to us.

This is an urgent request, please drop everything else that you are doing. 

Thx. Elaine will be calling each of you as well. 


Rajiv, r, 		Pref, 95122					

Seth, sr, 		Pref, 95122

Dan Veditz,	r	log on, log off, 95117/95121	

Doug Turner, sr, 	log on, log off, 95117/95121	
				
Bill Law,	r	xpfe/bootstrap for log on, off	
				
Alec, sr, 		xpfe/bootstrap for log on, off 


Tiantian
=========================

Comment 9

[Rajiv Dayal]

Please find the updated patch with the review comments attached below.

1) done. i thought the nsIFile will return a pointer to a URL string that is a 
member, but it does not, it creates the URL string each time GetURL is called. 
Also the GetURL returns a char * (takes in char **), i didn't know i can use 
XPIDLString there ! thanks.

2) done

Comment 10

[Rajiv Dayal]

Attachment: updated patch with review comments incorporated

Comment 11

[Jean-Francois Ducarroz]

Comment on attachment 49768 [details] [diff] [review]
updated patch with review comments incorporated

There are still 2 occurences in your code where you call pFile->GetURL without using an nsXPIDLCString!

Comment 12

[Rajiv Dayal]

Attachment: patch with the XPIDLString for GetURL

Comment 13

[Jean-Francois Ducarroz]

Comment on attachment 49826 [details] [diff] [review]
patch with the XPIDLString for GetURL

R=ducarroz

Comment 14

[Rajiv Dayal]

Hi David,

Can u please super review the latest patch, attachment (id=49826) ?

thanks,

- rajiv.

Comment 15

[Tiantian Kong]

Reassign it to Rajiv, since he has already completed the coding, got r, and is
currently working on sr comments. 

Good job! Rajiv.

Thanks to Jean-Francois for the speedy review feedback, and David for the speedy
sr feedback for both send mail and send doc.

Comment 16

[David :Bienvenu]

I've asked Seth to have a quick sr look at this, since there's a lot of string
stuff going on, and Seth is much better at looking at that stuff. He'll look at
it in an hour or so.

Comment 17

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

I'll go review the patch now.

Comment 18

[Rajiv Dayal]

thanks Seth. - rajiv.

Comment 19

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

Comment on attachment 49826 [details] [diff] [review]
patch with the XPIDLString for GetURL


>+    nsCString strDelimChars, strFilePaths;

why are these declared as nsCStrings?  If they are short ( < 64 bytes)
declare them as nsCAutoStrings so they'll get allocated on the stack and not the heap.

>+    // check for comma in filename 
>+    if (strDelimChars.Find (",") == -1)  // if comma is not in the delimiter specified by user
>+    {
>+        if (strFilePaths.Find(",") != -1) // if comma found in filenames return error
>+            return NS_ERROR_FILE_INVALID_PATH ;
>+    }

instead of -1, please use kNotFound

>+    nsCString Attachments ;

again, how long do you expect Attachments to be?  (nsCAutoString vs nsCString)
note, if an AutoString grows to be more than 64 bytes, it gets copied into a nsCString, so
if it is going to be big, use nsCString to avoid the copy.

>+
>+    // only 1 file is to be sent, no delim specified
>+    if (!strDelimChars.Length())
>+    {
>+        nsCOMPtr <nsILocalFile> pFile = do_CreateInstance (NS_LOCAL_FILE_CONTRACTID, &rv) ;
>+        if (NS_FAILED(rv) || (!pFile) ) return rv ;        
>+        pFile->InitWithPath (strFilePaths.get()) ;
>+
>+        PRBool bExist ;
>+        rv = pFile->Exists(&bExist) ;
>+        if (NS_FAILED(rv) || (!bExist) ) return NS_ERROR_FILE_TARGET_DOES_NOT_EXIST ;
>+
>+        nsXPIDLCString pURL ;
>+        pFile->GetURL (getter_Copies(pURL)) ;
>+        if (pURL)
>+            Attachments.Assign(pURL) ;
>+
>+        // set attachments for comp field and return
>+	    rv = aCompFields->SetAttachments (Attachments.get());
>+        return rv ;
>+    }
>+
>+    // multiple files to be sent, delim specified
>+    nsCOMPtr <nsILocalFile> pFile = do_CreateInstance (NS_LOCAL_FILE_CONTRACTID, &rv) ;
>+    if (NS_FAILED(rv) || (!pFile) ) return rv ;        
>+    PRInt32 offset = 0 ;
>+    char * newFilePaths = (char *) strFilePaths.get() ;
>+    while (offset != -1)

since you are using Find(), please use kNotFound instead of -1.

>+    {
>+        nsCString RemainingPaths ;

again, the nsCString vs nsCAutoString decision.

>+        RemainingPaths.Assign(newFilePaths) ;
>+        offset = RemainingPaths.Find (strDelimChars) ;
>+        if (offset != -1)

again, kNotFound instead of -1

>+        {
>+            RemainingPaths.SetLength (offset) ;
>+            newFilePaths += offset + strDelimChars.Length() ;

this never sets newFilePaths past the end of strFilePaths, does it?

Comment 20

[Rajiv Dayal]

In fact i was in process of changing all nsCStrings to nsCAutoStrings here .. 
but i didnot know if they grow > 64 bytes they get copied to nsCString !! 
thanks. Only the strDelimChar will be < 64 bytes and the other string will be > 
64. So i will change the nsCString for strDelimChar to nsCAutoString and leave 
others to nsCStrings. Also will use the macros instead of -1.

Comment 21

[Rajiv Dayal]

Attachment: patch updated with review comments

Comment 22

[Rajiv Dayal]

Please find the patch with the review comments :
- changed nsCString to nsCAutoString for strDelimChar
- used kNotFound instead of -1 for result of Find
- check the newFilePaths never gets past the end to take care of even any user 
error.
- removed proxied objects in nsMapiImp.cpp, no need since it runs on the main 
thread, same as the UI thread.

Comment 23

[David :Bienvenu]

+STDMETHODIMP nsMapiImp::SendDocuments

Is the return value for this correct? If we succeed, we return S_OK, but if we
fail, we return the NS error code as is. Is that correct as far as MAPI is
concerned, or do we need to convert the NS error code into a MAPI-style error
code? ns error codes are negative, in the 0x80000000 range.

Other than that, it looks OK to me.

Comment 24

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

I don't think PopulateCompFieldsForSendDocs() will work if aDelimChar and
aFilePaths are non ASCII.

I think you're going to want to rewrite that code to use nsAutoStrings and
nsStrings, instead of nsCAutoStrings and nsCStrings.

You'll want to change the code to use pFile->InitWithUnicodePath(), and in the
case where (!(aFlags & MAPI_UNICODE)), you can safely convert up to nsAutoString
from a char *.

Comment 25

[Rajiv Dayal]

I have tested the code with non ASCII / Unicode strings and it seems to work 
perfectly fine. In the (aFlags & MAPI_UNICODE) i convert the Unicode strings to 
nsCString/nsCAutoString using AssignWithConversion and it seems to work fine.

Comment 26

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

what were the unicode strings you used for testing?  

Do you have a machine where the system charset is Japanese (like Shift_JIS) or
Chinese (Big5)?

Did you test with file paths that included non ASCII characters?

Comment 27

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

talking to ravij over AIM, it sounds like he's only tested with unicode strings
that contained US-ASCII characters.  I'm pretty sure it would not work for
unicode strings that didn't contain US-ASCII strings.

making PopulateCompFieldsForSendDocs() unicode friendly should not be difficult.

one thing, instead of doign the pointer arithmetic when assigning newFilePaths,
I suggest you use Substring() or Right() 

+    char * newFilePaths = (char *) strFilePaths.get() ;
+    while (offset != kNotFound)
+    {
+        nsCString RemainingPaths ;
+        RemainingPaths.Assign(newFilePaths) ;
+        offset = RemainingPaths.Find (strDelimChars) ;
+        if (offset != kNotFound)
+        {
+            RemainingPaths.SetLength (offset) ;
+            if ((offset + strDelimChars.Length()) < FilePathsLen)
+                newFilePaths += offset + strDelimChars.Length() ;
+        }
+

Comment 28

[Rajiv Dayal]

Attachment: patch after integration, updated for making Populate.. unicode friendly

Comment 29

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

rajiv, what about bienvenu's comments on 2001-09-19 12:14?

Comment 30

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

the last patch looks safe for i18n, I'll wait to hear rajiv's response to 
bienvenu comment before finishing the super review.

Comment 31

[Rajiv Dayal]

That is benn taken care of. You can see it in the msgMapiImp.cpp, the function 
nsMapiImp::SendDocuments converts the return values from mozilla and mailnews 
functions to MAPI return values. This is there in the latest patch (id=50482) 
attached.

Comment 32

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

Comment on attachment 50482 [details] [diff] [review]
patch after integration, updated for making Populate.. unicode friendly

sr=sspitzer

Comment 33

[Jaime Rodriguez, Jr.]

check it in - PDT+

Comment 34

[Rajiv Dayal]

feature done, has r and sr.

Comment 35

[Srilatha Moturi]

Checked into the branch.

Comment 36

[Trix Supremo]

verified on 2001-10-12-05-0.9.4