882104 - Command line option to read message body from file

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, enhancement)

Description

[Tristan Miller]

According to <http://kb.mozillazine.org/Command_line_arguments_%28Thunderbird%29> it is possible to use the command line to instruct Thunderbird to open a new composer window and to pre-fill certain fields, such as the To: and Subject: headers and the message body.

However, for long messages it is not convenient to specify the entire message body on the command line.  Bug 258887 makes it even less convenient.

Other e-mail clients such as KMail have a command-line argument which instructs the composer to read the message body from a file:

$ kmail --help
[...]
Options:
  -s, --subject <subject>   Set subject of message
  -c, --cc <address>        Send CC: to 'address'
  -b, --bcc <address>       Send BCC: to 'address'
  -h, --header <header>     Add 'header' to message
  --msg <file>              Read message body from 'file'
  --body <text>             Set body of message
  --attach <url>            Add an attachment to the mail. This can be repeated

Please consider implementing such a command-line argument for Thunderbird and SeaMonkey.  This will make it much easier to use them to send personalized form letters.

Comment 1

[Magnus Melin [:mkmelin]]

As a workaround you could probably make a script that reads the file and puts it's content in the body and passes it to tb.

Comment 2

[Tristan Miller]

Yes, though as I said, it's not convenient.  In fact, it might not even be possible.  To get around Bug 258887 you need to %-encode all the commas.  And if you want to send a long message you might end up hitting your shell's limit for command line length.

Comment 3

[Manuel Grießmayr]

Attachment: message.patch

Comment 4

[Manuel Grießmayr]

Hey guys, just added a patch to add this feature. I commented out line 4548 and the following line at nsMsgCompose.cpp. I am not sure if this is really needed. At least for the file loading in our case it doesn't make sense. Can anybody please review. Many thanks :)

I also had the idea to add the field "from=" to the command line option "-compose". So the user can specify the from email address. At the moment he can only set the id with "preselectid=" which is also not that convenient.

Another little improvement would a better explanation of the "-compose" option when we type "thunderbird -h". There is no word how the string after "-compose" should look like. I had to do an Internet search and found this page: http://kb.mozillazine.org/Command_line_arguments_(Thunderbird).

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 8785992 [details] [diff] [review]
message.patch

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2382,5 @@
> +	 promise = promise.then(
> +	   function onSuccess(array) {
> +	     composeFields.body = decoder.decode(array);        // Convert this array to a text
> +	   }
> +	 );

This block could be simplified to 

OS.File.read(args.message, { encoding: "utf-8" })
  .then(function onSuccess(text) {
      composeFields.body = text
    }
  );
);

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +4545,5 @@
>    // if type is new, but we have body, this is probably a mapi send, so we need to
>    // replace '\n' with <br> so that the line breaks won't be lost by html.
>    // if mailtourl, do the same.
> +  //if (m_composeHTML && (mType == nsIMsgCompType::New || mType == nsIMsgCompType::MailToUrl))
> +  //  MsgReplaceSubstring(body, NS_LITERAL_STRING("\n"), NS_LITERAL_STRING("<br>"));

you'll have to figure out what to do here, instead of just commenting it out

::: mailnews/compose/src/nsMsgComposeService.cpp
@@ +1435,5 @@
>        arg->SetData(uristr);
>  
>      nsCOMPtr<mozIDOMWindowProxy> opened;
>      wwatch->OpenWindow(nullptr, DEFAULT_CHROME, "_blank",
> +      "chrome,dialog=no,all", arg, getter_AddRefs(opened));

please don't change this. Both styles are ok...

Comment 6

[Manuel Grießmayr]

Attachment: message.patch

Comment 7

[Manuel Grießmayr]

In the latest patch I assume that the user has a HTML file as input when the field and value "format=1" is available. See MsgComposeCommands.js. Is that OK?

Comment 8

[Manuel Grießmayr]

Well... when I think about it I assume it is better to test for ".htm" and ".html" only. Maybe a user wants to specify a .txt file but also wants to load it for a HTML email message.

Comment 9

[Magnus Melin [:mkmelin]]

Please make sure to get the patch reviewed - https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_reviewed

(You can set the review flag to my email address)

Comment 10

[Manuel Grießmayr]

Comment on attachment 8786135 [details] [diff] [review]
message.patch

Please also see my comments 7 and 8.

Comment 11

[Manuel Grießmayr]

Attachment: message.patch

Improved performance a little bit with the latest patch. No need to remove line breaks in HTML input anymore.

Comment 12

[Manuel Grießmayr]

I have not done any automated tests or Litmus tests. If I should do so please let me know.

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 8786752 [details] [diff] [review]
message.patch

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2381,5 @@
> +          .then(function onSuccess(text) {
> +              if (args.message.endsWith(".htm") || args.message.endsWith(".html"))
> +                composeFields.bodyIsHTML = true;
> +              composeFields.body = text;
> +          }

gah, since OS.File functions are async it's pure luck if this happens to work in some cases. By the time the file is read it's quite possible the compose window is already set up and such... 

I suppose you have the options of reading syncrounously (see "Reading a file" - https://developer.mozilla.org/en-US/Add-ons/Code_snippets/File_I_O). Or perhaps things can be re-factored...

Comment 14

[Manuel Grießmayr]

Attachment: message.patch

Okay, I used now the synchronous approach as listed in the example. But I am not sure if this now prevents the race condition.

Comment 15

[Manuel Grießmayr]

Maybe there is a better solution:
We could load the file and compose the new email window in parallel. When both threads are finished we could insert the content of the file into the window as body text. Just an idea... Not sure if I have the skills for this. Probably I would need some guidance/advice.

Comment 16

[aleth [:aleth]]

(In reply to Manuel Grießmayr from comment #15)
> Maybe there is a better solution:
> We could load the file and compose the new email window in parallel. When
> both threads are finished we could insert the content of the file into the
> window as body text. Just an idea... Not sure if I have the skills for this.
> Probably I would need some guidance/advice.

I don't know this code well, but I don't think adding synchronous file I/O during startup is a good way to handle this, especially if there are errors along the way. Can't you keep the OS.File approach and fill in the required fields once the file load is completed? If you want to ensure this happens after the compose window is "ready", and if initializing the compose window is itself async, you can use e.g. Promise.all() to wait on both the file load completion and the composer initialization. (see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise)

Comment 17

[aleth [:aleth]]

Comment on attachment 8789431 [details] [diff] [review]
message.patch

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

Requesting f? from the compose window expert ;)

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 8789431 [details] [diff] [review]
message.patch

This has been through various review loops with Magnus and is nearing completion. I know nothing about command line options, which seem to be mainly a Linux thing since Windows/Mac users usually don't use the command line to send e-mail. From what I can see it looks reasonable, but I'm not getting involved at this late stage. My feedback: Remove trailing spaces ;-)

Comment 19

[aleth [:aleth]]

The command line handler is in https://dxr.mozilla.org/comm-central/source/mail/components/nsMailDefaultHandler.js#435, see also e.g. https://dxr.mozilla.org/comm-central/source/mail/components/nsMailDefaultHandler.js#412.

Another possibility might be to do the file read in that file, and open the compose window in the .then().

Comment 20

[aleth [:aleth]]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #18)
> This has been through various review loops with Magnus and is nearing
> completion.

Sure, none of my comments are intended to supersede mkmelin's review. Manuel just asked for some feedback on #maildev. My main feedback would be that sync or async, the error handling for things happening during startup should be rock solid.

Comment 21

[Magnus Melin [:mkmelin]]

Comment on attachment 8789431 [details] [diff] [review]
message.patch

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2376,5 @@
>        }
>        if (args.newshost)
>          composeFields.newshost = args.newshost;
> +      if (args.message) {
> +        let msg_file = new FileUtils.File(args.message);

we dont use snake_naming for variables. camelCase is most common

@@ +2381,5 @@
> +        if (msg_file.exists()) {
> +          let fstream = Components.classes["@mozilla.org/network/file-input-stream;1"].
> +                        createInstance(Components.interfaces.nsIFileInputStream);
> +          let cstream = Components.classes["@mozilla.org/intl/converter-input-stream;1"].
> +                        createInstance(Components.interfaces.nsIConverterInputStream);

please put the dot on the next line:
  .createInstance

(yes, you just copied the snippet)

@@ +2393,5 @@
> +            read = cstream.readString(0xffffffff, str); // read as much as we can and put it in str.value
> +            data += str.value;
> +          } while (read != 0);
> +          cstream.close(); // this closes fstream
> +          

please remove trailing space

::: mailnews/compose/src/nsMsgCompFields.h
@@ +158,5 @@
>    bool        m_useMultipartAlternative;
>    bool        m_returnReceipt;
>    bool        m_DSN;
>    bool        m_bodyIsAsciiOnly;
> +  bool        m_bodyIsHTML;

I would just drop all this and allow plain text only.
It's not like it really works now - you only use it to handle line breaks, but there's so much more html that could be in there...

Comment 22

[Magnus Melin [:mkmelin]]

aleth: async would have been nice, but, this is only run if that special argument was given, so people can wait that extra 50ms (or whatever it takes). Code would have been much nicer with OS.File but even with that figured out we would give you a compose window that might be loading content still. What if you already wrote something? It just seems over complicated...

Comment 23

[Manuel Grießmayr]

> +  bool        m_bodyIsHTML;

Magnus Melin, what do you mean with "there's so much more html there could be in there..."? Can you give an example? Are you speaking about signatures? For me the main reason I implemented this feature is because of HTML files. If the boolean variable is not fine I could back to the previous solution where I remvoved all line breaks.

> composeFields.body = text.replace(/(\r\n|\n|\r)/gm,"");

Comment 24

[Magnus Melin [:mkmelin]]

Yes, let's use the previous solution

Comment 25

[Manuel Grießmayr]

Attachment: message.patch

With the current solution the user can only specify a file with an absolute path. If there is an easy way to change this please let me know.

Comment 26

[Jorg K (CEST = GMT+2)]

Comment on attachment 8791991 [details] [diff] [review]
message.patch

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

Drive by comment, see below. Also, we don't attempt to send HTML messages any more? I'm confused, also by Magnus' comment "let's use the previous solution" referring to what exactly? You can actually refer to an attachment here via its number.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2398,5 @@
> +
> +          if (args.message.endsWith(".htm") || args.message.endsWith(".html")) {
> +            /* we remove line breaks because otherwise they'll be converted to
> +            <br> in nsMsgCompose::BuildBodyMessageAndSignature() */
> +            data = data.replace(/\r?\n|\r/g,"");

Don't you need to replace with a single space or else you glue words together.

Comment 27

[Jorg K (CEST = GMT+2)]

Magnus, your one liners are causing us all headaches.

1)
> I would just drop all this and allow plain text only. It's not like it really works now - ...
Doesn't sending a HTML file as HTML message work?

> you only use it to handle line breaks, but there's so much more html that could be in there...
Please clarify what you're referring to.

2)
> Yes, let's use the previous solution
??

Comment 28

[Manuel Grießmayr]

Attachment: message.patch

Funny that I haven't seen this. Jorg K, you are right. I haven't noticed this because in my test HTML file everything was right. Added space now:

@@ -2370,18 +2371,47 @@ function ComposeStartup(aParams)
> data = data.replace(/\r?\n|\r/g," ");

Comment 29

[Manuel Grießmayr]

Jorg K., regarding you questions:
1) Of course we can send an HTML file as an HTML message. But the question is if the HTML code is loaded correctly into the body. In my case I had little problems with the line breaks. Magnus didn't like my solution so he proposed to load only simple plain text files.

2) The previous solution is the one you quoted:
> data = data.replace(/\r?\n|\r/g," ");
I replaced this with a boolean as you can see in patch 8786752 so I could write this:

+++ b/mailnews/compose/src/nsMsgCompose.cpp
@@ -4540,17 +4540,19 @@ nsMsgCompose::BuildBodyMessageAndSignatu
> +  bool isHTMLCode;
> +  m_compFields->GetBodyIsHTML(&isHTMLCode);
> +  if (m_composeHTML && (mType == nsIMsgCompType::New || mType == nsIMsgCompType::MailToUrl) && !isHTMLCode)
>     MsgReplaceSubstring(body, NS_LITERAL_STRING("\n"), NS_LITERAL_STRING("<br>"));

I don't understand why he didn't like it. So the answer from Magnus to your first question would be interesting for me too.

Comment 30

[aleth [:aleth]]

Comment on attachment 8792058 [details] [diff] [review]
message.patch

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +19,5 @@
>  Components.utils.import("resource://gre/modules/PluralForm.jsm");
>  Components.utils.import("resource://gre/modules/Services.jsm");
>  Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
>  Components.utils.import("resource://gre/modules/AppConstants.jsm");
> +Components.utils.import("resource://gre/modules/FileUtils.jsm");

You could use a lazy getter here (see e.g. https://dxr.mozilla.org/comm-central/source/chat/protocols/xmpp/xmpp.jsm#25) as in most cases this module won't be used.

@@ +2376,5 @@
>        }
>        if (args.newshost)
>          composeFields.newshost = args.newshost;
> +      if (args.message) {
> +        let msgFile = new FileUtils.File(args.message);

This assumes args.message is an absolute path, you probably want to use the CurProcD directory key. Alternatively, the command line handler probably has the path somewhere.

@@ +2393,5 @@
> +            // read as much as we can and put it in str.value
> +            read = cstream.readString(0xffffffff, str);
> +            data += str.value;
> +          } while (read != 0);
> +          cstream.close(); // this closes fstream

I hate to repeat myself, but what happens if there is a file I/O error? You just seem to be assuming everything works OK.

Comment 31

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #30)
> I hate to repeat myself, but what happens if there is a file I/O error? You
> just seem to be assuming everything works OK.

Here's an example for error handling using the same API
https://dxr.mozilla.org/comm-central/source/mozilla/toolkit/mozapps/extensions/nsBlocklistService.js#814

Comment 32

[Manuel Grießmayr]

(In reply to aleth [:aleth] from comment #30)

> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +19,5 @@
> > +Components.utils.import("resource://gre/modules/FileUtils.jsm");
> 
> You could use a lazy getter here (see e.g.
> https://dxr.mozilla.org/comm-central/source/chat/protocols/xmpp/xmpp.jsm#25)
> as in most cases this module won't be used.

This would be also lazy loading I assume. Correct me if I am wrong:
> var file = Components.classes["@mozilla.org/file/local;1"].
>           createInstance(Components.interfaces.nsILocalFile);
> file.initWithPath("/home");


> @@ +2376,5 @@
> >        }
> >        if (args.newshost)
> >          composeFields.newshost = args.newshost;
> > +      if (args.message) {
> > +        let msgFile = new FileUtils.File(args.message);
> 
> This assumes args.message is an absolute path, you probably want to use the
> CurProcD directory key. Alternatively, the command line handler probably has
> the path somewhere.

Hmm... even when I have the current working directory of the command line, how do I know if the user specified a relative or an absolute path? Do I have to figure out that myself and also respect different OSes with different path syntax? So parsing the path myself?
By the way also the attachment='' field of the -compose option on the command line does need an absolute path. Not really convenient of course.

Comment 33

[aleth [:aleth]]

(In reply to Manuel Grießmayr from comment #32)
> This would be also lazy loading I assume. Correct me if I am wrong:
> > var file = Components.classes["@mozilla.org/file/local;1"].
> >           createInstance(Components.interfaces.nsILocalFile);
> > file.initWithPath("/home");

Yes, something like that is fine too. That's not lazy loading though, it just avoids using the FileUtils module at all ;) 

> > This assumes args.message is an absolute path, you probably want to use the
> > CurProcD directory key. Alternatively, the command line handler probably has
> > the path somewhere.
> 
> Hmm... even when I have the current working directory of the command line,
> how do I know if the user specified a relative or an absolute path? 
> Do I have to figure out that myself and also respect different OSes with
> different path syntax? So parsing the path myself?
> By the way also the attachment='' field of the -compose option on the
> command line does need an absolute path. Not really convenient of course.

Parsing paths is an OS-dependent nightmare, you definitely want to use the existing libraries (e.g. OS.Path) for that.

I guess you could add which is required (relative or absolute) to the command line --help message? I don't know of a good way to tell if the path is relative or not, maybe checking if OS.Path.dirname == "." would work, I'm not sure.

Comment 34

[Manuel Grießmayr]

Attachment: message.patch

Thank you for your help and feedback Aleth :)

Are you fine with the logging regarding errors when the message file is loaded?
I now use the sync and the async functions during file load. Not sure if this is fine.

Comment 35

[Manuel Grießmayr]

I mean the async file functions are used to determine the absoulte path of the message file and the sync file functions are used for the loading of the message file.

Comment 36

[Magnus Melin [:mkmelin]]

Comment on attachment 8792198 [details] [diff] [review]
message.patch

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

You resolve paths using https://dxr.mozilla.org/mozilla-central/source/toolkit/components/commandlines/nsICommandLine.idl#130

::: mail/components/compose/content/MsgComposeCommands.js
@@ +22,5 @@
>  Components.utils.import("resource://gre/modules/AppConstants.jsm");
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "OS", "resource://gre/modules/osfile.jsm");
> +XPCOMUtils.defineLazyServiceGetter(this, "gConsole", "@mozilla.org/consoleservice;1"
> +                                   ,"nsIConsoleService");

comma on previous line please

@@ +2418,5 @@
> +            } while (read != 0);
> +          } catch (e) {
> +            let msg = "Error: MsgComposeCommands.js:ComposeStartup(): Failed to load message file " + e + "\n";
> +            dump(msg);
> +            gConsole.logStringMessage(msg);

Error handling for missing files and such is usually an alert. Just printing it in console will not be noticed by users in general. (And the message must be localizable.)

@@ +2429,5 @@
> +
> +          if (data) {
> +            if (args.message.endsWith(".htm") || args.message.endsWith(".html")) {
> +              /* we replace line breaks because otherwise they'll be converted to
> +              <br> in nsMsgCompose::BuildBodyMessageAndSignature() */

nit: // style comments please. And capitalize sentences + add period.

Comment 37

[Jorg K (CEST = GMT+2)]

Attachment: message.patch (JK v1).

I got a little frustrated with this bug since no one answered my questions and I wanted to know whether send a HTML file works. So I tried it myself. While I was there, I addressed the review comments. If you want to see what I've done, use interdiff.

For the record, my command line:
thunderbird.exe -p testing -no-remote -compose "to=[e-mail],subject=dinner,message=d:\desktop\bugzilla-down.html"

Now one should think, everyone is happy, but not so. The alert doesn't come out at all if the file doesn't exist. Even without the patch specifying a non-existent attachment results in:

JavaScript error: file:///C:/mozilla-source/comm-central/obj-i686-pc-mingw32/dist/bin/components/nsPrompter.js, line 351: NS_ERROR_NOT_AVAILABLE: Cannot call openModalWindow on a hidden window

But that mat be subject to another bug :-(

Comment 38

[Jorg K (CEST = GMT+2)]

Attachment: message.patch (JK v2).

(Oops, bundle was wrong.)

Comment 39

[Jorg K (CEST = GMT+2)]

Attachment: message.patch (JK v3).

Fixed the alert problem. The first argument needs to be null.
I tried it with a non-existent file, works fine, and I also withdrew access to the file using Windows access control, and the second error, "can't load" is displayed. So who could ask for more ;-)

Comment 40

[Jorg K (CEST = GMT+2)]

Attachment: message.patch (v4).

Oh man, there was a tab in the patch. We hate tabs around here.

Comment 41

[Manuel Grießmayr]

(In reply to Jorg K (GMT+2, never had any PTO during summer) from comment #37)

> I got a little frustrated with this bug since no one answered my questions
> and I wanted to know whether send a HTML file works.

At least I tried to answer your questions. Thanks for your help by the way.

Comment 42

[Jorg K (CEST = GMT+2)]

(In reply to Manuel Grießmayr from comment #41)
> At least I tried to answer your questions.
Oh, yes, you did in comment #29. I guess the C++ changes to the comp fields, the m_compFields->GetBodyIsHTML(&isHTMLCode) etc., weren't necessary since you can do the replacement in JS as you do now. Sorry, I didn't see that you had already done so in a previous patch which Magnus referred to as the "previous" solution. Sorry to be impatient/tired and not read your comment properly. In my defence I'd like to mention that your English derailed here:
> In my case I had little problems with the line breaks.
I had little problems = I had close to no problem (Ich hatte kein/wenig Problem/e mit ...). You meant to say:
I had *a* little problem (Ich hatte *ein* kleines Problem) ;-)

Re. the HTML vs. text: I can load a HTML file alright, I haven't tried a text file. BTW, the "documentation" here
http://kb.mozillazine.org/Command_line_arguments_%28Thunderbird%29
mentions a format parameter. Have you tried that?

> Thanks for your help by the way.
You're welcome. I hope it lands one day without too many more loops ;-)

Comment 43

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #21)
> It's not like it really works now - you only use it to handle line breaks,
> but there's so much more html that could be in there...
The thing with Magnus is that he is mostly right with his one liners, only it takes us weeks to work this out ;-)
Try sending this:

<html><body>
<div style="background-color:#eee;border:1px solid #000;padding:10px; height:50px;" contenteditable="">
<p style="background:red;"><font color=green><tt>xyz</tt></font><br></p>
</div>
<div style="background-color:#eee;border:1px solid #000;padding:10px; height:50px;" contenteditable="">
<p style="background:red;">xyz<br></p>
</div>
</body></html>

It looks bad because we add a <p> and position into the first <div>. It works OK if you insert a text before the first <div>.

I experimented a little to ...
1) ... see what the format parameter does:
   format=1 (default) gives a HTML compose window, format=2 gives a plain text one.
   That appears to work.
2) ... see what a plain text file does.
   It gets included into a HTML editor unless you set format=2.

I'm asking myself whether we should manipulate gMsgCompose.composeHTML, but give the above, it's apparently not necessary.

So I think it's all good ;-)

Comment 44

[Jorg K (CEST = GMT+2)]

Comment on attachment 8792252 [details] [diff] [review]
message.patch (v4).

(In reply to Jorg K (GMT+2, never had any PTO during summer) from comment #43)
> So I think it's all good ;-)
No, it's not. When format==2, you must not replace the line endings. Can you change that?

Comment 45

[Jorg K (CEST = GMT+2)]

Attachment: message.patch (JK v5).

This should do it.

Comment 46

[Jorg K (CEST = GMT+2)]

Attachment: message.patch (JK v6).

(accidentally lost newline in previous patch.)

Comment 47

[Manuel Grießmayr]

(In reply to Jorg K (GMT+2, never had any PTO during summer) from comment #42)
In my defence I'd like to mention that your English derailed here:
> > In my case I had little problems with the line breaks.

Ooops, sorry...just one little character :)

Comment 48

[Manuel Grießmayr]

(In reply to Magnus Melin from comment #36)

> You resolve paths using
> https://dxr.mozilla.org/mozilla-central/source/toolkit/components/
> commandlines/nsICommandLine.idl#130

You think I can simplify the construction of the absolute path, right? Well, if I create an instance of nsICommandline like this:

> var cmdLine = Components.classes["@mozilla.org/toolkit/command-line;1"]
>               .createInstance(Components.interfaces.nsICommandLine);
> let resFile = cmdLine.resolveFile(args.message);

This will not work because the mWorkingDir property is not initialized:

> [3661] WARNING: NS_ENSURE_TRUE(mWorkingDir) failed: file /home/manu/source/comm-central/mozilla/toolkit/components/commandlines/nsCommandLine.cpp, line 247

Is there a way to access an already initialised nsICommandLine?

Comment 49

[Manuel Grießmayr]

(In reply to Jorg K (GMT+2, never had any PTO during summer) from comment #44)

> When format==2, you must not replace the line endings.

When we take this so serious we should also check the user preference mail.identity.id[n].compose_html which represents the checkbox "Compose messages in HTML format" available under Edit->Account Settings->Composition & Addressing. I'll try to implement it.

Comment 50

[Magnus Melin [:mkmelin]]

Comment on attachment 8792279 [details] [diff] [review]
message.patch (JK v6).

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

To use the command line resolver, you need access to the command line (nsMailDefaultHandler.js) ... but maybe this is also ok.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2325,5 @@
>          params.type = args.type;
> +      // Only use valid values.
> +      if (args.format &&
> +          (args.format == Components.interfaces.nsIMsgCompFormat.PlainText ||
> +           args.format == Components.interfaces.nsIMsgCompFormat.HTML))

This change looks wrong. This is not the place to validate it, and there are other formats too that you're missing.

@@ +2382,5 @@
>          composeFields.newshost = args.newshost;
> +      if (args.message) {
> +        let msgFile = Components.classes["@mozilla.org/file/local;1"]
> +                      .createInstance(Components.interfaces.nsILocalFile);
> +        if(OS.Path.dirname(args.message) == ".") {

nit: space after if

@@ +2385,5 @@
> +                      .createInstance(Components.interfaces.nsILocalFile);
> +        if(OS.Path.dirname(args.message) == ".") {
> +          let pwd = Components.classes["@mozilla.org/file/directory_service;1"]
> +                    .getService(Components.interfaces.nsIProperties)
> +                    .get("CurWorkD", Components.interfaces.nsIFile);

Services.dirsvc.get("CurWorkD", Ci.nsIFile);

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +176,5 @@
>  errorFileAttachMessage=The file %1$S does not exist so could not be attached to the message.
>  
> +## String used if a file to serve as message body does not exist or cannot be loaded when passed
> +## as a command line argument
> +errorFileMessageTitle=Message File

File Not Found

and probably

Error Reading File 

... for the other

Comment 51

[Jorg K (CEST = GMT+2)]

Now I need to defend the changes I made to someone else's work. Well, here goes.

(In reply to Magnus Melin from comment #50)
> >          params.type = args.type;
> > +      // Only use valid values.
> > +      if (args.format &&
> > +          (args.format == Components.interfaces.nsIMsgCompFormat.PlainText ||
> > +           args.format == Components.interfaces.nsIMsgCompFormat.HTML))
> 
> This change looks wrong. This is not the place to validate it, and there are
> other formats too that you're missing.
I haven't tested whether it's validated elsewhere. I had the impression any old value is accepted.
There are four values:
https://dxr.mozilla.org/comm-central/rev/c3daef7d184b8e55f269e168c50ca1cde3035348/mailnews/compose/public/nsIMsgComposeParams.idl#47
    const long Default                  = 0;
    const long HTML                     = 1;
    const long PlainText                = 2;
    const long OppositeOfDefault        = 3;
and only two make sense to accept here. I don't know whether there is documentation elsewhere, but this page
http://kb.mozillazine.org/Command_line_arguments_%28Thunderbird%29
only mentions 1 and 2. If the user wants the default, they wouldn't specify format= and I don't think you want to allow 3. So, as far as I can say, this change is right an necessary.

> mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
> @@ +176,5 @@
> >  errorFileAttachMessage=The file %1$S does not exist so could not be attached to the message.
> >  
> > +## String used if a file to serve as message body does not exist or cannot be loaded when passed
> > +## as a command line argument
> > +errorFileMessageTitle=Message File
> File Not Found
> and probably
> Error Reading File 
> ... for the other
As you can see in the section you're quoting, there *IS* a special compose error for not finding an attachment. So I followed that and added special compose errors for not finding the message file or failing to read it.

Of course you could use the generic "File Not Found" which you can see here:
https://dxr.mozilla.org/comm-central/search?q=fileNotFoundTitle
but that would make the behaviour inconsistent, depending on which file is missing.
So as far as I can see, this change is also correct.

Comment 52

[Jorg K (CEST = GMT+2)]

(In reply to Manuel Grießmayr from comment #49)
> > When format==2, you must not replace the line endings.
> When we take this so serious ...
I don't see what is "so serious" about not ignoring the explicit choice of the user.
If they explicitly ask for text, we can't go in there and string all the lines together and destroy the data.

> ...we should also check the user preference
> mail.identity.id[n].compose_html which represents the checkbox "Compose
> messages in HTML format" available under Edit->Account Settings->Composition
> & Addressing. I'll try to implement it.
I think that's overcomplicating the solution. We also have another preference mail.html_compose that kicks in in certain cases:
https://dxr.mozilla.org/comm-central/rev/c3daef7d184b8e55f269e168c50ca1cde3035348/mailnews/compose/src/nsMsgComposeService.cpp#237

The user can set the format with format=. Anyway, I've already burned my fingers here by interfering, so no idea what Magnus wants. He even didn't seem to like the special compose error I introduced for the failures.

Comment 53

[Manuel Grießmayr]

(In reply to Jorg K (GMT+2, never had any PTO during summer) from comment #52)

> I don't see what is "so serious" about not ignoring the explicit choice of
> the user.
> If they explicitly ask for text, we can't go in there and string all the
> lines together and destroy the data.

For me it just seems unlikely that a user wants to load an HTML file and also set the email format to plain text.

Comment 54

[Jorg K (CEST = GMT+2)]

Subject: Look at the HTML I'm working on, see below.

Unlikely but possible. Who says that computer software only works in likely cases? The user made the choice to send plaintext, so they system must not ignore that.

What you proposed in comment #49 is even more unlikely, that is, that someone sends a file and wants the default account setting applied. In fact, it doesn't make sense. If you send an HTML file, you get a HTML composition. The system knows better what window is suitable for the data. If you explicitly want to suppress that, say: format=2.

Comment 55

[Manuel Grießmayr]

Attachment: message.patch

Hmmm... opinions seem to go in different directions. Magnus please advice again how you would like to have the log messages. What do you think about the arguments from Jörg K.?

Comment 56

[Jorg K (CEST = GMT+2)]

Since you removed the argument checking:
Have you tested what happens when the user types format=55? Where is that checked and corrected? I don't care where it's checked and perhaps my code was in the wrong place, but we need to make sure that it's checked. You can even raise an error.

Comment 57

[Jorg K (CEST = GMT+2)]

Comment on attachment 8792470 [details] [diff] [review]
message.patch

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2375,5 @@
>            }
>          }
>        }
>        if (args.newshost)
>          composeFields.newshost = args.newshost;

Magnus, would you like the format checking here?
if (args.format) ...

Comment 58

[Manuel Grießmayr]

(In reply to Jorg K (GMT+2, never had any PTO during summer) from comment #56)
> Have you tested what happens when the user types format=55?

At least in our case with file loading this seems to be fine. I made a quick test.

Comment 59

[Jorg K (CEST = GMT+2)]

Comment on attachment 8792470 [details] [diff] [review]
message.patch

As I said, you must check the value of format passed in. Maybe you want to do this in a different bug, but it would be best to do it here while the box is open.

I tested it with an additional debug dump statement. The value specified on the command line *IS* assigned to params.format.

I think the first rule of data processing is that you must absolutely check all user input before assigning value to *internal* control variables. That your quick test works, since 55 != 2 is obvious. But it doesn't remove the need for correct checking.

And removing a hunk from a patch without replacement is also not the way forward.

I think it would be best to add some code where I indicated in comment #57 and reject any values other than 1 and 2 with an alert and then default to 0. Functionally, this is what I did without an alert, but for some reason Magnus didn't like it.

I suggest to wait until he lets us know how he wants that checking to be done.

Comment 60

[Manuel Grießmayr]

I experimented a little bit with reading data from standard input so the user can specify for example "cat body.htm | thunderbird -compose message=-". It works fine with this code which I call from Javascript:

> NS_IMETHODIMP
> nsCommandLine::GetStdInput(nsACString& aResult)
> {
>   char c;
> 
>   while(std::cin.get(c)) {
>     aResult.Append(c);
>   }
> 
>   return NS_OK;
> }

As you can see in my test I extended nsICommandline but I am sure Magnus will say you cannot add this function here. Think of another way or use this API...

So two questions:
- What would be a good way to implement it?
- How do we know if standard input is HTML code or plain text? We could add another field for example "htmlmessage=-".

Let me know what you think. Many thanks :)

Comment 61

[aleth [:aleth]]

(In reply to Manuel Grießmayr from comment #60)
> I experimented a little bit with reading data from standard input 

That's an interesting idea, but I would suggest opening a fresh bug for that so that this one can land soon ;)

Comment 62

[Manuel Grießmayr]

(In reply to aleth [:aleth] from comment #61)

> I would suggest opening a fresh bug...

Done. https://bugzilla.mozilla.org/show_bug.cgi?id=1305218

Comment 63

[Manuel Grießmayr]

(In reply to Jorg K (GMT+2) from comment #43)
> Try sending this:
> 
> <html><body>
> <div style="background-color:#eee;border:1px solid #000;padding:10px;
> height:50px;" contenteditable="">
> <p style="background:red;"><font color=green><tt>xyz</tt></font><br></p>
> </div>
> <div style="background-color:#eee;border:1px solid #000;padding:10px;
> height:50px;" contenteditable="">
> <p style="background:red;">xyz<br></p>
> </div>
> </body></html>
> 
> It looks bad because we add a <p> and position into the first <div>. It
> works OK if you insert a text before the first <div>.

Sorry JorgK, I don't get it. What looks bad? When I load this code via the message='' option it looks identical compared to the view in Firefox. Only the font style looks different because of different default font style settings.

Comment 64

[Jorg K (CEST = GMT+2)]

If your default compose mode is "paragraph", the system adds a paragraph to the HTML that's loaded from the file. In the example above, we add the <p> into the <div> and that looks bad. It's not important for this bug, I just tried to make sense of Magnus' "It's not like it really works now".

Comment 65

[Manuel Grießmayr]

(In reply to Jorg K (GMT+2) from comment #64)
> If your default compose mode is "paragraph", the system adds a paragraph to
> the HTML that's loaded from the file.

Thanks, got it now. I didn't knew this feature. The system adds a <p><br></p> and so when the user starts typing into the body field he already is typing within a paragraph. So the system "thinks" the body section will be empty on startup which is not true. We have the same issue when we load a plain text file and also when using the body='' option.

This behaviour is defined in MsgComposeCommands.js function NotifyComposeBodyReadyNew:
> let pElement = editor.createElementWithDefaults("p");
> pElement.appendChild(editor.createElementWithDefaults("br"));
> editor.insertElementAtSelection(pElement, false);

I can easily avoid this code by adding a boolean variable and then your HTML code example loads fine. Magnus sounds like there are other cases too where HTML Code is inserted or changed. So for correct file loading we would need to find all cases and correct the appropriate code. I don't know these cases.

Comment 66

[Jorg K (CEST = GMT+2)]

The correct way to fix this would be something similar to bug 1266916 which I fixed recently. There paragraph stuff only happens, when the composition is not filled by a ?body=this is the body.

Can I see some detail of you solution with the boolean?

Sadly, we have other problems to fix here and Magnus still hasn't had time to address the questions. I can't see any further problem that would warrant saying "It's not like it really works now".

Comment 67

[Manuel Grießmayr]

(In reply to Jorg K (GMT+2) from comment #66)

> Can I see some detail of you solution with the boolean?

I only did this for testing purposes to see if it works. I simply set a global boolean when loading a file via message='...' and then checked that boolean before inserting <p><br></p>.

Comment 68

[Manuel Grießmayr]

Does anybody feel happy when the coding style says you have to indent with 2 white spaces? Other projects indent with 8! Also a maximum line width of 80 characters is pretty old school in my opinion.

Comment 69

[Jorg K (CEST = GMT+2)]

Well, the indentation with two spaces is standard. Depending our your reviewer, the 80-character-limit is not always enforced. But lines longer than 90 get messy.

As for the insertion of the <p>: Maybe add your global here:
  let insertParagraph = gMsgCompose.composeHTML && useParagraph && !gIsInsertingMessageFromFile;

Comment 70

[Magnus Melin [:mkmelin]]

Comment on attachment 8792470 [details] [diff] [review]
message.patch

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

So the reason I didn't want parameter checking is that how this function is called is a mess (with window arguments, non-used input parameters, a very long call chaning etc.) so you might easily make it not work for e.g. shift-click compose. We basically don't validate the other parameters much either. Junk in, junk out. Users of this functionality would test it out anyway before real usage.

I may have been mistaken about what didn't work, but it's anyway nicer to just do the replacement in JS where it is now.

Seems there are still some things to sort out (<p> mode at least). Jörg seems engaged here, so please direct reviews to him

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2381,5 @@
> +        let msgFile = Components.classes["@mozilla.org/file/local;1"]
> +                      .createInstance(Components.interfaces.nsILocalFile);
> +        if (OS.Path.dirname(args.message) == ".") {
> +          let pwd = Services.dirsvc.get("CurWorkD", Components.interfaces.nsILocalFile);
> +          args.message = OS.Path.join(pwd.path, OS.Path.basename(args.message));

While it looks like it works, would really be great to fix nsICommandLine to assume CurWorkD when mWorkingDir is not set. (For another bug though.)

@@ +2431,5 @@
> +                (args.message.endsWith(".htm") || args.message.endsWith(".html"))) {
> +              // We replace line breaks because otherwise they'll be converted to
> +              // <br> in nsMsgCompose::BuildBodyMessageAndSignature().
> +              // Don't do the conversion if the user asked explicitly for plain text.
> +              data = data.replace(/\r?\n|\r/g," ");

I'd assume /\r?\n/g is enough. Besides, BuildBodyMessageAndSignature  only really deals with \n

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +182,5 @@
> +## LOCALIZATION NOTE (errorFileMessageMessage): %1$S will be replaced by the non-existent file name. Do not translate
> +errorFileMessageMessage=The file %1$S does not exist and could not be used as message body.
> +
> +## LOCALIZATION NOTE (errorLoadFileMessageMessage): %1$S will be replaced by the name of the file that can't be loaded.
> +## Do not translate

Please remove this, it's confusing as to what not to translate. And localizers will know anyway

Comment 71

[Magnus Melin [:mkmelin]]

And yes we're happy with 2 space indent + 80char limit. 2 space makes blocks not too wide, and 80ch or so is still good for reading diffs.

Comment 72

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #70)
> So the reason I didn't want parameter checking is that how this function is
> called is a mess (with window arguments, non-used input parameters, a very
> long call chaning etc.) so you might easily make it not work for e.g.
> shift-click compose. We basically don't validate the other parameters much
> either. Junk in, junk out. Users of this functionality would test it out
> anyway before real usage.
Well, we do test the other parameters. If the file isn't there, we let the user know ;-)
So please add checking below (see comment #57) with a proper error message: Invalid value for 'format'. Valid are 1 = HTML, 2 = plain text.

> I may have been mistaken about what didn't work, but it's anyway nicer to
> just do the replacement in JS where it is now.
Yes.

> Seems there are still some things to sort out (<p> mode at least). Jörg
> seems engaged here, so please direct reviews to him.
OK, I'll do the review. I want to see this landed one day and forgotten ;-)

> While it looks like it works, would really be great to fix nsICommandLine to
> assume CurWorkD when mWorkingDir is not set. (For another bug though.)
Sorry, I don't understand this comment.

> I'd assume /\r?\n/g is enough. Besides, BuildBodyMessageAndSignature  only
> really deals with \n
Why don't we make it clearer? We want to replace CRLF or LF, so \r\n|\n.

OK, let's see a patch with
- parameter checking.
- Maybe Magnus' comment addressed.
- The paragraph issue fixed, globals ahoy ;-(
- The localiser's note removed.

Comment 73

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2) from comment #72)
> Well, we do test the other parameters. If the file isn't there, we let the
> user know ;-)

That one yes, but it's not a parameter you would set anywhere else but from command line. Format may be set anywhere.

> So please add checking below (see comment #57) with a proper error message:
> Invalid value for 'format'. Valid are 1 = HTML, 2 = plain text.

Please don't. Like I said, you're making assumptions about where this code is called. The parameter may have been set to something else elsewhere. If not now, maybe in the future, or now in add-ons.

> > While it looks like it works, would really be great to fix nsICommandLine to
> > assume CurWorkD when mWorkingDir is not set. (For another bug though.)
> Sorry, I don't understand this comment.

See comment 48

> 
> > I'd assume /\r?\n/g is enough. Besides, BuildBodyMessageAndSignature  only
> > really deals with \n
> Why don't we make it clearer? We want to replace CRLF or LF, so \r\n|\n.

That's what /\r?\n/g is.

Comment 74

[Jorg K (CEST = GMT+2)]

Magnus, please indicate which form of parameter checking for the format would be acceptable, if any.
"Junk in, junk out" is not nice.

Where would this be called:
    if (args) { //Convert old fashion arguments into params
      var composeFields = params.composeFields;
      if (args.bodyislink == "true")
        params.bodyIsLink = true;
      if (args.type)
        params.type = args.type;
      if (args.format)
        params.format = args.format;

At least we would need to check the valid values 0, 1, 2, and 3.

Comment 75

[Magnus Melin [:mkmelin]]

Well the code that uses the format parameter later defaults to default format anyway later, so it's actually already garbage in, handled gracefully (but silently). So you'd check all valid values, but get no profit. Why would you want to do that?

Where it would be called? Yes that's the problem, call sites are hard to follow and anticipate here.

Comment 76

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #75)
> Well the code that uses the format parameter later defaults to default
> format anyway later, so it's actually already garbage in, handled gracefully
> (but silently).
Where? (Saves me looking for something you already know.)

Comment 77

[Magnus Melin [:mkmelin]]

I just tested it, didn't look up where.

Comment 78

[Manuel Grießmayr]

(In reply to Magnus Melin from comment #73)
> That's what /\r?\n/g is.

I think Jörg is referring to the readability.

Comment 79

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #77)
> I just tested it, didn't look up where.
Right, I tested it too ;-(

(In reply to Manuel Grießmayr from comment #78)
> I think Jörg is referring to the readability.
Yes.

Manual, look, I'm terribly sorry about we've been putting you through. Magnus and I disagree at times, it must be terrible to be in the middle. Let's do this. Do a last patch here with:
- regexp addressed.
- looks like we need to leave
  let pwd = Services.dirsvc.get("CurWorkD", Components.interfaces.nsILocalFile);
  Can you change the variable name, pwd is a Unix command and we don't print anything here.
- The paragraph issue fixed, globals ahoy ;-(
- The localiser's note removed.

I will then look into the parameter checking myself.

Comment 80

[Jorg K (CEST = GMT+2)]

Looks like the format is used/checked here:
https://dxr.mozilla.org/comm-central/rev/ca4b376232c7386fc72fab6fc3b895fb60e90e4d/mailnews/compose/src/nsMsgComposeService.cpp#203

By accident, all values which are not HTML, plain or opposite are treated as default.

I think it would still be good practice then to do (from my patch):
> +      // Only use valid values.
> +      if (args.format &&
> +          (args.format == Components.interfaces.nsIMsgCompFormat.PlainText ||
> +           args.format == Components.interfaces.nsIMsgCompFormat.HTML ||
              args.format == Components.interfaces.OppositeOfDefault)) {
for some basic checking. Magnus, can you settle for that?

Why do I want to check it if it works already by accident? Well, just to have a *clear* gate that will reject bad values early.

Comment 81

[Manuel Grießmayr]

(In reply to Jorg K (GMT+2) from comment #79)
> Manuel, look, I'm terribly sorry about we've been putting you through.
This is totally fine if we have better code in the end :)
What's not so cool is to indent with 2 spaces ;)

> - regexp addressed.
Is there a difference in performance when we compare /\r?\n/g to /\r\n|\n/g ?

> - The paragraph issue fixed, globals ahoy ;-(
What about avoiding the global boolean and instead writing something like this in MsgComposeCommands.js in function NotifyComposeBodyReadyNew():
> if (insertParagraph &&
>     (gComposeType == Components.interfaces.nsIMsgCompType.MailToUrl ||
>     gComposeType == Components.interfaces.nsIMsgCompType.New)) {
>   // Check for empty body before allowing paragraph to be inserted.

Comment 82

[Jorg K (CEST = GMT+2)]

(In reply to Manuel Grießmayr from comment #81)
> What's not so cool is to indent with 2 spaces ;)
You can't fight that, you'd have all of Mozilla against you.

> > - regexp addressed.
> Is there a difference in performance when we compare /\r?\n/g to /\r\n|\n/g ?
No idea. You are the author, you use whichever you prefer.

> > - The paragraph issue fixed, globals ahoy ;-(
> What about avoiding the global boolean and instead writing something like
> this in MsgComposeCommands.js in function NotifyComposeBodyReadyNew():
> > if (insertParagraph &&
> >     (gComposeType == Components.interfaces.nsIMsgCompType.MailToUrl ||
> >     gComposeType == Components.interfaces.nsIMsgCompType.New)) {
Yes. That could work. Although I don't really like the processing for new messages only to cater for the very remote case of someone using -compose message=.

Comment 83

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2) from comment #80)
> Looks like the format is used/checked here:
> https://dxr.mozilla.org/comm-central/rev/
> ca4b376232c7386fc72fab6fc3b895fb60e90e4d/mailnews/compose/src/
> nsMsgComposeService.cpp#203
> 
> By accident, all values which are not HTML, plain or opposite are treated as
> default.

I wouldn't call it accident... it does exactly what it's suppose to no?

> Why do I want to check it if it works already by accident? Well, just to
> have a *clear* gate that will reject bad values early.

Ok, add it if you must but the point was, this is not necessarily the gate for format.

Comment 84

[Jorg K (CEST = GMT+2)]

Attachment: message.patch (JK v7).

I got bored on a rainy Saturday evening. Use the interdiff to see what I did. Is this OK?

Comment 85

[Jorg K (CEST = GMT+2)]

Attachment: message.patch (JK v7b).

Sorry, spurious break left behind.
Also I changed the ß in the author's name to ss since Mercurial can't deal with anything bug ASCII there.

Also, interdiff seems to have gone mad in comparing this it Manuel's last version.

Comment 86

[Jorg K (CEST = GMT+2)]

Comment on attachment 8796876 [details] [diff] [review]
message.patch (JK v7b).

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2356,5 @@
> +      if (args.format &&
> +          (args.format == Components.interfaces.nsIMsgCompFormat.PlainText ||
> +           args.format == Components.interfaces.nsIMsgCompFormat.HTML ||
> +           args.format == Components.interfaces.nsIMsgCompFormat.OppositeOfDefault))
> +       params.format = args.format;

I'll fix the indent here before landing.

Comment 87

[Jorg K (CEST = GMT+2)]

Attachment: message.patch (JK v7c).

Sorry about the noise. I knew the switch could be done more elegantly ;-)

Comment 88

[Jorg K (CEST = GMT+2)]

Comment on attachment 8796883 [details] [diff] [review]
message.patch (JK v7c).

Please let me know what you think. If you approve, let's get this landed.

Comment 89

[Jorg K (CEST = GMT+2)]

Attachment: message.patch (JK v7c) - refreshed.

Had to refresh the patch (was rotted by bug 1266916).

Comment 90

[Manuel Grießmayr]

Looks like you forgot the localisation notes.

Regarding args.format value checking. What about checking for nsIMsgCompFormat.Default? What about setting to nsIMsgCompFormat.Default when the values are smaller than 0 or bigger than 3?

> fstream.init(msgFile, -1, 0, 0); // open in read only mode
Nit. Not sure if Magnus wants first character in upper case and a dot at the end. (Yes, comment has been made by me.)

The following behaviour is not bad/critical for me and I assume it can be ignored. I only want to mention it. When specifying body="<br>" we end up with <br><br>. When specifying body="<p></p>" it will be removed and then replaced with <p><br></p>. When specifying body=" " and I click into the body widget then the cursor will not be visible until I begin to type. Also the white space is removed.

Regard everything as suggestions and ideas. Land the patch if you are happy :)

Comment 91

[Jorg K (CEST = GMT+2)]

(In reply to Manuel Grießmayr from comment #90)
> Looks like you forgot the localisation notes.
I removed the "Do not translate". The localisation note explaining the parameters should stay.

> Regarding args.format value checking. What about checking for
> nsIMsgCompFormat.Default? What about setting to nsIMsgCompFormat.Default
> when the values are smaller than 0 or bigger than 3?
Not necessary since params.format is properly initialised to 0. Do you agree?

> > fstream.init(msgFile, -1, 0, 0); // open in read only mode
> Nit. Not sure if Magnus wants first character in upper case and a dot at the
> end. (Yes, comment has been made by me.)
We're more flexible on inline comments. Does -1 mean "read only"?

> The following behaviour is not bad/critical for me and I assume it can be
> ignored. I only want to mention it. When specifying body="<br>" we end up
> with <br><br>. When specifying body="<p></p>" it will be removed and then
> replaced with <p><br></p>. When specifying body=" " and I click into the
> body widget then the cursor will not be visible until I begin to type. Also
> the white space is removed.
We know that empty paragraphs (<p></p>) are suppressed by the M-C editor. I guess the body is considered empty and gets the standard <p><br></p> if in paragraph mode.

body= problems are pre-existing, right? So I don't have to fix them here, do I? If you care, file another bug ;-)

If you kindly put f+ on the patch I'll land it.

Comment 92

[Jorg K (CEST = GMT+2)]

Yes, -1 is default mode/read-only:
https://dxr.mozilla.org/mozilla-central/rev/7c576fe3279d87543f0a03b844eba7bc215e17f1/netwerk/base/nsIFileStreams.idl#20

Comment 93

[Manuel Grießmayr]

(In reply to Jorg K (GMT+2) from comment #91)
> > Regarding args.format value checking. What about checking for
> > nsIMsgCompFormat.Default? What about setting to nsIMsgCompFormat.Default
> > when the values are smaller than 0 or bigger than 3?
> Not necessary since params.format is properly initialised to 0. Do you agree?
I agree: https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgComposeParams.cpp#10

Comment 94

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/35368039b787

Landed with a few more white-space adjustments and the comment for read-only changed.

Thanks to all involved!

<off topic>
If you like German proverbs, here a few that apply somewhat to this bug ;-)
Gut Ding will Weile haben.
Viele Köche verderben den Brei (I hope not!).
Wer viel fragt, kriegt viel Antwort.
Es gibt nichts Gutes, außer man tut es.
Wer vielen Herren dient, bekommt viele Orden und viele Fußtritte.
Ende gut, alles gut.
</off topic>

Comment 95

[Manuel Grießmayr]

Yes, thanks to anyone who helped.

Jörg, some of your proverbs I didn't even know but I like them. On IRC someone said: "landing patches is hard". Now I understand :)

So this feature will be shipped with thunderbird 52. Is there already a release date or a roughly time frame for it?

Comment 96

[Jorg K (CEST = GMT+2)]

Really you can use it from tomorrow in a Daily build. I'm using daily builds for a while now, and unless you hit a bad day, they work fine. Since I am the manager for Aurora/alfa/Earlybird and Beta releases, I could uplift this feature since I don't believe that it will cause any damage ;-)

As for the regular releases, please consult the bible of Mozilla development, the so-called Rapid Release Calendar: https://wiki.mozilla.org/RapidRelease/Calendar
TB 52 goes to Earlybird/Beta/Release on 2016-11-07/2017-01-23/2017-03-06.

Since we don't do a TB 51 release (beta only) this would hit users in March 2017 in TB 52. If uplifted to TB 51 you'd get it in Earlybird the day after the uplift or in TB 51 beta in November.

All clear?

Comment 97

[Manuel Grießmayr]

> All clear?

Yes, thank you very much :)

Comment 98

[Manuel Grießmayr]

No uplifting needed from my side.

Comment 99

[Manuel Grießmayr]

(In reply to Magnus Melin from comment #70)
> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +2381,5 @@
> > +        let msgFile = Components.classes["@mozilla.org/file/local;1"]
> > +                      .createInstance(Components.interfaces.nsILocalFile);
> > +        if (OS.Path.dirname(args.message) == ".") {
> > +          let pwd = Services.dirsvc.get("CurWorkD", Components.interfaces.nsILocalFile);
> > +          args.message = OS.Path.join(pwd.path, OS.Path.basename(args.message));
> 
> While it looks like it works, would really be great to fix nsICommandLine to
> assume CurWorkD when mWorkingDir is not set. (For another bug though.)
Bug has been created: https://bugzilla.mozilla.org/show_bug.cgi?id=1307607

Comment 100

[Manuel Grießmayr]

After thinking a little bit how to implement standard input I found that this works on my Linux system:
> cat message.htm | thunderbird -compose message=/dev/stdin
Only the line breaks are not removed of course.

Comment 101

[Jorg K (CEST = GMT+2)]

Maybe instead of
  if (params.format != Components.interfaces.nsIMsgCompFormat.PlainText &&
      (args.message.endsWith(".htm") || args.message.endsWith(".html"))) {

we should have implemented
  if (params.format != Components.interfaces.nsIMsgCompFormat.PlainText &&
      (args.message.endsWith(".htm") ||
       args.message.endsWith(".html") ||
       params.format == Components.interfaces.nsIMsgCompFormat.HTML)) {

so you can force HTML independent of the file extension.

I would accept a patch to that effect ;-)

Comment 102

[Manuel Grießmayr]

(In reply to Jorg K (GMT+2) from comment #101)
In the first patches here I actually implemented it similar to your idea but then decided against it because of the following. Maybe there is a user who wants to send plain text but wants to edit the message in the html editor and so he writes
> thunderbird -compose message=text.txt,format=1
He then wonders why all line breaks have been removed.
Related to that is also why line breaks are replaced with <br> in nsMsgCompose::BuildBodyMessageAndSignature I assume:
> MsgReplaceSubstring(body, NS_LITERAL_STRING("\n"), NS_LITERAL_STRING("<br>"));

On the other side one could argue it is stupid to send plain text to the html editor.

For me it would be fine to implement it in the way you suggested. I'm just not sure if everyone else is happy then. What do you think?

Comment 103

[Manuel Grießmayr]

See also comment 7 and comment 8.

Comment 104

[Jorg K (CEST = GMT+2)]

(In reply to Manuel Grießmayr from comment #102)
> What do you think?
Honestly, I have other problems. I'm happy to review anything you deem fit. I just helped along to get this landed.

In general I think this:
For format=1 the composition is HTML, for format=2 the composition is text.
The format determines the compose format.

If the file is a HTML file (.html?) then it should be imported without extra linebreaks.
To the extension gives an indication for the input format, so for HTML input we need to strip the linebreaks.

That's what we implemented more or less.

cat message.htm | thunderbird -compose message=/dev/stdin,format=1
doesn't work, since stdin doesn't carry a file extension to indicate a HTML file.

If you wanted to do it cleanly, add "input=1" to indicate HTML input, or better, check the data you're reading to see whether it's HTML; there should be some tags ;-)

Comment 105

[aleth [:aleth]]

Drive-by comment: if there is room for confusion, improving the help message printed to the console e.g. describing --format might help whatever you decide to do here.

Comment 106

[Manuel Grießmayr]

Can we change "Product" to "Thunderbird" so that this bug fix is listed here:
https://bugzilla.mozilla.org/buglist.cgi?classification=Client%20Software&query_format=advanced&bug_status=RESOLVED&bug_status=VERIFIED&bug_status=CLOSED&target_milestone=Thunderbird%2052.0&product=Thunderbird&resolution=FIXED

This list is linked by the Thunderbird release notes here:
https://www.mozilla.org/en-US/thunderbird/52.0/releasenotes/

It would be also cool if this new feature would be listed on the release notes page and not only in the generated bug list :)

Many thanks.

Comment 107

[Jorg K (CEST = GMT+2)]

The TB 52 release notes are already too long, and this is a pretty geeky feature. Thanks again for your contribution but I don't think I'll list it there.

Comment 108

[Manuel Grießmayr]

Indeed, this is geeky :) I saw I have the rights to change the flags myself so at least this is listed in the bug listing now.

Comment 109

[Magnus Melin [:mkmelin]]

You need a much more complex query to actually list all the bugs fixed "for tb 52"...
The correct component is MailNews Core beucase it also affects SeaMonkey.

Comment 110

[Jorg K (CEST = GMT+2)]

Magnus, what makes you think this is Mailnews? The changed files are in mail/ and just a little tweak in Mailnews. SM is not affected and hasn't ported this. Sorry, I'll change this back.

Comment 111

[Magnus Melin [:mkmelin]]

Ah, should have looked more closely!

Comment 112

[Manuel Grießmayr]

(In reply to Magnus Melin from comment #109)
> You need a much more complex query to actually list all the bugs fixed "for
> tb 52"...
When I need a much more complex query then why is this one not used in the bug list of the release notes? And why is it called "complete list of changes"?

Comment 113

[Jorg K (CEST = GMT+2)]

The link on the release notes "complete list of changes" is pretty much useless, to say it nicely. I just had this discussion again today.

The query only checks for bugs fixed in product Thunderbird in versions 46 to 52. It misses Mailnews, Chat, Instantbird, Calendar and any fixes we get thought Mozilla Core modules, like Editor or Layout. It also misses bugs fixed in versions 53 to 55 where the fix was backported.

It also lists every little bug that the user never saw, since something broke due to some other change elsewhere and was fixed immediately.

I know how you feel: I have many contributions that never made it onto the release notes, so I complained so many times that now I get to *write* the release notes. It's not fun, though, I can assure you ;-)