1222176 - Implement user pref to map 'prefers-unknown' to 'prefers-plain|HTML|Both' for better auto-detection of delivery format in nsMsgCompose::DetermineHTMLAction

Status: RESOLVED WONTFIX

(MailNews Core :: Composition, enhancement)

Description

[Thomas D. (:thomas8)]

Attachment: Patch v.1: Implement triple pref for unknown=plain|html|both, auto-detect logic and send options UI. *** CAVEAT: To be applied ON TOP OF ATTACHMENT 8683673! ***

+++ This bug was initially created as a clone of Bug #1215791 +++
+++ This bug was initially created as a clone of Bug #1210244 +++

Prefers=Unknown case should be disambiguated in nsMsgCompose::DetermineHTMLAction, to give user more explicit control over recipient-centric auto-detection of delivery format.

This bug addresses the same problem like bug 1215791.
Proposed solutions differ, and WADA does not want to mix different proposals, hence separate bugs.

Bug 1215791 proposes: prefers-unknown = prefers-plain|HTML      (dual pref)
This bug proposes:    prefers-unknown = prefers-plain|HTML|Both (triple pref)

Both solutions require a single dropdown-box in Send Options dialogue, with 2 or 3 menuitems respectively, nicely tucked away in the menulist.

Comment 1

[Thomas D. (:thomas8)]

Please note that attachment 8683863 [details] [diff] [review] has been coded purely by analysis of code, as I can't test this. Anything odd or erroneous, please let me know.

I'd think the recipient-centric send logic of attachment 8683863 [details] [diff] [review] should work as intended, but some further optimizations of the code may be possible/desirable.

Keep in mind that by inherent design, send option set by user for cases of "anyPlain" must supersede any other recipient scope except allHTML or allBoth, because prefers-plain might require message downgrading, or sending both. That's why the scope sequence is allHTML - anyPlain - anyBoth.

1. allHTML:  If everybody wants HTML, send HTML.
2. anyPlain: If anybody (or everybody) wants plain, use send options (so user decides whether to
             ask/downgrade/send-both. Send-HTML-only is nonsense).
3. anyBoth:  If not everybody wants HTML (but maybe some), nobody wants plain, but anybody (or
             everybody) wants Both, send Both.

So user finally has a simple way of defining prefers-Both as quasi-default send format for prefers-unknown, without prejudicing any other recipient-centric logic as might be required for prefers-plain.

Comment 2

[Thomas D. (:thomas8)]

For feedback/review of attachment 8683863 [details] [diff] [review], please note:

That big red chunk which has the tabbox for Plain Text Domains and HTML domains has NOT been removed nor altered in any way, only the indenting changed, because I according to ux-natural-hierarchy, it's part of the settings for Auto-Detect, so it should be inside the big groupbox.

The only thing I changed on those tabs was the description, &domaindesc.label;

Comment 3

[Thomas D. (:thomas8)]

Important CAVEAT for attachment 8683863 [details] [diff] [review]
******************************************

attachment 8683863 [details] [diff] [review] must be applied ON TOP OF ATTACHMENT 8683673 [details] [diff] from Bug 136502 Comment 137, which implements the switch for auto-downgrade (fully message-centric) and redirects the allPlain case to send options to avoid silent, forced dataloss of full formatting.

So the full logic becomes:

(Optional) Auto-Downgrade-if-convertible::plain - allHTML - anyPlain - anyBoth

AnyPlain scope can ursurp/trump combined scopes like somePlain and someBoth, otherwise all scopes are mutually exclusive.

Comment 4

[Thomas D. (:thomas8)]

Mental note for code optimization:

1) allPlain scope is no longer needed, could be eliminated as it's now covered by anyPlain scope
2) for iterating the delivery format preference of each recipient, we can exit as soon as anyPlain==true. When there's one prefers-plain recipient, anyPlain scope applies (use send options for anyPlain, which is the traditional mail.default_html_action pref). But we really have to iterate all recipients until we find at least one prefers-plain (including prefers-unknown-mapped-to-prefers-plain).
3) as long as we don't find at least one prefers-plain, we also have to iterate all recipients in search of unkown-prefers-both (because if there's no prefers-plain, a single unknown-prefers-both recipient will trigger send-both).
4) But already before iteration, we know exactly what unknown prefers, from new user pref:
mailnews.sendformat.unknown_prefers = 1 (plaintext) | 2 (html) | 3 (both)
So I'm wondering if we can use that prior knowledge to further short-circuit the iteration if we come across prefers-unknown. Definitely for unknown==plain. Probbly not for unknown==html or unknown==both?

Comment 5

[Thomas D. (:thomas8)]

I hasten to add that this will actually enable recipient-centric users to enjoy the best of both worlds:

- Default to sending HTML or Both for prefers-unknown via an explicit pref
- Set ASK ME for the rare cases where recipients actually want/need plaintext, and enable controlled downgrading. If user sets unknown_prefers to anything other than plaintext, we'll no longer ask for all the unknown recipients as these are mapped to HTML or both (by user's choice).

This eliminates the current anachronism that we'll ask whenever "recipient is NOT (explicitly) listed as being able to receive HTML", which makes sense only under the wrong/outdated assumption that "unknown==prefers-plain".
Asking is only needed for downgrading, and downgrading is only needed for prefers-plain.
So we finally enable user to escape asking when he considers unknown==prefers-html; but to keep asking for the actual prefers-plain cases where downgrading might be wanted or required.

Comment 6

[Jorg K (CEST = GMT+2)]

I was asked to do a try build of this. So I compiled it locally first. You will want to fix this warning:
c:\mozilla-source\comm-central\mailnews\compose\src\nsmsgcompose.cpp(5075) :
warning C4715: 'nsMsgCompose::DetermineHTMLAction' : not all control paths return a value

I hope the logic is right, so here is your try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a6c91947e376
Once completed, builds and logs will be available at:
https://archive.mozilla.org/pub/thunderbird/try-builds/[e-mail]-a6c91947e376246f24a0adf94099b4dd6131581e/

This DOES include attachment 8683673 [details] [diff] [review], although you don't see it in the push. I has been pushed before, so here it doesn't show again, take my word for it ;-)

For the record, all you need to do is this, Level 1 access rights assumed:
hg qnew -m "try: -b o -p win32 -u none" try
hg push -f -r tip cc-try && hg qpop && hg qdelete try

The "cc-try" is an alias in the "hgrc" file in the .hg directory. Mine reads:
[paths]
default = https://hg.mozilla.org/comm-central
cc-try = ssh://[e-mail]@hg.mozilla.org/try-comm-central

Comment 7

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #6)
> I was asked to do a try build of this. So I compiled it locally first. You
> will want to fix this warning:
> c:\mozilla-source\comm-central\mailnews\compose\src\nsmsgcompose.cpp(5075) :
> warning C4715: 'nsMsgCompose::DetermineHTMLAction' : not all control paths
> return a value

Hmmm. Probably a requirement of c++ with which I am not familiar, maybe this:

> [Except 'main' function which assumes return=0 if no explicit return statement is found],
> all other functions with a return type shall end with a proper return statement that includes a
> return value, even if this is never used.

Oh. Is it because I have placed all "return..." statements inside if blocks?
The if blocks cover all possible cases, so we'll never get back to the function thread... yeah, so in fact, the last if block, if (anyBoth)..., probably does not have to be an if block, but could just be an implicit else, with a return statement on the main function control path.

Comment 8

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #7)
> Oh. Is it because I have placed all "return..." statements inside if blocks?
Yes.

Without having looked at it in detail:
It is preferable to structure the logic with if ... else if ... else.
It would also be great to only have one return, which is easier if you want to debug this, because you only need to set one breakpoint.
So:
if (xx) {
  *returnvalue = zz;
} else if (yy) {
  *returnvalue = tt;
} else {
  *returnvalue = kk;
}
return NS_OK.

Comment 9

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #8)
> (In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment
> #7)
> > Oh. Is it because I have placed all "return..." statements inside if blocks?
> Yes.
> 
> Without having looked at it in detail:
> It is preferable to structure the logic with if ... else if ... else.
> It would also be great to only have one return, which is easier if you want
> to debug this, because you only need to set one breakpoint.
> So:
> if (xx) {
>   *returnvalue = zz;
> } else if (yy) {
>   *returnvalue = tt;
> } else {
>   *returnvalue = kk;
> }
> return NS_OK.

I remember that Magnus and Aceman told me exactly otherwise in another bug (which I can't find now):
Iirc, they said I should use:

if (xx) {
  *returnvalue = zz;
  return NS_OK
}
if (yy) {
  *returnvalue = tt;
  return NS_OK
}
// else
*returnvalue = kk;
return NS_OK

Do not use else, use only 'if blocks', and let each if block have it's own return statement, so that reading the code becomes easier because we can close that chapter in our mind, as opposed to reading on along all the if/else blocks to see if there's still more general code before return statement. Magnus, can you comment?

Or maybe the following would be better readable in this case (using else if, but keeping return statements inside if blocks)?

if (xx) {
  *returnvalue = zz;
  return NS_OK
}
else if (yy) {
  *returnvalue = tt;
  return NS_OK
}
// else
*returnvalue = kk;
return NS_OK

Comment 10

[Jorg K (CEST = GMT+2)]

Yes, writing good readable code is an art. If the whole logic, like in this case, fits on one screen, I'd avoid multiple returns, since, as I said, they are a pain to debug. More compact code, less lines, is also easier to read. In the end it comes down to personal taste and experience. But your approach gave you a path with out returning a value, which wouldn't have happened to me ;-)

Comment 11

[Thomas D. (:thomas8)]

Attachment: Patch v.2 [buggy](streamlined): Implement triple pref for unknown=plain|html|both, auto-detect logic and send options UI. *** CAVEAT: To be applied ON TOP OF ATTACHMENT 8683673! ***

In this patch, I've streamlined the code logic per comment 4, and removed scope variables which are no longer needed. I also tried to fix the error described in Jorg's comment 6, and added one "}" which was missing somewhere.

Final Auto-Detect logic of this patch, higher scopes trump lower scopes:

Message-centric:
1) (Optional) Auto-Downgrade-if-convertible::plain: Send plain text (regardless of any recipient preferences)

Recipient-centric:
2) allHTML:  Send HTML
3) anyPlain: Use Send Options ("If any recipient prefers plaintext...")
4) anyBoth (now implicit): Send both

(In reply to Jorg K (GMT+1) from comment #6)
> I was asked to do a try build of this. So I compiled it locally first. You
> will want to fix this warning:
> c:\mozilla-source\comm-central\mailnews\compose\src\nsmsgcompose.cpp(5075) :
> warning C4715: 'nsMsgCompose::DetermineHTMLAction' : not all control paths
> return a value

This should be fixed now. I think. I hope.

Comment 12

[Thomas D. (:thomas8)]

Jorg, can you pls try local compilation and report back if the warning is gone.
If yes, can you pls push another try build, on top of patch attachment 8683673 [details] [diff] [review] in bug 136502. Tia.

Comment 13

[Jorg K (CEST = GMT+2)]

Compiles without warning now.

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=49149a684ab4
Results:
https://archive.mozilla.org/pub/thunderbird/try-builds/[e-mail]-49149a684ab49e2ae1939b7600c1dc0d21268259/

Comment 14

[Jorg K (CEST = GMT+2)]

Sorry, here:
https://archive.mozilla.org/pub/thunderbird/try-builds/[e-mail]-49149a684ab4/

Comment 15

[Thomas D. (:thomas8)]

Comment on attachment 8688162 [details] [diff] [review]
Patch v.2 [buggy](streamlined): Implement triple pref for unknown=plain|html|both, auto-detect logic and send options UI. *** CAVEAT: To be applied ON TOP OF ATTACHMENT 8683673 [details] [diff]! ***

Sorry, there are technical problems with this patch (missing entity, missing pref entry), which unfortunately make the whole thing fail.
Any tools to avoid such obvious errors, except building?
Jorg, thanks for the try build!

Comment 16

[Thomas D. (:thomas8)]

Attachment: Patch v.3 (entity & pref fixed): Implement triple pref for unknown=plain|html|both, auto-detect logic and send options UI. *** CAVEAT: To be applied ON TOP OF ATTACHMENT 8683673! ***

Fix Comment 15 (missing entity, missing pref).
So hopefully patch is now operational without errors.

Magnus, Aceman, for feedback/review, please refer to Comment 11 which has a concise description of what this patch does.

Jorg, could you pls push another try build (I don't have access), on top of patch attachment 8683673 [details] [diff] [review] in bug 136502. Tia.

Comment 17

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d2556d77dd2b
Once completed, builds and logs will be available at:
https://archive.mozilla.org/pub/thunderbird/try-builds/[e-mail]-d2556d77dd2be7c315c53bf3760faa31a06f6298/

Comment 18

[:aceman]

Comment on attachment 8688708 [details] [diff] [review]
Patch v.3 (entity & pref fixed): Implement triple pref for unknown=plain|html|both, auto-detect logic and send options UI. *** CAVEAT: To be applied ON TOP OF ATTACHMENT 8683673 [details] [diff]! ***

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

This looks great to me! Yes, this is what I had in mind to implement when you and WADA talked about this pref.
So the patch seems to contain everything needed. It does compile fine.
I have not yet tested individual scenarios as there is the logic error with the domains. I try to test it when you fix that.
This feature could use some automated tests, but I am not yet sure how to implement them (this is a bit different than bug 584313).

::: mail/locales/en-US/chrome/messenger/preferences/sendoptions.dtd
@@ +32,5 @@
>  <!ENTITY HTMLTab.accesskey            "H">
>  <!ENTITY PlainTextTab.label           "Plain Text Domains">
>  <!ENTITY PlainTextTab.accesskey       "P">
> +<!ENTITY HTMLDomainsDescr.label       "Recipients of the following domains prefer to receive messages formatted as HTML, unless specified otherwise in the recipients' contact properties.">
> +<!ENTITY PlainTextDomainsDescr.label  "Recipients of the following domains prefer to receive messages formatted as plain text, unless specified otherwise in the recipients' contact properties.">

Nice touch.

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +4910,1 @@
>    nsCOMPtr<nsIPrefBranch> prefBranch(do_GetService(NS_PREFSERVICE_CONTRACTID));

The contents of prefService and prefBranch seem to be identical. Are they changed differently later on? I think you can use prefBranch also for your new prefs.

@@ -4908,2 @@
>    bool allHtml = true;
> -  bool allPlain = true;

So you remove the allPlain case? If all recipients were plain, we silently downgraded the message (even if not convertible). Is this case being removed? So you consult Send options also in this case? Is that what you think Magnus wants? Or will it be made toggleable in some bug?

@@ +4932,5 @@
> +
> +  // The initial value of unknownPrefers will be overwritten by pref value; default: 2 (HTML).
> +  int unknownPrefers = 2;
> +  rv = prefService->GetIntPref("mailnews.sendformat.unknown_prefers", &unknownPrefers);
> +  NS_ENSURE_SUCCESS(rv, rv);

What if the user puts some random value into the pref? You need to sanitize it here.

@@ +4959,5 @@
> +        {
> +          int32_t atPos = recipient.mEmail.FindChar('@');
> +          if (atPos < 0)
> +            continue;
> +      

Added whitespace.

@@ +4967,5 @@
> +            preferFormat = nsIAbPreferMailFormat::plaintext;
> +          else if (IsInDomainList(emailDomain, htmlDomains))
> +            preferFormat = nsIAbPreferMailFormat::html;
> +        }
> +        else

The branching (if..else) seems incorrect here. If the recipient is in neither domainList, you want to use the unknownPrefers value. Not skipping using unknownPrefers if there is anything in plaintextDomains or htmlDomains.

::: mailnews/mailnews.js
@@ +173,5 @@
>  //        auto-detection of delivery format; auto-downgrade and silently send as plain text.
>  // false: Don't auto-downgrade; use recipient-centric auto-detection of best delivery format,
>  //        including send options.
> +pref("mailnews.sendformat.auto_downgrade", true);
> +pref("mailnews.sendformat.unknown_prefers", 2);        // 1=plain, 2=html, 3=both

I vote for 3 being the shipped default.

Comment 19

[:aceman]

Thomas, kudos to you for stepping in here and implementing a proof of concept for your proposal. You seem to have great code copying skills :)

Comment 20

[Jorg K (CEST = GMT+2)]

Now all he needs to do is compile his own stuff, so he doesn't need a slave to do it ;-)

Comment 21

[:aceman]

(In reply to Jorg K (GMT+1) from comment #20)
> Now all he needs to do is compile his own stuff, so he doesn't need a slave
> to do it ;-)

Well, that would be the best final result, but not always reachable or required.
I was also using others to push to try server for 2 years or so (for legal reasons) :) Everybody has his own priorities/skills and also building requires some HW resources. Thomas has his own worthy skills he may decide to pursue instead of fighting with building ;)

Comment 22

[Thomas D. (:thomas8)]

Attachment: Patch v.4 (fix domain logic error): Implement triple pref for unknown=plain|html|both, auto-detect logic and send options UI. *** CAVEAT: To be applied ON TOP OF ATTACHMENT 8683673! ***

All right! This is taking shape. :)

This patch addresses all issues of Aceman's review in comment 18, and as a bonus, improves the behaviour for newsgroups.

a) fix logic error with plaintext/html-domains (thanks for finding that!)
b) use prefbranch only (I rely on Aceman's expertise here)
c) Guard/sanitize unknown_prefers pref against out-of-range user values
d) default unknown_prefers pref to 3 (Both plaintext and HTML).
e) whitespace

Improved behaviour for Newsgroups:
f) For auto-Downgrade==true (user pref/option from bug 136502), TB will no longer nag users with "HTML mail question" for cases of isConvertible::Plain. Iow, newsgroup posts just using plaintext formatting like *pseudo-bold* and /pseudo-italic/ will conveniently be sent silently as plaintext by default.
(This isn't a full fix for newsgroups, see xref bug 396395, but improves things for all traditional newsgroups).

Aceman had another question:
> So you remove the allPlain case?
g) allPlain scope: Not removed, but I have conflated allPlain and somePlain scopes into anyPlain scope, which is handled by send option ("If any recipient prefers to receive messages as plaintext: ..."). Note that prefers-unknown scope has been removed from that send option (unless user maps it back by setting unknown-prefers=prefers-plain), so anyPlain scope is now exclusively for all recipient scopes involving at least one prefers-plain recipient, and defaults to 3 ("Send both plain text and HTML"). So this avoids the forced default dataloss criticized by Magnus. Users who want/need to accept the dataloss of formatting when force-downgrading even consequential HTML for their prefers-plaintext recipients can set the pref to 'Ask me' or 'silently send plaintext-only'. I guess that such users will typically not care if any prefers-unknown or prefers-HTML recipient will receive the same downgraded message, so bundling allPlain and somePlain into anyPlain Send option should be fine. This also keeps the options UI slim (avoid separate option for allPlain, as seen in attachment 8673807 [details]), and the behaviour transparent and controllable (as opposed to creating hidden, uncontrollable behaviour again). As a more radical interpretation of Magnus input, we could discuss in another bug if we EVER want to allow users to opt into *silent* dataloss for cases of isConvertible::No.

Aceman, why do you want unknown-prefers default of 3 (prefers-both: Plaintext and HTML)? This inflates the message size for all prefers-unknown/undefined recipients by sending both formats. Sending HTML-only by default is not sufficient? We don't trust other mail readers to downgrade our HTML into plaintext if needed?

Comment 23

[Thomas D. (:thomas8)]

I had some problems with branching in TortoiseHg, but I hope the patch still applies cleanly.

Note to self: Options UI: need bigger separator between AnyPlain send option dropdown and Plaintext/HTML-domain tabs.

Comment 24

[Thomas D. (:thomas8)]

FYI, this is the priority sequence of recipient-centric Auto-Detect:

Recipient-centric delivery format preference:
1 per-contact [text|html] (for anyone) >
2 per-domain [text|html] (only for prefers-unknown contacts) >
3 global-unknown-prefers [text|html|both] (only for prefers-unknown contacts not in domain lists)

So explicit per-contact trumps per-domain, and explicit per-domain trumps global unknown-prefers.

Comment 25

[Thomas D. (:thomas8)]

Comment on attachment 8689411 [details] [diff] [review]
Patch v.4 (fix domain logic error): Implement triple pref for unknown=plain|html|both, auto-detect logic and send options UI. *** CAVEAT: To be applied ON TOP OF ATTACHMENT 8683673 [details] [diff]! ***

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

Grrrr. Another nit in this patch which breaks the whole logic.

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +5026,5 @@
> +  // the actual send format (aka delivery format) after analysing recipients'
> +  // format preferences above.
> +
> +  // If all recipients prefer HTML, then return HTML.
> +  else if (allHtml)

'Else' is wrong here because I moved up the 'if' block (auto-downgrade).
So this must be:

if (allHtml)

Comment 26

[Thomas D. (:thomas8)]

Attachment: Patch v.5 (trimmed to suit bug 136502): Implement triple pref for unknown=plain|html|both, auto-detect logic and send options UI. *** CAVEAT: To be applied ON TOP OF ATTACHMENT 8690421! ***

Give bug 136502 what belongs to bug 136502... which conveniently makes this patch slimmer.

This patch must be applied on top of patch *** attachment 8690421 [details] [diff] [review] *** from bug 136502.

Patch concept has feedback+ from Aceman's comment 18.

(In reply to :aceman from comment #18)
> Comment on attachment 8688708 [details] [diff] [review]
> Review of attachment 8688708 [details] [diff] [review]:
> -----------------------------------------------------------------
> This looks great to me! Yes, this is what I had in mind to implement when
> you and WADA talked about this pref.

Aceman, pls review if I addressed issues of your comment 18 correctly.

Magnus, here's a TL;DR why something like this is needed:

* Current pref "mail.default_html_action" aka 'Send option" covers loads of recipient format preference scopes with opposing needs:
- somePlain, someHTML, someUnknown, allUnknown.
Does it really make sense to have the same send format behaviour for all of these and combinations thereof?

* More specifically, the current (legacy) option "When sending messages in HTML format and one or more recipients are not listed as being able to receive HTML..." combines *two opposite purposes which annihilate each other* :

a) *Conflict resolution for recipient scope (somePlain)*:
When some, but not all recipients prefer plain text, but the message has consequential HTML - what should be done? Satisfy recipients or preserve the message as composed?
For recipient scope of (allPlain), we're even forcing the user into dataloss resolution of the conflict.

vs.

b) *Default send format for recipients of type prefers-unknown*:
By default, what format should be sent to recipients not yet in AB, or whose format preference is unknown?

As a result, *both Prefers-Plain-Conflict-resolution and Prefers-Unknown-Default-send-format are crippled* :
- Conflict resolution is crippled: User can't set ASK or force-downgrade for somePlain, because it will ask or force-downgrade for unknown, too, but asking or dataloss-downgrading can never be reasonable default for unknown recipients.
- Default send format is crippled: User can't set HTML-only for unknown, because it would send HTML-only for prefers-plain recipients, too, which is nonsense.

That's why as a matter of fact, the only reasonable setting is "Send both", which is why we've made that the default. The current option is pretty much useless for all purposes (apart from lying to the user that it would also cover the allPlain recipient scope).

In detail:

Why the current (legacy) conglomerate send option is useless because of combining opposing functions:

ad a) *Conflict resolution for recipient scope (somePlain)*:

0) "Ask me what to do" (user-controlled conflict resolution)
1) "Convert the message to plaintext" (conflict 'resolution' by silent, forced downgrade with massive uncontrolled dataloss potential - why do we even offer that?)
3) "Send the message in both plain text and HTML" (Silent compromise: conflict resolution by sending both formats)

Note that the remaining option, 2) "Send the message in HTML anyway" can never be used as a default for conflict resolution, because it would make prefers-plain a NOP.

ad b) *Defining default send format for recipients of type prefers-unknown*:

2) "Send the message in HTML anyway" - That looks like a valid default send format in the year 2015, avoiding unnecessary message size inflation by multipart/alternative. Recipient mail readers are free to downgrade or morph if the target device requires that.
3) "Send the message in both plain text and HTML" - Valid compromise format because well, it's unknown what the recipient prefers or needs, also depending on devices used for reading, so we proactively send both formats.

Note that the other two options can never be default send formats:
0) "Ask me what to do" - extremely annoying if you get that for every mail
1) "Convert the message to plaintext" - silent, forced dataloss certainly cannot be the default for sending HTML-messages (use plain text editor instead).

Comment 27

[Thomas D. (:thomas8)]

So in a nutshell, patch of attachment 8690429 [details] [diff] [review] separates the two functions of current send option:

a) Default format for prefers-unknown (default:prefers-both):
unknown-prefers-pref "Assume that recipients whose preference is unknown prefer to recieve messages formatted as..." -> plain | html | both

b) Conflict resolution for (anyPlain) scope (default:prefers-both):
Send option "If any recipient prefers plain text..." -> ask | plain | html | both
Where ask==controlled dataloss downgrading, and plain==uncontrolled dataloss downgrading.

User can happily set both options independently according to his needs and preferences, and their interaction is clear and predictable.
For purposes of delivery format Auto-Detect, we can't define a *guaranteed* send format for "prefers-unknown" because if there's a prefers-plain recipient, downgrading might be required or desired, which will override the default send format. Hence senformat.unknown-prefers and not sendformat.unknown.default.
In fact, with my patch we should consider renaming mail.default_html_action to:
mailnews.sendformat.anyPlain_default_action

Guaranteed, message-centric default send format means defaulting to a fixed delivery format other than Auto-Detect, which is prevalently recipient-centric, albeit coupled with message-centric Auto-Downgrade.
Implementing per-identity 'Default message delivery format', i.e. allowing user to opt out of 'Auto-Detect' black box algorithm entirely, is the other half of bug 136502 (needs a new bug).

Comment 28

[Thomas D. (:thomas8)]

Attachment: Patch v.6 (updated to suit bug 136502): Implement triple pref for unknown=plain|html|both, auto-detect logic and send options UI. *** CAVEAT: To be applied ON TOP OF ATTACHMENT attachment 8690447! ***

Updated to make compatible with new patch attachment 8690447 [details] [diff] [review] in bug 136502.
This patch must be applied on top of that.

Description: see Comment 26.

Comment 29

[Thomas D. (:thomas8)]

Attachment: Patch v.6.1 (UTF-8): Implement triple pref for unknown=plain|html|both, auto-detect logic and send options UI. *** To be applied on top of attachment 8690447! ***

Same as patch v. 6 attachment 8690450 [details] [diff] [review], but in UTF-8 for the convenience of reviewers.

To be applied on top of patch v. 6 from bug 136502, attachment 8690447 [details] [diff] [review].

Description: see Comment 11 / Comment 26.

Comment 30

[Thomas D. (:thomas8)]

Attachment: Patch v.6.2 (fix pref sanitization): Implement triple pref for unknown=plain|html|both, auto-detect logic and send options UI. *** To be applied on top of attachment 8690447 ***

Fix incomplete pref sanitization.

And hooray, a new label for the pref, short and crisp, easier and more to the point (the user setting a de-facto default delivery format for recipients of type prefers-unknown):

Preferred delivery format for recipients whose preference is unknown:
Plain Text | HTML | *Both Plain Text and HTML*

This patch to be applied on top of patch v. 6 from bug 136502, attachment 8690447 [details] [diff] [review].

Description: see Comment 11 / Comment 26.

Comment 31

[Thomas D. (:thomas8)]

Attachment: Screenshot 1 (Patch v.6.2): Send Options with pref for unknown_prefers

Comment 32

[Thomas D. (:thomas8)]

Jorg, could you please try one more trybuild? Thank you!

New Patch from bug bug 136502: attachment 8690447 [details] [diff] [review]
On top of that, patch from this bug: attachment 8690529 [details] [diff] [review]

Comment 33

[Jorg K (CEST = GMT+2)]

Doesn't compile, two errors:

rv = prefBranch->ResetBranch("mailnews.sendformat.unknown_prefers"); - upper case R!
if (unknownPrefers < 1 || 3 < unknownPrefers) - was If with upper case I, haha. Must be lower case.

This looks ugly, please turn it around:
if (unknownPrefers < 1 || unknownPrefers > 3)

If you want to check inside limits you can go:
if (lower_limit < thing && thing < upper_limit).

Once completed, builds and logs will be available at:
https://archive.mozilla.org/pub/thunderbird/try-builds/[e-mail]-ca6e7882afcafd3301b72e80aaf76686395166bc/

Comment 34

[Richard Marti (:Paenglab)]

Comment on attachment 8690530 [details]
Screenshot 1 (Patch v.6.2): Send Options with pref for unknown_prefers

All in all this looks the right way, but:

I'm not so happy with the 'delivery'. You are mixing it with 'message' in this dialog. I prefer to use always 'message' as this is the object we are working on and not on a delivery.

You are using 'Plain Text' with capitals in your menulist. The old menulist is all lower case. This should be synchronised.

The sentences you are using aren't clear when my server setting is set to plain text. With your sentence 'Preferred delivery format for recipients whose preference is unknown:' and the pref is set to both, does it mean my plain text is converted to both formats? The old sentence is clearer in this.

Maybe the dialog needs a rename to something like 'HTML Send Options...' because all this options apply only when sending a HTML message. Then your new sentences are correct again.

f- because it needs still some tuning.

Comment 35

[Thomas D. (:thomas8)]

(In reply to Richard Marti (:Paenglab) from comment #34)
> Comment on attachment 8690530 [details]
> Screenshot 1 (Patch v.6.2): Send Options with pref for unknown_prefers
> 
> All in all this looks the right way, but:

Thanks Richard for rapid feedback :)
Looking generally right is very good, thanks! :)

> I'm not so happy with the 'delivery'. You are mixing it with 'message' in
> this dialog. I prefer to use always 'message' as this is the object we are
> working on and not on a delivery.

Hmmm, tricky... I've deliberately used "delivery format" to be ux-consistent with "Options > Delivery Format", which seems to be the official term explicitly introduced for the primary menu UI by Bug 339887.
Just "Message format" might be misunderstood as "message format for composition", which can unfortunately differ from "message delivery format" when Auto-Detect (plus Auto-Downgrade) is used... Which is the whole crux of the matter as documented in delivery-format-ux tracker bug 889315...

> Maybe the dialog needs a rename to something like 'HTML Send Options...'
> because all this options apply only when sending a HTML message. Then your
> new sentences are correct again.

You are right. In fact, everything in "Send Options" dialogue only applies to:
- Messages composed in HTML
- AND with *Delivery Format: Auto-Detect*.
For any other explicit delivery format from Options > Delivery format (plain | HTML | both), 'Send Options' have no effect at all. So we should make it easy for the user to assocciate the Send Options dialogue with the menu.

That is why in [Send Options] dialogue, I chose "Delivery Format Auto-Detect" as the main group box (frame) caption, so we mirror the menu sequence exactly:

Menu:    Options > Delivery Format > Auto-Detect
Options: [Send Options] > Delivery Format Auto-Detect

In addition, as you say, we still need to add the scope "for messages composed in HTML format" somewhere upfront, which is the *first* condition sine qua non, so we must let the user know *before* going into "Send options" dialogue.

So how's this?   (ni?paenglab: feedback please...)

*** Options > Composition > General ***

<snip>

+--- HTML ---
| Font...
| Text Color...
+------------

For messages composed in HTML format², configure how they will be sent:    [Send Options...]

*** Send Options dialogue: ***

[ Send Options                               ]
| +--- Delivery Format Auto-Detect ---
| |
| | [x] Send messages as plain text if possible
| |
| | Preferred message format for recipients whose preference is unknown:
| |     [Both plain text and HTML    v]
| | Note: Use the address book to specify the preferred message format for individual recipients.
| | 
| | If any recipient prefers to receive messages formatted as plain text:
| |     [Send the message in both plain text and HTML]
| | 
| | [HTML Domains][Text Domains]
| | <snip>
| |
| +--------------
+------------------


> You are using 'Plain Text' with capitals in your menulist. The old menulist
> is all lower case. This should be synchronised.

Yes, I already saw that when making the screenshot. Lower case feels more natural here.

> f- because it needs still some tuning.

Thanks for the tuning tips :)

²: In account options, we have "[x] Compose messages in HTML format", so I've tried to pick up that wording here: "For messages composed in HTML format, configure how they will be sent:"
Which also clarifies that beyond that account checkbox, there's more related options which influence the actual message format which will be sent (aka 'delivery format').

Comment 36

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #33)
> Doesn't compile, two errors:
> 
> rv = prefBranch->ResetBranch("mailnews.sendformat.unknown_prefers"); - upper
> case R!

That's correctly spelled "resetBranch" (lower case 'r') in my patch v. 6.2 (attachment 8690529 [details] [diff] [review]), not sure what you are looking at?
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIPrefBranch#resetBranch%28%29

> if (unknownPrefers < 1 || 3 < unknownPrefers) - was If with upper case I,
> haha. Must be lower case.

Thanks. Will fix that with next iteration. Hope I'll remember...
 
> This looks ugly, please turn it around:
> if (unknownPrefers < 1 || unknownPrefers > 3)
> If you want to check inside limits you can go:
> if (lower_limit < thing && thing < upper_limit).

My patch has (after correcting the If to if):
> if (unknownPrefers < 1 || 3 < unknownPrefers)

Which visually reflects the condition exactly:
unknownprefers is *outside* the range of (1 to 3).
I don't see anything wrong with that.

> Once completed, builds and logs will be available at:
> https://archive.mozilla.org/pub/thunderbird/try-builds/[e-mail]-
> ca6e7882afcafd3301b72e80aaf76686395166bc/

Thanks!

Comment 37

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #33)
> Doesn't compile, two errors:
> rv = prefBranch->ResetBranch("mailnews.sendformat.unknown_prefers"); - upper
> case R!

Well, upper case or lower case, either one won't work because it's not implemented: Bug 720419.
http://mxr.mozilla.org/mozilla-central/source/modules/libpref/nsPrefBranch.cpp#518
> 518 NS_IMETHODIMP nsPrefBranch::ResetBranch(const char *aStartingAt)
> 519 {
> 520   return NS_ERROR_NOT_IMPLEMENTED;
> 521 }

So I think we'll just have to use this one:

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIPrefBranch#clearUserPref%28%29

Comment 38

[Thomas D. (:thomas8)]

Attachment: Patch v.7 (strings, pref sanitization): Implement triple pref for unknown=plain|html|both, auto-detect logic and send options UI. *** To be applied on top of attachment 8690447 ***

Fix pref sanitization.
New strings as requested by Richard's  Comment 34; described in Comment 35.

This patch to be applied on top of patch v. 6 from bug 136502, attachment 8690447 [details] [diff] [review]. Description: see Comment 11 / Comment 22 g) / Comment 26.

Jorg, does this work for compilation?

Documentation says lower-case initials for functions, but from within our c++ source files, looks like we're calling them with upper case. Maybe because the original functions in c++ are implemented upper-case?

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIPrefBranch#clearUserPref%28%29
> clearUserPref()
> Called to clear a user set value from a specific preference. This will, in
> effect, reset the value to the default value. If no default value exists the
> preference will cease to exist.

http://mxr.mozilla.org/mozilla-central/source/modules/libpref/nsPrefBranch.cpp#475
> 475 NS_IMETHODIMP nsPrefBranch::ClearUserPref(const char *aPrefName)

Comment 39

[Thomas D. (:thomas8)]

Attachment: Screenshot 2 (patch v.7): Options > Composition

New Send Options description to define the scope:

For messages composed in HTML format, configure how they will be sent:

Comment 40

[Thomas D. (:thomas8)]

Attachment: Screenshot 3 (patch v.7): Send Options with pref for unknown-prefers (using 'message format')

Comment 41

[Jorg K (CEST = GMT+2)]

Comment on attachment 8690674 [details] [diff] [review]
Patch v.7 (strings, pref sanitization): Implement triple pref for unknown=plain|html|both, auto-detect logic and send options UI. *** To be applied on top of attachment 8690447 [details] [diff] [review] ***

Methods defined in IDL are lower case in JS and uppercase in C++. Further reading:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Interface_development_guide/IDL_interface_rules

Can you please remove the trailing colon in:
For messages composed in HTML format, configure how they will be sent: <----

I'm confused, what other feedback do I need to give? Do you want another compile? For general questions, please use NI, that's easier to clear.

Comment 42

[Jorg K (CEST = GMT+2)]

I was going to keep out of this discussion, but I came to wonder which "auto-downgrade" related bugs we are going to land for TB 45.

Bug 414299 - (paraphrased): Better detection when not to downgrade - stalled.
Bug 1202165 - Save delivery format with draft - stalled.
Bug 136502 - I asked for a screenshot to see what this is about.
Bug 1222176 - this bug here.

About this bug here I read in bug 136502 comment #145 written by Magnus (quote):
  I'm not all the enthusiastic about bug 1222176 [this bug here].

Finally I am able to look at some screenshots, attachment 8690677 [details]. Perhaps I cannot judge it fully without seeing the change from bug 136502. Once this arrives, I will comment further.

So far I'd just like to say that it seems confusing. Can we not make this MUCH simpler?

You only need:
  Send the message as plain text if possible.

Question:
Is it safe to assume that 'possible' depends on some analysis of the HTML to determine that the loss would not be too big? Note: It is always possible to downgrade, there are interfaces in the box (the serialisers) that will turn any HTML (lossfully) into text.

The problem is this:
You have a message and a group of recipients.
Each recipient has a preference or has none. Note that preferences can also arise from the domain.

Now this is what should happen:

Case 1: The sender explicitly sets a/the delivery format(s). Then this/these format(s) is/are sent.

In the following we treat the case where the system has to decide what to do with HTML:

Case 2: Sender writes a HTML message and does NOT ask for conversion to plain text.
Case 2a: The message cannot be downgraded.
  All recipients receive HTML.
Case 2b: The message can be downgraded.
  If no preferences from recipients whatsoever, send only HTML.
  If all recipients prefer HTML, send only HTML.
  If all recipients prefer plain, send only plain.
  Else: send both.

Case 3: Sender writes a HTML message and but prefers to send plain text if possible.
Case 3a: The message cannot be downgraded.
  All recipients receive HTML.
Case 3b: The message can be downgraded.
  All recipients receive plain (because the author said so overriding their preferences).

Tell me it's more difficult then that. I can explain this algorithm to anyone ;-)
Basically, the recipients get what they asked for, if possible and not overridden by the sender.

Comment 43

[Jorg K (CEST = GMT+2)]

Oops, just for completeness:
Bug 584313 - (paraphrased): Better detection when not to downgrade.

Comment 44

[:aceman]

(In reply to Jorg K (GMT+1) from comment #42)
> I was going to keep out of this discussion, but I came to wonder which
> "auto-downgrade" related bugs we are going to land for TB 45.
> 
> Bug 414299 - (paraphrased): Better detection when not to downgrade - stalled.
Waiting on bug 584313 and the least important of the lot.

> Bug 1202165 - Save delivery format with draft - stalled.
Just Needs refresh, promised these days. Otherwise mostly done. And actually does not affect the decision logic in this whole problem, just saves state when the user stashes away an unfinished draft.

> Bug 584313 - (paraphrased): Better detection when not to downgrade.
> Bug 136502 - I asked for a screenshot to see what this is about.
> Bug 1222176 - this bug here.
Actively being worked on.

So I think the situation is quite good and they could make TB45. They mostly just need reviews. If you want to help here, check compile errors in Thomas' patches, which you already do, so thanks for that :)

> You only need:
>   Send the message as plain text if possible.

Yes, but for this to work, many places in code need to work correctly. And that is why we have at least those 5 bugs now.

> Question:
> Is it safe to assume that 'possible' depends on some analysis of the HTML to
> determine that the loss would not be too big?
Yes, and that determination is buggy too (like 584313).

> Note: It is always possible to
> downgrade, there are interfaces in the box (the serialisers) that will turn
> any HTML (lossfully) into text.
I think yes.

> The problem is this:
> You have a message and a group of recipients.
> Each recipient has a preference or has none. Note that preferences can also
> arise from the domain.

> Tell me it's more difficult then that. I can explain this algorithm to
> anyone ;-)
> Basically, the recipients get what they asked for, if possible and not
> overridden by the sender.

The problems arise when there is a mix of preferences (like one wants plain and one wants HTML) so we must choose something (Both is not always the answer). The determination of this is the main problem.
We must choose 1 format to meet the requirements of all the recipients. We can't send out multiple copies of the same email with different format.

To prevent this thread to become another flood of comments reiterating everything already said, I think we (Thomas, WADA, aceman) mostly understand what to do. The bugs are filed and just need review. If you are interested in the whole story and all the fun, you could read the 200+ comments in bugs like bug 1210244 and bug 1204379 and bug 1215791 and the ones you listed.

Comment 45

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #44)
> The problems arise when there is a mix of preferences (like one wants plain
> and one wants HTML) so we must choose something (Both is not always the
> answer). The determination of this is the main problem.
My algorithm specifies that.

> We must choose 1 format to meet the requirements of all the recipients. We
> can't send out multiple copies of the same email with different format.
Understood.

> To prevent this thread to become another flood of comments reiterating
> everything already said, I think we (Thomas, WADA, aceman) mostly understand
> what to do.
Sorry, I'll do it like Thomas. I came late to the party, I gave this some fresh thought and I want to be heard. This is way to complicated and Magnus will veto it, as he said. I sat in front of the options for one hour and I couldn't work out what this will do. No simple user will understand this. (And frankly: I don't understand how Mr. UX can propose something so twisted and unclear.)

So please, you take the time to read my algorithm, one comment, and tell me what's wrong with it.

Comment 46

[:aceman]

OK, as you please :)

(In reply to Jorg K (GMT+1) from comment #42)
> Case 2: Sender writes a HTML message and does NOT ask for conversion to
> plain text.

How does the user NOT ask for conversion? In some cases (for some recipients) the conversion is hardcoded and can't be turned off. Bug 136502.

> Case 2a: The message cannot be downgraded.
How does TB determine what can be downgraded? Bug 584313, 414299. And even after those fixed, there will surely be some cases when TB guesses wrong. So that is why there is bug 136502 to turn this determination off (and assume HTML if composed in HTML mode).

>   All recipients receive HTML.
Why? We still have all the other options (both/plain/ask) if there is at least one plain text recipient.
That is when Send Options come in.

> Case 2b: The message can be downgraded.
>   If no preferences from recipients whatsoever, send only HTML.
Why? I'd want Both (multipart/alternative).
The bug here 1222176 adds the option to say what to send to "no preferences"-recipients.

>   If all recipients prefer HTML, send only HTML.
Yes.

>   If all recipients prefer plain, send only plain.
Yes.

>   Else: send both.
> 
> Case 3: Sender writes a HTML message and but prefers to send plain text if
> possible.
If possible? User will have this option to choose only after bug 136502.

> Case 3a: The message cannot be downgraded.
>   All recipients receive HTML.
Why? We still have all the other options (both/plain/ask) if there is at least one plain text recipient.
That is when Send Options come in.

> Case 3b: The message can be downgraded.
>   All recipients receive plain (because the author said so overriding their
> preferences).
OK.

(In reply to Jorg K (GMT+1) from comment #45)
> No simple user will understand this.
> (And frankly: I don't understand how Mr. UX can propose something so twisted
> and unclear.)

Nobody forces anybody to use the new options. They will have defaults that should be have the same as today. But if anybody really wants to tweak the output format, he can use the new options. Some users understand enough to have filed these bugs because their messages are incorrectly downgraded. After these bugs are done, we can at least tell them what to set to fix their problem. Currently there are not enough otpions and their formatting is unconditionally lost.

Comment 47

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #46)

> (In reply to Jorg K (GMT+1) from comment #42)
> > Case 2: Sender writes a HTML message and does NOT ask for conversion to
> > plain text.
> How does the user NOT ask for conversion? In some cases (for some
> recipients) the conversion is hardcoded and can't be turned off. Bug 136502.
I'll all for fixing bug 136502. I've just compiled it to see. This gives us this option:
  Send the message as plain text if possible.

> > Case 2a: The message cannot be downgraded.
> How does TB determine what can be downgraded? Bug 584313, 414299. And even
> after those fixed, there will surely be some cases when TB guesses wrong. So
> that is why there is bug 136502 to turn this determination off (and assume
> HTML if composed in HTML mode).
Do I need to repeat that I'm in favour of bug 136502 and the ones it depends on?

> >   All recipients receive HTML.
> Why? We still have all the other options (both/plain/ask) if there is at
> least one plain text recipient.
> That is when Send Options come in.
No. We already determined that the message *cannot* be downgraded in case 2a.
So 'both' is not an option and 'ask' is not an option, since the only option is to send the HTML since it cannot be downgraded.
Or do you really want to ask like this:
  The system determined that your HTML e-mail can not be converted to plain text
  without major loss of formatting. Do you really want to proceed?

> > Case 2b: The message can be downgraded.
> >   If no preferences from recipients whatsoever, send only HTML.
> Why? I'd want Both (multipart/alternative).
> The bug here 1222176 adds the option to say what to send to "no
> preferences"-recipients.
If I have no preferences, then I can send anything I want. Since the user typed HTML, I'll send HTML. If the sender cares about the preferences for certain recipients, he/she should set the preference in the address book. I don't need a switch to support laziness. However, I can live with this option. It's like a default for when you don't have anything specified in your address book. Fair enough.

> > Case 3: Sender writes a HTML message and but prefers to send plain text if
> > possible.
> If possible? User will have this option to choose only after bug 136502.
No. The system determines "possible" as I understand it. Somewhere in the code that reads:
(aConvertible == nsIMsgCompConvertible::Plain))
The user is thrifty and doesn't want to send "Hi Klaus, see you at 6 PM. Greg." as HTML. He trusts the auto-downgrade. Whether we wants to trust the system, he can determine himself with bug 136502. Do I need to repeat that I am in favour? ;-)

> > Case 3a: The message cannot be downgraded.
> >   All recipients receive HTML.
> Why? We still have all the other options (both/plain/ask) if there is at
> least one plain text recipient.
> That is when Send Options come in.
No. Same as case 2a.

> Nobody forces anybody to use the new options. They will have defaults that
> should be have the same as today. But if anybody really wants to tweak the
> output format, he can use the new options. Some users understand enough to
> have filed these bugs because their messages are incorrectly downgraded.
> After these bugs are done, we can at least tell them what to set to fix
> their problem. Currently there are not enough otpions and their formatting
> is unconditionally lost.

Let me repeat:
I am 500% in favour of bug 136502.
I am 1000% in favour of fixing anything to avoid that the system thinks it can auto-downgrade and loses formatting (bug 584313 and bug 414299).
I can live with the "let's have a default for all the lazy people", to specify a value for all the ones that don't have a preference in the address book. You don't show the full pulldown. Is one of the options to leave it "as is", so let them have what over comes and not take part in the decision process? Or what does "unspecified" default to today?

What really cracks me up is the third thing you're proposing:
  If any recipient prefers to receive messages formatted as plain text:

What case is this good for? How does that interact with "Send the message as plain text if possible."

And also, how do I maintain the exiting (hopefully improved) auto-downgrade?
Check "Send the message as plain text if possible." - and what about the other two?

When you post a screenshot, can you please post one that shows *all* the menu options.

Comment 48

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #42)
> I was going to keep out of this discussion, but I came to wonder which
> "auto-downgrade" related bugs we are going to land for TB 45.

Thank you for taking interest and encouraging us to actually land things :)

More precisely, you're talking about "Delivery-Format-Auto-*Detect*"-related bugs, where Auto-Detect (in UI) == message-centric Auto-Downgrade AND recipient-centric Auto-Detect of best delivery format, per current terminology of the initiated. So pls don't confuse us and others... It took long to disentangle the two... ;)

> Bug 414299 - (paraphrased): Better detection when not to downgrade - stalled.
> Bug 1202165 - Save delivery format with draft - stalled.
> Bug 136502 - I asked for a screenshot to see what this is about.
> Bug 1222176 - this bug here.
> Bug 584313 - (paraphrased): Better detection when not to downgrade. (from comment 43)

Yeah, those look the current focus of activity in this area. Delivery-format-ux tracker bug 889315 knows them all.

> Finally I am able to look at some screenshots, attachment 8690677 [details]
> [details]. Perhaps I cannot judge it fully without seeing the change from
> bug 136502. Once this arrives, I will comment further.
> 
> So far I'd just like to say that it seems confusing. Can we not make this
> MUCH simpler?

Most of what you suggest below requires exactly the same options which are already there, plus what I'm pushing in these bugs.
Iow, your suggestion would be only minimally simpler (to not do this bug), and only because it overlooks important points.

> You only need:
>   Send the message as plain text if possible.

That's an oversimplification which is not even consistent with your own proposal.

> Question:
> Is it safe to assume that 'possible' depends on some analysis of the HTML to
> determine that the loss would not be too big? Note: It is always possible to
> downgrade, there are interfaces in the box (the serialisers) that will turn
> any HTML (lossfully) into text.
> 
> The problem is this:
> You have a message and a group of recipients.
> Each recipient has a preference or has none. Note that preferences can also
> arise from the domain.

Gotcha. Two full option tabs with lists, buttons and all for domain settings.
And the code logic also needs to handle them. And we need to give clues to the user how to override the per-domain setting via per-contact setting. Just a little bit of UI complexity here and there.

> Now this is what should happen:
> 
> Case 1: The sender explicitly sets a/the delivery format(s). Then this/these
> format(s) is/are sent.

Problem 1: Opt-in silent Downgrade to plain text - imho still needs warning at least for isConvertible::no
Message is full-blown HTML with inline images, formatting and all.
User checks "Options > Delivery Format > Plain text" (maybe accidentally?), which is clearly nonsensical.
We dump everything and send plain text silently. Massive Dataloss, no warning.
But we'll warn if you add a single dot to your message and try to close it, and we don't allow you to opt out of that warning afasik (I might be wrong). - Some bugs filed.

Problem 2: Missing per-identity option for message-centric Default Delivery Format.
If user wants to send *all* his messages as HTML (without ever worrying about Auto-Detect Black box), that's a legitimate behaviour. Surprisingly, we don't offer that choice, and force recipient-centric Auto-Detect on everyone. Including message-centric Auto-Downgrade (bug 136502). But we don't tell you about that. Even after bug 136502, it will still be buried deep down in options (needs another bug to come to primary UI). Having to set format each time again is annyoing. The missing twin of bug 136502 (not yet filed).

> In the following we treat the case where the system has to decide what to do
> with HTML:
> 
> Case 2: Sender writes a HTML message and does NOT ask for conversion to
> plain text.

As Aceman said, the first time we ever give the user a choice is after my patch in bug 136502.
And it's still very hidden, far from transparent.

> Case 2a: The message cannot be downgraded.
>   All recipients receive HTML.

Why not "Both plain text and HTML"? Different users, different recipients, different needs.
And what if any or all recipients are prefers-plain? How do you resolve the conflict between message-centric isConvertible::No and recipient-centric:Downgrade-please !?
So... you need at least one option for resolving conflicts (anyPlain recipient scope).
That's the existing send option, only unfortunately you can't resolve any conflicts with it, because we've put prefers-unknown into the mix, for which you want to set HTML as default message delivery format. For details, please read user story at the top of this bug. Current send option conglomerate scope makes that option a NOP, which only works for all scenarios if you set it to "Both".

> Case 2b: The message can be downgraded.
>   If no preferences from recipients whatsoever, send only HTML.

That's all-prefers-unknown case (allUnknown scope). Why not "Both plain text and HTML"? Different users, different recipients, different needs. Uncle Ben might even want plaintext for prefers-unknown, as a matter of principle.

>   If all recipients prefer HTML, send only HTML.

Yes. (allHTML scope) That's the only thing we can be sure about, if message-centric auto-Downgrade==false (needs my patch of bug 136502).

>   If all recipients prefer plain, send only plain.

(allPlain scope)
Ouch! Massive silent dataloss potential of full HTML. Magnus has already registered his disagreement with that behaviour. This is worse than Problem 1 above. User has never agreed to downgrade this particular message. Maybe he forgot that 10 years ago, he set Uncle Ben and Aunt Simple to 'prefers-plain'.
I originally suggested to have separate option for allPlain case.
I withdrew that suggestion only for pragmatic reasons, to have one less send option dropdown.
We might get away with bundling allPlain and somePlain into a single option, but only if we remove (prefers-unknown scopes) from that option (this bug).
Essentially, whenever there's anyPlain recipient, there's a conflict what to do with the message if it has HTML, so we need a purified variant of the existing send option for conflict resolution.

>   Else: send both.

Now we're getting to the heart of this bug.
This is mixed case, right?
So e.g. prefers-html && prefers-unknown.

In 2b.1, you said allUnknown should get HTML-only. So it looks like you generally want prefers-unknown==prefers-HTML (Sorry, but current TB doesn't care about such extravagant wishes. Who do you think you are? You think as a /plain/ user (pun intended), you can just tell us your preferred default format for unknown recipients and we'll listen to that? That would be too simple. Think again mate. We wrote this, so we know better than you what you /really/ want! And if you don't want that, we'll just force you into it! Take it or leave it! </sarcasm>)

So here, you have a recipient prefers-unknown (==prefers-html according to Jorg), and another recipient prefers-html. Wait a minute, isn't that the same then as your case 2b.2 (allHTML)?
Which according to Jorg should get... drumrolls... HTML-only!?
So why now you want to send *both*? It's inconsistent.
This bug will let you have your will, and bring consistency.

> Case 3: Sender writes a HTML message and but prefers to send plain text if
> possible.
> Case 3a: The message cannot be downgraded.
>   All recipients receive HTML.

Wow. So now we're ignoring all recipient preferences (including prefers-plain), plus the user's hint that sending plaintext is actually wanted (auto-downgrade==true), and just send HTML, not even with an alternative plaintext version. Try tell that to uncle Ben in the desert of Timbuktu with his traditional mini mobile phone, or outdated hardware/software. And make sure that our uncle Ben doesn't hear you. You'll get (wont-)fixed left, right, and centre.

Why not "Both plain text and HTML", or even "Plain" - after asking? Different users, different recipients, different needs.
For conflict resolution in case of (anyPlain), we probably need Both. Or maybe even Plain (after asking).
Otherwise, it would be fine to send "HTML" as you suggest. If the user so wishes, and told us so by setting a default for prefers-unknown (this bug).

> Case 3b: The message can be downgraded.
>   All recipients receive plain (because the author said so overriding their
> preferences).

Indeed. That's fully message-centric, but only after my patch in bug 136502, where I made Auto-Downgrade actually downgrade *always* when the message is convertible::plain (even for allHTML recipient scope). And newsgroup recipients (which we haven't talked about yet...) will benefit, too.

> Tell me it's more difficult then that. I can explain this algorithm to
> anyone ;-)
> Basically, the recipients get what they asked for, if possible and not
> overridden by the sender.

Conclusions:

Put differently, your proposal has at least two shortcomings:
A) Silent dataloss for cases involving downgrading to plaintext (allPlain, per-message-send-plain)
B) No explicit user choice between "HTML" and "Both" (which covers legitimate use cases).

So yes, things are more difficult than your proposal. If you'd actually number all of your subcases, your proposal also has considerable complexity already, and requires pretty much the same options which we already have or are implementing now. Plus those cases which you omitted.

1) "recipients get what they asked for" - so recipients of type prefers-unknown, what are they asking for? (a gap in recipient-centric Auto-Detect which is filled by this bug by mapping prefers-unknown into prefers-something, according to users wishes and needs (see 4).
2) "if possible" - who defines how much dataloss of formatting etc. is acceptable or not? Opinions vary widely. Bugs filed and ongoing work. See 4).
3) "if not overridden by sender" - yeah, sender == user is the one who ultimately decides everything, even what recipients "prefer". Hence: See 4).
4) WE SHOULD LET THE USER DECIDE which format he really wants to send (this bug, and others, filed and not yet filed).

Comment 49

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) [impatiently awaiting seven reviews] from comment #45)
> (In reply to :aceman from comment #44)
> > The problems arise when there is a mix of preferences (like one wants plain
> > and one wants HTML) so we must choose something (Both is not always the
> > answer). The determination of this is the main problem.
> My algorithm specifies that.

Only insufficiently.
 
> > To prevent this thread to become another flood of comments reiterating
> > everything already said, I think we (Thomas, WADA, aceman) mostly understand
> > what to do.
> Sorry, I'll do it like Thomas. I came late to the party, I gave this some
> fresh thought and I want to be heard. This is way to complicated and Magnus
> will veto it, as he said.

Huh? Please don't exaggerate. What's complicated about adding one *single* dropdown to the already existing options when we find that they don't work?
In your Address book, the default is prefers-unknown, so that's most likely the majority of contacts. With this bug, we'll let you declare a default delivery format of your preference that TB should send for all recipients whose preference is unknown. What's complicated about that? It's long overdue, if anything. 

Besides, I take strong objection against translating Magnus' comment "I'm not that enthusiastic about this bug" as "Magnus will veto it". That's dataloss downgrading of speech, and I guess it's well-known that I hate dataloss downgrading.

> I sat in front of the options for one hour and I
> couldn't work out what this will do. No simple user will understand this.
> (And frankly: I don't understand how Mr. UX can propose something so twisted
> and unclear.)

Please! Trust me, the complexity in this issue was here before me.
I'm working very hard to make things more logical, transparent, predictable, and user-controlled.
Ultimately: Easier and clearer. We're not done yet.
Maybe message-centric default delivery format (not yet filed) is the answer for you.
We should let you skip Auto-Detect entirely and just default to sending HTML if you so wish. Or Both plaintext and HTML.

> So please, you take the time to read my algorithm, one comment, and tell me
> what's wrong with it.

Done. See Comment 48.

Comment 50

[Richard Marti (:Paenglab)]

Comment on attachment 8690676 [details]
Screenshot 2 (patch v.7): Options > Composition

Yes, I think this works.

Comment 51

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #46)
> OK, as you please :)

Jorg is still blissfully unaware of the minefield that is delivery-format-ux. Just when you think you've escaped them all, something will blow up right in front of you... ;)

> (In reply to Jorg K (GMT+1) from comment #42)
> > Case 2: Sender writes a HTML message and does NOT ask for conversion to
> > plain text.
> > Case 2a: The message cannot be downgraded.
> How does TB determine what can be downgraded? Bug 584313, 414299. And even
> after those fixed, there will surely be some cases when TB guesses wrong. So
> that is why there is bug 136502 to turn this determination off (and assume
> HTML if composed in HTML mode).

Aceman, please note:
(Auto-Downgrade==false) != (send-HTML==true)

We do NOT assume anything with message-centric Auto-Downgrade==false.
We just let recipient-centric Auto-Detect take it's course without the bias of message-centric Auto-Downgrade-before-Auto-Detect-if-convertible::plain.
So it all depends on what user has configured for Auto-Detect/Send Options, and what the actual recipients want.
So for cases of anyPlain, we'll use Send-Option for anyPlain to resolve the conflict with any of ASK | send-Plain | Send-Both as chosen by user (send-HTML is nonsense for conflict resolution).
By which the end result *might* be plain text again, even without Auto-Downgrade.

> >   If all recipients prefer plain, send only plain.
> Yes.

At least if the message is convertible::no, we should warn, as Magnus requested.
I solve that problem by conflating allPlain into anyPlain scope, so that send option will apply whenever there's any recipient who prefers-plain. This is possible because we removed prefers-unknown scope from the recipient scopes set of that send option.

Otherwise I agree with what you said.

Comment 52

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #46)
> OK, as you please :)

Jorg is still blissfully unaware of the minefield that is delivery-format-ux. Just when you think you've escaped them all, something will blow up right in front of you... ;)

> (In reply to Jorg K (GMT+1) from comment #42)
> > Case 2: Sender writes a HTML message and does NOT ask for conversion to
> > plain text.
> > Case 2a: The message cannot be downgraded.
> How does TB determine what can be downgraded? Bug 584313, 414299. And even
> after those fixed, there will surely be some cases when TB guesses wrong. So
> that is why there is bug 136502 to turn this determination off (and assume
> HTML if composed in HTML mode).

Aceman, please note:
(Auto-Downgrade==false) != (send-HTML==true)

We do NOT assume anything with message-centric Auto-Downgrade==false.
We just let recipient-centric Auto-Detect take it's course without the bias of message-centric Auto-Downgrade-before-Auto-Detect-if-convertible::plain.
So it all depends on what user has configured for Auto-Detect/Send Options, and what the actual recipients want.
So for cases of anyPlain, we'll use Send-Option for anyPlain to resolve the conflict with any of ASK | send-Plain | Send-Both as chosen by user (send-HTML is nonsense for conflict resolution).
By which the end result *might* be plain text again, even without Auto-Downgrade.

But for the default settings which we ship, yes, Auto-Downgrade==false will automatically send "Both" to recipients with the default preference, prefers-unknown.
I could imagine having prefers-unknown==prefers-html as well, you still owe me an answer on why "both" is better, with some sub-questions in Comment 22.

> >   If all recipients prefer plain, send only plain.
> Yes.

No. At least if the message is convertible::no (consequential HTML formatting), we should warn against the downgrading-dataloss, as Magnus requested. As I said, user might not even be aware that all recipients happen to be prefers-plain, and we wouldn't know if that preference is still up-to-date or not.

As a compromise for slim UI, I 'solve' the allPlain conflict resolution problem by conflating allPlain into anyPlain scope, so that conflict resolution by that send option will apply whenever there's any recipient who prefers-plain. This is possible because we removed prefers-unknown scope from the recipient scopes set of that send option ("If any recipient prefers plain text...")

Otherwise I agree with what you said.

Comment 53

[Richard Marti (:Paenglab)]

Comment on attachment 8690677 [details]
Screenshot 3 (patch v.7): Send Options with pref for unknown-prefers (using 'message format')

It looks complicated but I see now no other way to do this.

Thomas, the dialog is cut on bottom, is this because you patched the dialog or should the min-height be adapted?

Comment 54

[Thomas D. (:thomas8)]

Sorry, double posting due to network issues. Please scrap comment 51. Comment 52 is the correct version with more content.

Comment 55

[Thomas D. (:thomas8)]

(In reply to Richard Marti (:Paenglab) from comment #53)
> Comment on attachment 8690677 [details]
> Screenshot 3 (patch v.7): Send Options with pref for unknown-prefers (using
> 'message format')
> 
> It looks complicated but I see now no other way to do this.
> 
> Thomas, the dialog is cut on bottom, is this because you patched the dialog
> or should the min-height be adapted?

I did use DomInspector to patch the dialogue, which made it worse. However, there's some general problem with the dialogue height. Also happens in release version when you add the first domain: Buttons are pushed down. Not as far as on my screenshot. Can't reproduce in try-build right now (although I thought I saw it there, too). Please look into that and adapt as required.
Might also relate to screen resolution magnifying (where in windows, you set it that pixels shall be magnified by e.g. 150% to avoid getting miniprint on high resolution screens). I have set that.

Comment 56

[Richard Marti (:Paenglab)]

Here is the min-height set: https://dxr.mozilla.org/comm-central/source/mail/components/preferences/sendoptions.xul#22. You can try to increase the value.

Comment 57

[Jorg K (CEST = GMT+2)]

Thank you for your long answers. I see it's more difficult. You know I'm pragmatic, so let's cut to the point:

First new option:
Send the message as plain text if possible.
We all agree its good.

Second new option:
Preferred message format for recipients whose preference is unknown.
Choices: Plain, HTML, Both. "Don't care" doesn't seem to be an option.
OK, I can live with that.
Currently recipients with "unknown" preference (all my contacts)
get HTML if not downgradable, plain otherwise.
Never both. (1) How can I do this in the future, what do I set the options to?

Third new option:
If any of the recipients prefers to receive messages formatted as plain text:
(  ^^^^^^^^^^^^^^^^^^^^ sounds better)
Choices: Ask, convert, send HTML, both.
This is where you prevent dataloss. (2) Correct?
I still don't understand this. (3) If you have two recipients, one wants plain the other HTML and the message can be downgraded. They you *must* send both, to satisfy both, no?
(4) And if the sender sends something that can be downgraded, the sender asks for downgrading and the recipient wants plain, what happens? You would send plain, so the option is ignored. I don't get it, sorry.
Can you try to explain the option. I will have to explain it to me 80 y/o mother.

(5) How does that interact with "Send the message as plain text if possible"?
(6) Not at all, right? They are independent.

Please answer as briefly as possible. Six questions, and an explanation for the third option.

Of course I can read the code to see what it does, but I'd like to understand the governing principle and the algorithm.

Comment 58

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) [impatiently awaiting seven reviews] from comment #57)
> Thank you for your long answers. I see it's more difficult. You know I'm
> pragmatic, so let's cut to the point:
> 
> First new option:
> Send the message as plain text if possible.
> We all agree its good.

Great! Just waiting for Magnus' review.

> Second new option:
> Preferred message format for recipients whose preference is unknown.
> Choices: Plain, HTML, Both. "Don't care" doesn't seem to be an option.

We'll probably default this to "Both", like before (otherwise "HTML").
So we're not forcing you to choose.
TB must send *some format* out of these three, so "Don't care" is not a valid format in a recipient-centric approach. Auto-Detect is predominantly recipient-centric (with a message-centric appendix of Auto-Downgrade if convertible::plain).

> OK, I can live with that.
> Currently recipients with "unknown" preference (all my contacts)
> get HTML if not downgradable, plain otherwise.
> Never both.

Then you changed the pref to "Send HTML only".
The current default for unknown is "both", and ironically, you have defined that with your answer to that question about recipients not being able to receive HTML (conflict resolution for dataloss downgrading). That's the nonsense conglomerate which this bug is trying to solve.

> (1) How can I do this in the future, what do I set the options
> to?

Hmmm. I'm surprised that it's not obvious...
You want:

Auto-Downgrade (send plaintext if the message can be losslessly converted)
Unknown==HTML (unknown-prefers default format setting)

So you set

[x] Send plain text if possible (Auto-Downgrade)

and

Preferred message format for recipients whose preference is *unknown*:
[HTML]

Maybe we could improve the wording of Auto-Downgrade?
I think unknown-prefers is relatively easy to understand; it's correct wording within recipient-centric logic where each recipient has a preference; but if there's a better wording, please share.
Maybe this?

Default message format for recipients whose preference is unknown:
[...]

It's not 100% correct, but maybe easier to understand that this is where you set the de-facto default (which we can't guarantee, but only in those rare cases where you have prefers-plain).

> Third new option:
> If any of the recipients prefers to receive messages formatted as plain text:
> (  ^^^^^^^^^^^^^^^^^^^^ sounds better)
> Choices: Ask, convert, send HTML, both.
> This is where you prevent dataloss. (2) Correct?

You can prevent or opt into dataloss here. correct.

> I still don't understand this. (3) If you have two recipients, one wants
> plain the other HTML and the message can be downgraded. They you *must* send
> both, to satisfy both, no?

Not really. Prefers-plain recipient theoreticaly may not be able to read HTML message, but HTML recipient is always able to read plaintext message. And because the message is claimed to be "losslessly convertible", the difference should be near zero (but is not currently, due to bugs in the convertible algorithm).

> (4) And if the sender sends something that can be downgraded, the sender
> asks for downgrading and the recipient wants plain, what happens? You would
> send plain, so the option is ignored. I don't get it, sorry.

Very simple:

Auto-Downgrade is inherently message-centric ("Send plaintext if the message is convertible"), and therefore trumps all other settings of recipient-centric Auto-Detect. Hence you find that checkbox at the very top. So yes, for cases where Auto-Downgrade is possible and wanted (via checkbox), anything else is ignored.
Maybe we need to make that clearer in the UI, that this will bypass any recipient-centric logic, including that setting for "If any recipient prefers plaintext..."

I suggested a tooltip to clarify the meaning of Auto-Downgrade checkbox, but it was rejected. Not sure what's wrong with tooltips, but they just said we won't have them here in options. In addition, the text of my tooltip was perhaps still prone to misunderstandings. Auto-Downgrade is not easy to explain to your 80year-old granny.

Any other ideas with as few words as possible?

> Can you try to explain the option. I will have to explain it to me 80 y/o
> mother.
> 
> (5) How does that interact with "Send the message as plain text if possible"?
> (6) Not at all, right? They are independent.

Well, guess that's right. "Send as plain text if possible", if checked and applicable (message is 'losslessly' convertible to plaintext), will trump "If any recipient prefers plain..."

> Please answer as briefly as possible. Six questions, and an explanation for
> the third option.
> 
> Of course I can read the code to see what it does, but I'd like to
> understand the governing principle and the algorithm.

My understanding is that we have different governing principles, and Delivery-Format Auto-Detect is so confusing because it combines two principles which are orthogonal:
Delivery Format Auto-Detect (conglomerate algorithm) ==
  1) message-centric Auto-Downgrade &&
  2) recipient-centric Auto-Detect.

It's confusing because before bug 136502, Auto-Downgrade is not visible nor controllable at all, but it will actually bypass 2) recipient-centric Auto-Detect for certain unpredictable cases.

Plus, recipient-centric auto-detect is not fully controllable because someone tried to squeeze everything into a single option:
a) conflict resolution for anyPlain (potentially requiring dataloss downgrading to plain text) and 
b) default format for prefers-unknown (normally requiring HTML or Both)
which is a complete design failure because the only possible option covering both intentions is "send both" (this bug). Also the current wording does not hint at the fact that this is where you de-facto have to set your default format for prefers-unknown recipients.

Plus, current send option lies to cover "anyPlain" scope (plus anyUnknown scope), but currently excludes "allPlain" scope (fixed by this bug). Both anyPlain and allPlain require conflict resolution:
allPlain: resolve conflict that message has HTML, but all recipients want plain
anyPlain: resolve same conflict, PLUS the conflict that other recipients explicitly want HTML (or Both, after this bug).
Essentially all these conflicts are about downgrading or (send both).
Downgrading can be silently (send plain) or controlled (ask me).
So I think it might be possible to group them into one option.
Definitely better than the current NOP conglomerate option described above.

Comment 59

[Jorg K (CEST = GMT+2)]

OK, just a few things.

Firstly, you were right, I was blissfully unaware of the issues and you've convinced me that it's all in good hands ;-) - And thanks for the patience and effort!

<off topic>
When I currently (without this patch) send HTML to an "unknown" recipient, HTML is sent, not both.
</off topic>

Since we now all agree, perhaps we should make the options a little clearer so my 80 y/o mother (not granny) understands them.

Maybe
  [ ] Send the message as plain text if no formatting is lost in the down-grade
      from HTML (rich text).
  Note: If this option is selected, recipient preferences for HTML will be ignored.
  --------------- (separator) -----------------

Then the other two options.

(I know, this was hotly discussed in the other bug).

Great! (I mean it). We will finally cut through the Gordian knot and clean up this ghastly mess.

(Didn't I say it before: Patches help!)

Comment 60

[:aceman]

Comment on attachment 8690674 [details] [diff] [review]
Patch v.7 (strings, pref sanitization): Implement triple pref for unknown=plain|html|both, auto-detect logic and send options UI. *** To be applied on top of attachment 8690447 [details] [diff] [review] ***

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

This patch is bitrotted badly, probably due to bug 136502. I'll update it right now.

Looks like it is time for me to create some tests for this so we can see if all cases behave correctly.
Thomas do you have a matrix of types of recipients and expected resulting send format?
Probably also taking the option in bug 136502 into account.

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +4951,5 @@
> +  bool anyPlain = false;
> +
> +  // The initial value of unknownPrefers will be overwritten by pref value;
> +  // default: 3 (Both Plain Text and HTML).
> +  int unknownPrefers = 3;

What if you used the value nsIAbPreferMailFormat::unknown (0) instead of a new value of 3 here? Would that work for you?

@@ +4955,5 @@
> +  int unknownPrefers = 3;
> +  rv = prefBranch->GetIntPref("mailnews.sendformat.unknown_prefers", &unknownPrefers);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  // Sanitize out-of-range pref values which user might have set, by resetting pref.
> +  if (unknownPrefers < 1 || 3 < unknownPrefers)

You should not assume the constants are always 1,2,3 so you can make a continuous interval out of them. Because later on we reference them as nsIAbPreferMailFormat::html etc. I'll fix this part too.

@@ +4959,5 @@
> +  if (unknownPrefers < 1 || 3 < unknownPrefers)
> +  {
> +    rv = prefBranch->ClearUserPref("mailnews.sendformat.unknown_prefers");
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    rv = prefBranch->GetIntPref("mailnews.sendformat.unknown_prefers", &unknownPrefers);

Why do you get the pref again, when you just cleared it? I'll remove this.

Comment 61

[:aceman]

Attachment: Patch v.7.1 (strings, pref sanitization): Implement triple pref for unknown=plain|html|both, auto-detect logic and send options UI.

Unbitrotted patch.

Comment 62

[Thomas D. (:thomas8)]

Comment on attachment 8693236 [details] [diff] [review]
Patch v.7.1 (strings, pref sanitization): Implement triple pref for unknown=plain|html|both, auto-detect logic and send options UI.

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

Ace, thanks for upgrading the patch; yes, the other bug made it bitrot.

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +4947,5 @@
> +  // allPlain recipient scope is subsumed here.
> +  bool anyPlain = false;
> +
> +  // The initial value of unknownPrefers will be overwritten by pref value;
> +  // default: 3 (Both Plain Text and HTML).

Magnus wanted this comment to go away, and I have already removed it in the other bug.

@@ +4954,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  // Sanitize out-of-range pref values which user might have set, by resetting pref.
> +  if (unknownPrefers != nsIAbPreferMailFormat::html &&
> +      unknownPrefers != nsIAbPreferMailFormat::plaintext &&
> +      unknownPrefers != 3)

I think 3 is very intuitive and correct here, because it's same as html default action pref values. 0 would be confusing, because it stands for Ask, or truly unknown (But Both is no longer unknown).

@@ +4957,5 @@
> +      unknownPrefers != nsIAbPreferMailFormat::plaintext &&
> +      unknownPrefers != 3)
> +  {
> +    (void) prefBranch->ClearUserPref("mailnews.sendformat.unknown_prefers");
> +    unknownPrefers = 3;

Hmmm, no. Here you are hardcoding the pref default value, which is unwanted. As in my patch, we need to get it again from the pref after resetting the pref to the default value.

@@ +5041,5 @@
>    {
> +    // Check the preference to see what action we should default to.
> +    // The initial value of action will be overwritten by pref value.
> +    // For SeaMonkey, the pref defaults to 0 (Ask the user which format to send).
> +    // For Thunderbird, the pref defaults to 3 (Send both plain text and HTML).

Last 3 lines of comment should go, said Magnus, to avoid hardcoding the pref default even in comments, and I already removed them on the other bug.

Comment 63

[:aceman]

Attachment: tests

I put up these tests to check the various combinations of the options. Thomas, can you check if I got the results right? I thought about what values the test (with your code patch) produces and I did not find any logic problems in your code. But maybe you will know better what send formats it should produce.

Comment 64

[:aceman]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #62)
> Comment on attachment 8693236 [details] [diff] [review]
> Patch v.7.1 (strings, pref sanitization): Implement triple pref for
> unknown=plain|html|both, auto-detect logic and send options UI.
> 
> @@ +4954,5 @@
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +  // Sanitize out-of-range pref values which user might have set, by resetting pref.
> > +  if (unknownPrefers != nsIAbPreferMailFormat::html &&
> > +      unknownPrefers != nsIAbPreferMailFormat::plaintext &&
> > +      unknownPrefers != 3)
> 
> I think 3 is very intuitive and correct here, because it's same as html
> default action pref values. 0 would be confusing, because it stands for Ask,
> or truly unknown (But Both is no longer unknown).

0 does not stand for Ask, because you take this value from nsIAbPreferMailFormat (http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/public/nsIAbCard.idl#12) , not from nsIMsgCompSendFormat (as in default html action). It would seem safer to me to use the existing constant (unknown=0) from nsIAbPreferMailFormat instead of inventing a new out or range one that needs to be hardcoded in the code. Otherwise what happens if somebody adds a a new constant for 3 into nsIAbPreferMailFormat?

> @@ +4957,5 @@
> > +      unknownPrefers != nsIAbPreferMailFormat::plaintext &&
> > +      unknownPrefers != 3)
> > +  {
> > +    (void) prefBranch->ClearUserPref("mailnews.sendformat.unknown_prefers");
> > +    unknownPrefers = 3;
> 
> Hmmm, no. Here you are hardcoding the pref default value, which is unwanted.
> As in my patch, we need to get it again from the pref after resetting the
> pref to the default value.

OK, I understand. We use prefService->GetDefaultBranch (and then getintpref on it) for getting the default value, but getting the pref after clearing it hopefully does the same with less code ;)

Comment 65

[:aceman]

Attachment: Patch v.7.2 (strings, pref sanitization): Implement triple pref for unknown=plain|html|both, auto-detect logic and send options UI.

Addressed the problems.

Comment 66

[Thomas D. (:thomas8)]

Comment on attachment 8693292 [details] [diff] [review]
Patch v.7.2 (strings, pref sanitization): Implement triple pref for unknown=plain|html|both, auto-detect logic and send options UI.

Aceman, please name any patch files with .patch file extension, which will automatically mark the file as a patch. If not marked as patch (checkbox in attachment details), the 'diff' and 'review' attachment commands are missing on bmo (only 'details' will be offered), which makes the patch hard to access and evaluate.

Comment 67

[:aceman]

Yes, of course, must have been late night :)

Comment 68

[Thomas D. (:thomas8)]

Attachment: Patch v.8: Implement triple pref for unknown=plain|html|both, auto-detect logic and send options UI. Implement nsIAbPreferMailFormat::both.

Magnus, it would be nice to hear some of your preliminary thoughts on this bug if you have time...

(In reply to :aceman from comment #64)
> (In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment
> #62)
> > Comment on attachment 8693236 [details] [diff] [review]
> > Patch v.7.1 (strings, pref sanitization): Implement triple pref for
> > unknown=plain|html|both, auto-detect logic and send options UI.
> > 
> > @@ +4954,5 @@
> > > +  NS_ENSURE_SUCCESS(rv, rv);
> > > +  // Sanitize out-of-range pref values which user might have set, by resetting pref.
> > > +  if (unknownPrefers != nsIAbPreferMailFormat::html &&
> > > +      unknownPrefers != nsIAbPreferMailFormat::plaintext &&
> > > +      unknownPrefers != 3)
> > 
> > I think 3 is very intuitive and correct here, because it's same as html
> > default action pref values. 0 would be confusing, because it stands for Ask,
> > or truly unknown (But Both is no longer unknown).
> 
> 0 does not stand for Ask, because you take this value from
> nsIAbPreferMailFormat
> (http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/public/
> nsIAbCard.idl#12) , not from nsIMsgCompSendFormat (as in default html
> action).

Indeed.

> It would seem safer to me to use the existing constant (unknown=0)

Yes, but it's illogical and confusing to use nsIAbPreferMailFormat::*unknown* where we really mean *both*.

> from nsIAbPreferMailFormat instead of inventing a new out or range one that
> needs to be hardcoded in the code. Otherwise what happens if somebody adds a
> a new constant for 3 into nsIAbPreferMailFormat?

Yes, what will happen? Let's try it! :)

So this patch has all the goodness of previous patches, plus:

a) Better label for unknown-prefers

...indicating that this will be the de-facto default for auto-detect unless trumped by send option for conflict resolution of 'anyPlain' recipient scope:

  *Default message format* for recipients whose preference is unknown:
  [Plain text | HTML | Both plaintext and HTML]

b) Implement nsIAbPreferMailFormat::both

For our own convenience, and more user control.
If "Both plaintext and HTML" is a legitimate send format, there's no reason not to offer that as a preferred send format, even for individual recipients, if the recipient-centric user so wishes.
At the cost of only about 5 lines of code, this doesn't hurt nor confuse anyone but just completes the set of options in the "Prefers to receive messages formatted as:" dropdown.
So you can now set your 'Default message format' for prefers-unknown to 'HTML-only' (for slim messages) and specify individual exceptions for recipients who want or need 'Both plaintext and HTML'.
Interestingly, that doesn't change the auto-detect code logic of this patch at all.

Patch is untested, please compile and report back any errors. If no errors, try build would be nice.

Comment 69

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #63)
> Created attachment 8693291 [details] [diff] [review]
> tests
> 
> I put up these tests to check the various combinations of the options.
> Thomas, can you check if I got the results right? I thought about what
> values the test (with your code patch) produces and I did not find any logic
> problems in your code. But maybe you will know better what send formats it
> should produce.

Please find the full table of expected send format with delivery format auto-detect and different recipient preferences here (Table in PDF file):

attachment 8675716 [details]

Two cosmetic changes over that table:

Unknown-both-recipient -> Unknown-prefers-both recipient or prefers-both-recipient (you may want to test these separately, but the results should be the same)

Sendoptions1 (allPlain), Sendoptions2 (somePlain) -> single SendOption for anyPlain scope (If any recipient prefers plain text... currently known as mail.default_html_action pref).

Comment 70

[:aceman]

Comment on attachment 8693334 [details] [diff] [review]
Patch v.8: Implement triple pref for unknown=plain|html|both, auto-detect logic and send options UI. Implement nsIAbPreferMailFormat::both.

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

Yes, adding the constant into nsIAbPreferMailFormat would solve the problem for me ;)

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +4946,5 @@
> +  // according to user's send options per global mail.default_html_action pref.
> +  // allPlain recipient scope is subsumed here.
> +  bool anyPlain = false;
> +
> +  int unknownPrefers = 3;

Use nsIAbPreferMailFormat::both here too now :)

Comment 71

[Thomas D. (:thomas8)]

Comment on attachment 8693334 [details] [diff] [review]
Patch v.8: Implement triple pref for unknown=plain|html|both, auto-detect logic and send options UI. Implement nsIAbPreferMailFormat::both.

Note:

We should probably change pref names for added clarity and better grouping of 
prefs in config editor:

> mail.default_html_action

mailnews.sendformat.autodetect.anyPlain_action

With the patch here, this pref no longer determines the default format for recipients whose preference is unknown; it's now only conflict resolution for recipient scopes involving anyPlain.
How does this affect current consumers of this pref?
Maybe we have to break it for them because the pref now has a new meaning.

> mailnews.sendformat.unknown_prefers

mailnews.sendformat.autodetect.default_format
- or perhaps - 
mailnews.sendformat.unknown_default
mailnews.sendformat.unknown_default_format

unknown_prefers is technically more correct, but hinting to the de-facto default format for auto-detect as set by this pref might be more descriptive.

Comment 72

[:aceman]

Changing the pref name would break it for users who have changed the value. They would have to set it anew when they notice and until then the behaviour would change behind their backs. And you would have to update all the wiki docs.

I think we try to not rename prefs.

Comment 73

[:aceman]

The patch compiles and the new test passes locally.
I pushed both patches to try server: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=46b4908562cc .

Comment 74

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #72)
> Changing the pref name would break it for users who have changed the value.

Well, let's look at that in detail:

I suggested this new pref...

mailnews.sendformat.autodetect.anyPlain_action (recipient scope: anyPlain)

...to replace the current pref:

mail.default_html_action (all recipient scopes involving anyPlain and anyUnknown, including mixed with someHTML).

The current pref can take following values:

nsIMsgCompSendFormat::AskUser (0):
nsIMsgCompSendFormat::PlainText (1):
nsIMsgCompSendFormat::HTML (2):
nsIMsgCompSendFormat::Both (3):

ad 0) "Ask me what to do": nobody in their right senses can currently set that because it will nag you every time you send a message, whenever there are prefers-unknown or mixed recipients.

ad 1) "Convert the message to plain text": nobody in their right senses can currently set that because it would downgrade *all* HTML messages by default, almost completely irrespective of HTML convertibility or recipient preferences.

Both of these user settings (1) and (2) however much more plausible options after our patch here limits the recipient scope of the pref to 'anyPlain'. So to preserve these values, we could do once-only pref migration.

ad 2) "Send the message in HTML anyway": This is currently reasonable if used as a default setting for the majority of prefers-unknown recipients; however it is *no longer* reasonable when we limit the scope of the old pref to 'anyPlain'. It's absurd to send to send 'HTML-only' to recipients who explicitly prefer plaintext (because that would make prefers-plain a NOP). So imo we actually need to migrate this value to something safe and reasonable in the new scheme of things, which is 'both' for anyPlain and 'HTML' as a default only for prefers-unknown.

