Closed Bug 134492 Opened 23 years ago Closed 21 years ago

Print list of attachments / List of attached files in printed E-Mails

Categories

(MailNews Core :: MIME, defect)

Product:
MailNews Core
Component:
MIME
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: BenB, Assigned: BenB)

References

(Blocks 1 open bug)

Details

(Whiteboard: Don't spam me, it hurts :-()

Attachments

(3 files, 5 obsolete files)

Proposed Fix, Version 3, diff -uw
6.50 KB, patch
dmosedale
: review-
Details | Diff | Splinter Review
Rev 5 - brenden's comments
7.65 KB, patch
klteuscher
: review+
klteuscher
: superreview+
Details | Diff | Splinter Review
Idea for a Design of the attachment information box
1.40 KB, text/html
Details 
When printing out, there should be at least a list of attachments. urrently, we
seem to silently drop that inof during printing, if the attachments are not
shown inline in the viewer.
Is this 4xp?
Note: I just picked this report from the webforum at heise.de. I should have
filed it unconfirmed.
Keywords: 4xp
QA Contact: gayatri → trix
*** Bug 110944 has been marked as a duplicate of this bug. ***
*** Bug 141076 has been marked as a duplicate of this bug. ***
-->varada
Assignee: ducarroz → varada
Summary: Print list of attachments → Should print headers when print compose message
"compose" (in summary)? I meant message displayed in the the msg viewer, not the
Mailnews Composer.
The summary should contains "attachment" . I think this bug should get attention
because lot of people print mail and they want the file attachment list!

MacOS 8.6.
Maybe ducarroz just misunderstood the bug. Restoring summary until he explains
what he meant.
Summary: Should print headers when print compose message → Print list of attachments
*** Bug 159846 has been marked as a duplicate of this bug. ***
*** Bug 160525 has been marked as a duplicate of this bug. ***
*** Bug 161864 has been marked as a duplicate of this bug. ***
Setting OS to all as some of the dupes are Windows OSes.
Is MIME the best component for this or is it better in Printing?
I know this feature was in NS4.x, but should this be marked as an enhancement?
OS: Linux → All
This feature should be an priority. Lot of people at the office ask for. 
I got asked about this again from a business customer.  Is there any chance
of getting this worked on any time soon?  
--Tony
aewell@gbis.com
I installed Mozilla 1.1 (Version Info shows: 
Mozilla/5.0 (Windows; U; Windows NT 5.0; de-AT; rv:1.1) Gecko/20020826 )

I tried to print out a mail where a Word-document was attached.
The text from the mail was printed fine, but there is still NO INFO about
the attachements included in this mail on the printed page.
Some old version of Netscape had this feature and it was very useful.
Here in my company the big boss wants this feature.
And he told me, that Microsoft Outlook has this feature.
Please implement this feature as soon as possible, otherwise I have
to install Microsoft Outlook on some PCs here in my company, and I
really do not want to do this.
Thanks!

*** Bug 167953 has been marked as a duplicate of this bug. ***
*** Bug 168540 has been marked as a duplicate of this bug. ***
In earlier versions the list of attachements was printed out.  
Netscape 6.1 does it, Netscape 6.2 doesn't.  
So the bug was implemented in the versions arount 0.92x til 0.94x (Windows). 
*** Bug 177263 has been marked as a duplicate of this bug. ***
Thanks for R.K.Aa for moving me over. May I consider that the description or
keyword selection is to closely formulated as there are many new bugs created
that are in reality only new instances of this one?

I did some researches but was not able to find anything though I assumed that
ther e must be others looking for the feature.

In my company everybody is still using Netscape 4.x as most of the people rely
on their paper print outs and even delete the messages in their message base
from time to time. With the print they need to see which files they have send in
the E-Mails, mostly drawings.

Are there any information whether somebody is working on this or whether this
topic is not regarded as worth working on?
Tweaking summary to try and stop getting so many dupes.
Summary: Print list of attachments → Print list of attachments / List of attached files in printed E-Mails
*** Bug 182296 has been marked as a duplicate of this bug. ***
We switched from Netscape 4.7x to Mozilla 1.2 without being aware of this
problem. Printing a list of attachments is an absolute essential function. I
really cannot understand why this functionality was removed after Netscape 4.7x
We often rely on printouts of our e-mails and of course sent / received
attachments are part of the e-mails.
Please give me an idea when a reimplementation of this feature will possibly happen.


Bernhard Goebl : 
"was removed after Netscape 4.7x"

Mozilla is not a new version of netscape. (Mozilla is new Code)
We never had this FEATURE but we are open source and we accept a patch.


Bernhard Goebl : 
"was removed after Netscape 4.7x"

Matti:
Mozilla is not a new version of netscape. (Mozilla is new Code)
We never had this FEATURE but we are open source and we accept a patch.

Until Mozilla ~0.92 the feature was implemented (Netscape 6.1), afterwards it
was removed. I've no idea why...
-> It was removed after Mozilla 0.92
Bernhard, Mozilla is not intended for end-user (including company) use, unless
you distribute it as your own product. If you want this bug fixed, pay someone
to write a patch.

Christian:
> Until Mozilla ~0.92 the feature was implemented

Can you prove that?
Whiteboard: Don't spam me, it hurts :-(
So Netscape is supposed to add this "patch" to all versions of Mozilla it
repackages?

I took a few minutes, downloaded 0.9.2, 0.9.3, 0.9.4.  The attachement box is
displayed in 0.9.2 and 0.9.3.  None in 0.9.4.

Here's the proof:
http://www.wbgroup.com/mozilla/test_0.9.2.pdf
http://www.wbgroup.com/mozilla/test_0.9.3.pdf
http://www.wbgroup.com/mozilla/test_0.9.4.pdf 

With attitudes like that, no wonder Microsoft is gaining more market share.
taking all of varada's bugs.
Assignee: varada → sspitzer
Hi All,

    This sounds like a some what simple cut and paste from 0.9.3 to
solve this problem.  Can we please upgrade the priority on this?

--Tony
aewell@gbis.com
Copying code from 0.93 would be the fastest way. Great idea.
I vote for an upgrade of the priority as well.

at Sören 'Chucker' Kuklau:
"Bernhard, Mozilla is not intended for end-user (including company) use"

Can you please tell me who mozilla is for?
Everybody should be happy if mozilla is widely spread. Furthermore bug-reports
from companies using mozilla on many PCs in their network are a valuable
information.
I offer a US $ or € reward of 500 for somebody fixing the bug.
Thanks for the offer!

reassinging to me

Seth, I assume it's fine with you to take your bug. If not, please undo it.

If anybody else wants to beat me (i.e. be faster than me and take the money),
please don't hestitate to contact me or post here (so that we don't duplicate work).
Assignee: sspitzer → ben.bucksch
Regression caused by fix for bug 71865. Found thanks to Eric Cantin's comment
27. Unapplying the changes for that bug fixes this one (with 1.0.x). Now looking
into how to still keep bug 71865 fixed, if necessary.
I have a fix.

