Improve command line experience for email composition

RESOLVED FIXED in Thunderbird 52.0

Status

enhancement
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: abspack, Assigned: abspack)

Tracking

Trunk
Thunderbird 52.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

3 years ago
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160920074044
(Assignee)

Comment 1

3 years ago
- add from field for example from=mozilla@provider.com
- add string values for format field: HTML/Text
- try to detect html files for standard input steams
- improve thunderbird -h explanation for email compositioning
(Assignee)

Comment 2

3 years ago
Posted patch cli_improvement.patch (obsolete) — Splinter Review
Attachment #8801771 - Flags: feedback?(jorgk)
(Assignee)

Updated

3 years ago
Severity: normal → enhancement
OS: Unspecified → All
Hardware: Unspecified → All
(Assignee)

Updated

3 years ago
Component: Message Compose Window → Composition
Product: Thunderbird → MailNews Core

Comment 3

3 years ago
Comment on attachment 8801771 [details] [diff] [review]
cli_improvement.patch

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

Looks good. Aceman, can you please comment as well.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2359,5 @@
> +          params.format = args.format;
> +        else if (args.format.toLowerCase().trim() == "html")
> +          params.format = Components.interfaces.nsIMsgCompFormat.HTML;
> +        else if (args.format.toLowerCase().trim() == "text")
> +          params.format = Components.interfaces.nsIMsgCompFormat.PlainText;

OK, so you allow format=html and format=text.

