157566 - Enhance MailNews to work with WinZip's "Zip and e-mail" shell extension

Status: RESOLVED FIXED

(MailNews Core :: Attachments, defect)

Description

[parish]

The latest versions of WinZip have a really useful feature that adds "Zip and
e-mail" to the Explorer context (right-click) menu. This means that you can
right-click on foo.exe in Explorer, select "Zip and e-mail" and it opens the
default MAPI client, starts a new message, and attaches foo.zip.

This works just fine with Mozilla, except that by the time you've added some
body text, a Subject, and addressed the mail and then click Send Mozilla barfs
because it can't find the file.

The reason is because WinZip creates the Zip file in %TEMP%\XXXXXX\foo.zip,
where XXXXXX is a random string, but deletes it as soon as the mail client
starts. Looking at the Moz source it appears that Moz doesn't actually attach
the file until you Send the message, by which time WinZip has deleted it.

The behaviour of Moz appears to be by design and there is no way to prevent
WinZip from deleting the file (which is reasonable as programs shouldn't leave
temp files cluttering the disk).

WinZip uses Simple MAPI to perform this function and it occurs to me that it
*must* wait for some acknowledgement from the e-mail program before it deletes
the Zip file.

Can Moz detect that the new message has been started by another program and that
a file has been attached? If so, could it be enhanced to not send the
"acknowledgement" to WinZip until the user clicks send and the file has actually
been attached?

Comment 1

[parish]

I thought about this a bit more and think it would be better if Moz made it's
own temp copy of the file because if it delayed sending the acknowledgement to
WinZip it would leave WinZip in a waiting state which may prevent, or interfere
with, another instance of WinZip starting.

Comment 2

[R.K.Aa.]

related: bug 12865

Comment 3

[WD]

Confirming behavior.

With Eudora for example, this zip file is created in a temp folder as described
in the original bug.   But a copy of this file is made and placed in the folder
for attachments as specified in Eudora's preferences.     This way, when the
email is sent, the file still exists.

Rather than attaching the file f:\temp\<random>\zipfile.zip (as mozilla does)
Eudora attaches d:\in\attach\zipfile.zip

I'm not sure if this is an RFE or a bug.

Comment 4

[Alfonso Martinez]

*** Bug 165786 has been marked as a duplicate of this bug. ***

Comment 5

[Alfonso Martinez]

*** Bug 186896 has been marked as a duplicate of this bug. ***

Comment 6

[Cyril Chaboisseau]

Attachment: patch for bug 189174 and 157566

here is a fix for the following bugs :
(corrected by an external company by someone named Eric HAUSWALD)

bug #189174 which also depends on 157566
http://bugzilla.mozilla.org/show_bug.cgi?id=189174
http://bugzilla.mozilla.org/show_bug.cgi?id=157566


actually, this patch corrects 2 bugs :

one triggered by WinZip/WinRAR w/ MAPISendMail (in function
nsMapiHook::HanddleAttachments) and the other one that uses the older
MAPISendDocuments (through function
nsMapiHook:PopulateCompFieldsForSendDocs)

so now, when a file is sent from an application that deletes it
afterwards or if you modify the file before you send the email, it's the
attachment's version copy when the MAPI function is called that is used

this patch is tested against Mozilla version 1.3.1 and 1.4b with both Winrar
and the old DMS for which I reported the bug
<http://bugzilla.mozilla.org/show_bug.cgi?id=189174#c4> and that is using the
old MAPI function

Comment 7

[parish]

Kudos to you Cyril for doing this. I'm going to put the patch into my local tree
and try it out.

One thought/suggestion though:

> so now, when a file is sent from an application that deletes it
> afterwards or if you modify the file before you send the email,
> it's the attachment's version copy when the MAPI function is
> called that is used

I wonder if, for a more complete solution, it would be better to check if the
original file exists and, if it does, attach that (as per the current behaviour)
and only attach Moz's local copy if the original doesn't exist. What do you think?

Comment 8

[Sam Katz]

