Closed Bug 1310738 Opened 9 years ago Closed 9 years ago

Improve command line experience for email composition

Categories

(Thunderbird :: Message Compose Window, enhancement)

Product:
Thunderbird
Component:
Message Compose Window
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 52.0

People

(Reporter: abspack, Assigned: abspack)

Details

Attachments

(1 file, 5 obsolete files)

cli_improvement.patch
8.06 KB, patch
jorgk-bmo
: review+
aceman
: feedback+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0 Build ID: 20160920074044
- 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
Attached patch cli_improvement.patch (obsolete) — — Splinter Review
Attachment #8801771 - Flags: feedback?(jorgk)
Severity: normal → enhancement
OS: Unspecified → All
Hardware: Unspecified → All
Component: Message Compose Window → Composition
Product: Thunderbird → MailNews Core
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+
Attached 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)
How about -compose "from=...,to=...,cc=...,bcc=..., etc." Home-cooked syntax ;-)
Attached patch cli_improvement.patch (obsolete) — — Splinter Review
Attachment #8801858 - Attachment is obsolete: true
Attachment #8801858 - Flags: feedback?(acelists)
Attachment #8801875 - Flags: feedback?(acelists)
Attached patch cli_improvement.patch (obsolete) — — Splinter Review
Attachment #8801875 - Attachment is obsolete: true
Attachment #8801875 - Flags: feedback?(acelists)
Attachment #8801878 - Flags: feedback?(acelists)
Only added some white space in the latest patch into the help string.
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+
@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)
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
Attached 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)
In the latest patch I've done pretty printing for the command line help string for command lines with a width of 80 characters.
Assignee: nobody → abspack
-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,..."
Attachment #8802154 - Attachment is obsolete: true
Attachment #8802154 - Flags: feedback?(acelists)
Attachment #8802548 - Flags: feedback?(acelists)
(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 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+
(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 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 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.
I'll address the nits an land this now.
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
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 52.0
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.

Attachment

General

Creator:
Created:
Updated:
Size: