mime.properties file should contain correct HTML

RESOLVED FIXED in Thunderbird 13.0

Status

MailNews Core
MIME
P3
minor
RESOLVED FIXED
17 years ago
5 years ago

People

(Reporter: Henrik Gemal, Assigned: aceman)

Tracking

Trunk
Thunderbird 13.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

17 years ago
The "mime.properties" contains incorrect html, weird chars, etc..
(Reporter)

Comment 1

17 years ago
Created attachment 17437 [details] [diff] [review]
Diff to fix incorrect HTML
(Reporter)

Updated

17 years ago
Keywords: patch, review

Comment 2

17 years ago
Sorry, this isn't happening for rtm.

- rhp
Status: NEW → ASSIGNED
Target Milestone: --- → Future
(Reporter)

Comment 3

17 years ago
Perhaps this could be checked in now, since we're pasted rtm...:)

Comment 4

17 years ago
reassigning to ducarroz
Assignee: rhp → ducarroz
Status: ASSIGNED → NEW

Updated

16 years ago
Status: NEW → ASSIGNED
Target Milestone: Future → mozilla0.9.7

Updated

16 years ago
Target Milestone: mozilla0.9.7 → mozilla0.9.9

Updated

16 years ago
Keywords: nsbeta1-
Target Milestone: mozilla0.9.9 → Future
(Reporter)

Updated

16 years ago
Attachment #17437 - Attachment is obsolete: true
(Reporter)

Comment 5

16 years ago
is this still nsbeta-
Keywords: patch, review
(Reporter)

Updated

16 years ago
Blocks: 62715
Product: MailNews → Core
Assignee: ducarroz → nobody
Status: ASSIGNED → NEW
QA Contact: esther → mime
Product: Core → MailNews Core
(Assignee)

Comment 6

6 years ago
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.
(Assignee)

Comment 8

6 years ago
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

Comment 9

6 years ago
The properties in question may/should go away altogether soonish - see bug 492329.
(Assignee)

Comment 10

6 years ago
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.

Comment 11

6 years ago
Yes, bug 492329 comment 12 - squib is planning on getting rid of DisplayHTMLInMessagePane which I think is where those strings are used.
(Assignee)

Comment 12

6 years ago
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: → bug 492329

Comment 13

6 years ago
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.
(Assignee)

Comment 14

6 years ago
Magnus, what do you think?

Comment 15

6 years ago
Yeah, sorry i was mistaken.
(Assignee)

Comment 16

6 years ago
OK, I'll try to come up with something.
Status: NEW → ASSIGNED
(Assignee)

Comment 17

6 years ago
Created attachment 578416 [details] [diff] [review]
HTML updated to hopefully more modern style (HTML5)

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)
(Assignee)

Comment 19

6 years ago
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).
(Assignee)

Comment 20

6 years ago
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)

Comment 21

6 years ago
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...
(Assignee)

Comment 22

6 years ago
Thanks, I always forget about that.
(Assignee)

Comment 23

6 years ago
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.

Comment 24

6 years ago
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.
(Assignee)

Comment 25

6 years ago
This will tell me how to change the 1037 number?

Comment 26

6 years ago
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.
(Assignee)

Comment 27

6 years ago
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)?

Comment 28

6 years ago
They should be unique for MimeGetStringByID() usage, so yes unique to that file afaikt (ok to change to 1045).
(Assignee)

Comment 29

6 years ago
Thanks a lot.
(Assignee)

Comment 30

5 years ago
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 31

5 years ago
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)
(Assignee)

Comment 34

5 years ago
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 35

5 years ago
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+
(Assignee)

Comment 36

5 years ago
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?

Comment 37

5 years ago
(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...
(Assignee)

Comment 38

5 years ago
(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.
(Assignee)

Comment 39

5 years ago
Created attachment 594805 [details] [diff] [review]
patch v3

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)
(Assignee)

Updated

5 years ago
Attachment #594805 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

5 years ago
Summary: property file should contain correct HTML → mime.properties file should contain correct HTML
Target Milestone: Future → ---
(Assignee)

Updated

5 years ago
Attachment #594805 - Flags: ui-review?(bwinton)

Comment 40

5 years ago
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+
(Assignee)

Comment 41

5 years ago
(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 42

5 years ago
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+

Comment 43

5 years ago
(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.)

Comment 44

5 years ago
(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.
(Assignee)

Comment 45

5 years ago
Created attachment 595834 [details] [diff] [review]
patch v4

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)

Comment 46

5 years ago
(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.

Comment 47

5 years ago
(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.
(Assignee)

Comment 48

5 years ago
It is less readable, but I have already done it in v4.
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/03d5af18339f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 13.0
(Assignee)

Comment 50

5 years ago
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.