Not that I care, since I use The Bat!, (though kudos to you for fixing the 
bug).. why isn't this checked in? I don't have the ability or the know-how to 
add the required "review ?" flag.

In addition to that, somebody should check this patch thoroughly against 
Thunderbird.. as this is the next fronteir.

(1.4 is out now.)

Comment 9

[Malte Rücker (mbr)]

*** Bug 218795 has been marked as a duplicate of this bug. ***

Comment 10

[Asa Dotzler [:asa]]

Not going to block the release for a feature request. Features need to happen in
the alpha cycles, not the last few days of a final release cycle. Nominating
feature requests for blocking a final milestone release is almost always a waste
of time.

Comment 11

[Henrik Gemal]

dupe

*** This bug has been marked as a duplicate of 103819 ***

Comment 12

[Gary]

If this is being marked as a duplicate, shouldn't the patch be copied over to
the bug 103819 so that it may get resolved?

Comment 13

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified DUP.

Comment 14

[Wayne Mery (:wsmwk)]

severity should be set to normal, not enhancement - definitely not behaving
according to mapi spec

Can patch be transferred to bug 189174 and grab someone's attention to get patch
review?

Comment 15

[Michael Lefevre]

makes more sense to dupe this the other way around...

Comment 16

[Michael Lefevre]

*** Bug 103819 has been marked as a duplicate of this bug. ***

Comment 17

[Michael Lefevre]

cc'ing all the mailnews peers in a somewhat desperate attempt for attention.

if anyone is able to look at this patch which has been awaiting review since
last May, and drive it in, that would be much appreciated.

Scott - for the record, I understand that this bug does affect Thunderbird as well.

Comment 18

[Scott MacGregor]

I'll try to drive this patch in.

At a casual glance it looks like there might be some string changes for this
patch to synch up with the latest stuff before it is ready to get checked in.

Comment 19

[Cyril Chaboisseau]

almost 1 year since a patch has been posted and this bug is still "hanging"

we (the company I work for) think this problem is blocking to use Mozilla "as
is" and with 1.7 coming very fast and dubbed "long-lived stable" release, I
wouldn't like to postpone it forever;
so what are the necessary steps to have this bug nailed down (I'm willing to
take on my personal time if need be to help the correction)

can someone confirm me that if I work whatever hours on that bug (i.e. get the
latest CVS, adapt the patch to the latest version etc.) it will be accepted

...because franckly I already worked a lot on that one and I don't want to have
it forgotten altogether again

Comment 20

[David :Bienvenu]

Cyril, can you attach a -uw diff so I can see what really changed? Thx!

Comment 21

[parish]

I've just merged this patch into my tree and rebuilt. It fails on this line
(from the patch):

+            if (NS_FAILED(rv)) return rv ;
         }
-        rv = aCompFields->SetAttachments (Attachments.get());
+
+        //aCompFields->SetSubject(Subject.get());
+        rv = aCompFields->SetBody(Subject.get()) ;   <<=== THIS LINE
     }
 
     return rv ;


