Closed
Bug 202355
Opened 21 years ago
Closed 21 years ago
[mach-o] Can't send AppleSingle and AppleDouble attachments
Categories
(MailNews Core :: Composition, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.4beta
People
(Reporter: cavin, Assigned: cavin)
Details
Attachments
(1 file, 2 obsolete files)
20.13 KB,
patch
|
cavin
:
review+
sfraser_bugs
:
superreview+
sspitzer
:
approval1.4b+
|
Details | Diff | Splinter Review |
The code is not turned on for OS X in mailnew/compose.
Assignee | ||
Updated•21 years ago
|
Summary: ([mach-o] Can't send Apple single and double attachments → [mach-o] Can't send Apple single and double attachments
Assignee | ||
Comment 1•21 years ago
|
||
not sure which of these we will get to, but here are some test cases: send email from eudora 5.2 binhex apple single apple double uuencode base64 send email from 4.x (CFM) mime uuencode email from 7.02 (mozilla, CFM) sending email from mach-o to: eudora 4.x 7.02 7.1 when sending / receiving, we should try simple images and sending applications (which are true apple double documents)
Assignee | ||
Comment 2•21 years ago
|
||
let's start with: 1) receiving apple double (an application, like GraphingCalculator) 2) sending apple double (an application, like GraphingCalculator)
Assignee | ||
Comment 3•21 years ago
|
||
Sending and receiving apple double files seems to work now. We don't seem to send files in apple single format but receiving apple single files from eudora works fine.
Assignee | ||
Comment 4•21 years ago
|
||
Fix apple single/double sending and receiving problems.
Assignee | ||
Comment 5•21 years ago
|
||
In nsMsgAppleCodes.h, I have to turn on: #pragma options align=mac68k to make encoding of resource folk work.
Assignee | ||
Updated•21 years ago
|
Attachment #121302 -
Flags: review?(ccarlen)
Comment 6•21 years ago
|
||
Comment on attachment 121302 [details] [diff] [review] Proposed patch, v1 Just a few little things. With them, r=ccarlen >Index: mailnews/compose/src/Makefile.in >=================================================================== >RCS file: /cvsroot/mozilla/mailnews/compose/src/Makefile.in,v >retrieving revision 1.65 >diff -u -w -r1.65 Makefile.in >--- mailnews/compose/src/Makefile.in 15 Mar 2003 01:03:16 -0000 1.65 >+++ mailnews/compose/src/Makefile.in 22 Apr 2003 18:20:17 -0000 >@@ -126,6 +126,17 @@ > nsSmtpDelegateFactory.h \ > $(NULL) > >+# MacOSX requires the MoreFiles module >+ifeq ($(OS_ARCH),Darwin) According to cls, you should use the same toolkit test here: >+ifneq (,$(filter cocoa mac, $(MOZ_WIDGET_TOOLKIT))) as above instead of Darwin test. >+REQUIRES += macmorefiles \ >+ $(NULL) >+CPPSRCS += nsMsgAppleDoubleEncode.cpp \ >+ nsMsgAppleEncode.cpp \ >+ $(NULL) >+EXPORTS += nsMsgAppleDouble.h \ >+ $(NULL) >Index: mailnews/compose/src/nsMsgAppleDoubleEncode.cpp >=================================================================== >@@ -64,7 +67,14 @@ > *encoding = NULL; > > FInfo fndrInfo; >+#if defined(XP_MAC) > OSErr err = FSpGetFInfo( fs->GetFSSpecPtr(), &fndrInfo ); >+#else >+ FSSpec fsSpec; >+ FSPathMakeFSSpec((UInt8 *)fs->GetNativePathCString(), &fsSpec, NULL); >+ OSErr err = FSpGetFInfo (&fsSpec, &fndrInfo); Granted, this code is full of FSSpec usage, but try not to introduce any new FSSpec usage into XP_MACOSX code if there's a way to do the same thing using FSRefs. You can do the same thing here by: FSPathMakeRef() followed by FSGetFinderInfo(). >Index: mailnews/compose/src/nsMsgAttachmentHandler.cpp >=================================================================== >+#if defined(XP_MAC) || defined(XP_MACOSX) >+PRBool nsMsgAttachmentHandler::HasResourceFolk(FSSpec *fsSpec) It's Fork with an 'r' >Index: mailnews/compose/src/nsMsgAttachmentHandler.h >=================================================================== >Index: mailnews/base/src/Makefile.in >=================================================================== >RCS file: /cvsroot/mozilla/mailnews/base/src/Makefile.in,v >retrieving revision 1.103 >diff -u -w -r1.103 Makefile.in >--- mailnews/base/src/Makefile.in 15 Mar 2003 01:03:13 -0000 1.103 >+++ mailnews/base/src/Makefile.in 22 Apr 2003 18:20:18 -0000 >@@ -108,6 +108,11 @@ > nsCidProtocolHandler.cpp \ > $(NULL) > >+# MacOSX requires the MoreFiles module >+ifeq ($(OS_ARCH),Darwin) Use the toolkit test, like above. >+REQUIRES += macmorefiles >+endif >Index: mailnews/base/src/nsMessenger.cpp >=================================================================== > nsresult rv = NS_OK; >-#if defined(XP_MAC) >+#if defined(XP_MAC) /* reviewed for 1.4, XP_MACOSX not needed */ What do you mean here? > /* We need to truncate the name to 31 characters, this even on MacOS X until the file API > correctly support long file name. Using a nsILocalFile will do the trick... > */ > if (NS_SUCCEEDED(mimeinfo->GetMacType(&aMacType)) && NS_SUCCEEDED(mimeinfo->GetMacCreator(&aMacCreator))) >+#if defined(XP_MAC) > realSpec.SetFileTypeAndCreator((OSType)aMacType, (OSType)aMacCreator); >+#else >+ { >+ nsCOMPtr<nsILocalFileMac> macFile = do_QueryInterface(outputFile, &rv); >+ if (NS_SUCCEEDED(rv) && macFile) >+ { >+ macFile->SetFileCreator((OSType)aMacCreator); >+ macFile->SetFileCreator((OSType)aMacType); >+ } >+ } >+#endif XP_MAC and XP_MACOSX should be the same here - they both have nsILocalFileMac. Not that we really care about XP_MAC, but no need to throw in additional #ifdefs for it. > } > } > >Index: mailnews/base/util/nsMsgUtils.cpp >=================================================================== > nsresult NS_MsgHashIfNecessary(nsCAutoString &name) > { >-#if defined(XP_MAC) >+#if defined(XP_MAC) || defined(XP_MACOSX) > const PRUint32 MAX_LEN = 25; Why is XP_MACOSX being limited to this value? Are we using FSSpecs here too? > #elif defined(XP_UNIX) || defined(XP_BEOS) > const PRUint32 MAX_LEN = 55;
Attachment #121302 -
Flags: review?(ccarlen) → review+
Assignee | ||
Comment 7•21 years ago
|
||
Incorporate comment. Also, the changes to mailnews/mime/src were missing so they are included here.
Assignee | ||
Updated•21 years ago
|
Attachment #121302 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #121465 -
Flags: review?(ccarlen)
Comment 8•21 years ago
|
||
Comment on attachment 121465 [details] [diff] [review] Proposed patch, v2 Just 2 things - fix those and r=ccarlen >Index: mailnews/compose/src/nsMsgAttachmentHandler.cpp >=================================================================== >+#if defined(XP_MAC) || defined(XP_MACOSX) >+PRBool nsMsgAttachmentHandler::HasResourceFork(FSSpec *fsSpec) >+{ >+#if defined(XP_MACOSX) >+ FSRef fsRef; >+ if (::FSpMakeFSRef(fsSpec, &fsRef) == noErr) >+ { >+ FSCatalogInfo catalogInfo; >+ OSErr err = ::FSGetCatalogInfo(&fsRef, kFSCatInfoDataSizes + kFSCatInfoRsrcSizes, &catalogInfo, nsnull, nsnull, nsnull); >+ return (err == noErr && catalogInfo.rsrcLogicalSize != 0); You should be able to do the XP_MACOSX code here. Both FSpMakeFSRef and FSGetCatalogInfo are OS routines that are available in either build. >+ } >+ return PR_FALSE; >+#else >+ long dataSize = 0; >+ long resSize = 0; >+ return (::FSpGetFileSize(&fsSpec, &dataSize, &resSize) == noErr && resSize != 0); >+#endif >+} >+#endif >+ > // else don't need to use apple double. >- if (::FSpGetFileSize(&fsSpec, &dataSize, &resSize) == noErr && resSize == 0) >- sendResourceFork = PR_FALSE; >+#if defined(XP_MAX) || defined(XP_MAXOSX) Typo - XP_MAC, not XP_MAX (both places) >+ sendResourceFork = HasResourceFolk(&fsSpec); >+#endif > } >
Attachment #121465 -
Flags: review?(ccarlen) → review+
Assignee | ||
Comment 9•21 years ago
|
||
>>+#if defined(XP_MAX) || defined(XP_MAXOSX) > >Typo - XP_MAC, not XP_MAX (both places) > >>+ sendResourceFork = HasResourceFolk(&fsSpec); > Good catch, for the tests I did it never went through the path so I did not notice it.
Assignee | ||
Comment 10•21 years ago
|
||
Incorporated comments.
Attachment #121465 -
Attachment is obsolete: true
Assignee | ||
Comment 11•21 years ago
|
||
Comment on attachment 121471 [details] [diff] [review] Proposed patch, v3 Carry over r=ccarlen.
Attachment #121471 -
Flags: review+
Assignee | ||
Updated•21 years ago
|
Attachment #121471 -
Flags: superreview?(sfraser)
Assignee | ||
Updated•21 years ago
|
Flags: blocking1.4b?
Comment 12•21 years ago
|
||
Comment on attachment 121471 [details] [diff] [review] Proposed patch, v3 The XP_MAC stuff looks fine, but I'd like to see bugs filed on two other issues: i) fix the code that decides whether to AppleSingle to be smarter than just sniffing for a resource fork ii) Behave correctly when the user tries to attach a packaged application or document (i.e. something that is really a directory).
Attachment #121471 -
Flags: superreview?(sfraser) → superreview+
Comment 15•21 years ago
|
||
Comment on attachment 121471 [details] [diff] [review] Proposed patch, v3 a=sspitzer
Attachment #121471 -
Flags: approval1.4b+
Assignee | ||
Comment 16•21 years ago
|
||
Fix checked in.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Hardware: PC → Macintosh
Summary: [mach-o] Can't send Apple single and double attachments → [mach-o] Can't send AppleSingle and AppleDouble attachments
Comment 17•21 years ago
|
||
+ macFile->SetFileCreator((OSType)aMacCreator); + macFile->SetFileCreator((OSType)aMacType); I'm copying some of this code for another bug, #182174 that code doesn't look right. shouldn't it be SetFileType()? I'll include it in my patch for the other bug.
Comment 18•21 years ago
|
||
I mean the second line doesn't look right.
Assignee | ||
Comment 19•21 years ago
|
||
Yeah, it's not right.
Comment 20•21 years ago
|
||
I've fixed that line in nsMessenger.cpp.
Assignee | ||
Comment 21•21 years ago
|
||
Thanks Seth, good catch.
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
•