1677062 - [Compose API] Add onNew, onReply and onForward event or expose gComposeType

Status: RESOLVED FIXED

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

Description

[John Bieling (:TbSync)]

It should be possible to know whether a composer window was created as new, as a reply or as a forward.

We could add onNew, onReply and on Forward events, and/or expose the gComposeType in a usable way. Maybe as part of the ComposeDetails object?

Comment 1

[John Bieling (:TbSync)]

Attachment: bug1677062_expose_gComposeType.patch

Adding a new type to the ComposeDetails, being "new", "reply", "forward" or "draft".

Comment 2

[John Bieling (:TbSync)]

Comment on attachment 9204539 [details] [diff] [review]
bug1677062_expose_gComposeType.patch

Geoff has a few other reviews, so asking you.

Comment 3

[Geoff Lankow (:darktrojan)]

Comment on attachment 9204539 [details] [diff] [review]
bug1677062_expose_gComposeType.patch

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

Stealing this review.

::: mail/components/extensions/parent/ext-compose.js
@@ +291,5 @@
> +    case Ci.nsIMsgCompType.ForwardInline:
> +      type = "forward";
> +      break;
> +    default:
> +      type = composeWindow.gComposeType;

Not sure we could ever get here but it's probably best to leave type undefined if we do.

::: mail/components/extensions/schemas/compose.json
@@ +110,5 @@
>              "optional": true
>            },
> +          "type": {
> +            "type": "string",
> +            "description": "Read-only. Basic type of the compose window.",

This isn't very clear. Maybe something like "What sort of message is being composed."?

::: mail/components/extensions/test/browser/browser_ext_compose_details.js
@@ +718,5 @@
> +  await extension.startup();
> +
> +  // The first part of the test is done in the background script using the
> +  // compose API to open compose windows. For the second part we need to open
> +  // a draft, which is not possible with the compose API.

We should fix that. But not here, obviously.

Comment 4

[John Bieling (:TbSync)]

Attachment: bug1677062_expose_gComposeType_v2.patch

Fixing the reported issues.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/947d10fcdca7
Expose gComposeType as type in ComposeDetails. r=darktrojan

Comment 6

[Addons]

Excuse the "drive-by" comment, but why do you distinguish starting from a draft from the "new" set which includes Ci.nsIMsgCompType.EditTemplate. In the latter case, the message is most likely not going to be sent, but instead saved as a template again. You can also edit a draft "as new", the difference is that in that case the original draft is not deleted when the message is sent. In comment #0 you said that you wanted to distinguish new, reply and forward, but now you also have draft but don't differentiate messages that were based on another message ("as new") or a template. Hard to tell whether add-ons care.

Comment 7

[John Bieling (:TbSync)]

During my tests draft seemed special, as independent of the original type (new, reply, forward), the saved email would be of type draft and it has a special "This is a draft" notification in the display message area (the original type was no longer recoverable, iirc). Other types like EditTemplate behave like "new".

So from the perspective of an add-on developer I asked myself what types could be useful, trying not to expose too much internal stuff which could get removed/changed in the future. What do you propose to change?

Comment 8

[Addons]

I don't understand the first paragraph. Ci.nsIMsgCompType.Draft means that the composition was started from a draft. It's not really related to the fact that a saved draft in the Drafts folder displays a "This is a draft" notification. Internally and technically, new message from draft/template and "edit new" are mostly the same code path.

As for the question: This is a tough call. You're designing an interface without knowing the future use. I've just noticed an inconsistency. New, reply and forward are clear, but suddenly you've split the "draft new" from the other "news".

I guess I'd have the three initial categories, new, reply, forward, or if you want to split "new" some more, then I'd split it completely:
new - as in brand new
new from draft
new from template (EditTemplate would fall in to the "new from template" category, but different from "Template")
new from other message (edit as new).

You could even stick this into the string, so people who just care about new, can do a pattern matching/substring, so something like:
new, new.draft, new.template, new.edit.

Comment 9

[John Bieling (:TbSync)]

How do I trigger the two template options ("edit template" and "new from template") from within Thunderbird?

Comment 10

[Addons]

First you need to prepare a template by starting a new composition, entering the template information (typically some formatting or standard reply, see below) and saving it as a template. Then go to the template folder, you have "New Message from Template" and "Edit Template" in the context/right-click menu. Double-click triggers the former. FYI, templates are also used in the filter manager, there is a filter action "Reply with Template", so that's TB's version of the "out of office" reply. But it only works if TB is running.

Comment 11

[John Bieling (:TbSync)]

Thanks.

Thinking about this, I tend to remove the drafts type. I see the inconsistency now.

Comment 12

[Addons]

Tough call. Personally, I would have just handed the numeric compose type back. I'd be in favour of returning more information rather than less. You don't want to revisit this when it turns out that people want to distinguish the different types of "new". That said, you filed the bug, so did you have any special requirement in any particular add-on in mind?

Comment 13

[John Bieling (:TbSync)]

I filed in on behalf of someone from discuss.thunderbird.net. Sadly I did not put the bug number in the thread back then and I failed to find it again. But it was about being able to detect replies.

From other implementation I worked I know the WebExtension interface must be sort of independent of TB internals, so we do not break anything if for whatever reason the ids change. So the type must be a well-defined string.

Let us see if I can get some broader feedback.

Comment 14

[Addons]

... it was about being able to detect replies.

With your technique you can only detect the reply when the user replies. If he/she gets interrupted, saves the reply as draft and resumes replying later, you will detect it as new/draft. To detect the message as reply again, one would have to inspect the headers, like the "in-reply-to", which appears to be saved on the draft. I guess that's used to tag the original message as replied when the reply is finally sent, even if it was stored a few times as draft in the meantime. It would be a bit trickier for forward, looks like the "in-reply-to" header appears there, too, and there is also a "X-Forwarded-Message-Id" header. It all depends on what you want to do and what your requirements/specification are.

Comment 15

[John Bieling (:TbSync)]

But that would mean we have to keep the draft type and allow to inspect the headers in order for extension to know what it was before. I think there is a bug for that request already. This would also mean we would need the template type.

Comment 16

[Addons]

(In reply to John Bieling (:TbSync) from comment #11)

Thinking about this, I tend to remove the drafts type. I see the inconsistency now.

(In reply to John Bieling (:TbSync) from comment #15)

But that would mean we have to keep the draft type and allow to inspect the headers in order for extension to know what it was before. I think there is a bug for that request already. This would also mean we would need the template type.

So what's the way forward here? Follow-up bug? Some other bug already exists?

Comment 17

[John Bieling (:TbSync)]

Follow up bug adding template.

Comment 18

[Addons]

Please post the number here.

Comment 19

[John Bieling (:TbSync)]

Comment on attachment 9204547 [details] [diff] [review]
bug1677062_expose_gComposeType_v2.patch

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined:
Testing completed (on c-c, etc.):
Risk to taking this patch (and alternatives if risky):
I am evaluating.