Building deps for
/cygdrive/c/mozilla_src/mozilla/mailnews/mapi/mapihook/src/msgMapiHook.cpp
/cygdrive/c/mozilla_src/mozilla/build/cygwin-wrapper cl -FomsgMapiHook.obj -c
-DOSTYPE=\"WINNT5.1\" -DOSARCH=\"WINNT\" -DHAVE_DEPENDENT_LIBS -DUNICODE
-D_UNICODE  -I../../../../dist/include/xpcom
-I../../../../dist/include/xpcom_obsolete -I../../../../dist/include/string
-I../../../../dist/include/MapiProxy -I../../../../dist/include/appshell
-I../../../../dist/include/windowwatcher -I../../../../dist/include/dom
-I../../../../dist/include/profile -I../../../../dist/include/msgbase
-I../../../../dist/include/pref -I../../../../dist/include/msgbaseutil
-I../../../../dist/include/msgcompose -I../../../../dist/include/mailnews
-I../../../../dist/include/mime -I../../../../dist/include/msgimap
-I../../../../dist/include/necko -I../../../../dist/include/intl
-I../../../../dist/include/editor -I../../../../dist/include/msgdb
-I../../../../dist/include/uriloader -I../../../../dist/include/unicharutil
-I../../../../dist/include/embedcomponents -I../../../../dist/include/msgMapi
-I../../../../dist/include -I../../../../dist/include/nspr    -I.       -TP
-nologo -W3 -nologo -Gy -FdmsgMapi.pdb  -DNDEBUG -DTRIMMED -MD           
-DX_DISPLAY_MISSING=1 -DMOZILLA_VERSION=\"1.8a\" -DHAVE_SNPRINTF=1 -D_WINDOWS=1
-D_WIN32=1 -DWIN32=1 -DXP_WIN=1 -DXP_WIN32=1 -DHW_THREADS=1 -DWINVER=0x400
-DSTDC_HEADERS=1 -DWIN32_LEAN_AND_MEAN=1 -DNO_X11=1 -D_X86_=1 -DD_INO=d_ino
-DMOZ_DEFAULT_TOOLKIT=\"windows\" -DMOZ_APP_NAME=\"mozilla\" -DOJI=1 -DIBMBIDI=1
-DMOZ_VIEW_SOURCE=1 -DACCESSIBILITY=1 -DMOZ_XPINSTALL=1 -DMOZ_JSLOADER=1
-DMOZ_MATHML=1 -DMOZ_LOGGING=1 -DMOZ_USER_DIR=\"Mozilla\" -DMOZ_XUL=1
-DMOZ_PROFILESHARING=1 -DMOZ_PROFILELOCKING=1 -DMOZ_DLL_SUFFIX=\".dll\"
-DJS_THREADSAFE=1 -DNS_PRINT_PREVIEW=1 -DNS_PRINTING=1
-DMOZILLA_LOCALE_VERSION=\"1.8a\" -DMOZILLA_REGION_VERSION=\"1.8a\"
-DMOZILLA_SKIN_VERSION=\"1.5\"  -D_MOZILLA_CONFIG_H_ -DMOZILLA_CLIENT
/cygdrive/c/mozilla_src/mozilla/mailnews/mapi/mapihook/src/msgMapiHook.cpp
msgMapiHook.cpp
c:/mozilla_src/mozilla/mailnews/mapi/mapihook/src/msgMapiHook.cpp(875) : error
C2664: 'SetBody' : cannot convert parameter 1 from 'const unsigned short *' to
'const class nsAString &'
        Reason: cannot convert from 'const unsigned short *' to 'const class
nsAString'
        No constructor could take the source type, or constructor overload
resolution was ambiguous
make[6]: *** [msgMapiHook.obj] Error 2
make[6]: Leaving directory
`/cygdrive/c/mozilla_src/mozilla/Mozilla/mailnews/mapi/mapihook/src'
make[5]: *** [libs] Error 2
make[5]: Leaving directory
`/cygdrive/c/mozilla_src/mozilla/Mozilla/mailnews/mapi/mapihook'
make[4]: *** [libs] Error 2
make[4]: Leaving directory `/cygdrive/c/mozilla_src/mozilla/Mozilla/mailnews/mapi'
make[3]: *** [libs] Error 2
make[3]: Leaving directory `/cygdrive/c/mozilla_src/mozilla/Mozilla/mailnews'
make[2]: *** [tier_97] Error 2
make[2]: Leaving directory `/cygdrive/c/mozilla_src/mozilla/Mozilla'
make[1]: *** [default] Error 2
make[1]: Leaving directory `/cygdrive/c/mozilla_src/mozilla/Mozilla'
make: *** [build] Error 2
red-shift:/cygdrive/c/mozilla_src/mozilla{8}$ 

Comment 22

[timeless]

<+        rv = aCompFields->SetBody(Subject.get()) ;
>+        rv = aCompFields->SetBody(Subject);

Comment 23

[parish]

Thanks timeless, that fixed it. I've done some testing and it works fine:

Explorer context menu, WinZip->Zip and E-mail:

    Attachment is sent correctly
    Temp file created in %TEMP%\moz_mapi
    Temp file deleted on send
    Cancel message (before sending), temp file is deleted
    File->Save in composition, file is saved with the draft but temp file not
    deleted as the compose window is still open
    File->Save As->Draft/Template as File->Save

I've also done some regression testing:

1. Explorer context menu, Send To->Mail Recipient:
    File is sent correctly but no temp file created.

2. Word 2002/Excel 2002, File->Send To->Mail Recipient:
    File is sent correctly.
    Temp file created (as Word/Excel has the original open)
    Temp file deleted on send.
    If the message is cancelled the temp file is deleted.
    File->Save in composition, file is saved with the draft but temp file not
    deleted as the compose window is still open
    File->Save As->Draft/Template as File->Save

Comment 24

[parish]

Attachment: New patch with mod from comment 22

New patch with timeless' mod as per comment 22 also deleted the commented out
line of code before that:

+	 //aCompFields->SetSubject(Subject.get());

Comment 25

[parish]

I'll also test this with Thunderbird soon

Comment 26

[David :Bienvenu]

again, if you do a -uw diff, then I can see what changes you've made that
*aren't* just whitespace changes.

Comment 27

[Michael Lefevre]

Attachment: diff -uw of the updated patch

Comment 28

[Michael Lefevre]

Comment on attachment 146874 [details] [diff] [review]
New patch with mod from comment 22

apologies for spam - I shouldn't have obsoleted this

Comment 29

[parish]

Yep, works with T-bird too.

David: Sorry, missed your request for a diff -uw, but Michael has addressed that.

Michael: You weren't spamming, but can you flag your patch 'review?' and see if
we can't get this bug put to bed.

Comment 30

[Michael Lefevre]

Comment on attachment 146877 [details] [diff] [review]
diff -uw of the updated patch

requesting review

(Parish - re spamming - was just apologising for generating unnecessary bugmail
by making the wrong change and reversing it)

Comment 31

[Scott MacGregor]

Comment on attachment 146877 [details] [diff] [review]
diff -uw of the updated patch

Just some minor comments.

How about removing the braces in the if/else clauses for single line statments
such as:

+		     if (aFiles[i].lpszFileName)
			 {
+			 fileName.Assign(aFiles[i].lpszFileName);
+		     } 
+		     else 
+		     {
+			 fileName = NS_LITERAL_STRING("");
+		     }

2) Why use the obsolete nsFileSpec: nsSpecialSystemDirectory to get the temp
directory  and then converting the file spec to a nsIFile. We can use the
nsIFile equivalent:
http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsIDirectoryService.idl

3) The one part of the change I'm not sure of is creating a sub directory off
of temp for storing these files. We could also try calling CreateUnique on the
nsILocalFile name to force it to get a -##number appended to it. But then the
message may get sent with that file name which we don't want.  If we have to go
the directory route, how about: moz_mapi?

4) Subject.AppendWithConversion(", ");

I think the new string way to do this is to not use withConversion, instead:
Subject.AppendWithConversion(NS_LITERAL_STRING(", ");

Comment 32

[Scott MacGregor]

that last comment should have read:

Subject.Append(NS_LITERAL_STRING(", "));

Comment 33

[Scott MacGregor]

Attachment: branch friendly version of the patch

This version of the patch is friendly for the 0.6 branch and the 1.7 branch
(friendly meaning it will compile :)

I followed my review comments from above: 

1) Removed nsFileSpec::nsSpecialSystemDirectory, using the nsIFile equivalents.

2) used NS_LITERAL_STRING instead of AssignWithConversion
3) fixed some white space issues (i'm sure there are more)
4) tried to simplify the logic a little bit.

I got scared after doing this to make anymore changes to the patch for fear of
breaking something so I stopped.

Comment 34

[David :Bienvenu]

