Last Comment Bug 694514 - Expose mailnews.reply_header_type preference
: Expose mailnews.reply_header_type preference
Status: RESOLVED FIXED
[mentor=IanN][lang=js]
:
Product: SeaMonkey
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- enhancement with 2 votes (vote)
: seamonkey2.10
Assigned To: Edmund Wong (:ewong)
:
:
Mentors:
Depends on:
Blocks: 13818 107884 731223
  Show dependency treegraph
 
Reported: 2011-10-14 02:40 PDT by Ian Neal
Modified: 2012-04-03 05:59 PDT (History)
7 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Expose mailnews.reply_header_type preference. (v1) (3.60 KB, patch)
2012-02-29 01:22 PST, Edmund Wong (:ewong)
no flags Details | Diff | Splinter Review
Expose mailnews.reply_header_type preference. (v2) (3.52 KB, patch)
2012-02-29 02:43 PST, Edmund Wong (:ewong)
iann_bugzilla: review-
Details | Diff | Splinter Review
Expose mailnews.reply_header_type preference. (v3) (4.04 KB, patch)
2012-03-01 01:05 PST, Edmund Wong (:ewong)
iann_bugzilla: review+
Details | Diff | Splinter Review
Expose mailnews.reply_header_type preference. (v4) (3.51 KB, patch)
2012-03-04 05:09 PST, Edmund Wong (:ewong)
ewong: review+
Details | Diff | Splinter Review

Description Ian Neal 2011-10-14 02:40:52 PDT
There exists the ability to change the reply header type using the mailnews.reply_header_type preference:
// 0 - No Reply-Text
// 1 - "[Author] wrote:"
// 2 - "On [date] [author] wrote:"
// 3 - User-defined reply header.

Bug 107884 deals with configuring when set it is set to 3, but we should expose this preference to users with some sort of warning/note when users set it to 3 for the moment.
Comment 1 rsx11m 2011-10-14 06:57:48 PDT
Actually, the "3" switches the order to show mailnews.reply_header_authorwrote followed by mailnews.reply_header_ondate and is the default in some locales for Thunderbird where you need a "<name> wrote on <date>" order.

There is currently a placeholder in nsMsgCompose.cpp for "4" being the value for free user-defined headers, which may or may not be implemented in bug 107884.
Comment 2 Raymond 2011-10-14 15:27:41 PDT
I suggest:
// 4 - same as when we do a "Forward".
Comment 3 rsx11m 2011-10-14 16:20:10 PDT
As long as "4" doesn't perform any function it should be just omitted from any UI implemented here. It should be easy to add later, e.g., as another menu item.
Comment 4 Edmund Wong (:ewong) 2012-02-29 01:22:05 PST
Created attachment 601552 [details] [diff] [review]
Expose mailnews.reply_header_type preference. (v1)

Added to Mail & Newsgroups -> Composition.
Comment 5 Edmund Wong (:ewong) 2012-02-29 02:43:40 PST
Created attachment 601565 [details] [diff] [review]
Expose mailnews.reply_header_type preference. (v2)
Comment 6 Ian Neal 2012-02-29 16:28:20 PST
Comment on attachment 601565 [details] [diff] [review]
Expose mailnews.reply_header_type preference. (v2)

Have a look at:
http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#2299
which explains what each value does better than I did.
Comment 7 Edmund Wong (:ewong) 2012-03-01 01:05:54 PST
Created attachment 601899 [details] [diff] [review]
Expose mailnews.reply_header_type preference. (v3)
Comment 8 Raymond 2012-03-01 15:23:13 PST
Is it possible to add:
+              <menuitem value="5"
+                        label="&sameAsForwardOption.label;"/>
Comment 9 Ian Neal 2012-03-01 15:27:47 PST
(In reply to Raymond from comment #8)
> Is it possible to add:
> +              <menuitem value="5"
> +                        label="&sameAsForwardOption.label;"/>

5 is not a valid option in the backend as far as I am aware.
Comment 10 Raymond 2012-03-01 15:30:56 PST
In fact - we were expecting to have &sameAsForwardOption.label;"/>
instead of one of the others ...
Comment 11 rsx11m 2012-03-01 15:39:11 PST
Raymond - if it's not supported in the backend (and indeed it is *not*) it doesn't make sense to add a label to an option which isn't implemented. Thus, your efforts are moot here.

BTW: That's bug 218258 where you'll need to advocate for it, the only reason why it showed up in bug 107884 was that it would have been a good/comprehensive template to use as a default.
Comment 12 Raymond 2012-03-01 15:49:39 PST
Is it so difficult to implement it, when we know that the coding is already done when forwarding. It's a pity situation when we see that nobody take this https://bugzilla.mozilla.org/show_bug.cgi?id=218258 evenwhile with 32 votes instead of 2 here - and also because it's 8 years ...old.
Comment 13 rsx11m 2012-03-01 15:51:03 PST
> http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#2320

Option #4 currently just defaults to whatever #1 does, thus I'm wondering if it should be offered at this time or just hidden pending any action in bug 107884. Your patch shows it right now but then adds another label explaining that it's not implemented yet, which would have to be removed once it actually has been implemented. So, either way it means follow-up work whenever more options become available.
Comment 14 rsx11m 2012-03-01 15:51:43 PST
Previous comment was directed to Edmund, comment #12 is off-topic.
Comment 15 Ian Neal 2012-03-03 06:36:25 PST
Comment on attachment 601899 [details] [diff] [review]
Expose mailnews.reply_header_type preference. (v3)

>+++ b/suite/locales/en-US/chrome/mailnews/pref/pref-composing_messages.dtd

>+<!ENTITY userDefinedOption.label              "User-defined reply header">
>+<!ENTITY userDefinedNoticeValue.label         "User defined reply header is not yet implemented">
I'd not bother with the user defined option here.

>+++ b/suite/mailnews/compose/prefs/pref-composing_messages.xul
>+            <menupopup>
>+              <menuitem value="0"
>+                        label="&noReplyOption.label;"/>
>+              <menuitem value="1"
>+                        label="&authorWroteOption.label;"/>
>+              <menuitem value="2"
>+                        label="&onDateWroteOption.label;"/>
>+              <menuitem value="3"
>+                        label="&authorOnDateWroteOption.label;"/>
>+              <menuitem value="4"
>+                        label="&userDefinedOption.label;"/>
>+              <menuseparator/>
No need for a menuseparator here (or user defined).

r=me with those fixed.
Comment 16 Edmund Wong (:ewong) 2012-03-04 05:09:24 PST
Created attachment 602710 [details] [diff] [review]
Expose mailnews.reply_header_type preference. (v4)
Comment 17 Edmund Wong (:ewong) 2012-03-04 18:38:07 PST
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/9f6c12aa2efb
Comment 18 Thomas D. (currently busy elsewhere; needinfo?me) 2012-03-04 23:59:33 PST
Edmund, can you attach screenshot of implemented SeaMonkey solution?

Is this something Thunderbird could/should do, too?
Comment 19 Serge Gautherie (:sgautherie) 2012-03-05 04:46:59 PST
Comment on attachment 602710 [details] [diff] [review]
Expose mailnews.reply_header_type preference. (v4)

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

::: suite/locales/en-US/chrome/mailnews/pref/pref-composing_messages.dtd
@@ +71,5 @@
> +<!ENTITY selectHeaderType.label               "Select reply header type :">
> +<!ENTITY selectHeaderType.accesskey           "e">
> +<!ENTITY noReplyOption.label                  "No Reply Header">
> +<!ENTITY authorWroteOption.label              "[Author] wrote:">
> +<!ENTITY onDateWroteOption.label              "On [date] [author] wrote:">

Care to s/onDateWroteOption/onDateAuthorWroteOption/g?
Comment 20 rsx11m 2012-03-05 09:03:59 PST
(In reply to Serge Gautherie (:sgautherie) from comment #19)
> Care to s/onDateWroteOption/onDateAuthorWroteOption/g?

Agreed, that's longer but would be more consistent.
Comment 21 Jens Hatlak (:InvisibleSmiley) 2012-03-05 10:00:52 PST
Comment on attachment 602710 [details] [diff] [review]
Expose mailnews.reply_header_type preference. (v4)

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

::: suite/locales/en-US/chrome/mailnews/pref/pref-composing_messages.dtd
@@ +67,5 @@
>  <!ENTITY fontColor.accesskey                  "T">
>  <!ENTITY bgColor.label                        "Background:">
>  <!ENTITY bgColor.accesskey                    "B">
> +
> +<!ENTITY selectHeaderType.label               "Select reply header type :">

Ian, are you really OK with the space before the colon or did you miss it? I think it looks terrible.
Comment 22 Jens Hatlak (:InvisibleSmiley) 2012-03-05 10:09:14 PST
Comment on attachment 602710 [details] [diff] [review]
Expose mailnews.reply_header_type preference. (v4)

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

::: suite/locales/en-US/chrome/mailnews/pref/pref-composing_messages.dtd
@@ +72,5 @@
> +<!ENTITY selectHeaderType.accesskey           "e">
> +<!ENTITY noReplyOption.label                  "No Reply Header">
> +<!ENTITY authorWroteOption.label              "[Author] wrote:">
> +<!ENTITY onDateWroteOption.label              "On [date] [author] wrote:">
> +<!ENTITY authorOnDateWroteOption.label        "[Author] on [date] wrote:">

onDateWroteOption.label should really be "On [date], [author] wrote:", and authorOnDateWroteOption.label should really be "[Author] wrote, On [date]:".

Just go through the options yourself and check what the effect is. I did, hence the corrections.
Comment 23 rsx11m 2012-03-05 10:38:33 PST
If you want to do it really sophisticated you'd get the current values of mailnews.reply_header_{authorwrote,colon,ondate,separator} and build those labels on the fly, but that may be overdoing it.
Comment 24 Ian Neal 2012-03-05 13:33:08 PST
(In reply to Jens Hatlak (:InvisibleSmiley) from comment #21)
> Comment on attachment 602710 [details] [diff] [review]
> Expose mailnews.reply_header_type preference. (v4)
> 
> Review of attachment 602710 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: suite/locales/en-US/chrome/mailnews/pref/pref-composing_messages.dtd
> @@ +67,5 @@
> >  <!ENTITY fontColor.accesskey                  "T">
> >  <!ENTITY bgColor.label                        "Background:">
> >  <!ENTITY bgColor.accesskey                    "B">
> > +
> > +<!ENTITY selectHeaderType.label               "Select reply header type :">
> 
> Ian, are you really OK with the space before the colon or did you miss it? I
> think it looks terrible.
Yes, I did miss the space, sorry.
Comment 25 Ian Neal 2012-03-05 13:41:30 PST
(In reply to Jens Hatlak (:InvisibleSmiley) from comment #22)
> Comment on attachment 602710 [details] [diff] [review]
> Expose mailnews.reply_header_type preference. (v4)
> 
> Review of attachment 602710 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: suite/locales/en-US/chrome/mailnews/pref/pref-composing_messages.dtd
> @@ +72,5 @@
> > +<!ENTITY selectHeaderType.accesskey           "e">
> > +<!ENTITY noReplyOption.label                  "No Reply Header">
> > +<!ENTITY authorWroteOption.label              "[Author] wrote:">
> > +<!ENTITY onDateWroteOption.label              "On [date] [author] wrote:">
> > +<!ENTITY authorOnDateWroteOption.label        "[Author] on [date] wrote:">
> 
> onDateWroteOption.label should really be "On [date], [author] wrote:", and
> authorOnDateWroteOption.label should really be "[Author] wrote, On [date]:".
> 
I did notice this but somehow I lost the comment. It did include talking about spinning off a bug for tweaking the format of these options and changing what SM's default should be (I think we should be matching TB on this).
Within this bug, yes, it needs to match the current format.
Comment 26 Ian Neal 2012-03-05 13:42:39 PST
(In reply to rsx11m from comment #23)
> If you want to do it really sophisticated you'd get the current values of
> mailnews.reply_header_{authorwrote,colon,ondate,separator} and build those
> labels on the fly, but that may be overdoing it.
Possible but probably more effort than it is worth.
Comment 27 rsx11m 2012-03-05 13:46:20 PST
Agreed to comment #26, only an advanced user would tweak those preferences, thus the labels can reflect the defaults.
Comment 28 Thomas D. (currently busy elsewhere; needinfo?me) 2012-03-05 14:46:04 PST
(In reply to Ian Neal from comment #25)
> > onDateWroteOption.label should really be "On [date], [author] wrote:", and
> > authorOnDateWroteOption.label should really be "[Author] wrote, On [date]:".

I assume this applies to English locale only (e.g. the first word order wouldn't work for German). The second proposal doesn't look good for English: Spelling: no capital after a comma, so at least, it should be "[Author] wrote, on [date]:" That's "Lisa wrote, on 2012-03-05 23:41:" I'm not sure if the comma is needed or good style. Perhaps we could do without: "Lisa wrote on 2012-03-05 23:41:"
Comment 29 rsx11m 2012-03-05 17:08:41 PST
Thomas, this bug cannot change the way how the MailNews backend works, it only provides a UI for it (and that's indeed what the backend comes up with).

Localizers can modify punctuation and wording as the language grammar requires it, so these cases are to that extent already considered.
Comment 30 rsx11m 2012-03-05 18:50:17 PST
Follow-up work is collected in bug 733258.
Comment 31 Alexander L. Slovesnik 2012-03-07 08:54:35 PST
Can anyone explain, should words inside brackets ([Author] and [date]) be translated?
I see that 3 locales (fr, nl and gl) left these words as in en-US, but es-AR locale have translated them as [Autor] and [fecha].
Comment 32 Jens Hatlak (:InvisibleSmiley) 2012-03-07 11:11:56 PST
(In reply to Alexander L. Slovesnik from comment #31)
> Can anyone explain, should words inside brackets ([Author] and [date]) be
> translated?

Yes, they should. Think of these as variables.

> I see that 3 locales (fr, nl and gl) left these words as in en-US, but es-AR
> locale have translated them as [Autor] and [fecha].

Oh, sorry for that. I'll post to the l10n newsgroup then. In theory these languages could spell these words exactly like en-US, but I doubt it. ;-)

Please note that bug 733258 will tweak the exact wording of the options (and some of the entities) again (hopefully really soon now), as noted in this bug.

If you want to do the right thing for your locale, please open MailNews and reply to some message after changing the new UI preference (or mailnews.reply_header_type in about:config) to each of the four supported options (values 0-3).

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