Closed Bug 57115 Opened 20 years ago Closed 9 years ago

mime.properties file should contain correct HTML

Categories

(MailNews Core :: MIME, defect, P3)

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 13.0

People

(Reporter: bugzilla, Assigned: aceman)

References

Details

Attachments

(1 file, 3 obsolete files)

The "mime.properties" contains incorrect html, weird chars, etc..
Attached patch Diff to fix incorrect HTML (obsolete) — Splinter Review
Keywords: patch, review
Sorry, this isn't happening for rtm.

- rhp
Status: NEW → ASSIGNED
Target Milestone: --- → Future
Perhaps this could be checked in now, since we're pasted rtm...:)
reassigning to ducarroz
Assignee: rhp → ducarroz
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Target Milestone: Future → mozilla0.9.7
Target Milestone: mozilla0.9.7 → mozilla0.9.9
Keywords: nsbeta1-
Target Milestone: mozilla0.9.9 → Future
Attachment #17437 - Attachment is obsolete: true
is this still nsbeta-
Keywords: patch, review
Blocks: 62715
Product: MailNews → Core
Assignee: ducarroz → nobody
Status: ASSIGNED → NEW
QA Contact: esther → mime
Product: Core → MailNews Core
Is this change wanted? Should I update it to current trunk?
(In reply to :aceman from comment #6)
> Is this change wanted? Should I update it to current trunk?

If something is still to be done : yes.
You mean if that file wasn't changed meanwhile in some other bug?
It seems not, so it is still applicable:
http://hg.mozilla.org/comm-central/file/c38d6dd2c8b7/mail/locales/en-US/chrome/messenger/mime.properties

However I need somebody to agree to the content of the changes.
Tags are changed to lowercase. Is there a reason for that, any spec? If not, I'd like it better unchanged (so uppercase). Also, there is a <table> inside <center>. I am not sure that is the most correct HTML, but maybe we don't care here. Or should CSS be avoided there?
Assignee: nobody → acelists
The properties in question may/should go away altogether soonish - see bug 492329.
Could you please specify the relation to that bug more?
This one is mostly about the "truncated message" (because of limit on downloaded message size). The other one is about expired newsgroup message. To me that is completely unrelated. Is there any big overhaul of both these placeholders planned? I couldn't see something like that in those comments.
Yes, bug 492329 comment 12 - squib is planning on getting rid of DisplayHTMLInMessagePane which I think is where those strings are used.
Ah, thanks. It isn't even filed yet. We'll see how soonish that one can be fixed. Thanks for catching that relation.
See Also: → 492329
Hm, I'm not sure that bug is actually related. As far as I can tell, these strings aren't referenced in the two other places that DisplayHTMLInMessagePane is called. I expect these strings are used at a lower level, probably in one of the MIME emitters. If that's the case, removing DisplayHTMLInMessagePane wouldn't affect this.
Magnus, what do you think?
Yeah, sorry i was mistaken.
OK, I'll try to come up with something.
Status: NEW → ASSIGNED
This just changes the current design using <table> to one using <div>s and CSS. But the overal appearance of the placeholder is preserved.

If anybody has any suggestions to make the placeholder nicer, just comment while I am at it :)
Attachment #578416 - Flags: ui-review?(bwinton)
Attachment #578416 - Flags: review?(dbienvenu)
Comment on attachment 578416 [details] [diff] [review]
HTML updated to hopefully more modern style (HTML5)

Review of attachment 578416 [details] [diff] [review]:
-----------------------------------------------------------------

So, a couple of notes:

1) I'm gonna need a set of steps to see these strings, cause as it is, it's fairly hard to review.
2) I prefer lowercase tag and attribute names.
3) All the Localization Notes are wrong.
4) I suspect all the "%"s you added should really be "%%".

I'm going to clear out the ui-review flag until point #1 is addressed, since it's the main thing that's blocking me.

Thanks,
Blake.
Attachment #578416 - Flags: ui-review?(bwinton)
1. This "placeholder" is shown inside a message when it is downloaded incomplete (e.g. due to account settings->disk space->do not download messages larger than X Kb, or when "only download headers" is set). Receive a message larger than that and you'll see this code used (at least on POP3).
2. I asked for that in comment 8. No problem, I can do that. It just optically looks better to me, and it is easier to spot where is code and where is text.
3. Yes, will fix when the HTML code is agreed upon.
4. I am not sure. The patch works this way for me (with single %). The percentages are interpreted correctly in the real message. I have even checked that the old versions with %% made it into the real message and were still shown as %% in DOM Inspector (it allows to save the current DOM to a file and the %% are still there).
Comment on attachment 578416 [details] [diff] [review]
HTML updated to hopefully more modern style (HTML5)

Maybe David will know if the % needs to be escaped (doubled).
Attachment #578416 - Flags: review?(dbienvenu) → feedback?(dbienvenu)
You'd also have to change the property names (so localizers would pick up). Which in this case is way painful :( I believe there's a bug about that painfulness...
Thanks, I always forget about that.
Do you know how to rename the names here? They are just numbers. Should I choose new ones? How? Which numbers are free to use? Is there any increasing ID counter?

There are also some @name identifiers inside comments. Are those automatically used anywhere? Should they be changed?

I have only updated .dtd files so far. This is my first real .properties file. Please advice me. Thanks.
Need to look at the @name in the properties file, and find all references to it. Like @name MIME_MSG_PARTIAL_FMT_1. Then just choose the next available number.
This will tell me how to change the 1037 number?
Yes, you need to change http://mxr.mozilla.org/comm-central/source/mailnews/mime/src/nsMimeStringResources.h#69 and similar for the other ones of course.
Does this mean I can change 1037 to 1045? Are these number unique globally across Thunderbird? Or do they just need to be unique in this file (and each file has its own number range)?
They should be unique for MimeGetStringByID() usage, so yes unique to that file afaikt (ok to change to 1045).
Thanks a lot.
Comment on attachment 578416 [details] [diff] [review]
HTML updated to hopefully more modern style (HTML5)

Still needs to resolve the % escaping problem (comment 20).
Attachment #578416 - Flags: feedback?(bugmail)
Comment on attachment 578416 [details] [diff] [review]
HTML updated to hopefully more modern style (HTML5)

I'm not the best person to review this, but it looks OK when I run with it.
Attachment #578416 - Flags: feedback?(dbienvenu) → feedback+
(In reply to David :Bienvenu from comment #31)
> Comment on attachment 578416 [details] [diff] [review]
> HTML updated to hopefully more modern style (HTML5)
> 
> I'm not the best person to review this, but it looks OK when I run with it.

Who should it be ?
Comment on attachment 578416 [details] [diff] [review]
HTML updated to hopefully more modern style (HTML5)

If it's not bienvenu, I think it's :squib, since he's been doing a lot of mime-related attachment stuff...

To reiterate Blake's request before he gives UX-review, the localization notes are still explicitly calling out the old HTML.
Attachment #578416 - Flags: feedback?(bugmail)
Comment on attachment 578416 [details] [diff] [review]
HTML updated to hopefully more modern style (HTML5)

Yes, the notes are wrong (and string IDs), that is why we did not request review. But we need somebody to decide if I should continue with the patch. If it will eventually be accepted.
Attachment #578416 - Flags: feedback?(squibblyflabbetydoo)
Comment on attachment 578416 [details] [diff] [review]
HTML updated to hopefully more modern style (HTML5)

Review of attachment 578416 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, this looks ok. I think when you go about renaming the string names, you can probably rename them to actual names, provided you update the relevant places in libmime that call this. That'll make life easier for anyone in the future.

::: mail/locales/en-US/chrome/messenger/mime.properties
@@ +151,5 @@
>  # Partial Message Format 1
>  ## @name MIME_MSG_PARTIAL_FMT_1
>  ## @loc
>  # LOCALIZATION NOTE (1037): In the following line, translate only the word, "Truncated!".
> +1037=<DIV STYLE="margin: 1em auto; border: 1px solid black; width: 80%"><DIV STYLE="margin: 5px; padding: 10px; border: 1px solid gray; font-weight: bold; text-align: center;"><SPAN STYLE="font-size: 120%;">Truncated!</SPAN><HR>

I'd really like to see the styles be defined in an external stylesheet, but that might be out of the scope for this bug.

@@ +166,5 @@
>  ## @name MIME_MSG_PARTIAL_FMT_3
>  ## @loc
>  # LOCALIZATION NOTE (1039): This section must begin with the ">" sign and end with the tags,"</B></TD></TR></TABLE></CENTER>"
>  # Do not translate "</A>" tag.
> +1039=">Click here to download the rest of the message.</A></DIV></DIV>

If we're going to change what text is the link, I think we should just improve the text. How about "Download the rest of the message."? "Click here" is redundant for links.

@@ +199,5 @@
>  ## @name MIME_MSG_PARTIAL_FMT2_3
>  ## @loc
>  # LOCALIZATION NOTE (1044): This section must begin with the ">" sign and end with the tags,"</B></TD></TR></TABLE></CENTER>"
>  # Do not translate "</A>" tag.
> +1044=">Click here to download the rest of the message.</A></DIV></DIV>

Likewise, let's change this text too.
Attachment #578416 - Flags: feedback?(squibblyflabbetydoo) → feedback+
Change the numbers to names as in other .properties files?
Can I then remove the defines from here: http://mxr.mozilla.org/comm-central/source/mailnews/mime/src/nsMimeStringResources.h#69 ? And just call the strings from source by the new name?

I would define the style in a .css but would not like it to be part of the theme. I think that would be needless duplication in all themes. Is there some TB private css file where this can be stuffed? Or where to create one?
(In reply to :aceman from comment #36)
> Change the numbers to names as in other .properties files?
> Can I then remove the defines from here:
> http://mxr.mozilla.org/comm-central/source/mailnews/mime/src/
> nsMimeStringResources.h#69 ? And just call the strings from source by the
> new name?

Correct.

> I would define the style in a .css but would not like it to be part of the
> theme. I think that would be needless duplication in all themes. Is there
> some TB private css file where this can be stuffed? Or where to create one?

Well, we're already well past that point for the theme code. We could* always make mail/themes/common and put common CSS there, but that's a pretty big undertaking. Again though, I wouldn't r- a patch that puts the styles in the strings like it is now.

Oh also, if it turns out to be easy, you could put the HTML in the .cpp files, but that's also totally optional.

* I think. Maybe it's not possible...
(In reply to Jim Porter (:squib) from comment #37)
> Oh also, if it turns out to be easy, you could put the HTML in the .cpp
> files, but that's also totally optional.
> 
> * I think. Maybe it's not possible...

Good idea, I can look at that.
Attached patch patch v3 (obsolete) — Splinter Review
What about this?
HTML included into code, string Names, HTML tags lowercased. Great for translators, no more notes needed. Duplicate strings of "Download the rest of the message" merged. Added the same strings for Seamonkey.
Anything else?
Attachment #578416 - Attachment is obsolete: true
Attachment #594805 - Flags: review?(squibblyflabbetydoo)
Attachment #594805 - Flags: review?(iann_bugzilla)
Summary: property file should contain correct HTML → mime.properties file should contain correct HTML
Target Milestone: Future → ---
Attachment #594805 - Flags: ui-review?(bwinton)
Comment on attachment 594805 [details] [diff] [review]
patch v3

>+  partialMsgHtml.AppendLiteral("<div style='margin: 1em auto; border: 1px solid black; width: 80%'>");
>+  partialMsgHtml.AppendLiteral("<div style='margin: 5px; padding: 10px; border: 1px solid gray; font-weight: bold; text-align: center;'>");
>+
>+  partialMsgHtml.AppendLiteral("<span style='font-size: 120%;'>");
As a follow-up bug, as suggested elsewhere, could these styles be moved to themes.

>+++ b/suite/locales/en-US/chrome/mailnews/mime.properties
>+# Partial Message Truncated Explanation
>+## @name MIME_MSG_PARTIAL_TRUNCATED_EXPLANATION
> ## @loc
>+MIME_MSG_PARTIAL_TRUNCATED_EXPLANATION=This message exceeded the Maximum Message Size set in Account Settings, so we have only downloaded the first few lines from the mail server.
Nit: Is "we" usually used in messages? Alternate suggestion "so only the first few lines have been downloaded from the mail server."

>+# Partial Message Not Downloaded Explanation
>+## @name MIME_MSG_PARTIAL_NOT_DOWNLOADED_EXPLANATION
>+## @loc
>+MIME_MSG_PARTIAL_NOT_DOWNLOADED_EXPLANATION=Only the headers for this message were downloaded from the mail server.
Nit: Potentially this should be "have been" or the other alternate explanation changed to "were"
Attachment #594805 - Flags: review?(iann_bugzilla) → review+
(In reply to Ian Neal from comment #40)
> Comment on attachment 594805 [details] [diff] [review]
> patch v3
> 
> >+  partialMsgHtml.AppendLiteral("<div style='margin: 1em auto; border: 1px solid black; width: 80%'>");
> >+  partialMsgHtml.AppendLiteral("<div style='margin: 5px; padding: 10px; border: 1px solid gray; font-weight: bold; text-align: center;'>");
> >+
> >+  partialMsgHtml.AppendLiteral("<span style='font-size: 120%;'>");
> As a follow-up bug, as suggested elsewhere, could these styles be moved to
> themes.

Yes it would be nice but I think also squib concluded that there is not yet infrastructure for this. We'd like this to be in some css file but not in the theme. Do you know such a place?
Comment on attachment 594805 [details] [diff] [review]
patch v3

Review of attachment 594805 [details] [diff] [review]:
-----------------------------------------------------------------

I just took a quick look at the code; I didn't compile with this. r=me, with the issues below fixed, and assuming iann made sure this works. :)

::: mailnews/mime/src/mimemsg.cpp
@@ +871,5 @@
>    if (gtPtr)
>      *gtPtr = 0;
>  
> +  bool msgBaseTruncated;
> +  msgBaseTruncated = (msg->bodyLength > MSG_LINEBREAK_LEN);

Just put the declaration/assignment on one line, please.

@@ +879,5 @@
>  
> +  partialMsgHtml.AppendLiteral("<div style='margin: 1em auto; border: 1px solid black; width: 80%'>");
> +  partialMsgHtml.AppendLiteral("<div style='margin: 5px; padding: 10px; border: 1px solid gray; font-weight: bold; text-align: center;'>");
> +
> +  partialMsgHtml.AppendLiteral("<span style='font-size: 120%;'>");

Use double quotes here for the style definition here and in other places. If you're unfamiliar with C++, you'll need to escape the quotes with backslashes like so: AppendLiteral("<div style=\"foo: bar;\">");

@@ +897,2 @@
>  
> +  partialMsgHtml.AppendLiteral("<a href='");

Double-quotes here.

@@ +915,5 @@
>  
>      partialMsgHtml.Append(item);
>    }
>  
> +  partialMsgHtml.AppendLiteral("'>");

Double-quotes here too.
Attachment #594805 - Flags: review?(squibblyflabbetydoo) → review+
(In reply to Jim Porter (:squib) from comment #42)
> > +  partialMsgHtml.AppendLiteral("<span style='font-size: 120%;'>");
> 
> Use double quotes here for the style definition here and in other places. 

Why do you want that? This seems like an ideal case for single quotes. 
(I think double quotes should be used when there is no reason to use single, but here it seems valid for readability.)
(In reply to Magnus Melin from comment #43)
> Why do you want that? This seems like an ideal case for single quotes. 
> (I think double quotes should be used when there is no reason to use single,
> but here it seems valid for readability.)

Most of the HTML we have in Thunderbird is actually formatted as XHTML, where double quotes are required. I think we should continue this habit.
Attached patch patch v4Splinter Review
Thanks to all, carrying over reviews.
Does this still need ui-review? The look of the table is mostly unchanged and I addressed bwinton's nits.
Attachment #594805 - Attachment is obsolete: true
Attachment #595834 - Flags: review+
Attachment #594805 - Flags: ui-review?(bwinton)
(In reply to Jim Porter (:squib) from comment #44)
> Most of the HTML we have in Thunderbird is actually formatted as XHTML,
> where double quotes are required. 

XHTML doesn't require double quotes.
(In reply to Magnus Melin from comment #46)
> (In reply to Jim Porter (:squib) from comment #44)
> > Most of the HTML we have in Thunderbird is actually formatted as XHTML,
> > where double quotes are required. 
> 
> XHTML doesn't require double quotes.

Hm. Well, I'd still prefer it, since it's pretty universally-used in Mozilla, but I won't hold it against the patch otherwise.
It is less readable, but I have already done it in v4.
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/03d5af18339f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 13.0
It looks like there are .css files in mail/base/content. Would it be OK to stuff the css in some of them?
You need to log in before you can comment on or make changes to this bug.