+            nsCOMPtr<nsIMsgAttachment> attachment =
do_CreateInstance(NS_MSGATTACHMENT_CONTRACTID, &rv);
+            if (NS_FAILED(rv) || (!attachment) ) return rv ;
createInstance must return an error on failure, I'd think... so you can just do
NS_ENSURE_SUCCESS(rv, rv);

other than that nit, I'll put an sr on it.

Comment 35

[David :Bienvenu]

Comment on attachment 146877 [details] [diff] [review]
diff -uw of the updated patch

clearing request...

Comment 36

[Scott MacGregor]

FYI folks, this patch is in the curren 0.6 thunderbird branch candidate build:

http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/latest-0.6/

You can help improve the chances of this getting into 1.7 by helping test this
tbird build to make sure it works :)

Comment 37

[Scott MacGregor]

Attachment: trunk version of the fix with david's review comment

Comment 38

[Scott MacGregor]

Comment on attachment 147290 [details] [diff] [review]
trunk version of the fix with david's review comment

carrying over david's sr

Comment 39

[Scott MacGregor]

this is also now fixed on the trunk.

Marking fixed, nominating for 1.7 assuming it tests out ok on the trunk and in
tbird 0.6.

Comment 40

[Scott MacGregor]

Comment on attachment 147181 [details] [diff] [review]
branch friendly version of the patch

nominating for 1.7 pending positive trunk and tbird 0.6 feedback

Comment 41

[parish]

I've repeated all the tests I did in comment 23 on both the T-bird build linked
to in comment 36 and a trunk build of Mozilla and everything works correctly.

Kudos to Cyril, Scott, and David for getting the work done.

Comment 42

[Asa Dotzler [:asa]]

Scott, time is getting short on RC2 and I'd be hesitant to take this after. Have
we seen any negative feedback from 0.6 or the trunk?

Comment 43

[David :Bienvenu]

yes, there might be problems - still investigating.

Comment 44

[Cyril Chaboisseau]

apparently it doesn't work _fully_ :

I've made the tests on both Winzip (+ email option) and our document system and
it works as long as only 1 mail is selected (which is the case by default w/ WinZip)

OTOH, when I select several files to attach, it doesn't work at all (the
composer doesn't even launch itself)

unfortunately I don't have any tool to debug that

the tests were done with Thunderbird and Mozilla (nightly build) 1.8a 
Gecko/20040506

I'm available to do any further testing

Comment 45

[Daniel Kilpatrick]

I have been testing Tbird 0.6b (20040428) since Scott's announcement on 4/29.
I have found that it works properly.
WinZipped one and several items. No problem found
Windows Context Menu SendTo EMail with one and several items. No problem found
I have canceled mails without incident
I have not found any impairment of existing code.
If I had more info I would try to duplicate chaboisseau's problems.
I really want to see this patch make it into Mozilla

Comment 46

[parish]

WFM too, as per comment 41

Cyril:
> OTOH, when I select several files to attach, it doesn't work at all (the
> composer doesn't even launch itself)

Do you mean select several files then Send To->Mail Recipient, or select several
files and Zip And E-mail? Both work here but, if it's Send To->Mail Recipient,
which one are you using? Windows has several Mail Recipients on the Send To menu
but only one (seems to) work with Moz/T-bird and that is Mail Recipient MAPI -
is that the one that you are using? I've deleted the rest as I was fed up with
picking the wrong one.

Comment 47

[Cyril Chaboisseau]

> Parish:
> Do you mean select several files then Send To->Mail Recipient, or select several
> files and Zip And E-mail? Both work here but, if it's Send To->Mail Recipient,
> which one are you using? Windows has several Mail Recipients on the Send To menu
> but only one (seems to) work with Moz/T-bird and that is Mail Recipient MAPI -
> is that the one that you are using? I've deleted the rest as I was fed up with
> picking the wrong one.


I meant having a program (unfortunately not Winzip) that will generate several
files to be sent

whatever you do under Winzip it will always generate only one .zip file to be sent

