1169184 - Save spell checking language with draft message and restore/remember it when editing the draft (Implement Content-Language header)

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[Jorg K (CEST = GMT+2)]

This could be stored in the X-Mozilla-Draft-Info header.

Comment 1

[Thomas D. (:thomas8)]

Jorg, thanks! That's a great improvement and I'm totally failing to see why nobody has suggested this before...
Yes, the X-Mozilla-Draft-Info looks like the right place.
Patch would be very welcome.

Check out the patch for manual attachment reminder where we've added a similar flag for that feature.
Btw we took 170 comments and 28 patch revisions there but the result was very good! :)

Bug 521158, attachment 825270 [details] [diff] [review]

> attribute boolean attachmentReminder;
> ...
> m_attachmentReminder

Comment 2

[Thomas D. (:thomas8)]

This might eventually be scrapped when editor allows having multiple languages for spell-check in a single document, but until then (whenever...), this is a fine improvement and easy to roll back.

Comment 3

[Magnus Melin [:mkmelin]]

Actually, we should probably just set the lang attribute of document.documentElement

Comment 4

[Jorg K (CEST = GMT+2)]

Instead of storing this in X-Mozilla-Draft-Info it would be better to store this in an "official" header, like Content-Language as per https://tools.ietf.org/html/rfc3282.

Then this bug is really only a partial aspect of bug 1201836.

Comment 5

[Jorg K (CEST = GMT+2)]

Magnus, perhaps one day I should start on this one. Can we agree where to store the language?
I'd put it into a mail header Content-Language as per https://tools.ietf.org/html/rfc3282.

Comment 6

[Magnus Melin [:mkmelin]]

Unfortunately that header is pretty much unreliable since Outlook (for one) sends it out always but doesn't care about the actual language used in the content. 

Setting it on the document.documentElement (or some section) should be easier. It also makes the content semantically more correct and you could create sane behavior if you use multiple languages for different sections in one mail, which is not *that* uncommon.

Comment 7

[Jorg K (CEST = GMT+2)]

And plain text e-mail?

We also have bug 1169184: For a draft, we can store it in the draft info or a proper header. I still prefer a proper header, even if Outlook doesn't work.

I've just looked at some e-mail I got from Catalonia and there I see:
Content-Language: ca-ES (Catalan)
That was sent with MS software.

And my German Outlook users send me:
Content-Language: de-DE

There is RFC 3282. So why are you so against a header? And if so, how can we come to an agreement here?

It can still go into the HTML, but as you know, M-C gives us the HTML of the document (editor DOM to text serialisation) and that typically doesn't include language information although we do set "lang" on document.documentElement for spell checking purposes. So you'd have to fiddle with the HTML received which I rather not do.

Comment 8

[Magnus Melin [:mkmelin]]

Well try the same German user sending English content...

I'm not that much against setting the header when we're reasonably sure (say just a few misspellings). I just think it's superfluous because of misuse elsewhere, in combination with all the cases with mixed language where you still need the content marked up inline. I don't think we should e.g. label a complete message English if it's a reply to a German content mail and you still have the quote there. Basically, a can of worms in the details, and all solvable by putting the lang property inline (except the plain text case). I don't know why the serializer wouldn't output lang if it's set in the document.

Comment 9

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #8)
> Well try the same German user sending English content...
Yes, as I said in my post to [e-mail], Outlook seems to send the language of the installation and not the content of the e-mail.

Be that as it may, we will do it right.

> Basically, a can of worms in the details, and
> all solvable by putting the lang property inline (except the plain text
> case).
That's right. It's no solution for the plain text case.

> I don't know why the serializer wouldn't output lang if it's set in
> the document.
It just doesn't. Just check any draft or use ThunderHTMLedit to see the HTML returned from the M-C editor.

In comment #0 I suggested to use the X-Mozilla-Draft-Info header (like delivery format, DNS, etc.) but Content-Language seems to be the correct place.

While the M-C editor doesn't support multi-languages, we have one spelling language and I think we should ship that out. There's also bug 1201836 where that would fit. As I said in my post, whether you use the received language as the default reply language is another story.

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: WIP: (v1).

This works.
New message stores the language used.
Editing a draft restores the language, if changed, draft gets updated.

Note, this is quite different to Suyash's saving the delivery format (bug 1202165) since it creates a new header (Content-Language) instead of modifying an existing one (X-Mozilla-Draft-Info).

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 8771014 [details] [diff] [review]
WIP: (v1).

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

Sorry about the tailing spaces. I'll fix them later.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2135,5 @@
>      return prefValue;
>    }
>  
> +  // Make sure draft language contains a valid value.
> +  if (dictList.includes(draftLanguage)) {

Better:
if (draftLanguage && dictList.includes(draftLanguage) {

Comment 12

[:aceman]

Comment on attachment 8771014 [details] [diff] [review]
WIP: (v1).

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

This seems to work for me for new messages.

Also if an incoming message contains the Content-language header and we edit it as new, will we pick up the value (if it is incorrect, the user will fix it if he cares for spell checking.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2262,5 @@
>    gLanguageObserver = new MutationObserver(function(mutations) {
>      mutations.forEach(function(mutation) {
>        if (mutation.type == "attributes" && mutation.attributeName == "lang") {
>          updateLanguageInStatusBar();
> +        

whitespace.

@@ +2410,5 @@
> +    draftLanguage = gMsgCompose.compFields.contentLanguage;
> +  }
> +
> +  let languageToSet = getValidSpellcheckerDictionary(draftLanguage);
> +  document.documentElement.setAttribute("lang", languageToSet);

So we now have both the new header and the 'lang' attribute with the same value?

@@ +2411,5 @@
> +  }
> +
> +  let languageToSet = getValidSpellcheckerDictionary(draftLanguage);
> +  document.documentElement.setAttribute("lang", languageToSet);
> +  

Trailing whitespace.

::: mailnews/compose/src/nsMsgCompFields.cpp
@@ +436,5 @@
> +  return SetAsciiHeader(MSG_CONTENT_LANGUAGE_ID, value);
> +}
> +
> +NS_IMETHODIMP nsMsgCompFields::GetContentLanguage(char **_retval)
> +{

NS_ENSURE_ARG_POINTER(_retval) here.

::: mailnews/mime/src/mimedrft.cpp
@@ +1114,5 @@
>    char *grps = 0;
>    char *foll = 0;
>    char *priority = 0;
>    char *draftInfo = 0;
> +  char *contentLanguage = 0;

I see you keep the style, but is this meant to be "" ? Or that does not work?

Comment 13

[Jorg K (CEST = GMT+2)]

Thanks for looking at this quickly.

(In reply to :aceman from comment #12)
> Also if an incoming message contains the Content-language header and we edit
> it as new, will we pick up the value (if it is incorrect, the user will fix
> it if he cares for spell checking.
Well, I only wanted to keep the language for our own messages, thus the
+  if (gMsgCompose.compFields.deliveryFormat && gMsgCompose.compFields.contentLanguage) {
+    draftLanguage = gMsgCompose.compFields.contentLanguage;
+  }
but we know from bug 1254666 that this doesn't work. I don't really want to pick up the language from "foreign" messages since, as Magnus pointed out various times, the information contained in the header is unreliable.

> > +  let languageToSet = getValidSpellcheckerDictionary(draftLanguage);
> > +  document.documentElement.setAttribute("lang", languageToSet);
> So we now have both the new header and the 'lang' attribute with the same
> value?
Yes. The draft language from the header in the draft and we set it here for the composition.

> ::: mailnews/mime/src/mimedrft.cpp
> @@ +1114,5 @@
> >    char *grps = 0;
> >    char *foll = 0;
> >    char *priority = 0;
> >    char *draftInfo = 0;
> > +  char *contentLanguage = 0;
> I see you keep the style, but is this meant to be "" ? Or that does not work?
No, it's meant to be 0 or nullptr. If zero, the header doesn't exist.

I'll fix it up and make it ready for review. I'll also do a test.

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1a).

OK, I addressed the feedback issues. This is based on bug 1287268 since it uses "isOwnDraftOrTemplate".

Do you want a test? I realised that a test is difficult to do, since the test environment comes with one dictionary only, the en-US one. So I would have to create and install a dictionary on the fly. Possible, but extra work. I've done it before during my M-C work, for example here:
https://dxr.mozilla.org/comm-central/source/mozilla/editor/composer/test/test_bug1200533.html#72

Comment 15

[:aceman]

I think you could at least update one of the existing draft tests to check if the Content-Language was even emitted into the saved draft with the en-US value. Yes, checking if reading the value back would be harder.

Comment 16

[Jorg K (CEST = GMT+2)]

OK, will do.

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: Simple test for the presense of the Content-Language header (v1a).

Comment 18

[Jorg K (CEST = GMT+2)]

Damn, I forgot to remove the dump().

Comment 19

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1a) not depending on bug 1287268

Here is the same solution with the dependency on bug 1287268 removed.
The language will only be restored when editing the draft.
I will reintroduce the concept of "own" message when I rebase bug 1287268.

Maybe this approach will speed up the review and landing.

Comment 20

[:aceman]

Comment on attachment 8771965 [details] [diff] [review]
Simple test for the presense of the Content-Language header (v1a).

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

Works for me.

::: mail/test/mozmill/composition/test-drafts.js
@@ +259,5 @@
> +  let draftMsg = select_click_row(0);
> +
> +  let draftMsgContent = getMsgSource(draftMsg);
> +
> +  dump(draftMsgContent);

Why so many blank lines? And remove the dump() :)

@@ +261,5 @@
> +  let draftMsgContent = getMsgSource(draftMsg);
> +
> +  dump(draftMsgContent);
> +
> +  if (!draftMsgContent.includes("Content-Language: en-US")) {

Would it be possible to check for "\nContent-Language: en-US\n" to not hit some accidental false positive?

Comment 21

[:aceman]

Comment on attachment 8772114 [details] [diff] [review]
Proposed solution (v1a) not depending on bug 1287268

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

Thanks, works for me.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2402,5 @@
>    SelectDeliveryFormatMenuOption(gSendFormat);
>  
> +  // Set document language to the draft language or the preference.
> +  let draftLanguage = null;
> +  if (gComposeType == nsIMsgCompType.Draft && gMsgCompose.compFields.contentLanguage) {

OK, so do not forget to change this place in bug 1287268.

Comment 22

[Jorg K (CEST = GMT+2)]

Comment on attachment 8771696 [details] [diff] [review]
Proposed solution (v1a).

Since we're still discussing in bug 1287268, let's have the other patch reviewed.

Comment 23

[Jorg K (CEST = GMT+2)]

Thanks for the review.

(In reply to :aceman from comment #21)
> OK, so do not forget to change this place in bug 1287268.
I promise I won't ;-)

I'll land it with the nits in the test fixed once the trees open again.

Comment 24

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #20)
> > +  if (!draftMsgContent.includes("Content-Language: en-US")) {
> Would it be possible to check for "\nContent-Language: en-US\n" to not hit
> some accidental false positive?
I tried and it didn't work. The mail usually has \r\n line endings. But it might depend on the platform, so I prefer not to add complication here. The chances for an "accidental false positive" are slim ;-)

Comment 25

[:aceman]

Maybe draftMsgContent.split("\n").some(line => (line.trim() == "Content-Language: en-US")) could work :)

Comment 26

[Jorg K (CEST = GMT+2)]

No wonder our tests run forever with such complicated and unnecessary processing. "Content-Language: en-US" won't drop from the sky anywhere else in the message. IMHO it would be better to do a real test with a second dictionary, etc.

Comment 27

[Jorg K (CEST = GMT+2)]

Attachment: Simple test for the presense of the Content-Language header (v1b).

Removed empty lines and dump(), added Aceman's trickery.

Comment 28

[Jorg K (CEST = GMT+2)]

Since we're adding a new header, here's a try run to see whether anything breaks:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3ae756076b20

Comment 29

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/5855f51dead5 (code)
https://hg.mozilla.org/comm-central/rev/5fdf4efb7ad1 (test)

Comment 30

[Jorg K (CEST = GMT+2)]

This may be of interest to SM. As far as I know, you don't have a language indicator in the status bar, so the implementation would be different since there is MutationObserver:
https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2257

I've just noticed that there hasn't been any action on bug 1279055 either.

Comment 31

[rsx11m]

Thanks. I've filed bug 1289493, but this doesn't look like a straight port of the patch here.

Comment 32

[Frank-Rainer Grahl (:frg)]

Jorg,

now that SeaMonkey 2.47 is in comm-release a few users already use unofficial builds. SeaMonkey now inserts an empty language-content field into the header which causes problems with some servers. 

Bug 1289493 is still open but not sure if the check in mailnews/compose/src/nsMsgCompUtils.cpp is correct in all cases.

>  char* contentLanguage = nullptr;
>  fields->GetContentLanguage(&contentLanguage);
>  if (contentLanguage) {

I didn't debug this but if I am right I assume we get an empty string back and this is not covered by the check so it sets an empty header. If you think this might be the problem I would file a followup bug.

Comment 33

[Jorg K (CEST = GMT+2)]

Well, we don't produce empty Content-Language: headers so we don't need to deal with empty headers.

That said, changing this to
  if (contentLanguage && *contentLanguage)
is no problem.

Why is setting an empty compose field a problem? This code runs when opening a saved draft.

To avoid the empty header being written we should this line:
  mCompFields->SetContentLanguage(fields->GetContentLanguage());
That should get the content language from 'fields', test it, and if set, call the set on mCompFields. That's also useful if no dictionary is installed.

If you can wait until tonight, I can do a bug and a patch. Otherwise the bug should be:
  Empty Content-Language: header produced when no dictionary is installed.

Comment 34

[Frank-Rainer Grahl (:frg)]

>>
If you can wait until tonight, I can do a bug and a patch. Otherwise the bug should be:
  Empty Content-Language: header produced when no dictionary is installed.
<<

Bwfore I mangle something with my less than stellar c++ knowledge yes please :) Just put me in cc and I will test it as soon as it is ready.