`compose.sendMessage` options.mode is missing many send modes
Categories
(Thunderbird :: Add-Ons: Extensions API, enhancement)
Tracking
(thunderbird_esr91 wontfix)
Tracking | Status | |
---|---|---|
thunderbird_esr91 | --- | wontfix |
People
(Reporter: siefkenj, Assigned: TbSync)
References
Details
Attachments
(2 files)
48 bytes,
text/x-phabricator-request
|
Details | Review | |
Bug 1747456 - Make folder optional in MessageHeader (which it is) to fix debug test fails. r=mkmelin
48 bytes,
text/x-phabricator-request
|
Details | Review |
Steps to reproduce:
The compose.sendMessage(tabId, options)
call allows options.mode === "default" || "sendNow" || "sendLater"
. However, there are many more modes supported.
Seems to list 7 options. Of particular interest to me is saveAsDraft
. But maybe most (all?) should be exposed?
Comment 1•3 years ago
|
||
For my own add-on Mail Merge having the two modes "saveAsDraft" and "saveAsTemplate" would be necessary.
Assignee | ||
Comment 2•3 years ago
•
|
||
@Magnus: Before working on this, I need to address a few issues, which makes adapting the code difficult for me.
Issue #1:
The function CompleteGenericSendMessage
is async, but is not awaited in GenericSendMessage
, which causes what I call "async early exits".
For example:
function SendMessage() {
pillifyRecipients();
let sendInBackground = Services.prefs.getBoolPref(
"mailnews.sendInBackground"
);
if (sendInBackground && AppConstants.platform != "macosx") {
let count = [...Services.wm.getEnumerator(null)].length;
if (count == 1) {
sendInBackground = false;
}
}
GenericSendMessage(
sendInBackground
? Ci.nsIMsgCompDeliverMode.Background
: Ci.nsIMsgCompDeliverMode.Now
);
ExitFullscreenMode();
}
GenericSendMessage
returns before the first await is hit in CompleteGenericSendMessage
which means ExitFullscreenMode()
is executed BEFORE the actual send process. Do not know if that is intentional. Should we add a comment to point that out?
Same happens here:
function SaveAsDraft() {
gAutoSaveKickedIn = false;
gEditingDraft = true;
GenericSendMessage(Ci.nsIMsgCompDeliverMode.SaveAsDraft);
defaultSaveOperation = "draft";
}
Why do we set defaultSaveOperation
after the call to GenericSendMessage
?
I am asking all this, because I want to return the Promise so the caller can decide, whether to await the entire send process. I could do this:
function SaveAsDraft() {
gAutoSaveKickedIn = false;
gEditingDraft = true;
return GenericSendMessage(Ci.nsIMsgCompDeliverMode.SaveAsDraft).then(() => {
defaultSaveOperation = "draft";
})
}
And also add a return here
This would however change the execution flow, defaultSaveOperation
would now be set only once CompleteGenericSendMessage
has finished, and not before the call to gMsgCompose.sendMsg
.
Issue #2
gMsgCompose.sendMsg
returns before the actual send has finished. The progress listener is monitoring the send process, which ends here.
So all the code after gMsgCompose.sendMsg
is executed before the send has finished and also on failed sends. Including the aftersend event and telemetry collection. The catch around gMsgCompose.sendMsg
does not catch any errors encountered during send.
Did that behaviour change with the switch to the JavaScript send implementation?
Would it be better to promisify and await the progress listener?
For the WebExt API caller, I would like to be able to await the entire send process, so I would like to change the code to await the progress listener there as well. Objections?
Comment 3•3 years ago
|
||
Duplicate of bug 1732554? Yes we should fix that.
Comment 4•3 years ago
|
||
Yes, I think issue#1 is bug 1732554. We should add await.
Issue #2
gMsgCompose.sendMsg returns before the actual send has finished. The progress listener is monitoring the send process, which ends here.
So all the code after gMsgCompose.sendMsg is executed before the send has finished and also on failed sends. Including the aftersend event and telemetry collection. The catch around gMsgCompose.sendMsg does not catch any errors encountered during send.
I think this is the same before the js rewrite, listeners are used to notify sending error and finished. nsIMsgCompose.sendMsg returns only means the message has been assembled and passed over to the smtp/nntp module. The communications with servers are async.
Would it be better to promisify and await the progress listener?
Currently we have several listeners (progress, sending, copy), they're difficult to follow. I think we have plan to rewrite nsMsgCompose.cpp (nsIMsgCompose.idl) in JS. Before that, perhaps it's easier to just wait for listeners.
Assignee | ||
Comment 5•3 years ago
|
||
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 6•3 years ago
|
||
- I do not know why I get a clang-format error, it does not tell me what he does not like and I get no lint errors locally
- I had to requestLongerTimeout for Linux, I already split the test and I probably need to split it more, but since this includes a string, I would like to get it landed before merge day.
Comment 7•3 years ago
|
||
I do not know why I get a clang-format error, it does not tell me what he does not like and I get no lint errors locally
Maybe related to bug 1771483, I think you can ignore the error.
Updated•3 years ago
|
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/114f30010216
Allow compose.sendMessage() to await the entire send process and add compose.saveMessage(). r=mkmelin
Comment 9•3 years ago
|
||
Looks like debug test failure: https://treeherder.mozilla.org/logviewer?job_id=379535332&repo=comm-central&lineNumber=5030
Assignee | ||
Comment 10•3 years ago
•
|
||
All platforms or just MacOS? Only that test (of the new API function) or also tests of core functionality?
Comment 11•3 years ago
|
||
Looks like all platforms for debug. Just that test I think.
Assignee | ||
Comment 12•3 years ago
|
||
Assignee | ||
Updated•3 years ago
|
Comment 13•3 years ago
|
||
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/70099087423a
Make folder optional in MessageHeader (which it is) to fix debug test fails. r=mkmelin
Description
•