the software I'm using (the document management system) allow the sending of one
or several document at once and while it works well with 1 file to be sent, the
MAPI calling fails completely when I choose to sent several files
(i.e. the composer doesn't even open)

I guess that the MAPI functions allow sending several document but
unfortunately, I don't have a way for you to test that fully (maybe with a C
program that calls MAPI with several arguments ?!!)

Comment 48

[David :Bienvenu]

our mapi test program allows you to send multiple files. I'll try it.

Comment 49

[David :Bienvenu]

Also, do you know what the file names are that your program is sending? Are they
just leaf names, e.g., doc1.doc, or fully qualified paths, e.g., c:\tmp\foo.doc
? I believe the latter doesn't work in the single or multiple file case.

Comment 50

[parish]

Cyril, did sending multiple files from your program work before?

Comment 51

[Cyril Chaboisseau]

(In reply to comment #49)
> David :
> Also, do you know what the file names are that your program is sending? Are they
> just leaf names, e.g., doc1.doc, or fully qualified paths, e.g., c:\tmp\foo.doc
> ? I believe the latter doesn't work in the single or multiple file case.

the file look like the following : FILE123_.XLS

(In reply to comment #50)
> parish :
> Cyril, did sending multiple files from your program work before?

yes,
but it never worked with any Mozilla but those with the patched DLL that was
provided back in the 1.3 / 1.4 era (built with the patch I sent with attachement
id #124311 along with bug #189174)

so no official release could work with sending 1 or multiples files (only a
patched version here where I work)

if you want I can provide the binary DLL

Comment 52

[parish]

Thanks Cyril, I was trying to establish whether this fix had broken something
which was working before.

My gut feeling is that this fix should go into the next release and the problem
you are seeing should be addressed in bug 189174 as it is related but not the
same issue as this bug. What is puzzling is that you can select multiple files
in Explorer and Send To->Mail Recipient MAPI and it works. I would expect
Explorer to use the same mechanism as MS Office, which you say your app is part
of (comment 4 in bug 189174).

Comment 53

[Cyril Chaboisseau]

(In reply to comment #52)
> parish:
> Thanks Cyril, I was trying to establish whether this fix had broken something
> which was working before.

indeed it never worked on stock release but somehow at one point it worked here
(with my patch against 1.3 and 1.4.x) and now the re-engineered patch doesn't
anymore !

> My gut feeling is that this fix should go into the next release

which one ?
1.7 (long lived) or 1.8 ?!

I really hope that it could go with RC2 and even if it doesn't fully work (fine
with one file but fail with many) it's better than nothing
...then Winzip issue at least is resolved

> and the problem
> you are seeing should be addressed in bug 189174 as it is related but not the
> same issue as this bug.

I think it is related to the same issue

> What is puzzling is that you can select multiple files
> in Explorer and Send To->Mail Recipient MAPI and it works.

even when you delete all the files afterwards ?

maybe the MAPI is not called the same way (I've seen somewhere that MAPI could
be used in different ways)

could it be that MAPI is called several times which make Mozilla/T-bird to fail
launching the composer with the right files attached while Explorer calls MAPI
once with multiple parameters ?


> I would expect Explorer to use the same mechanism as MS Office, which
> you say your app is part of (comment 4 in bug 189174).

no !

my app is triggered by MS-Off but is clearly appart from it

Comment 54

[Wayne Mery (:wsmwk)]

(In reply to comment #53)
> (In reply to comment #52)
> > parish:
> > Thanks Cyril, I was trying to establish whether this fix had broken something
> > which was working before.
> 
> indeed it never worked on stock release but somehow at one point it worked here
> (with my patch against 1.3 and 1.4.x) and now the re-engineered patch doesn't
> anymore !
> 
> > My gut feeling is that this fix should go into the next release
> 
> which one ?
> 1.7 (long lived) or 1.8 ?!
> 
> I really hope that it could go with RC2 and even if it doesn't fully work (fine
> with one file but fail with many) it's better than nothing
> ...then Winzip issue at least is resolved

I agree, push it out - far better to work with one than none (besides, one file
may be the majority requests anyway).

Comment 55

[Asa Dotzler [:asa]]

Scott, David, we're down to the wire on RC2 (and unlikely to take features like
this after RC2) Is this something you feel comfortable landing today/tonight? If
not, then it's probably not going to make 1.7.

Comment 56

[Scott MacGregor]

no this should not go into 1.7. David found some nasty regressions that this
patch caused.

Comment 57

[Cyril Chaboisseau]

(In reply to comment #56)
> no this should not go into 1.7. David found some nasty regressions that this
> patch caused.

arghhh !!!!

how / when / why

where _exactly_ the patch failed ?

how much / who should I bribe^Wdonate to have this patch accepted
;-)

...just kidding


no, frankly sorry for this but I really tought it could make it
:-(

Comment 58

[David :Bienvenu]

the two known failures are that if you haven't saved a file in word 2003, and
try to send, you get an error (the copy code fails because the file path
contains a drive specification, e.g., c:), and if you try to send an excel 2002
document, the filename created is some hex number.tmp, instead of the name of
the excel document...

Comment 59

[parish]

David: Is the Word failure specific to Word 2003? I can't reproduce the problem
with Word 2002. Also, I can't reproduce the Excel problem with Excel 2002 (or
was that a typo and should have been Excell 2003?).

I'm testing this with Thunderbird rather than Moz, but it's the same code isn't it?

Comment 60

[David :Bienvenu]

I only have word 2003 here, so I can only speak to that. The excel 2002 problem
was reported by a user, and it was reported that the problem went away when the
patch was reversed. All of this is with Scott's patch that was checked in. I
have not tried your original patch - I'll try to get to that soon.

Comment 61

[parish]

Cyril was the original patch author, not me, but I am using the checked-in trunk
patch in my own builds, although I did try the T-bird branch candidate build
(comment 36) as well.

I've got Word 2002 and Excel 2002 here so if there is any specific test you
would like running (and/or any specific build you would like testing) just let
me know.

Comment 62

[David :Bienvenu]

Re excel 2002, can you try creating an empty spreadsheet, create a cell, and
then try to send it, w/o every saving the file?

I'm unable to reproduce the problems I was having before, and it occurs to me
now that those problems were because the lpszFilename was really a file path -
and that's not happening now. I don't know if I had a mismatch between
msgMapi.dll and mapi32.dll or what...

Comment 63

[David :Bienvenu]

So I'm not convinced that there is any problem with this patch...

Comment 64

[parish]

Re: comment 62. It works correctly here.

Comment 65

[Cyril Chaboisseau]

apparently the patch made it through 1.7rc2 but still fails in some ways :

it works fine with Winzip (zip + email) but doesn't with my application

what is most interresting is the fact that the patch I've sent one year ago
works well with Mozilla 1.3 and 1.4 but this one (re-engineered) doesn't with my
document management application !

Comment 66

[Jan Derk]

The current fix does not work for many applications, because it requires
lpszFileName to be set.

This parameter is optional in the simple MAPI definition:
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/mapi/html/_mapi1book_mapifiledesc_simple_mapi_.asp

<quote>
lpszFileName
Pointer to the attachment filename seen by the recipient, which may differ from
the filename in the lpszPathName member if temporary files are being used. If
the lpszFileName member is empty or NULL, the filename from lpszPathName is used.
</quote>

It is very simple to fix though. Just get lpszFileName from lpszPathName if it
has not been specified.

Comment 67

[Scott MacGregor]

Attachment: attempt to fix someof the behavioral issues

I'm not sure why this is the case, but we were only copying the file to
temp\moz_mapi if the following conditional was met:

if (dirPath.Find(tempPath, PR_TRUE) == 0 || aFiles[i].lpszFileName)

tempPath was the path to the temporary directory. Dirpath was the path of the
file we are being asked to send.

I got rid of this completely and said "always copy to tempdir\moz_mapi.

I then simplified the complexity of this routine quite a bit by getting rid of
duplicate code, using some nsILocalFile methods to reduce some work we were
doing by hand, etc.

I'll attach another patch which shows the routine as it looks now (not as a
diff) as that might be more readable and understandable.

This should fix this issue:
http://bugzilla.mozilla.org/show_bug.cgi?id=157566#c66

Cyril, do you have the means to try out this patch in a build of your own to
see if it fixes:
http://bugzilla.mozilla.org/show_bug.cgi?id=157566#c65 ?

Comment 68

[Scott MacGregor]

Attachment: here's the new routine in its entirety instead of as a diff

I think it's more readable to see the new routine in its entirety instead of
the patch I made.

Comment 69

[David :Bienvenu]

Comment on attachment 148878 [details]
here's the new routine in its entirety instead of as a diff

looks good - one spelling nit:

	    // copy the file to it's new location and file name


just "its"

Comment 70

[Cyril Chaboisseau]

(In reply to comment #67)
[...]
> This should fix this issue:
> http://bugzilla.mozilla.org/show_bug.cgi?id=157566#c66

great !

thanks a lot

> Cyril, do you have the means to try out this patch in a build of your own to
> see if it fixes:

actually no, I don't have access to a Windows devel environement anymore
:-(

but if someone is kind enough to provide me the MOZMAPI.DLL against 1.7RC2 I'd
be happy to make the the tests
(or if the nightly build could include this patch then I could download the
freshly built version and test it as well)

Comment 71

[Scott MacGregor]

I'll check this into the trunk tonight and leave the bug open so we can all play
around with it in a release build (for thunderbird and seamonkey)

Comment 72

[Scott MacGregor]

Ok it's in. Let's try this out in the 05/20 thunderbird and seamonkey trunk
builds to see how it looks.

Comment 73

[Scott MacGregor]

Cyril/Jan, have you had a chance to try out a current trunk build (5/20 or
later) to see if this supplemental patch is working as expected?

Comment 74

[Jan Derk]

The good news is: if lpszFileName is NOT set everything works as advertised. So
the issue in Comment #66 has been fixed.

The bad news is: There is a serious regression. The new patch does not work if
lpszFileName IS set when sending multiple attachments. This used to work with
the old patch. You won't see this bug in winzip as winzip only sends 1 single file.

Not sure what happens exactly, but if lpszFilename is set and I send 10 images
named image1.jpg, image2.jpg ... image10.jpg. Only the last image10.jpg will
show up as attachment and curiously a new file named "mapi23.dll". All other
files are ignored.

Comment 75

[Jan Derk]

Forgot to say: I checked in Mozilla Build ID:2004052208

Comment 76

[Jan Derk]

The problem I am seeing (Comment #74) is probably in my own test application.
The patch is probably working as it should. Just ignore Comment #74.

Comment 77

[Jan Derk]

Checked better this time. Everything works as advertised with the last patch.
Single attachments, multiple attachments, both with and without the lpszFilename
set. All works fine with the latest trunk. 

Comment 78

[Scott MacGregor]

The following attachment:

http://bugzilla.mozilla.org/attachment.cgi?id=148878&action=view

has now been checked into the old M4 branch and the aviary-1.0 branch in
addition to the trunk.

Reclosing until someone comes up with more issues :)

Comment 79

[Cyril Chaboisseau]

so, I've tested the lastest Thunderbird 0.6+ 20040523 and just like the previous
release, it works fine with 1 attachement but whenever I try to attach several,
it fails (without even opening the composer)

needless to say that Mozilla 1.8.a2 build 20030523 performs the same way

Comment 80

[Cyril Chaboisseau]

hi there,

sorry to disturb but I've tested the latest 1.7rc3 and it seems that some stuff
has been reverted which makes the situation worse concerning the bug

actually, with 1.7rc2 I could at least send _one_ file with my document
management system but now it's not possible anymore (neither one nor several
which was not possible before anyway)

any clue on what could have caused this ?

if this is not the right bug/place to discuss this special case (altough still a
MAPI issue) please let me know