Last Comment Bug 509616 - improve the look of the inline attachment separator
: improve the look of the inline attachment separator
Status: RESOLVED FIXED
[no l10n impact]
:
Product: Thunderbird
Classification: Client Software
Component: Message Reader UI (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 3.0b4
Assigned To: Blake Winton (:bwinton) (:☕️)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-08-10 17:15 PDT by Bryan Clark (DevTools PM) [@clarkbw]
Modified: 2009-08-25 06:33 PDT (History)
6 users (show)
clarkbw: blocking‑thunderbird3+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
current inline separator (4.96 KB, image/png)
2009-08-10 17:15 PDT, Bryan Clark (DevTools PM) [@clarkbw]
no flags Details
a couple of different variants (40.56 KB, image/png)
2009-08-10 18:03 PDT, Andreas Nilsson (:andreasn)
no flags Details
patch to enable Blake to proceed (5.52 KB, patch)
2009-08-20 16:57 PDT, David :Bienvenu
no flags Details | Diff | Review
WIP patch, to compare to Bienvenu's and to let Bryan and Andreas play with css. (22.80 KB, patch)
2009-08-20 17:05 PDT, Blake Winton (:bwinton) (:☕️)
no flags Details | Diff | Review
added qute and gnomestripe (24.09 KB, patch)
2009-08-20 22:51 PDT, Bryan Clark (DevTools PM) [@clarkbw]
no flags Details | Diff | Review
The previous patch, cleaned up and including the styles in all the themes. (25.93 KB, patch)
2009-08-21 11:46 PDT, Blake Winton (:bwinton) (:☕️)
mozilla: review-
clarkbw: ui‑review+
Details | Diff | Review
The previous patch, with fixes for the problems bienvenu pointed out. (26.83 KB, patch)
2009-08-21 17:54 PDT, Blake Winton (:bwinton) (:☕️)
no flags Details | Diff | Review
The previous patch, updated with Bienvenu's suggestions. (27.35 KB, patch)
2009-08-22 17:33 PDT, Blake Winton (:bwinton) (:☕️)
no flags Details | Diff | Review
The previous patch, without memory leaks. (27.38 KB, patch)
2009-08-23 12:10 PDT, Blake Winton (:bwinton) (:☕️)
mozilla: review+
neil: superreview+
Details | Diff | Review
The previous patch, with neil's nits. (27.22 KB, patch)
2009-08-24 19:42 PDT, Blake Winton (:bwinton) (:☕️)
no flags Details | Diff | Review

Description Bryan Clark (DevTools PM) [@clarkbw] 2009-08-10 17:15:30 PDT
Created attachment 393674 [details]
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) 2009-08-10 18:03:35 PDT
Created attachment 393679 [details]
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] 2009-08-11 12:06:49 PDT
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) 2009-08-11 17:40:58 PDT
(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 2009-08-20 15:23:10 PDT
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 2009-08-20 16:57:28 PDT
Created attachment 395719 [details] [diff] [review]
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) (:☕️) 2009-08-20 17:05:37 PDT
Created attachment 395720 [details] [diff] [review]
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 2009-08-20 17:08:35 PDT
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] 2009-08-20 22:51:21 PDT
Created attachment 395783 [details] [diff] [review]
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) (:☕️) 2009-08-21 11:46:06 PDT
Created attachment 395891 [details] [diff] [review]
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) (:☕️) 2009-08-21 11:55:37 PDT
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 2009-08-21 13:07:44 PDT
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] 2009-08-21 13:34:14 PDT
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 2009-08-21 14:01:48 PDT
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 2009-08-21 14:23:00 PDT
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 2009-08-21 17:48:10 PDT
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) (:☕️) 2009-08-21 17:54:36 PDT
Created attachment 396006 [details] [diff] [review]
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 2009-08-22 07:19:54 PDT
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) (:☕️) 2009-08-22 11:55:04 PDT
(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 2009-08-22 12:02:18 PDT
>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) (:☕️) 2009-08-22 17:33:35 PDT
Created attachment 396102 [details] [diff] [review]
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 2009-08-23 11:00:12 PDT
+  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) (:☕️) 2009-08-23 12:10:31 PDT
Created attachment 396155 [details] [diff] [review]
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 2009-08-23 18:48:46 PDT
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 2009-08-23 19:37:11 PDT
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 neil@parkwaycc.co.uk 2009-08-24 03:06:14 PDT
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 neil@parkwaycc.co.uk 2009-08-24 03:32:32 PDT
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) (:☕️) 2009-08-24 19:42:33 PDT
Created attachment 396370 [details] [diff] [review]
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) 2009-08-25 01:30:36 PDT
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 neil@parkwaycc.co.uk 2009-08-25 01:49:20 PDT
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) (:☕️) 2009-08-25 06:33:54 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.