But it outputs the attachment meta info for every attachment. I don't know how
to change that, because I don't have (and I don't know how to get) the info
about which attachments are displayable. Even if I found where they are
displayed, I don't think I would there have the info about when *not* to output
the attachments box (e.g. during message display). I hope that's not a problem
anyways.

The fix just undo the fix for bug 71865 and then adds a conditional statement to
make that old fix to depend on whether we're printing or not.
Status: NEW → ASSIGNED
Not that extensively tested, but I think it's OK.
Most of the new code is what has been removed back then.
> But it outputs the attachment meta info for every attachment.

hah, that's actually how it was back then in 0.9.3 as well.
The whole method of displaying attachments is seriously flawed.  As the
attachments currently display, the length of the display is totally inadequate,
and, most importantly, the extender is NOT displayed.  It would be preferable to
display attachments below the other header information, with the full filename
rendered so that a user can distinguish between xxx.mpg and xxx.mpg.pif.  This
is a serious security problem.
Ronald Hunter, I have no idea what the heck you're talking about. The extension
*is* being displayed, see comment 27. We're talking about *printing* here.
Attachment #111272 - Flags: review?(ducarroz)
Hi All,

   Not to ask too stupid of a question, but for us mere users, is the fix 
available for us to download and try out?

Many thanks,
--Tony
aewell@gbis.com
It might end up in Beonex Communicator 0.8.3
Comment on attachment 111272 [details] [diff] [review]
Proposed Fix, Version 1, diff -uw

I haven't tested it but looks  fine. The all code from 4.x which use to display
a table for each attachment is back but trigerred only when printing.
R=ducarroz
Attachment #111272 - Flags: review?(ducarroz) → review+
Comment on attachment 111272 [details] [diff] [review]
Proposed Fix, Version 1, diff -uw

J-F, would it be too much to ask to compile the code on Mac and - if possible -
on Windows? I don't have build environments for that, so I cannot test it
there.
Attachment #111272 - Flags: review+ → review?(ducarroz)
This patch build fine on Mac. I don't have a build env for windows right now...
Just to be sure... is this patch also print list of attached files for drafted
and sent messages? BTW thanks to Stefan for is offer and Ben for is work! Really
appreciated!
Is there a windows patch avialable yet?
Attached file Obsoleted (obsolete) — 
Includes new 'emitter.dll' for 1.2.1 and 1.3.a
So far, that patch works perfectly in Mozilla 1.2.1, and even for Netscape 7.01.

Thanks!
Great, it works perfectly. 

Is it possible to do something similar to the reply of a message. I would really
like to have an option where you can chose to display the attachments in your
reply, see bug 118962. Maybe you can reuse some of the code?  


