Closed
Bug 95116
Opened 24 years ago
Closed 23 years ago
Send document feature in Simple MAPI
Categories
(MailNews Core :: Simple MAPI, defect, P1)
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: tiantian, Assigned: rdayal)
References
Details
(Whiteboard: [PDT+])
Attachments
(5 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
bugzilla
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
sspitzer
:
superreview+
|
Details | Diff | Splinter Review |
This is to track the implementation for the send document feature in Simple MAPI.
Reporter | ||
Updated•24 years ago
|
mass qa assigning all simple MAPI bugs to olgac
QA Contact: hong → olgac
Updated•24 years ago
|
Keywords: nsenterprise → nsenterprise+
Reporter | ||
Comment 3•23 years ago
|
||
nsbranch+
Assignee | ||
Comment 5•23 years ago
|
||
Assignee | ||
Comment 6•23 years ago
|
||
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•23 years ago
|
||
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.
Attachment #49677 -
Flags: needs-work+
Reporter | ||
Comment 8•23 years ago
|
||
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 <tiantiankong8@netscape.com>
Reply-To: tiantiankong8@netscape.com
To: sspitzer@netscape.com, dveditz@netscape.com, Doug Turner <dougt@netscape.com>, law@netscape.com, alecf@netscape.com,
Rajiv Dayal <rdayal@netscape.com>
CC: Elaine King <elaineking@netscape.com>
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
=========================
Assignee | ||
Comment 9•23 years ago
|
||
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
Assignee | ||
Comment 10•23 years ago
|
||
Comment 11•23 years ago
|
||
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!
Attachment #49768 -
Flags: needs-work+
Assignee | ||
Comment 12•23 years ago
|
||
Comment 13•23 years ago
|
||
Comment on attachment 49826 [details] [diff] [review]
patch with the XPIDLString for GetURL
R=ducarroz
Attachment #49826 -
Flags: review+
Assignee | ||
Comment 14•23 years ago
|
||
Hi David,
Can u please super review the latest patch, attachment (id=49826) ?
thanks,
- rajiv.
Reporter | ||
Comment 15•23 years ago
|
||
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.
Assignee: srilatha → rdayal
Status: ASSIGNED → NEW
Comment 16•23 years ago
|
||
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•23 years ago
|
||
I'll go review the patch now.
Assignee | ||
Comment 18•23 years ago
|
||
thanks Seth. - rajiv.
Comment 19•23 years ago
|
||
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?
Assignee | ||
Comment 20•23 years ago
|
||
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.
Assignee | ||
Comment 21•23 years ago
|
||
Assignee | ||
Comment 22•23 years ago
|
||
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•23 years ago
|
||
+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•23 years ago
|
||
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 *.
Assignee | ||
Comment 25•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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() ;
+ }
+
Assignee | ||
Comment 28•23 years ago
|
||
Comment 29•23 years ago
|
||
rajiv, what about bienvenu's comments on 2001-09-19 12:14?
Comment 30•23 years ago
|
||
the last patch looks safe for i18n, I'll wait to hear rajiv's response to
bienvenu comment before finishing the super review.
Assignee | ||
Comment 31•23 years ago
|
||
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•23 years ago
|
||
Comment on attachment 50482 [details] [diff] [review]
patch after integration, updated for making Populate.. unicode friendly
sr=sspitzer
Attachment #50482 -
Flags: superreview+
Reporter | ||
Updated•23 years ago
|
Component: Composition → Simple MAPI
Updated•23 years ago
|
Whiteboard: PDT+ → [PDT+]
Assignee | ||
Comment 34•23 years ago
|
||
feature done, has r and sr.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 35•23 years ago
|
||
Checked into the branch.
Updated•20 years ago
|
Product: MailNews → Core
Updated•16 years ago
|
Product: Core → MailNews Core
You need to log in
before you can comment on or make changes to this bug.
Description
•