@@ +2373,5 @@
> +
> +        while (enumerator.hasMoreElements()) {
> +          ident = enumerator.getNext();
> +          if(args.from.toLowerCase().trim() == ident.email.toLowerCase()) {
> +            params.identity = ident;

OK, so the identity used is determined by the from address.

@@ +2485,5 @@
> +              char = data.charAt(pos);
> +              if (char != "\n" && char != "\r" && char != " " && char != "\t")
> +                break;
> +              pos++;
> +            }

Hmm, I'm not a JS specialist, but can't that be done more elegantly? Aren't there other white-space characters? At least you need \f as well.

@@ +2491,4 @@
>              if (params.format != Components.interfaces.nsIMsgCompFormat.PlainText &&
> +                (args.message.endsWith(".htm") || args.message.endsWith(".html") ||
> +                 data.substr(pos, 14).toLowerCase() == "<!doctype html" ||
> +                 data.substr(pos, 5).toLowerCase() == "<html")) {

OK, detect HTML documents.

::: mailnews/compose/src/nsMsgComposeService.cpp
@@ +1461,5 @@
>  
>  NS_IMETHODIMP
>  nsMsgComposeService::GetHelpInfo(nsACString& aResult)
>  {
> +  aResult.Assign(NS_LITERAL_CSTRING("  -compose [<data>]  Compose a mail or news message. See http://kb.mozillazine.org/Command_line_arguments_-_Thunderbird for an explanation of the data string.\n"));

Please don't reference mozillazine.org. We have nothing to do with them.
Attachment #8801771 - Flags: feedback?(jorgk)
Attachment #8801771 - Flags: feedback?(acelists)
Attachment #8801771 - Flags: feedback+
(Assignee)

Comment 4

3 years ago
Posted patch cli_improvement.patch (obsolete) — Splinter Review
I also had the idea to add this to "thunderbird -h":
> -compose ["{from|to|cc|bcc|newsgroups|subject|format|body|attachment}='data[,...]'[,...]"]
But maybe it's too complicated...
Attachment #8801771 - Attachment is obsolete: true
Attachment #8801771 - Flags: feedback?(acelists)
Attachment #8801858 - Flags: feedback?(acelists)

Comment 5

3 years ago
How about
-compose "from=...,to=...,cc=...,bcc=..., etc." Home-cooked syntax ;-)
(Assignee)

Comment 6

3 years ago
Posted patch cli_improvement.patch (obsolete) — Splinter Review
Attachment #8801858 - Attachment is obsolete: true
Attachment #8801858 - Flags: feedback?(acelists)
Attachment #8801875 - Flags: feedback?(acelists)
(Assignee)

Comment 7

3 years ago
Posted patch cli_improvement.patch (obsolete) — Splinter Review
Attachment #8801875 - Attachment is obsolete: true
Attachment #8801875 - Flags: feedback?(acelists)
Attachment #8801878 - Flags: feedback?(acelists)
(Assignee)

Comment 8

3 years ago
Only added some white space in the latest patch into the help string.

Comment 9

3 years ago
Comment on attachment 8801875 [details] [diff] [review]
cli_improvement.patch

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

Nice.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2476,5 @@
>                fstream.close();
>            }
>  
>            if (data) {
> +            let pos = data.search(/\S/); // Find first non white space character

Somewhat shorter than the original proposal ;-)

::: mailnews/compose/src/nsMsgComposeService.cpp
@@ +1461,5 @@
>  
>  NS_IMETHODIMP
>  nsMsgComposeService::GetHelpInfo(nsACString& aResult)
>  {
> +  aResult.Assign(NS_LITERAL_CSTRING("  -compose \"to='email,...',message=file,...\" Compose a mail or news message. Options: from,to,cc,bcc,newsgroups,subject,format,body,message,attachment\n"));

I don't quite get the logic here. OK, there are these classes:
- e-mail addresses or multiple addresses. They need ''?
- subject is just a string.
- format you should explain, either 1/2 or html/text.
- message and attachment are file names.
Very hard to fit it all into one line. Would it be possible to have a \n in the string?

-compose "to='email,...',subject=string,format=[html|text],message=file,..." Compose a mail or news message.
Options: from,to,cc,bcc,newsgroups,subject,format,body,message,attachment
Attachment #8801875 - Attachment is obsolete: false
Attachment #8801875 - Flags: feedback+
(Assignee)

Comment 10

3 years ago
@Jörg: How do you like that?

-compose "Option='Value,...',Options..." Compose a mail or news message.
Options: from, preselectid (number), to, cc, bcc, newsgroups, subject, format ("html"|"text"), body, message (file), attachment (files)

Comment 11

3 years ago
Almost ;-) I don't understand why you need to repeat "Options". I'd put:

-compose "Option='Value,...',Option=Value,..." Compose a mail or news message.
Options: from, preselectid (number), to, cc, bcc, newsgroups, subject, format ("html"|"text"), body, message (file), attachment (files)

According to http://kb.mozillazine.org/Command_line_arguments_-_Thunderbird the 'preselectid' is not a number but a string of the form id3. Personally, I'd not document this since we have 'from' now.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 12

3 years ago
Posted patch cli_improvement.patch (obsolete) — Splinter Review
Attachment #8801875 - Attachment is obsolete: true
Attachment #8801878 - Attachment is obsolete: true
Attachment #8801878 - Flags: feedback?(acelists)
Attachment #8802154 - Flags: feedback?(acelists)
(Assignee)

Comment 13

3 years ago
In the latest patch I've done pretty printing for the command line help string for command lines with a width of 80 characters.

Updated

3 years ago
Assignee: nobody → abspack

Comment 14

3 years ago
-compose ["[Option='Value,...'],..."]

Frankly, I don't like this. We're mixing regexp with home-grown syntax.

The case Option=Value isn't covered since you don't show that the single quotes are optional.
And if they leave out the [Option='Value,...'] they're left with ,... ??

Why don't you like my suggestion:
-compose "Option='Value,...',Option=Value,..."
(Assignee)

Comment 15

3 years ago
Attachment #8802154 - Attachment is obsolete: true
Attachment #8802154 - Flags: feedback?(acelists)
Attachment #8802548 - Flags: feedback?(acelists)
(Assignee)

Comment 16

3 years ago
(In reply to Jorg K (GMT+2) from comment #14)
> The case Option=Value isn't covered since you don't show that the single
> quotes are optional.
This case is a little bit "dangerous" to use but you're right, it's better when the user knows that it is possible.
> Why don't you like my suggestion:
> -compose "Option='Value,...',Option=Value,..."
Your suggestion is fine too.

Comment 17

3 years ago
Comment on attachment 8802548 [details] [diff] [review]
cli_improvement.patch

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

r=jorgk with the nit+issue below fixed. No idea where Aceman disappeared to.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2476,5 @@
>                fstream.close();
>            }
>  
>            if (data) {
> +            let pos = data.search(/\S/); // Find first non white space character

Nit: white-space. And let's put a full stop at the end ;-)

::: mailnews/compose/src/nsMsgComposeService.cpp
@@ +1461,5 @@
>  
>  NS_IMETHODIMP
>  nsMsgComposeService::GetHelpInfo(nsACString& aResult)
>  {
> +  aResult.Assign(NS_LITERAL_CSTRING("  -compose \"Option='Value,...',Option=Value,...\" Compose a mail or news message.\n                     Options: from, to, cc, bcc, subject, body, message (file),\n                     attachment, format (\"html\" or \"text\"), newsgroups\n"));

Not sure why 'newsgroups' moved to the end. It's like addressing, so it should come after 'bcc'. That's where it was in some previous patches.
Attachment #8802548 - Flags: feedback?(acelists) → review+

Comment 18

3 years ago
(In reply to Jorg K (GMT+2) from comment #17)
> > +            let pos = data.search(/\S/); // Find first non white space character
> Nit: white-space. And let's put a full stop at the end ;-)
Actually: make it "non-whitespace", that seems to be the most frequently used spelling.

Comment 19

3 years ago
Comment on attachment 8802548 [details] [diff] [review]
cli_improvement.patch

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

It's hard to comment when the file changes constantly :)

But I like the improvements.

Do you need to make the same changes into suite version of MsgComposeCommands.js?

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2365,5 @@
>        if (args.originalMsg)
>          params.originalMsgURI = args.originalMsg;
>        if (args.preselectid)
>          params.identity = getIdentityForKey(args.preselectid);
> +      else if (args.from) {

So preselectid takes precedence over from. Nice.

@@ +2372,5 @@
> +        let ident = {};
> +
> +        while (enumerator.hasMoreElements()) {
> +          ident = enumerator.getNext();
> +          if(args.from.toLowerCase().trim() == ident.email.toLowerCase()) {

Space after 'if'.

@@ +2476,5 @@
>                fstream.close();
>            }
>  
>            if (data) {
> +            let pos = data.search(/\S/); // Find first non white space character

Can we trim() the "data"? Or do we need to preserve the leading whitespace?

::: mailnews/compose/src/nsMsgComposeService.cpp
@@ +1461,5 @@
>  
>  NS_IMETHODIMP
>  nsMsgComposeService::GetHelpInfo(nsACString& aResult)
>  {
> +  aResult.Assign(NS_LITERAL_CSTRING("  -compose \"Option='Value,...',Option=Value,...\" Compose a mail or news message.\n                     Options: from, to, cc, bcc, subject, body, message (file),\n                     attachment, format (\"html\" or \"text\"), newsgroups\n"));

Would .AssignLiteral work here?
Attachment #8802548 - Flags: feedback+

Comment 20

3 years ago
Comment on attachment 8802548 [details] [diff] [review]
cli_improvement.patch

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

::: mailnews/compose/src/nsMsgComposeService.cpp
@@ +1461,5 @@
>  
>  NS_IMETHODIMP
>  nsMsgComposeService::GetHelpInfo(nsACString& aResult)
>  {
> +  aResult.Assign(NS_LITERAL_CSTRING("  -compose \"Option='Value,...',Option=Value,...\" Compose a mail or news message.\n                     Options: from, to, cc, bcc, subject, body, message (file),\n                     attachment, format (\"html\" or \"text\"), newsgroups\n"));

This does not align into a column with the other output on the command line. I get:
  --safe-mode        Disables extensions and themes for this session.
  -addressbook       Open the address book at startup.
  -compose "Option='Value,...',Option=Value,..." Compose a mail or news message.
                     Options: from, to, cc, bcc, subject, body, message (file),
                     attachment, format ("html" or "text"), newsgroups
  --jsconsole        Open the Browser Console.

Comment 21

3 years ago
I'll address the nits an land this now.

Comment 22

3 years ago
https://hg.mozilla.org/comm-central/rev/b34a52928c121eac4e7087c499d580e1e809e6bf

After much deliberation I went for:
  -addressbook       Open the address book at startup.
  -compose [ <options> ] Compose a mail or news message. Options are specified
                     as string "option='value,...',option=value,..." and
                     include: from, to, cc, bcc, newsgroups, subject, body,
                     message (file), attachment (file), format (html | text).
                     Example: "to=john@example.com,subject='Dinner tonight?'"
  --jsconsole        Open the Browser Console.

OK, gentlemen?
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 52.0
(Assignee)

Updated

2 years ago
Component: Composition → Message Compose Window
Product: MailNews Core → Thunderbird
You need to log in before you can comment on or make changes to this bug.