Closed Bug 1747456 Opened 3 years ago Closed 3 years ago

`compose.sendMessage` options.mode is missing many send modes

Categories

(Thunderbird :: Add-Ons: Extensions API, enhancement)

Thunderbird 91
enhancement

Tracking

(thunderbird_esr91 wontfix)

RESOLVED FIXED
102 Branch
Tracking Status
thunderbird_esr91 --- wontfix

People

(Reporter: siefkenj, Assigned: TbSync)

References

Details

Attachments

(2 files)

Steps to reproduce:

The compose.sendMessage(tabId, options) call allows options.mode === "default" || "sendNow" || "sendLater". However, there are many more modes supported.

https://searchfox.org/comm-central/rev/9d89afee9a36846454e2a4ef5226cb037c05b1e2/suite/mailnews/components/compose/content/MsgComposeCommands.js#1886

Seems to list 7 options. Of particular interest to me is saveAsDraft. But maybe most (all?) should be exposed?

For my own add-on Mail Merge having the two modes "saveAsDraft" and "saveAsTemplate" would be necessary.

@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?

Flags: needinfo?(mkmelin+mozilla)

Duplicate of bug 1732554? Yes we should fix that.

Flags: needinfo?(mkmelin+mozilla)
See Also: → 1732554

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: nobody → john
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #9277089 - Attachment description: Bug 1747456 - Allow compose.sendMessage() to await the entire send process. r=rnons → WIP: Bug 1747456 - Allow compose.sendMessage() to await the entire send process and add compose.saveMessage(). r=mkmelin
Attachment #9277089 - Attachment description: WIP: Bug 1747456 - Allow compose.sendMessage() to await the entire send process and add compose.saveMessage(). r=mkmelin → Bug 1747456 - Allow compose.sendMessage() to await the entire send process and add compose.saveMessage(). r=mkmelin

try run:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=0daae72edcce09674d6cfab6d276dac718c3d35b

  1. 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
  2. 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.

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.

Target Milestone: --- → 102 Branch

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

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED

All platforms or just MacOS? Only that test (of the new API function) or also tests of core functionality?

Looks like all platforms for debug. Just that test I think.

Flags: needinfo?(john)

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

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: