441659 - Implement UI for mail.reply_quote_inline pref

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Bruno 'Aqualon' Escherl]

(In reply to bug 429143 comment #12)
> Possibly look at implementing UI for new pref introduced in fix to bug 384599
> here.

Comment 1

[rsx11m]

Attachment: Proposed patch

Now that the toolkit transition for the Composition pane is complete, I've adapted the patch I made for Thunderbird for it and added a respective help text. Asking Ian for primary review, please let me know which other reviews are required if this finds your approval.

Comment 2

[rsx11m]

Attachment: Screen shot of Composition pref pane

The checkbox is placed right underneath the settings for the forwarding mode, thus keeping forward and reply behavior in a close context before proceeding to the autosave. To reiterate the motivation for this option:

> As summary from bug 384599, having the contents of attachments quoted in a
> reply to incoming messages is of interest whenever some sort of annotation or
> comments on those attachments is desired. Important use cases are review and
> ticketing systems (or a collection of jokes and cartoons, to name a more casual
> example) where inline commenting for an attachment is needed within the actual
> context of that attachment, similar to the ability of commenting on patches in
> bugzilla. In this way, the attachments become integral components of the
> message, even though they were sent as separate parts.
> 
> Another example, when replying to a message which has a message/rfc822
> attachment, it may be desirable to provide the forwarded message as part of
> the quote to be able to add inline replies. Again, the message attachment can
> be considered part of the forwarding message, even if forwarded as attachment.

Given that per bug 161775 attachment contents are no longer quoted by default, making that option more visible by a UI element would be beneficial for these use cases.

Comment 3

[Ian Neal]

Comment on attachment 335265 [details] [diff] [review]
Proposed patch

>diff -r 2854814076ae suite/locales/en-US/chrome/common/help/mail_help.xhtml

>+  <li><strong>Quote the contents of displayed inline attachments in
>+    replies</strong>: If this options is checked, images, text, and messages
"option" rather than "options". Is this an exhaustive list? Perhaps it should be something like:
"If this option is checked, attachments displayed inline (such as images, text
and messages) are included in the quote."
Though you might prefer commas to brackets and you could have "for example" instead of "such as"
>+    attached to an e-mail and displayed when replied to are included in the
>+    quote.</li>

>+<!ENTITY replyQuoteInline.label               "Quote the contents of displayed inline attachments in replies">
Not sure if this should be "contents" or "content"

r=me with those addressed, you will need an sr= from Neil

Comment 4

[rsx11m]

(In reply to comment #3)
> "option" rather than "options".

Oops, correct.

> Is this an exhaustive list?

It's probably not exhaustive, even though those should be the most frequently met attachment types displayed and quoted inline.

> Though you might prefer commas to brackets and you could have "for example"
> instead of "such as"

The brackets are probably better, "for example" would need another comma,
"such as" flows better, thus will take your first phrasing.

> Not sure if this should be "contents" or "content"

Thought it is "contents" given that there may be multiple attachments (thus multiple contents). I'm not quite sure either, maybe Neil can resolve that.

> r=me with those addressed, you will need an sr= from Neil

Thanks, I'll post an updated patch with help-file changes momentarily.

Comment 5

[rsx11m]

Attachment: Proposed patch (v2)

Updated version with help text changed per review comments. I retained the
"when replying to an e-mail" subsentence to keep the context unambiguous.

Forwarding r+ from attachment 335265 [details] [diff] [review].

Comment 6

[[e-mail]]

Comment on attachment 335306 [details] [diff] [review]
Proposed patch (v2)

>+<!ENTITY replyQuoteInline.label               "Quote the contents of displayed inline attachments in replies">
Does this preference depend on View Attachments Inline? The label seems to suggest so, but I think it's too wordy. It might be better to invert the checkbox (I think there's an attribute for it although I don't remember it offhand) and change the label to "Never quote inline attachments". Or you could at least tweak the wording to say "Quote the contents of attachments viewed inline".

Comment 7

[rsx11m]

(In reply to comment #6)
> Does this preference depend on View Attachments Inline?

Yes, this hasn't been changed. I think it's intuitive this way as contents not visible when viewing the e-mail shouldn't be included in the quote.

> invert the checkbox (I think there's an attribute for it ...)

I yet have to try this, <preference ... type="bool" inverted="true"/>

> and change the label to "Never quote inline attachments".

Personally, I prefer a positive phrase to avoid a double-negative logic of "disable the disable to enable this function" which may be confusing to many users. A negative expression also requires to change the help text to either
"If this option is <em>not</em> checked" or "are <em>not</em> included", whatever sounds better.

The word "reply" should probably be retained in the label, given that the preference before it relates to the "forward" functionality. Following suggestions for the label, with 'Q' being the access key in all cases:

"Quote attachments viewed inline when replying" (positive logic)
"For replies, quote attachments viewed inline"

"Never quote inline attachments in replies"   (negative logic w/o "viewed")
"For replies, don't quote inline attachments" (or "attachments viewed inline")

Ian, what do you think?

Comment 8

[Ian Neal]

I agree that the label is a bit too wordy but I couldn't think of anything better (well at the time of commenting anyway). Is it just replies or is it forwarded messages too? If both not sure if that helps though as you either drop the replying, add forwarding or change to something like composing (which is obvious because of the name of the pref pane).
Sometimes hard to avoid double negatives when talking about a label. As the pref is false as default I'd prefer not to have a negative phrase checked as default unless having it negative makes it considerably less wordy (which I don't think it does in this case).

Comment 9

[rsx11m]

(In reply to comment #8)
> Is it just replies or is it forwarded messages too?

This only affects replies (forwarding quoted has been depreciated).

> Sometimes hard to avoid double negatives when talking about a label. As the
> pref is false as default I'd prefer not to have a negative phrase checked as
> default unless having it negative makes it considerably less wordy

Ok, thus we both lean towards retaining the positive logic then.

> "Quote attachments viewed inline when replying" (positive logic)
> "For replies, quote attachments viewed inline"

Neil, any preference for either of those? The first phrasing is more fluent,
the second emphasizes that the checkbox relates to the reply action.

Comment 10

[[e-mail]]

(In reply to comment #9)
> "For replies, quote attachments viewed inline"
I like this one.

Comment 11

[[e-mail]]

Comment on attachment 335306 [details] [diff] [review]
Proposed patch (v2)

Wow, that's some bad copy & paste - copied the wrong line completely :-(

>+  <li><strong>Quote the contents of displayed inline attachments in
>+    replies</strong>: If this option is checked when replying to an e-mail,
>+    attachments displayed inline (such as images, text, or messages) are
>+    included in the quote.</li>
Quote attachments viewed inline in replies: When replying to an email, then if this option is checked then attachments (such as images, text or messages) viewed inline are included in the quote.

>+<!ENTITY replyQuoteInline.label               "Quote the contents of displayed inline attachments in replies">
Quote attachments viewed inline in replies

Comment 12

[rsx11m]

(In reply to comment #11)
> Quote attachments viewed inline in replies: When replying to an email, then if
> this option is checked then attachments (such as images, text or messages)
> viewed inline are included in the quote.

This reads a bit strange with two "then", how about the following:

When replying to an email, attachments (such as images, text, or messages) viewed inline are included in the quote if this option is checked.

Or is "When ..., then attachments ..." needed? It sounds better to me without.

BTW: The second comma after "text," is needed for en-US, not in en-GB locale.

> Quote attachments viewed inline in replies

Ok, that's the final label then.

Comment 13

[[e-mail]]

(In reply to comment #12)
> This reads a bit strange with two "then"
Unfortunately we're writing in English, not Perl, so we can't write "when (replying to an email) { if (this option is checked) { attachments view inline are included in the quote; } }", we have to use "then" instead. Like Perl, you can get rid of one or both of the "then" by moving the condition:

If this options is checked, then, when replying to an email, attachments (such as images, text or messages) viewed inline are included in the quote.

If this options is checked, then attachments (such as images, text or messages) viewed inline are included in the quote when replying to an email.

Attachments (such as images, text or messages) viewed inline are included in the quote when replying to an email if this option is checked.

Attachments (such as images, text or messages) viewed inline are included in the quote if this option is checked when replying to an email.

(Note that these last two are subtly different; the last one doesn't seem to make it clear that you have to check the option before replying.)

Comment 14

[rsx11m]

(In reply to comment #13)
> Unfortunately we're writing in English, not Perl, 

:-)

> If this option is checked, then attachments (such as images, text, or messages)
> viewed inline are included in the quote when replying to an email.

I like this one, it puts checking the box upfront and then states its result with the amount of commas minimized.

Comment 15

[[e-mail]]

IanN, are you OK with that one too?

Comment 16

[Ian Neal]

(In reply to comment #15)
> IanN, are you OK with that one too?
> 
Yes, I am even with the en-US comma ;)

Do we spell it email, e-mail, or message?

Comment 17

[rsx11m]

Attachment: Final patch (v3) (Checkin: Comment 20)

> Do we spell it email, e-mail, or message?

Searching through mail_help.xhtml, it is spelled "email" without a dash,
thus I kept it consistent. The term "message" can apply equally to an email
or newsgroup message (but can be content of an attachment to be quoted).

I will wait marking this for checkin until tomorrow evening, in case either
of you has any further last-minute comments.

Comment 18

[rsx11m]

Comment on attachment 335455 [details] [diff] [review]
Final patch (v3) (Checkin: Comment 20)

No further comments from the reviewers, thus push for comm-central please.

r=IanN, sr=Neil

Comment 19

[rsx11m]

I'm adding "checkin-needed" back to the keywords. It's somewhat ambiguous which one to use these days, but I didn't see "push-needed" anywhere else.

Comment 20

[Ian Neal]

Comment on attachment 335455 [details] [diff] [review]
Final patch (v3) (Checkin: Comment 20)

http://hg.mozilla.org/comm-central/rev/78cce3227be8