Yes, it would be possible and fairly trivial to do that. The question (and the
reason why I specifically didn't do that) is, if that's wanted, see my comments
there.
Yes probably it would be best to discuss this at the other topic. I would love
it though - as plain text or simple HTML or whatever you call it, just to make
sure that it will not harm my system (Though I don't care to much about my linux
system and more about the windows machines around).

Is there a possiblity to supply the patch also for linux (SuSE 8.0) without
recompiling it completely?
I am using the patch.  It seems to work in all cases but one.  When somebody
forwards a message to me and the original message has an attachment this
attachment does not get printed.
Attachment #111272 - Flags: superreview?(bienvenu)
Attachment #111272 - Flags: review?(ducarroz)
Attachment #111272 - Flags: review+
Comment on attachment 111272 [details] [diff] [review]
Proposed Fix, Version 1, diff -uw

heh, ops, that didn't work. I tried to set ducarroz's review flag again, after
he confirmed build on Mac and <sten@antisocial.com> confirmed build on MS
Windows (thanks to both).
Attachment #111272 - Flags: review+ → review?(ducarroz)
nsCRT::strcmp is deprecated in favor of just strcmp

don't use PR_FREEIF unless you need to null out the pointer - just use PR_Free :
+  PR_FREEIF(newValue);

Strange. I grabbed the zip archive from comment #46 and replaced emitter.dll in
the  components subdirectory of Mozilla (Version 1.2.1). All I get are memory
faults (access violation) under both Windows 2000 and Windows 98 by just viewing
mails, not even attempting to print them out. Is this really the fix? 
Mozilla hang when i show a printview or print when i m exiting to the display
message with XP home edition, Mozilla 1.2.1 IMAP account
The patch is for all platforms. If you want to try it out, you have to compile
it yourself or wait until full builds are made (e.g. mozilla.org's nightlies or
Beonex Communicator 0.8.2). Copying over the dll posted here by sten (it is
against bugzilla rules to attach binaries, BTW, it would fill up bugzilla's
store, but it's good to know that it compiles on Win) may or may not work. If it
doesn't work, the result could well be arbitary crashes during normal display
(nor printing) of messages. That's not the result of my patch, but of a binary
incompatibility between that dll and your build.
Attachment #112391 - Attachment is obsolete: true
OK, I will download a nightly build and test it out. Thanks, Ben.
*sigh*, there are no nightlies yet, because it's not yet checked in.
QA Contact: stephend → esther
Under the current fix, the attachment list is not printing for the sent folder 
messages(works fine for the inbox messages). The sent folder attachment list was 
printed like the inbox attachment list under Netscape 4.x.
Can this also be fixed. Thank You
version:Mozilla 1.2.1, Windows 2000, sp2 using new emitter.dll
Addressing bienvenu's comments.
Attachment #111272 - Attachment is obsolete: true
Attachment #113106 - Flags: superreview?
Mark <office@bennettyoung.com>: WFM.
Attachment #113106 - Flags: superreview? → superreview?(bienvenu)
OK, this one should actually compile *hide* and work.

I just noticed that when printing multiple inline attachments, the stats boxes
for all these are cumulated at the end, meaning that you have attachment 1 [details] [diff] [review]
inline, attachment 2 [details] [diff] [review] inline, attachment 1 [details] [diff] [review] stats box, attachments 2 stats box.
"Suboptimal". But I think that was the old behaviour, I just wanted to fix the
regression here.
Attachment #113106 - Attachment is obsolete: true
Attachment #113108 - Flags: superreview?(bienvenu)
Ben,
Not sure what happened. Something got corrupted in ver. 1.2.1 and I could not view
any message content. This happened after trying to print a sent message and
discovering the attachment list was not present. I then tried restarting the
software etc., to no avail. Also noticed that compacting my Inbox(780Mb) turned out
to be a 2 hour ordeal. I then checked Mozilla.org and found that this was a bug.
Decided to download and install 1.3A. Everthing now working fine.
Thanks for checking into the problem.
Hi All,

   I installed the 1.2.1 emitter.dll patch from comment #46 and loaded it 
on several customers with 1.2.1.  Every user noticed that when they went to 
reply to a message with an attachment, that 1.2.1 dies an instant, quick, 
rapid death.  :-(

   Upgrading them to 1.3b (1.3.0.2003013108) and using the emitter.dll for 1.3
solved the problem.  :-)

--Tony
aewell@gbis.com
*** Bug 193336 has been marked as a duplicate of this bug. ***
There is a preliminary build for Beonex Communicator 0.8.2 Windows
<http://download.developer.beonex.com/dev/beonex-comm-0.8.2-win1.zip>. It
contains no installer and is not official. It bases on Mozilla 1.0.2 branch and
additionally contains the fix for this bug. Please do not comment about the
build itself here, it is merely a courtesy for those eager to get a build with
this bug fixed.
Can anybody say me, please, if this patch is planned into release? (And which
release?)

I'm currently waiting for the (super)reviews. It won't be in 1.3, I guess.
This bug is a show stopper for company use! If you won't fix it, they'll never
use it. If they won't use it, mozilla won't spread to home users.
My ears.
Beonex Communicator 0.8.2 <http://www.beonex.com> (based on Mozilla 1.0.2)
contains the fix. Feel free to use that for your "company use". For the trunk,
we're still waiting for bienvenu's superreview.
*** Bug 198300 has been marked as a duplicate of this bug. ***
Hi all,
   i installed the 1.3 A emitter.dll patch from comment #46 and loaded it. Now, 
finally, i can see the file name attachments when i print out a mail, but when 
i save the files, Mozilla save theese with the same size of 32kb for all files 
also if the files size are different like 57kb, 1,2mb, 565kb, etc. and if i try 
to open this files with their application like Acrobat Reader, Word, the 
programs return to me an error message. If i reinstall the original emitter.dll 
the size when i save it's ok and the files are opened from their applications. 
I've tried on different computers in my company, but the result is the same. 
The configuration of my computers is Windows 98 or Windows98 SE, Mozilla 1.3 
(the last) and all computers are connected with an e-mail server so every 
accouts use IMAP protocol (not POP3). Please, is it possible and update 
emitter.dll for this problem?
Many thanks.
Attachment #111272 - Flags: superreview?(bienvenu)
Attachment #111272 - Flags: review?(ducarroz)
Attachment #113106 - Flags: superreview?(bienvenu)
*** Bug 190643 has been marked as a duplicate of this bug. ***
Updated emitter.dll (for Mozilla 1.3) can be downloaded from:
http://sten.freewebspace.com/

Hi. Now the updated emitter.dll proposed by Sten works perfectly in Mozilla 1.3 
so, finally, i can install and use Mozilla for every computer in my company.
Thank you.
Attachment #113108 - Flags: superreview?(bienvenu) → superreview?(bzbarsky)
It'll take me a bit to get to this review (see my queue).  If you need this by
1.4b, you may want to look for a different reviewer....
Boris Zbarsky wrote:
> It'll take me a bit to get to this review (see my queue).  If you need this by
> 1.4b, you may want to look for a different reviewer..

Which one would you suggest (1. note that this patch is rotting around since a
while / 2. 24 votes, one of the mailnews bugs with the highest vote count... :)
?
sspitzer and bienvenu are the srs for this area of code, and they tend to be
pretty responsive in my experience....
Attachment #113108 - Flags: superreview?(bzbarsky) → superreview?(sspitzer)
*** Bug 202240 has been marked as a duplicate of this bug. ***
*** Bug 202240 has been marked as a duplicate of this bug. ***
*** Bug 203064 has been marked as a duplicate of this bug. ***
*** Bug 203065 has been marked as a duplicate of this bug. ***
I am using the dll from comment #74, however I am having some problems.
Following on from  Fabiano comments about saving attachments 32kb in size,
Windows 2000 running on Fat32 has the same problem, however it seems to work OK
using NTFS.
Attachment #113108 - Flags: review?(dmose)
Is there any special reason why the patch waits a _month_ for review/superreview
?
Comment on attachment 113108 [details] [diff] [review]
Proposed Fix, Version 3, diff -uw

It sounds like this patch causes a regression. as per comment 83 (among
others).  Until that issue is sorted out, I don't think this is ready for
review.
Attachment #113108 - Flags: superreview?(sspitzer)
Attachment #113108 - Flags: review?(dmose)
Dan Mosedale wrote:
> (From update of attachment 113108 [details] [diff] [review])
> It sounds like this patch causes a regression. as per comment 83 (among
> others).  Until that issue is sorted out, I don't think this is ready for
> review.

I doubt this patch has caused the issue in comment 83 - IMHO it is more likely
that the precompiled windows DLL cited in that comment is simply incompabible to
the used build. The patch is pretty straightforward "portable" code and does not
contain any platform-specific (incl. windows-specific) code so I doubt it
strongly that it is responsible for this issue...
dmose, I second the last comment. I said in comment 56 that people should not
use the binary DLL, *esp.* not in combination with other releases. They ignored
me, and thus I ignored them, because I hoped it would be clear what's wrong.
Comment on attachment 113108 [details] [diff] [review]
Proposed Fix, Version 3, diff -uw

Requesting r=/sr= again per previous comments...
Attachment #113108 - Flags: superreview?(sspitzer)
Attachment #113108 - Flags: review?(dmose)
CC:'ing smontagu to test and confirm that the patch does work on FAT32
filesystems (well, the patch does not contain any Windows-specific or
platform-specific code nor it is likely that it triggers platform-specific
bugs... just to make dmose happy :) ...
I just tested Beonex Communicator 0.8.2 on Windows 98 (FAT32), which includes
this patch. The list of attachments printed fine; also, attachments were saved
with correct sizes (ie. not 32 kb)
dmose:
Can you give the patch a r= or sr= now, please (just in the hope that this
patch  still can make it's way into mozilla1.4 if possible...)  ?
Comment on attachment 113108 [details] [diff] [review]
Proposed Fix, Version 3, diff -uw

Generally looks good; a couple of small things:

> nsresult
> nsMimeHtmlDisplayEmitter::AddAttachmentField(const char *field, const char *value)
> {
>+  if (mSkipAttachment || BroadCastHeadersAndAttachments())
>+    return NS_OK;
>+
>+  // Don't let bad things happen
>+  if ( (!value) || (!*value) )
>+    return NS_OK;
>+

Shouldn't this last return really be an error code rather than NS_OK?  Or could
the strings actually be NULL or empty for some other reason than a bug?

> nsresult
> nsMimeHtmlDisplayEmitter::EndAttachment()
> {
>+  if (mSkipAttachment)
>+    return NS_OK;
>+    /* why wasn't that statement here before? shouldn't it have been here?
>+       if it's wrong here, then use
>+       (mFormat == nsMimeOutput::nsMimeMessagePrintOutput)
>+    */
>+  mSkipAttachment = PR_FALSE; // reset it for next attachment round

The above comment doesn't make any sense to me.  What is it intended to mean?
Attachment #113108 - Flags: review?(dmose) → review-
I tested the emitter.dll from comment #46 with
Mozilla 1.3 and Mozilla 1.3.1 (both german versions) and it works fine now.

Would be cool, if this patch will be included in the 1.4.x versions.
Thanks!
I tested the emitter.dll from comment #46 with Mozilla 1.4RC2 and it worked fine.
With Mozilla 1.4RC3 it does not work any more.
Can this be corrected and hopefully implemented into Mozilla 1.4 final release?
can we get this into 1.5a, please? 
especially companies might like/need this features.
Can anyone make a modified emitter.dll available for 1.4?  We've been using 1.3
with the available modified emitter.dll without any problem.  I have not tried
the emitter.dll from 1.3 since the above note indicates that it does not work. I
can't deploy 1.4 until my users can print their Email and have note of
attachments on the printout.  (Paperless office my rear.)
I use w/o any problem emitter.dll made/working for Mozilla 1.3.
> Can anyone make a modified emitter.dll available for 1.4?

No, and I wouldn't want anyone to do so in bugzilla. The amount of irrelevant,
misleading and incompetent comments in result of that DLL - despite my comments
to the contrary - was not only very annoying, but actually held back the checkin
in part because of the spurious "bug reports" here.

Again, please do *not* make any further comments or attachments here about
binary DLLs apart from maybe a single comment with a URL to a webpage, where you
offer one and allow people to comment about it *there*. Bugzilla is a
development platform, not a user forum.

Sorry that I didn't reply to the review yet, but the patch is so old that I
actually don't know the answers anymore. I'll have to dig deep into the problem
again.
Well, I can understand that you dont want people to mess around here with dll's
for relaase xyz or to get the comments "works not for xzy". But I am not able to
compile Mozilla (or the dll) for myself and I just dont have the time to get
through all the stuff.
I didnt find other emitter.dll than for 1.2.1 and 1.3a on the web,
so could someone be so kind to post a link here (btw does the language pack I
use play a role, too??) for newer releases (1.3.1 or 1.4, no betas).
Thanks
Olaf
No.
Again, please do *not* make any further comments or attachments here about
binary DLLs apart from maybe a single comment with a URL to a webpage, where you
offer one and allow people to comment about it *there*. Bugzilla is a
development platform, not a user forum.
I'll repeat this until people get it.
I tested the emitter.dll from comment #46 with Mozilla 1.4 and it worked fine if
during installation of Mozilla 1.4 a new directory was used (not the one of an
older version of Mozilla).
Comment on attachment 112391 [details]
Obsoleted

Again, please do *not* make any further comments or attachments here about
binary DLLs apart from maybe a single comment with a URL to a webpage, where
you
offer one and allow people to comment about it *there*. Bugzilla is a
development platform, not a user forum.
I'll repeat this until people get it.
Attachment #112391 - Attachment description: Compiled nsMimeHtmlEmitter.cpp for Windows (emitter.dll) → Removed
Attachment #112391 - Attachment description: Removed → Obsoleted
Attachment #112391 - Attachment mime type: application/x-zip-compressed → text/null
Hi there, is there any possibility to reduce the space ultilized by the name of 
the attachment.  At present it is in a box with alot of additional information 
and it results in alot of pages being printed when clients attach alot of 
documents.  It would be most ideal if the format could be smthing like this:

test.doc
invent.doc
numbers.xls

This would really spruce up the neatness of the printout too!
It shoudl be fairly easy to change the source code to only output the filename.
But I'd think that some people want to see that information, not? And giving
users the choice would be more work.
I think the most important is that the printout shows at first glance that there
are attached files.
Basically, the solution of the often mentioned emitter.dll is OK for me.
I can't see "a lot of additional information". The additional information is not
a must for me, but 2 lines (Content-Type and Content-Encoding) instead of 1 line
(only file name) is not very space consuming in the printout. Also, I personally
never had so many attached files to result in several additional pages of printout.
But whatever is easier to implement ASAP, as long as the file names are listed
it would be OK for me.
Thanx
Let me explain why this issue is important to our operation.  Contrary to the
desire to obtain a paperless organization, we find that it is necessary to
maintain a subset of our messages as paper records.  One of the requirements for
these records is an indication of what files were sent and received to and from
whom and when.  This means that to use Mozilla (1.4 in this case) we must
manually annotate the message printouts before filing them.  Obviously this is a
real pain in the posterior.  While it doesn't have to be the same format as in
Netscape 4.X or very extensive, it is essential that both the sent and received
message printouts include at least the name of the files attached to the
message, irrespective of whether any of them were included in line in the
message.  If there are so many files attached to a message that their notation
on the printout requires multiple pages, then so be it.  It's good to print
economically, but the essential requirement here is that the printed record of
both the message and what was attached be maintained.
Does this patch also add the list of attachments when the message is saved as 
text file? Or should I file a new bug/rfe for this?
Blocks: 214584
Ok, if somebody would like it too, I made bug 214584 for it.
An update to the patch that applies to the current tree.

Also some replies to dmose's comments:

> ------- Additional Comment #92 From Dan Mosedale: Away from moz until 8/1 
2003-06-05 23:42 -------
> 
> (From update of attachment 113108 [details] [diff] [review])
> Generally looks good; a couple of small things:
> 
>> nsresult
>> nsMimeHtmlDisplayEmitter::AddAttachmentField(const char *field, const char
*value)
>> {
>>+  if (mSkipAttachment || BroadCastHeadersAndAttachments())
>>+    return NS_OK;
>>+
>>+  // Don't let bad things happen
>>+  if ( (!value) || (!*value) )
>>+    return NS_OK;
>>+
> 
> Shouldn't this last return really be an error code rather than NS_OK?  Or 
> could the strings actually be NULL or empty for some other reason than a bug?

> 
The second return value just follows the convention with the other mime
emitters (see nsMimeXmlEmitter.cpp at 
http://lxr.mozilla.org/mozilla/source/mailnews/mime/emitters/src/nsMimeXmlEmitter.cpp#108
).  It appears that if the values are empty, the process just quietly continues
on.

>> nsresult
>> nsMimeHtmlDisplayEmitter::EndAttachment()
>> {
>>+  if (mSkipAttachment)
>>+    return NS_OK;
>>+    /* why wasn't that statement here before? shouldn't it have been here?
>>+	  if it's wrong here, then use
>>+	  (mFormat == nsMimeOutput::nsMimeMessagePrintOutput)
>>+    */
>>+  mSkipAttachment = PR_FALSE; // reset it for next attachment round
> 
> The above comment doesn't make any sense to me.  What is it intended to mean?

> 
> 
If I understand what Ben wrote, he was questioning why there wasn't a statement
to return early from this function if mSkipAttachment == PR_TRUE in version
1.51 of the file (before all of this stuff got cut out) and if this condition
is not the right condition to check, then a check on whether printing is
occurring should be used so that the html codes that follow aren't included if 

the conditions aren't met. 

Asking mscott for a review.
Attachment #129157 - Flags: review?(mscott)
Attachment #129157 - Flags: review?(mscott) → review?(scott)
I want to get this into thunderbird. I'll do the review right now.
Comment on attachment 129157 [details] [diff] [review]
Rev 3a - update patch to current tree 

>-    // HACK: news urls require us to use the originalSpec. everyone else uses GetURI
>-    // to get the RDF resource which describes the message.
>+      // HACK: news urls require us to use the originalSpec. Everyone
>+      // else uses GetURI to get the RDF resource which describes the message.
>     nsCOMPtr<nsINntpUrl> nntpUrl (do_QueryInterface(mURL, &rv));
>     if (NS_SUCCEEDED(rv) && nntpUrl)
>       rv = msgurl->GetOriginalSpec(getter_Copies(uriString));

tabs? you adjusted the spacing for the HACK comment and it looks off.

>+  UtilityWrite("<div align=right class=\"headerdisplayname\" style=\"display:inline;\">");

Do we actually load a CSS file that defines style rules for class
headerdisplayname? Is this necessary?


> nsresult
> nsMimeHtmlDisplayEmitter::EndAttachment()
> {
>+  if (mSkipAttachment)
>+    return NS_OK;
>+    /* why wasn't that statement here before? shouldn't it have been here?
>+       if it's wrong here, then use
>+       (mFormat == nsMimeOutput::nsMimeMessagePrintOutput)
>+    */
>+  mSkipAttachment = PR_FALSE; // reset it for next attachment round
>+  if (BroadCastHeadersAndAttachments())
>+    return NS_OK;
>+
Shouldn't we only be doing something here if mFormat ==
nsMimeOutput::nsMimeMessagePrintOutput ? 

>+  UtilityWrite("</table>");
>+  UtilityWrite("</td>");
>+  UtilityWrite("</tr>");
>+
>+  UtilityWrite("</table>");
>+  UtilityWrite("</center>");
>+  UtilityWrite("<br>");
>   return NS_OK;
> }
Attachment #129157 - Flags: superreview?(scott)
Attached patch rev4 (obsolete) — — Splinter Review 
Comments to scott's review:

>>-    // HACK: news urls require us to use the originalSpec. everyone else
uses GetURI
>>-    // to get the RDF resource which describes the message.
>>+	 // HACK: news urls require us to use the originalSpec. Everyone
>>+	 // else uses GetURI to get the RDF resource which describes the
message.
>>     nsCOMPtr<nsINntpUrl> nntpUrl (do_QueryInterface(mURL, &rv));
>>     if (NS_SUCCEEDED(rv) && nntpUrl)
>>	 rv = msgurl->GetOriginalSpec(getter_Copies(uriString));

>tabs? you adjusted the spacing for the HACK comment and it looks off.

Flubbed the diff.  new patch shows that everything is shifted into an if block
and everything lines up.

>>+  UtilityWrite("<div align=right class=\"headerdisplayname\"
style=\"display:inline;\">");

>Do we actually load a CSS file that defines style rules for class
>headerdisplayname? Is this necessary?

Yes the CSS is loaded in header section [EndHeader subroutine] (see
http://lxr.mozilla.org/seamonkey/source/mailnews/mime/emitters/src/nsMimeHtmlEmitter.cpp#338

Is is necessary? Your call; we may want to let skins override this style.  (XML
emitter uses the same class.

>>+  if (mSkipAttachment)
>>+    return NS_OK;
>>+    /* why wasn't that statement here before? shouldn't it have been here?
>>+	  if it's wrong here, then use
>>+	  (mFormat == nsMimeOutput::nsMimeMessagePrintOutput)
>>+    */
>>+  mSkipAttachment = PR_FALSE; // reset it for next attachment round
>>+  if (BroadCastHeadersAndAttachments())
>>+    return NS_OK;
>>+
>Shouldn't we only be doing something here if mFormat ==
>nsMimeOutput::nsMimeMessagePrintOutput ? 

Changed to output only when printing.
Comment on attachment 129556 [details] [diff] [review]
rev4

Thanks for addressing my comments.

sr=mscott
Attachment #129556 - Flags: superreview+
*** Bug 215821 has been marked as a duplicate of this bug. ***
Attachment #129556 - Flags: review?(sspitzer)
>tabs? you adjusted the spacing for the HACK comment and it looks off.

It was a diff -uw

>>+  if (mSkipAttachment)
>>+    return NS_OK;

>Shouldn't we only be doing something here if mFormat ==
>nsMimeOutput::nsMimeMessagePrintOutput ? 

If  mFormat != nsMimeOutput::nsMimeMessagePrintOutput, then
mSkipAttachment == false, so the check for Print is unnecessary here (with the
current code):

In StartAttachment():
+    mSkipAttachment = PR_TRUE;
...
+  else if (mFormat == nsMimeOutput::nsMimeMessagePrintOutput)
...
+  else
...
+    mSkipAttachment = PR_TRUE;

Ben:  Sorry for the possible confusion, but Scott's comments were to my update
of your patch.  Hope you don't mind that I'm tweaking your work :).  In Rev 3a,
I also did a diff -uw, but I didn't note that in comments.  Rev 4 is a diff -u
patch.

>>>+  if (mSkipAttachment)
>>>+    return NS_OK;
>
>>Shouldn't we only be doing something here if mFormat ==
>>nsMimeOutput::nsMimeMessagePrintOutput ? 
>
>If  mFormat != nsMimeOutput::nsMimeMessagePrintOutput, then
>mSkipAttachment == false, so the check for Print is unnecessary here (with the
>current code):
>
>In StartAttachment():
>+    mSkipAttachment = PR_TRUE;
>...
>+  else if (mFormat == nsMimeOutput::nsMimeMessagePrintOutput)
>...
>+  else
>...
>+    mSkipAttachment = PR_TRUE;
>

The section of code that Scott was mentioning is in EndAttachment().  I added a
check in that area to only spit out the codes if we are printing.  

I asked for a review from sspitzer on this patch in hopes that I might get all
the boxes checked in order to make it into 1.5b.  Anything else to do to keep
this moving? (e.g. different reviewer, mail to ping someone, etc.)

By the way, someone else will have to check this in once it gets approval (no
rights to cvs)

Kevin
> The section of code that Scott was mentioning is in EndAttachment().

Yes,
+  if (mSkipAttachment)
+    return NS_OK;
is in EndAttachement(), and StartAttachment sets mSkipAttachment = true unless
we are in print mode.

> someone else will have to check this in once it gets approval

I can do it. Thanks for driving this.
I'm gonna flag this for 1.5b because I'd like to get it into the next
thunderbird milestone release
Flags: blocking1.5b?
Attachment #129556 - Flags: superreview?(bienvenu)
Attachment #129556 - Flags: superreview+
Attachment #129556 - Flags: review?(sspitzer)
Attachment #129556 - Flags: review+
Comment on attachment 129556 [details] [diff] [review]
rev4

sr=bienvenu - I guess Ben will check this in, once it's approved. If for some
reason, he can't, Scott or I would be happy to check it in.
Attachment #129556 - Flags: superreview?(bienvenu) → superreview+
Comment on attachment 113108 [details] [diff] [review]
Proposed Fix, Version 3, diff -uw

(clearing sr request as a newer version of this patch is available)
Attachment #113108 - Flags: superreview?(sspitzer)
Comment on attachment 129157 [details] [diff] [review]
Rev 3a - update patch to current tree 

(clearing sr request as a newer version of this patch is available)
Attachment #129157 - Flags: superreview?(scott)
Attachment #129157 - Flags: review?(scott)
Comment on attachment 129556 [details] [diff] [review]
rev4

Seeking approval for checkin.  Returns a much requested feature that
disappeared around 0.9.4.  Scott also wants this for the next milestone of
Thunderbird.  Low risk change but some exercising in the beta cycle would be
good before 1.5 final
Attachment #129556 - Flags: approval1.5b?
> Low risk change

I should add that most of the code added here is merely code which was wrongly
removed earlier and only readded here.
Comment on attachment 129556 [details] [diff] [review]
rev4

Nit: no else after return (twice) below:

>+    headerSink->HandleAttachment(contentType, url /* was escapedUrl */,
>+                                 unicodeHeaderValue, uriString,
>+                                 aNotDownloaded);
>+
>+    nsCRT::free(escapedUrl);
>+    mSkipAttachment = PR_TRUE;
>+  }
>+  else if (mFormat == nsMimeOutput::nsMimeMessagePrintOutput)
>   {
>-    unicodeHeaderValue.Assign(NS_ConvertUTF8toUCS2(name));
>+    // then we need to deal with the attachments in the body by inserting
>+    // them into a table..
>+    return StartAttachmentInBody(name, contentType, url);
>+  }
>+  else
>+  {
>+    // If we don't need or cannot broadcast attachment info, just ignore it
>+    mSkipAttachment = PR_TRUE;
>+    return NS_OK;
>+  }
>+
>+  return rv;
>+}

Why not set rv = StartAttachmentInBody(...); in the last else-if's then clause,
and set rv = NS_OK in the else clause at the end?  I'm assuming it's ok to
suppress an early error (NS_FAILED(rv)).

>+nsresult
>+nsMimeHtmlDisplayEmitter::StartAttachmentInBody(const char *name, const char *contentType, const char *url)
>+{
>+  if ( (contentType) &&
>+        ((!strcmp(contentType, APPLICATION_XPKCS7_MIME)) ||
>+         (!strcmp(contentType, APPLICATION_XPKCS7_SIGNATURE)) ||
>+         (!strcmp(contentType, TEXT_VCARD))
>+        )
>+     )
>+  {
>+    mSkipAttachment = PR_TRUE;
>+    return NS_OK;
>   }
>+  else
>+    mSkipAttachment = PR_FALSE;

The else and indentation before mSkipAttachment = PR_FALSE; here are nonsense,
especially in light of the rest of the method's body not being indented or in
the bogo-else clause.

> 
>-  headerSink->HandleAttachment(contentType, url /* was escapedUrl */, unicodeHeaderValue, uriString, aNotDownloaded);
>+  if (!mFirst)
>+    UtilityWrite("<hr width=\"90%\" size=4>");
> 
>-  nsCRT::free(escapedUrl);
>+  mFirst = PR_FALSE;
> 
>-  return rv;
>+  UtilityWrite("<center>");
>+  UtilityWrite("<table border>");
>+  UtilityWrite("<tr>");
>+  UtilityWrite("<td>");
>+
>+  UtilityWrite("<div align=right class=\"headerdisplayname\" style=\"display:inline;\">");
>+
>+  UtilityWrite(name);
>+
>+  UtilityWrite("</div>");
>+
>+  UtilityWrite("</td>");
>+  UtilityWrite("<td>");
>+  UtilityWrite("<table border=0>");
>+  return NS_OK;
> }
> 
> nsresult
> nsMimeHtmlDisplayEmitter::AddAttachmentField(const char *field, const char *value)
> {
>+  if (mSkipAttachment || BroadCastHeadersAndAttachments())
>+    return NS_OK;
>+
>+  // Don't let bad things happen
>+  if ( (!value) || (!*value) )
>+    return NS_OK;

Too many parens, and can either of these bad things happen?  Should they be
hushed up with NS_OK here, or shouldn't they have resulted in a failure
nsresult earlier?

> nsresult
> nsMimeHtmlDisplayEmitter::EndAttachment()
> {
>+  if (mSkipAttachment)
>+    return NS_OK;
>+    /* why wasn't that statement here before? shouldn't it have been here?
>+       if it's wrong here, then use
>+       (mFormat == nsMimeOutput::nsMimeMessagePrintOutput)
>+    */

What's this comment about, and why is it indented oddly?  If it belongs to the
first if in the method, move it up and exdent it one level.  If it belongs to
the next statement, exdent it and maybe put a blank line above it.

>+  mSkipAttachment = PR_FALSE; // reset it for next attachment round
>+  if (BroadCastHeadersAndAttachments())
>+    return NS_OK;
>+
>+  if (mFormat == nsMimeOutput::nsMimeMessagePrintOutput) {
>+    UtilityWrite("</table>");
>+    UtilityWrite("</td>");
>+    UtilityWrite("</tr>");
>+
>+    UtilityWrite("</table>");
>+    UtilityWrite("</center>");
>+    UtilityWrite("<br>");
>+  }
>   return NS_OK;
> }
> 

Nits only, but worth picking if this is to get past the 1.5b police.

/be
Comment on attachment 129556 [details] [diff] [review]
rev4

a=asa (on behalf of drivers) for checkin to 1.5beta as long as brendan's
comments are heeded. Time is short and if this doesn't make it in before
tomorrow's builds then it might not make it.
Attachment #129556 - Flags: approval1.5b? → approval1.5b+
patch updated per brenden's comments.  See additional comments in following
comment
Attachment #129157 - Attachment is obsolete: true
Attachment #129556 - Attachment is obsolete: true
Replies to be:

Just in case you haven't read all of what is in this bug (and who would want to
with all of this spam), I'll just point out a couple of things.

This code is basically a resurrection of what was in the file back in version
1.51 (I think that is the version) that was cut out. (I believe it was by
mistake).  Ben made a patch that basically added the code as it was back.  So
some of the logic is neither mine or Ben's.  Ben has become busy, Scott is
chasing more important things (and doing a very good job, I must say) and so a
rookie code jockey (me) is trying to muddle through getting this feature that I
would like.  Since I am a rookie, I'm still learning the ropes and may not have
all of the answers for you, but I'm plugging along and will attempt to answer
your comments.

On to the responses:
>(From update of attachment 129556 [details] [diff] [review])
>Nit: no else after return (twice) below:
>
>>+    headerSink->HandleAttachment(contentType, url /* was escapedUrl */,
>>+                                 unicodeHeaderValue, uriString,
>>+                                 aNotDownloaded);
>>+
>>+    nsCRT::free(escapedUrl);
>>+    mSkipAttachment = PR_TRUE;
>>+  }
>>+  else if (mFormat == nsMimeOutput::nsMimeMessagePrintOutput)
>>   {
>>-    unicodeHeaderValue.Assign(NS_ConvertUTF8toUCS2(name));
>>+    // then we need to deal with the attachments in the body by inserting
>>+    // them into a table..
>>+    return StartAttachmentInBody(name, contentType, url);
>>+  }
>>+  else
>>+  {
>>+    // If we don't need or cannot broadcast attachment info, just ignore it
>>+    mSkipAttachment = PR_TRUE;
>>+    return NS_OK;
>>+  }
>>+
>>+  return rv;
>>+}

>Why not set rv = StartAttachmentInBody(...); in the last else-if's then clause,
>and set rv = NS_OK in the else clause at the end?  I'm assuming it's ok to
>suppress an early error (NS_FAILED(rv)).

Changed to set rv as suggested in both cases (no else after return now).

>>+nsresult
>>+nsMimeHtmlDisplayEmitter::StartAttachmentInBody(const char *name, const char
>*contentType, const char *url)
>>+{
>>+  if ( (contentType) &&
>>+        ((!strcmp(contentType, APPLICATION_XPKCS7_MIME)) ||
>>+         (!strcmp(contentType, APPLICATION_XPKCS7_SIGNATURE)) ||
>>+         (!strcmp(contentType, TEXT_VCARD))
>>+        )
>>+     )
>>+  {
>>+    mSkipAttachment = PR_TRUE;
>>+    return NS_OK;
>>   }
>>+  else
>>+    mSkipAttachment = PR_FALSE;

>The else and indentation before mSkipAttachment = PR_FALSE; here are nonsense,
>especially in light of the rest of the method's body not being indented or in
>the bogo-else clause.

else clause removed and order reversed to flow better. (default is don't skip;
exit early if the attachment matches one of the types listed)

>> nsresult
>> nsMimeHtmlDisplayEmitter::AddAttachmentField(const char *field, const char
>*value)
>> {
>>+  if (mSkipAttachment || BroadCastHeadersAndAttachments())
>>+    return NS_OK;
>>+
>>+  // Don't let bad things happen
>>+  if ( (!value) || (!*value) )
>>+    return NS_OK;

>Too many parens, and can either of these bad things happen?  Should they be
>hushed up with NS_OK here, or shouldn't they have resulted in a failure
>nsresult earlier?

Extra parens removed.  The logic follows the other mime emitters (see
http://lxr.mozilla.org/mozilla/source/mailnews/mime/emitters/src/nsMimeXmlEmitter.cpp#169)
where if the value, or what it points to is empty just continue on.  I believe
this is done because there is no prior knowledge in the emitter of whether the
value exists in the message or not, so if it isn't there, don't output anything
and continue.  You'd have to ask Scott about this more since it was his code
originally (see the blame for the previous code reference). 

>> nsresult
>> nsMimeHtmlDisplayEmitter::EndAttachment()
>> {
>>+  if (mSkipAttachment)
>>+    return NS_OK;
>>+    /* why wasn't that statement here before? shouldn't it have been here?
>>+       if it's wrong here, then use
>>+       (mFormat == nsMimeOutput::nsMimeMessagePrintOutput)
>>+    */

>What's this comment about, and why is it indented oddly?  If it belongs to the
>first if in the method, move it up and exdent it one level.  If it belongs to
>the next statement, exdent it and maybe put a blank line above it.

Comment removed.  Should have been removed when I responded to scott's review
earlier.

Note that I added the changes to nsMimeHtmlEmitter.h from rev3a that got dropped
in rev4 inadvertently.

Kevin


Carrying over r & sr, seeking approval1.5b again
Attachment #130397 - Flags: superreview+
Attachment #130397 - Flags: review+
Attachment #130397 - Flags: approval1.5b?
Comment on attachment 129556 [details] [diff] [review]
rev4

Clearing flags on obsolete patch
Attachment #129556 - Flags: superreview+
Attachment #129556 - Flags: review+
Attachment #129556 - Flags: approval1.5b+
Kevin, thanks for your extra diligence with this patch. Per the comments by Asa,
please go ahead and check this latest patch in now that you've addressed
Brendan's comments. (If you don't have CVS access, I can do this for you tonight)
Scott:  I don't have cvs access, so you'll have to do the checkin

Kevin
checked in. thanks for all the help Kevin and Ben.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Thanks a lot, Kevin, for driving the checkin!
Flags: blocking1.5b?
Attachment #130397 - Flags: approval1.5b?
Thanks a lot for all the work on this bug, now I can stop to always take a
pencil for writing the attachments file names by hand on the print-outs, which I
need for my work!

I have only one additional suggestion about the form of the information added to
the print-out. With Thunderbird 0.2 the information is directly added at the end
of the document without any space in boxes, one for every attachment. When you
handle the papers, it would me more useful to find the information about the
attachments on top of the first page, as additional lines (not boxes) below the
"Subject - From - Date - To - CC - BCC" block. For the daily work, I also think,
that the information about content-type and content-encoding is not urgently
needed in every case for "normal" users, who only want to know, which
attachments came / went with the mail.

I know, it is not always possible to find a solution, which is the optimum for
everyone, so I can also live with the current solution. But maybe there will be
a forum to pick up my idea....

Thank you again

Erich Cramer
This is my isea how this should look today. The MD5 sum should just ba
calculated on request of the user not automatically
What about this look?
Just as an idea.

<table cellpadding="2" cellspacing="2" border="1" style="text-align: left;
width: 75%;">
  <tbody>
    <tr>
      <td style="text-align: center; vertical-align: middle;"><a
href="Link%20to%20Attachment">Attached Archive.zip</a></td>
      <td style="vertical-align: top;">
      <table cellpadding="2" cellspacing="2" border="0" style="text-align: left;
width: 100%;">
        <tbody>
          <tr>
            <td style="vertical-align: top; text-align: right; font-weight:
bold;"><small>MIME Type:</small></td>
            <td style="vertical-align: top; font-weight:
bold;"><small>application/zip<br>
            </small></td>
          </tr>
          <tr>
            <td style="vertical-align: top; text-align: right; font-weight:
bold;"><small>Size:</small></td>
            <td style="vertical-align: top; font-weight: bold;"><small>378 kB
(386,587 byte)<br>
            </small></td>
          </tr>
          <tr>
            <td style="vertical-align: top; text-align: right; font-weight:
bold;"><small>MD5-sum:</small></td>
            <td style="vertical-align: top; font-weight:
bold;"><small>02EF17C3656F9E89BBB31BF50F1719F0</small></td>
          </tr>
        </tbody>
      </table>
      </td>
    </tr>
  </tbody>
</table>
Just a suggestion. Would it be possible to create an option such that those who 
need the addition details(in the table format with the application type and 
series of numbers) can install that version and those who only need the file 
name.  As in Microsoft, there is only the file name with the extension (minus 
the table and all other details) eg. test.doc 

This would be more than sufficient for the average user.

Thank you
Please file new bugs about suggestions and minor problems with the feature. This
bug is closed. Thanks.
*** Bug 220989 has been marked as a duplicate of this bug. ***
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: