509616 - improve the look of the inline attachment separator

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: current inline separator

the current separator for the inline attachment is an HR element relic of the web 0.2 days.  I think it would be nice to have a much nicer display with a bit more information shown. (see attachment for screenshot of separator)

I'm assigning this to Andreas for working on the display of a new separator and then maybe dmose or bwinton could take a look at making it happen.

So far I think we should be using a light colored single line that goes (almost) all the way across the message reader.  I think we should also be including the name of the file with the separator as well.  All these element in a light, GrayText like, color.

So likely (in a poor case) we'll get something like:

_Part 1.4_____________________________________________________________

Comment 1

[Andreas Nilsson (:andreasn)]

Attachment: a couple of different variants

I like top-left or top-right best myself. The other two variants are a bit too heavy and in your face.

Comment 2

[Bryan Clark (DevTools PM) [:clarkbw]]

Nice work! I like the top left version the most.

Assigning this to Blake to look into making this happen.  After talking with Bienvenu it sounds like (i have no idea what these words really mean) it will require a simple change to the libmime emitter so we get the name of the attachment at HTML generation time. (i assume you understood that!)

Likely we'll have to use colors like GrayText and GrayTextBackground? to play nicely with a11y, Andreas can give more guidance there.

Comment 3

[Andreas Nilsson (:andreasn)]

(In reply to comment #2)
> Likely we'll have to use colors like GrayText and GrayTextBackground? to play
> nicely with a11y, Andreas can give more guidance there.

As far as I can see text area is always white, regardless of what your theme settings are (at least on Linux), so a11y seems broken here anyway (might be a bug open about that already). That means we can possibly use any colors we want.

Comment 4

[David :Bienvenu]

I think I'm going to need to tweak libmime in order for this to be possible. I need to get the attachment name to the code that writes the separator, which should be fairly easy - it's tantalizingly close.

Comment 5

[David :Bienvenu]

Attachment: patch to enable Blake to proceed

This patch should allow Blake to tweak the html we emit to get the result he wants, just by changing the same parts of the code I changed. I hope it should be pretty clear how to change the code, based on this patch.

However, getting the HR element for forwarded as attachment messages to show the subject name with the HR element isn't currently doable - I can fight with libmime to make it possible, but it seems less important, since the next thing after the HR element is the subject header. I've tweaked the code to change the height of that particular separator to be 2 instead of 4, so it looks nicer.

Please let me know if you have any questions, Blake.

Comment 6

[Blake Winton (:bwinton) (:☕️)]

Attachment: WIP patch, to compare to Bienvenu's and to let Bryan and Andreas play with css.

This is what I had so far.  There's still some cleanup and I need to add some Doxygen comments to nsIMimeEmitter.idl before it's ready for review, but I figure it's worth posting so that we can see if I've missed anything obvious.

Thanks for your patch,
Blake.

Comment 7

[David :Bienvenu]

In general, that looks a lot better. You may need to tweak gloda's mime emitter as well (which was why I was a little reluctant to tweak nsIMimeEmitter, though that's required for the message/rfc822 attachments.)

Comment 8

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: added qute and gnomestripe

I got so excited to see it and then it had not theme-ing :(  But here I just copied over the pinstripe theme into the other themes and it is... awesome.  Amazing how some small things can make such a difference!

Comment 9

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, cleaned up and including the styles in all the themes.

I added some Doxygen comments to nsIMimeEmitter.idl, and tidied up some stuff, and copied the pinstripe css into the other themes, and tweaked gloda's mime emitter (by just adding the parameters, cause the methods didn't do anything).

And so here we are.

Thanks,
Blake.

Comment 10

[Blake Winton (:bwinton) (:☕️)]

Oh, one thing to watch out for.

I don't quite know what happened with the newlines for the qute theme, so let me know if that patch doesn't apply cleanly, and I'll try something else.

Comment 11

[David :Bienvenu]

Comment on attachment 395891 [details] [diff] [review]
The previous patch, cleaned up and including the styles in all the themes.

I had to remove the context line from each of the css diffs:

 @@ body[selected="false"] {

then the patch applied. I'll try running with it now.

Comment 12

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 395891 [details] [diff] [review]
The previous patch, cleaned up and including the styles in all the themes.

soooo sweet!

Comment 13

[David :Bienvenu]

I'm not see any divider on Vista, though I am seeing the attachment names - perhaps an issue with the format of the patch?

Comment 14

[David :Bienvenu]

Comment on attachment 395891 [details] [diff] [review]
The previous patch, cleaned up and including the styles in all the themes.

in general, this looks great. A few nits:

Instead of duplicating this code:

+  char *name = MimeHeaders_get_name(hdrs, opt);
+  char *fname = NULL;
+  fname = MIME_DecodeMimeHeader(name, opt->default_charset, false, true);
+  if (fname && (fname != name)) {
+    if (name)
+      PR_Free(name);
+    name = fname;
+  }
+

could you add a little utility function? mimehdrs.cpp/h seems like a reasonable place for it.

Also, PR_Free checks for null, so you don't need to say

if (name)
  PR_free(name);

you can just say

PR_Free(name);

And surely this must leak fName?

+    char *fname = NULL;
+    fname = MIME_DecodeMimeHeader(name, opt->default_charset, false, true);
+    if (fname && (fname != name))
+      name = fname;
+    if (attachments) {
+      PR_Free(attachments);
+      attachments = NULL;
+    }
+
+    return emitter->EndHeader(nsDependentCString(name ? name : ""));

I guess that code must make it a bit tricky to share the code. If so, the leak should still be fixed.  Maybe make name an nsCString and use something like name.Adopt(fname); to let the compiler handle the cleanup for you.

Comment 15

[David :Bienvenu]

I forgot to say that the missing divider was caused by an error applying the patch...if I apply the css changes by hand, I see the divider.

Comment 16

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, with fixes for the problems bienvenu pointed out.

(In reply to comment #14)
> in general, this looks great. A few nits:

Thanks!  It's nice to be getting back into some C++ coding.

> Instead of duplicating this code:
> +  char *name = MimeHeaders_get_name(hdrs, opt);
> +  char *fname = NULL;
> +  fname = MIME_DecodeMimeHeader(name, opt->default_charset, false, true);
> +  if (fname && (fname != name)) {
> +    if (name)
> +      PR_Free(name);
> +    name = fname;
> +  }
> could you add a little utility function? mimehdrs.cpp/h seems like a
> reasonable place for it.

Sure, I'll name it MimeHeaders_convert_header_value, and use my handy time machine to add it to mimehdrs.cpp a long time ago, as a different user.  :)

(A very similar function was already there, at http://mxr.mozilla.org/comm-central/source/mailnews/mime/src/mimehdrs.cpp#64  I made it non-static, and added it to the header file.)

> Also, PR_Free checks for null, so you can just say
> PR_Free(name);

Fixed, well, I changed the one that said "PR_Free(attachments);".  The others went away entirely.

> And surely this must leak fName?
[snip]
> I guess that code must make it a bit tricky to share the code. If so, the leak
> should still be fixed.  Maybe make name an nsCString and use something like
> name.Adopt(fname); to let the compiler handle the cleanup for you.

Yup, that's what I did.  Or at least, that's what I tried to do.

This diff applies cleanly for me on XP, so hopefully it should be good to go for you as well.

Any further suggestions would be much appreciated.

Thanks,
Blake.

Comment 17

[David :Bienvenu]

Comment on attachment 396006 [details] [diff] [review]
The previous patch, with fixes for the problems bienvenu pointed out.

I think the usage of Adopt is backwards here.

This is how it should be used:

nsCString name;
name.Adopt(some_function_that_allocates_its_return value());

so this would wrong:

+      if (NS_SUCCEEDED(rv) && attachments->real_name)
+        name.Adopt(attachments->real_name);

because it means name is going to free attachments->real_name, which it doesn't own. You can think of .Adopt as meaning "I promise to free this memory you allocated, instead of making a copy of the data for myself".

e.g.,

nsCString name;

name.Adopt(MimeHeaders_get_name(obj->headers, obj->options));

so I think you want
name.Assign(attachments->real_name); 


Can you explain why you changed this? :

-  if (n <= 0)
+  if (n < 0)
     return n;

Comment 18

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #17)
> (From update of attachment 396006 [details] [diff] [review])
> I think the usage of Adopt is backwards here.

Okay, that makes sense.  I'll fix those.

> Can you explain why you changed this? :
> 
> -  if (n <= 0)
> +  if (n < 0)
>      return n;

Yeah, if we call this function and there is only an inline message, it will return 0 at this point, which looks like it's being interpreted as NS_OK by the caller, which doesn't seem quite right.

The case that prompted me to make the change was mimemoz2.cpp, line 1923:
      nsresult rv = MimeGetAttachmentList(msd->obj, msd->url_name, &attachments);
      if (NS_SUCCEEDED(rv) && attachments->real_name)
        name.Adopt(attachments->real_name);
 
If we don't return when the value is 0, the method will continue and fill in the real_name with a useful value (which I later display).

The other place it's called is mimemoz2.cpp, line 974:
      nsMsgAttachmentData *attachments;
      nsresult rv = MimeGetAttachmentList(obj, msd->url_name, &attachments);
      if (NS_SUCCEEDED(rv))
      {
        NotifyEmittersOfAttachmentList(msd->options, attachments);
        MimeFreeAttachmentList(attachments);
      }

Does that make sense, and seem reasonable?
Should it go in a comment above the if?

Thanks,
Blake.

Comment 19

[David :Bienvenu]

>Does that make sense, and seem reasonable?
>Should it go in a comment above the if?

Yes, I think a comment would be good, thx!

Comment 20

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, updated with Bienvenu's suggestions.

Okay, I changed the call to Adopt to call Assign instead, and added a fairly lengthy comment before the "if (n < 0)".

Thanks,
Blake.

Comment 21

[David :Bienvenu]

+  nsCString name(MimeHeaders_get_name(obj->headers, obj->options));
+  MimeHeaders_convert_header_value(obj->options, name, false);

This would leak the return value of MimeHeaders_get_name() because name would make a copy of the string and store it in the nsCString, leaking the original return value. To fix this, you could do something like this:

nsCString name;
name.Adopt(MimeHeaders_get_name(obj->headers, obj->options));

This tells nsCString to just use the return value of MimeHeaders_get_name directly instead of making a copy of the string.

Comment 22

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, without memory leaks.

(In reply to comment #21)
> +  nsCString name(MimeHeaders_get_name(obj->headers, obj->options));
> This would leak the return value of MimeHeaders_get_name()

My apologies.  I swear I fixed those before, but I guess I didn't, or reverted them before posting the patch.  Anyways, here's a new patch with those fixed.

As a side note, is there a tool/test I can run to find memory leaks like these?  I tried "make xpcshell-tests", but they took hours, and I couldn't really tell if there were any new leaks in the output.

Thanks,
Blake.

Comment 23

[David :Bienvenu]

Comment on attachment 396155 [details] [diff] [review]
The previous patch, without memory leaks.

string leaks are a bit harder to track. On Linux, I think valgrind might help. On the Mac, x-code has a tool that can detect leaks. For Windows, Purify can help, though it's fairly expensive.

Comment 24

[David :Bienvenu]

I am seeing this assertion running with the patch, which is related to the if (n <= 0) change.

 	ntdll.dll!_DbgBreakPoint@0() 	
 	xpcom_core.dll!Break(const char * aMsg=0x0035e7a4)  Line 491	C++
 	xpcom_core.dll!NS_DebugBreak_P(unsigned int aSeverity=0x00000001, const char * aStr=0x682f4bac, const char * aExpr=0x682f4ba4, const char * aFile=0x682f4b64, int aLine=0x000000f8)  Line 364 + 0xc bytes	C++
 	mime.dll!ValidateRealName(nsMsgAttachmentData * aAttach=0x0ed73248, MimeHeaders * aHdrs=0x00000000)  Line 248 + 0x22 bytes	C++
 	mime.dll!GenerateAttachmentData(MimeObject * object=0x0eda3fb8, const char * aMessageURL=0x0eda3cc8, MimeDisplayOptions * options=0x0eda3e40, int isAnAppleDoublePart=0x00000000, nsMsgAttachmentData * aAttachData=0x0ed73248)  Line 473 + 0x10 bytes	C++
>	mime.dll!MimeGetAttachmentList(MimeObject * tobj=0x0eda3fb8, const char * aMessageURL=0x0eda3cc8, nsMsgAttachmentData * * data=0x0035ee0c)  Line 596 + 0x1c bytes	C++
 	mime.dll!mime_display_stream_complete(_nsMIMESession * stream=0x0eda40a8)  Line 981 + 0x13 bytes	C++
 	mime.dll!nsStreamConverter::OnStopRequest(nsIRequest * request=0x0ed9e094, nsISupports * ctxt=0x0ed9d188, unsigned int status=0x00000000)  Line 1068 + 0xf bytes	C++

Comment 25

[[e-mail]]

Comment on attachment 396155 [details] [diff] [review]
The previous patch, without memory leaks.

>diff --git a/mail/themes/gnomestripe/mail/messageBody.css b/mail/themes/gnomestripe/mail/messageBody.css
I take it suite will need some theme changes at some point?

An easier way to get a similar effect to your negative margin hack is to use a fieldset and a legend something like this:

<fieldset style="border-style: none; border-top-style: solid;">
  <legend>attach.txt</legend>
<!-- put attachment data inside fieldset perhaps? -->
</fieldset>

(Obviously you wouldn't put the style inline like I did here.)

>+      nsresult rv = MimeGetAttachmentList(msd->obj, msd->url_name, &attachments);
>+      if (NS_SUCCEEDED(rv) && attachments->real_name)
>+        name.Assign(attachments->real_name);
>+    }
>+
>+    MimeHeaders_convert_header_value(opt, name, false);
>+    PR_Free(attachments);
MimeFreeAttachmentList(attachments);

Comment 26

[[e-mail]]

Comment on attachment 396155 [details] [diff] [review]
The previous patch, without memory leaks.

>+.mimeAttachmentHeader {
>+  width: 98%;
>+  margin-left:auto;
>+  margin-right:auto;
>+  border-top: 1px solid grey;
>+}
Nit: : auto;

>+.mimeAttachmentName {
>+  float: left;
Nit: prefer display: inline-block;

>+  background: white;
Nit: background-color:

>+  font-family: verdana,arial,sans-serif;
[I wouldn't want this in suite's theme]

>+  margin-top: -12px;
Pixel values don't work very well when you don't know how big the font is. Try -0.75em; with padding: 0px 4px;

>+  margin-left: 10px;
-moz-margin-start:

[Some of these would be made redundant by fieldset/legend.]

Comment 27

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, with neil's nits.

(In reply to comment #24)
> I am seeing this assertion running with the patch, which is related to the if
> (n <= 0) change.

Fixed.

(In reply to comment #25)
> (From update of attachment 396155 [details] [diff] [review])
> >diff --git a/mail/themes/gnomestripe/mail/messageBody.css
> I take it suite will need some theme changes at some point?

I would guess so.  I started to add them (without the font-family), but my build of SeaMonkey was really broken, so I figured better safe than sorry, and left them out of this patch.

> An easier way to get a similar effect to your negative margin hack is to use a
> fieldset and a legend something like this:

Changed.

> >+    PR_Free(attachments);
> MimeFreeAttachmentList(attachments);

Fixed.

(In reply to comment #26)
> (From update of attachment 396155 [details] [diff] [review])
> >+.mimeAttachmentHeader {
> >+  margin-left:auto;
> >+  margin-right:auto;
> Nit: : auto;

Fixed.

> >+.mimeAttachmentName {
> >+  float: left;
> Nit: prefer display: inline-block;

Removed.

> >+  background: white;
> Nit: background-color:

Removed.

> >+  margin-top: -12px;
> Pixel values don't work very well when you don't know how big the font is. Try
> -0.75em; with padding: 0px 4px;

Removed.

> >+  margin-left: 10px;
> -moz-margin-start:

Removed.

Thanks,
Blake.

Comment 28

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/ef841803601b

Blake, please raise bug on SeaMonkey if the theme changes are required there.

Comment 29

[[e-mail]]

Comment on attachment 396370 [details] [diff] [review]
The previous patch, with neil's nits.

>+    nsMsgAttachmentData *attachments = NULL;
Bah, I overlooked this; we're supposed to use nsnull.

Comment 30

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #28)
> Blake, please raise bug on SeaMonkey if the theme changes are required there.

Bug 512454 is entered, along with a first cut at a patch.

Thanks,
Blake.