ad 3) "Send the message in both plain text and HTML": This is the current default and will most likely be the new default, too, so we're not changing anything for this case.

Looking at 2), maybe we need to change the pref value anyway, so we could also do full pref migration?

> They would have to set it anew when they notice and until then the behaviour
> would change behind their backs.

See above; there's no reasonable case where users will actually lose anything if we do proper pref migration.

> And you would have to update all the wiki
> docs.

Yeah, such can happen when you improve the product...

Comment 75

[:aceman]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #69)
> attachment 8675716 [details]
> 
> Two cosmetic changes over that table:
> 
> Unknown-both-recipient -> Unknown-prefers-both recipient or
> prefers-both-recipient (you may want to test these separately, but the
> results should be the same)

OK, if the new UI with exposed value of Both passes, I can add this into the test.

Comment 76

[:aceman]

Attachment: tests v1.1

Added some more cases into the tests.

Comment 77

[Thomas D. (:thomas8)]

Comment on attachment 8694231 [details] [diff] [review]
tests v1.1

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

OK, I'm a complete newbie to tests (and not too keen to dig myself in), so pls consider that if you any of the following doesn't make sense.

::: mailnews/compose/test/unit/test_nsMsgCompose4.js
@@ +8,5 @@
>  var nsIMsgCompose = Components.interfaces.nsIMsgCompose;
>  var nsIMsgComposeParams = Components.interfaces.nsIMsgComposeParams;
>  var nsIMsgCompConvertible = Components.interfaces.nsIMsgCompConvertible;
>  var nsIMsgCompFields = Components.interfaces.nsIMsgCompFields;
> +var nsIAbPrefer = Components.interfaces.nsIAbPreferMailFormat;

nsIAbPrefers (with trailing s) might be more readable and more consistent with unknownPrefers. Not sure how helpful abbreviations of the original stuff are, but maybe they assist terseness/readability in this test environment.

@@ +47,5 @@
>    do_check_eq(msgCompose.determineHTMLAction(aConvertible), aSendFormat);
>  }
>  
> +/**
> + * Return a send format directly from the preference of the recipient.

Hmmm. Directly from one recipient's preference!? That's only possible for non-mixed sets of recipients.
But you are using it to tests sets of recipients which might have mixed individual delivery format preferences (even though they're not mixed in your lists).
And even for single recipient, return values aren't right imo...

@@ +49,5 @@
>  
> +/**
> + * Return a send format directly from the preference of the recipient.
> + * The function assumes the message is NOT convertible and conversion is NOT allowed.
> + *

Does it assume a single recipient (or same-preference sets), too? (see above)

@@ +55,5 @@
> + */
> +function formatFromPref(aUnknownPrefers) {
> +  switch (aUnknownPrefers) {
> +    case nsIAbPrefer.html:
> +      return SendFormat.HTML;

Only true for a single recipient or same-preference set of recipients.
Mixted recipients may require otherwise.
It's called UnknownPrefers because we do NOT always observe it in recipient-centric auto-detection; especially, any prefers-plain recipient in the mix can override all other preferences per default_html_action pref (which we should better call mailnews.sendformat.anyPlain.action).

@@ +57,5 @@
> +  switch (aUnknownPrefers) {
> +    case nsIAbPrefer.html:
> +      return SendFormat.HTML;
> +    case nsIAbPrefer.plaintext:
> +      return SendFormat.AskUser;

This is wrong (both before and after my patch here) even if you have only one recipient in the message and he prefers plaintext.
Before my patch, it's SendFormat.PlainText.
After my unknown-prefers patch, the send format if there's any prefers-plain recipient will be determined exclusively by default_html_action pref.
For TB, that pref defaults to 3.
So if you are checking pref default settings of TB, you want SendFormat.Both.
Otherwise, you want the return value of the pref.
One possible user-defined return value of the pref can be SendFormat.AskUser.
For SeaMonkey, different default (how do they survive with defaulting to ASK!?1?).
Note that there's some hack between the values of 4 and 0 for Asking:

48 interface nsIMsgCompSendFormat {
49     const long AskUser = 4;     /* Hack: Bug 44512. If this is 0 and passed
50                                    as results.action to the askSendFormat
51                                    dialog, the args object gets destroyed.*/
52     const long PlainText = 1;
53     const long HTML = 2;
54     const long Both = 3;
55 };

Which makes me think that other comment which you told me to keep, that pref default may default to 0, is actually non-applicable, because even SM must currently default to 4 (which was intended to be 0, but 0 is claimed to kill things when passed to the dialogue).

@@ +59,5 @@
> +      return SendFormat.HTML;
> +    case nsIAbPrefer.plaintext:
> +      return SendFormat.AskUser;
> +    case 3:
> +      return SendFormat.Both;

For a single recipient or same-pref sets of them, yes. For mixed, no.

@@ +115,1 @@
>    Services.prefs.setIntPref("mail.default_html_action", SendFormat.AskUser);

Why are we setting the pref to AskUser if that's not our default for TB?
Or if we want to test all possible pref values, why only this one but not the others?

@@ +115,3 @@
>    Services.prefs.setIntPref("mail.default_html_action", SendFormat.AskUser);
>    do_check_eq(msgCompose.determineHTMLAction(nsIMsgCompConvertible.No),
> +              SendFormat.Both);

What does this do? Call determineHTMLAction without any recipients set?
Yeah, if that's even possible, that should probably return SendFormat.Both according to the new logic. Or maybe I don't have sufficient understanding of this test.

@@ +124,5 @@
> +
> +/**
> + *
> + * @param aAllowConversion  The bool value of the pref indicating if conversion
> + *                          to plain text is allowed.

Please add the pref name here in the comment.
Please use paramater name which mirrors pref name.
Pref name suggested by my patch is AutoDowngrade, so this should be aAutoDowngrade?

@@ +131,5 @@
> + */
> +function test_send_format(aAllowConversion, aUnknownPrefers) {
> +  // test1 .. test3 have unknown format preference
> +  // test4 prefers plain text
> +  // test5 prefers HTML

All of these email addresses should have telling names mentioning their preferred format. It's really hard to know which is which when you're working with test1...5. And error prone for future adjustments to this test.

@@ +132,5 @@
> +function test_send_format(aAllowConversion, aUnknownPrefers) {
> +  // test1 .. test3 have unknown format preference
> +  // test4 prefers plain text
> +  // test5 prefers HTML
> +  // TestList1 contains test1 .. test3

Testlist1: 3 different recipients who all prefer unknown?
Why?
And shouldn't the list also have a telling name like:
TestList1-PrefersUnknown?

@@ +133,5 @@
> +  // test1 .. test3 have unknown format preference
> +  // test4 prefers plain text
> +  // test5 prefers HTML
> +  // TestList1 contains test1 .. test3
> +  // TestList2 contains test4

TestList2: One recipient who prefers plain? Is that a list?
Dito about list naming

@@ +134,5 @@
> +  // test4 prefers plain text
> +  // test5 prefers HTML
> +  // TestList1 contains test1 .. test3
> +  // TestList2 contains test4
> +  // TestList3 contains test5

List naming again.

I don't understand the lists. Don't we need lists like this:

TestList1-mixed-prefers-plain-html-unknown
(so it's a list with three different recipients who all prefer something else)

@@ +142,4 @@
>  
>    // Test - determineHTMLAction with plain text.
>  
> +  checkPopulate("[e-mail]", "", SendFormat.AskUser);

AskUser is the wrong result, see above.

@@ +143,5 @@
>    // Test - determineHTMLAction with plain text.
>  
> +  checkPopulate("[e-mail]", "", SendFormat.AskUser);
> +  checkPopulate("[e-mail]", "",
> +                aAllowConversion ? SendFormat.PlainText : SendFormat.AskUser,

Dito

@@ +163,4 @@
>  
>    // Test - determineHTMLAction with a list of one item.
>  
> +  checkPopulate("TestList2 <TestList2>", "", SendFormat.AskUser);

No, here we need the pref return value for default_html_action pref.

@@ +179,5 @@
>                  SendFormat.AskUser);
>    checkPopulate("[e-mail],[e-mail]", "",
> +                SendFormat.AskUser);
> +  checkPopulate("[e-mail],[e-mail]", "",
> +                aAllowConversion ? SendFormat.PlainText : SendFormat.AskUser,

After my patch, we default to Both, otherwise the pref action.

@@ +194,1 @@
>                  SendFormat.AskUser);

Dito about AskUser

@@ +204,5 @@
>    checkPopulate("[e-mail]", "mozilla.test", SendFormat.AskUser);
>    checkPopulate("[e-mail]", "mozilla.test", SendFormat.AskUser);
>    checkPopulate("", "mozilla.test", SendFormat.AskUser);
> +  checkPopulate("", "mozilla.test",
> +                aAllowConversion ? SendFormat.PlainText : SendFormat.AskUser,

better to use aAutoDowngrade, see above

@@ +211,5 @@
> +
> +function run_test() {
> +  basic_tests();
> +  // Bug 136502
> +  for (let allowConversion of [true, false]) {

dito, autoDowngrade is more descriptive imo

@@ +214,5 @@
> +  // Bug 136502
> +  for (let allowConversion of [true, false]) {
> +    Services.prefs.setBoolPref("mailnews.sendformat.auto_downgrade", allowConversion);
> +    // Bug 1222176
> +    for (let unknownPreference of [3, nsIAbPrefer.html, nsIAbPrefer.plaintext]) {

Yeah, let's hope we can make 3 a proper nsIAbPrefer.Both

Comment 78

[:aceman]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #77)
> nsIAbPrefers (with trailing s) might be more readable and more consistent
> with unknownPrefers. Not sure how helpful abbreviations of the original
> stuff are, but maybe they assist terseness/readability in this test
> environment.
Yes, they are just abbreviations as the original objects have very long names.

> @@ +47,5 @@
> >    do_check_eq(msgCompose.determineHTMLAction(aConvertible), aSendFormat);
> >  }
> >  
> > +/**
> > + * Return a send format directly from the preference of the recipient.
> 
> Hmmm. Directly from one recipient's preference!? That's only possible for
> non-mixed sets of recipients.
> But you are using it to tests sets of recipients which might have mixed
> individual delivery format preferences (even though they're not mixed in
> your lists).
> And even for single recipient, return values aren't right imo...
> 
> @@ +49,5 @@
> >  
> > +/**
> > + * Return a send format directly from the preference of the recipient.
> > + * The function assumes the message is NOT convertible and conversion is NOT allowed.
> > + *
> 
> Does it assume a single recipient (or same-preference sets), too? (see above)
> 
> @@ +55,5 @@
> > + */
> > +function formatFromPref(aUnknownPrefers) {
> > +  switch (aUnknownPrefers) {
> > +    case nsIAbPrefer.html:
> > +      return SendFormat.HTML;
> 
> Only true for a single recipient or same-preference set of recipients.
> Mixted recipients may require otherwise.
> It's called UnknownPrefers because we do NOT always observe it in
> recipient-centric auto-detection; especially, any prefers-plain recipient in
> the mix can override all other preferences per default_html_action pref
> (which we should better call mailnews.sendformat.anyPlain.action).

It is just a helper function to return what the send format would be if there was only one recipient (unknown)
and the pref would be set to the aUnknownPrefers value. Because some of the test results really are determined only by the value of the pref.

> @@ +57,5 @@
> > +  switch (aUnknownPrefers) {
> > +    case nsIAbPrefer.html:
> > +      return SendFormat.HTML;
> > +    case nsIAbPrefer.plaintext:
> > +      return SendFormat.AskUser;
> 
> This is wrong (both before and after my patch here) even if you have only
> one recipient in the message and he prefers plaintext.
> Before my patch, it's SendFormat.PlainText.
> After my unknown-prefers patch, the send format if there's any prefers-plain
> recipient will be determined exclusively by default_html_action pref.
> For TB, that pref defaults to 3.

Yes, and I have set default_html_action to Ask in the test. So this return value is correct. If there is a plaintext recipient, we use default_html_action.

> So if you are checking pref default settings of TB, you want SendFormat.Both.
> Otherwise, you want the return value of the pref.
> One possible user-defined return value of the pref can be SendFormat.AskUser.
> For SeaMonkey, different default (how do they survive with defaulting to
> ASK!?1?).
> Note that there's some hack between the values of 4 and 0 for Asking:
> 
> 48 interface nsIMsgCompSendFormat {
> 49     const long AskUser = 4;     /* Hack: Bug 44512. If this is 0 and
> passed
> 50                                    as results.action to the askSendFormat
> 51                                    dialog, the args object gets
> destroyed.*/
> 52     const long PlainText = 1;
> 53     const long HTML = 2;
> 54     const long Both = 3;
> 55 };
> 
> Which makes me think that other comment which you told me to keep, that pref
> default may default to 0, is actually non-applicable, because even SM must
> currently default to 4 (which was intended to be 0, but 0 is claimed to kill
> things when passed to the dialogue).

Even if mail.default_html_action is set to 0, determineHTMLAction converts it to Ask (4).

> For a single recipient or same-pref sets of them, yes. For mixed, no.
> 
> @@ +115,1 @@
> >    Services.prefs.setIntPref("mail.default_html_action", SendFormat.AskUser);
> 
> Why are we setting the pref to AskUser if that's not our default for TB?

To see whether determineHTMLAction returned the value of the pref when we expect that.
If I set the pref to Both, then there may be also other occasions where the return value will be Both, but not taken from the pref. E.g. at the end of determineHTMLAction in the case of "Not everyone prefers HTML (!allHTML), and nobody prefers plain text (!anyPlain)" it returns Both. So just by seeing return value of Both we can't be sure which code path was chosen to arrive at this value.

> Or if we want to test all possible pref values, why only this one but not
> the others?
If the determineHTMLAction returns the value of the mail.default_html_action when it is set to a value (like AskUser), the assumption is that it will do it for any other value so testing them all makes no sense.

> @@ +115,3 @@
> >    Services.prefs.setIntPref("mail.default_html_action", SendFormat.AskUser);
> >    do_check_eq(msgCompose.determineHTMLAction(nsIMsgCompConvertible.No),
> > +              SendFormat.Both);
> 
> What does this do? Call determineHTMLAction without any recipients set?
> Yeah, if that's even possible, that should probably return SendFormat.Both
> according to the new logic. Or maybe I don't have sufficient understanding
> of this test.
The recipients are set in the lines above this (fields.* =).

> @@ +124,5 @@
> > +
> > +/**
> > + *
> > + * @param aAllowConversion  The bool value of the pref indicating if conversion
> > + *                          to plain text is allowed.
> 
> Please add the pref name here in the comment.
> Please use paramater name which mirrors pref name.
> Pref name suggested by my patch is AutoDowngrade, so this should be
> aAutoDowngrade?

Ok.

> @@ +131,5 @@
> > + */
> > +function test_send_format(aAllowConversion, aUnknownPrefers) {
> > +  // test1 .. test3 have unknown format preference
> > +  // test4 prefers plain text
> > +  // test5 prefers HTML
> 
> All of these email addresses should have telling names mentioning their
> preferred format. It's really hard to know which is which when you're
> working with test1...5. And error prone for future adjustments to this test.

Yes, and how hard it was to find out what preferences those have when I didn't have that comment prepared by the previous author. Therefore I at least documented them. However, that same binary addressbook file is used in other tests so changing the names is not that trivial. Easiest would be to create a new addressbook, which may not be worth it.

> I don't understand the lists. Don't we need lists like this:
> 
> TestList1-mixed-prefers-plain-html-unknown
> (so it's a list with three different recipients who all prefer something
> else)
I took the existing lists. I didn't want to rewrite the test completely.
But adding new missing combinations of recipients is what I ask from you :) So yes, 3 recipients with different preferences should be added. I'll do it.

> @@ +142,4 @@
> >  
> >    // Test - determineHTMLAction with plain text.
> >  
> > +  checkPopulate("[e-mail]", "", SendFormat.AskUser);
> 
> AskUser is the wrong result, see above.
It is the value of mail.default_html_action which seems correct to me.

> @@ +143,5 @@
> >    // Test - determineHTMLAction with plain text.
> >  
> > +  checkPopulate("[e-mail]", "", SendFormat.AskUser);
> > +  checkPopulate("[e-mail]", "",
> > +                aAllowConversion ? SendFormat.PlainText : SendFormat.AskUser,
> 
> Dito
It is the value of mail.default_html_action which seems correct to me.

> @@ +163,4 @@
> >  
> >    // Test - determineHTMLAction with a list of one item.
> >  
> > +  checkPopulate("TestList2 <TestList2>", "", SendFormat.AskUser);
> 
> No, here we need the pref return value for default_html_action pref.
And we do. It is AskUser in this test.

> @@ +179,5 @@
> >                  SendFormat.AskUser);
> >    checkPopulate("[e-mail],[e-mail]", "",
> > +                SendFormat.AskUser);
> > +  checkPopulate("[e-mail],[e-mail]", "",
> > +                aAllowConversion ? SendFormat.PlainText : SendFormat.AskUser,
> 
> After my patch, we default to Both, otherwise the pref action.
Hm, in which case should the return be Both, regardless default_html_action?

> @@ +194,1 @@
> >                  SendFormat.AskUser);
> 
> Dito about AskUser
> 
> @@ +204,5 @@
> >    checkPopulate("[e-mail]", "mozilla.test", SendFormat.AskUser);
> >    checkPopulate("[e-mail]", "mozilla.test", SendFormat.AskUser);
> >    checkPopulate("", "mozilla.test", SendFormat.AskUser);
> > +  checkPopulate("", "mozilla.test",
> > +                aAllowConversion ? SendFormat.PlainText : SendFormat.AskUser,
> 
> better to use aAutoDowngrade, see above
Ok, I'll rename it.

Comment 79

[:aceman]

Attachment: tests v1.2

Updated the test.

Comment 80

[Thomas D. (:thomas8)]

Comment on attachment 8694395 [details] [diff] [review]
tests v1.2

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

OK, from cursory reading, this looks correct. Could polish comments. Thanks.

::: mailnews/compose/test/unit/test_nsMsgCompose4.js
@@ +48,5 @@
>  }
>  
> +/**
> + * Return a send format directly from the preference of the recipient.
> + * The function assumes the message is NOT convertible and conversion is NOT allowed.

More precisely:

Return a send format directly from unknown_prefers pref for message recipient(s) exclusively of type prefers-unknown. The function also assumes that the message is NOT convertible OR Auto-Downgrade is FALSE, and that mail.default_html_action pref is set to SendFormat.AskUser.

@@ +50,5 @@
> +/**
> + * Return a send format directly from the preference of the recipient.
> + * The function assumes the message is NOT convertible and conversion is NOT allowed.
> + *
> + * @param aUnknownPrefers  The nsIAbPreferMailFormat to use for unknown recipient.

Like in the other function, pls make pref explicit:

The nsIAbPreferMailFormat to use for unknown recipient as per pref (mailnews.sendformat.unknown_prefers).

@@ +115,1 @@
>    Services.prefs.setIntPref("mail.default_html_action", SendFormat.AskUser);

It might be surprising why we use this outdated SendFormat for testing, so why not explain ourselves a little as you explained to me?

For cases involving any prefers-plaintext recipient, we want to test if Send Option from mail.default_html_action pref is actually honored, so we set the pref to SendFormat.AskUser which can only occur from reading the pref.

@@ +124,5 @@
> +
> +/**
> + *
> + * @param aAllowDowngrade  The bool value of the pref (mailnews.sendformat.auto_downgrade)
> + *                         indicating if conversion to plain text is allowed .

Nit: no space before trailing dot.

@@ +126,5 @@
> + *
> + * @param aAllowDowngrade  The bool value of the pref (mailnews.sendformat.auto_downgrade)
> + *                         indicating if conversion to plain text is allowed .
> + * @param aUnknownPrefers  The value of the pref (mailnews.sendformat.unknown_prefers)
> + *                         indicating what format is prefered by unknown recipient.

spelling: preferred

@@ +133,5 @@
> +  // test1 .. test3 have unknown format preference
> +  // test4 prefers plain text
> +  // test5 prefers HTML
> +  // TestList1 contains test1 .. test3
> +  // TestList2 contains test4

Is TestList2 different from ListTest2 used below (and others similar)?

@@ +203,3 @@
>                  SendFormat.AskUser);
>  
> +  checkPopulate("TestList3 <TestList3>, ListTest1 <ListTest1>", "",

I have no idea what ListTest1 contains (assuming it's different from TestList1 above, should this be documented somewhere?

Comment 81

[:aceman]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #80)
> Is TestList2 different from ListTest2 used below (and others similar)?
> 
> @@ +203,3 @@
> >                  SendFormat.AskUser);
> >  
> > +  checkPopulate("TestList3 <TestList3>, ListTest1 <ListTest1>", "",
> 
> I have no idea what ListTest1 contains (assuming it's different from
> TestList1 above, should this be documented somewhere?

Yes I'll add ListTest* contents too.

Comment 82

[Jorg K (CEST = GMT+2)]

Hey guys, I said it before: There shouldn't be a colon at the end of:
For messages composed in HTML format, configure how they will be sent: <----
Am I right?

Comment 83

[Jorg K (CEST = GMT+2)]

Perhaps I'm wrong, there are other occasions where there is a label with a colon and then a button follows. I got confused by the large space between the label (with colon) and the button.

Comment 84

[:aceman]

Attachment: tests v1.3

OK.

Comment 85

[Magnus Melin [:mkmelin]]

I'm sorry but I'm going to have to mark this wontfix. 
While mapping unknown to something known might sound like a "why not", this significantly shifts the real sending format away from being message centric to being determined by the recipient pref, a pref which really is not a user pref any more which is rather twisted. While it seems I haven't convinced people yet, I'm believe the user-centric prefs for send format should be ripped out and can't condone adding even more prefs to navigate around all the unknowns it really creates.

Not to mention using this pref requires the user to know which users he has which prefs set for, both at the moment he sets the pref and at each send time. Not a reasonable expectation. 

Re prefers both, there really is no such thing...

Last but not least, we already do the right thing (send both if required) for unknown, so there's no reason to let the user mess with that.

Comment 86

[:aceman]

(In reply to Magnus Melin from comment #85)
> I'm sorry but I'm going to have to mark this wontfix. 
> While mapping unknown to something known might sound like a "why not", this
> significantly shifts the real sending format away from being message centric
> to being determined by the recipient pref, a pref which really is not a user
> pref any more which is rather twisted. While it seems I haven't convinced
> people yet, I'm believe the user-centric prefs for send format should be
> ripped out and can't condone adding even more prefs to navigate around all
> the unknowns it really creates.
No, I think that the combination of user-prefs and message-compose-format determining the final send format was good to have and allows everybody to get the behaviour he wants.

> Not to mention using this pref requires the user to know which users he has
> which prefs set for, both at the moment he sets the pref and at each send
> time. Not a reasonable expectation. 
Why? Usually you set prefs once so that later the app can do the right things automatically without you having to think about it each time.
 
> Re prefers both, there really is no such thing...
Why? I would want to receive both so I would be happy if people had to option to set that in their TB AB.

> Last but not least, we already do the right thing (send both if required)
> for unknown, so there's no reason to let the user mess with that.
And by default that would be still the case.
The feature just allowed people to send out HTML-only if they wished so.

So I can't agree with the dismissal here.
I think this is a well thought out feature (complementing the other bugs we are working on) and is now polished and tested well.

Comment 87

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #86)
> No, I think that the combination of user-prefs and message-compose-format
> determining the final send format was good to have and allows everybody to
> get the behaviour he wants.

Well I reject the idea that people could keep track of recipient prefs. All you could possibly think about is what kind of message you have in front of you.
 
> > Not to mention using this pref requires the user to know which users he has
> > which prefs set for, both at the moment he sets the pref and at each send
> > time. Not a reasonable expectation. 
> Why? Usually you set prefs once so that later the app can do the right
> things automatically without you having to think about it each time.

Just adding one recipient that's unknown/not remembered correctly would change the end result.
Not to mention many people probably don't understand it's the same message going out to everyone.

> > Re prefers both, there really is no such thing...
> Why? I would want to receive both so I would be happy if people had to
> option to set that in their TB AB.

Still you read a mail only in one format. There's no real need to get the data in several formats. 

> > Last but not least, we already do the right thing (send both if required)
> > for unknown, so there's no reason to let the user mess with that.
> And by default that would be still the case.
> The feature just allowed people to send out HTML-only if they wished so.

But we already added that pref.

Comment 88

[:aceman]

(In reply to Magnus Melin from comment #87)
> > > Re prefers both, there really is no such thing...
> > Why? I would want to receive both so I would be happy if people had to
> > option to set that in their TB AB.
> 
> Still you read a mail only in one format. There's no real need to get the
> data in several formats. 

No, you can read the same message on different clients where you may have different options. Think accessing one mailbox from a TB on PC and from a phone. You can you phone to quickly browse new msgs in plain text format. Then later download everything in full to your archive in TB on PC.

> > > Last but not least, we already do the right thing (send both if required)
> > > for unknown, so there's no reason to let the user mess with that.
> > And by default that would be still the case.
> > The feature just allowed people to send out HTML-only if they wished so.
> But we already added that pref.
A single pref for all cases (regardless of recipients)? That does not seem very flexible.

Also, I don't think gmail or Outlook have the feature to set preferences for each recipient in addressbook. We should look at this as a unique distinguishing feature and actually make more use of the data we have (in AB). Similar to the attachment notification. Let's not dumb down TB too much (it was already done when splitting from the suite). It is enough what FF is doing :)

Comment 89

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #88)
> No, you can read the same message on different clients where you may have
> different options. Think accessing one mailbox from a TB on PC and from a
> phone. You can you phone to quickly browse new msgs in plain text format.

But virtually every phone anybody uses can handle HTML. I haven't heard of any that even supports selecting format, and even then on the phone data overhead is the most important thing to keep small, which would count against your phone usage argument.

> Let's not dumb down TB too much

Indeed tb needs to be smart. That's exactly why users should be burdened with setting prefs of questionable and complex meaning one can't easily comprehend when thunderbird should just do the right thing.

Comment 90

[:aceman]

(In reply to Magnus Melin from comment #89)
> (In reply to :aceman from comment #88)
> > No, you can read the same message on different clients where you may have
> > different options. Think accessing one mailbox from a TB on PC and from a
> > phone. You can you phone to quickly browse new msgs in plain text format.
> 
> But virtually every phone anybody uses can handle HTML. I haven't heard of
> any that even supports selecting format, and even then on the phone data
> overhead is the most important thing to keep small, which would count
> against your phone usage argument.

In some of the bugs, somebody claimed to have seen a phone that only supports plain text.

> > Let's not dumb down TB too much
> 
> Indeed tb needs to be smart. That's exactly why users should be burdened
> with setting prefs of questionable and complex meaning one can't easily
> comprehend when thunderbird should just do the right thing.

Ok, so please finally provide your suggestion how TB will do the right thing. So far you let WADA and Thomas and me battle it out and then you rejceted some of the individual ideas. So what is your grand plan for all this? How to get all the wanted outcomes (for different TB users) with less options?

Comment 91

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #90)
> In some of the bugs, somebody claimed to have seen a phone that only
> supports plain text.

While I'm sure such phone has probably existed, it's all really far fetched... that the feature would be useful.

> Ok, so please finally provide your suggestion how TB will do the right
> thing. So far you let WADA and Thomas and me battle it out and then you
> rejceted some of the individual ideas. So what is your grand plan for all
> this? How to get all the wanted outcomes (for different TB users) with less
> options?

It's unfortunate people put work into this, of course. 
You're looking at this in a very technical way and want to eliminate an unknown - but we handle that unknown correctly as is. Outside of very narrow contexts you simply cannot get the per-user prefs to do what you want so the only solution to get the expected outcome in general is to ignore them.

Comment 92

[Jorg K (CEST = GMT+2)]

Comment on attachment 8693334 [details] [diff] [review]
Patch v.8: Implement triple pref for unknown=plain|html|both, auto-detect logic and send options UI. Implement nsIAbPreferMailFormat::both.

Looks like no further feedback is required here.