Closed Bug 1747408 Opened 5 months ago Closed 4 months ago

compose.getMessageDetails and compose.setMessageDetails should be reflexive

Categories

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

Thunderbird 91
enhancement

Tracking

(thunderbird_esr91 wontfix)

RESOLVED FIXED
98 Branch
Tracking Status
thunderbird_esr91 --- wontfix

People

(Reporter: siefkenj, Assigned: TbSync)

Details

Attachments

(1 file)

Steps to reproduce:

One should be able to duplicate a message by applying the results of getMessageDetails to setMessageDetails.

For example
await browser.compose.setMessageDetails(9, await browser.compose.getMessageDetails(8)) should duplicate the message details of the message in tab 8 to tab 9

Actual results:

setComposeDetails errors with Only one of body and plainTextBody can be specified..

Expected results:

The message details should be duplicated. Is there already some way to assign precedence to html vs plain text bodies?

I hesitate to change that historical return value definition, as it would be a backward incompatible change. Currently, you have to check the isPlainText flag to know, if the message is html or plain text and then include only the appropriate value (body or plainTextBody) to clone the mail.

The backward incompatible change would be to make isPlaintext a required property and base the format of the new email only on that value and no longer depend on body or plainTextBody being set. That will however break many add-ons. I think your best option is to write a littler wrapper, which removes body or plainTextBody from the ComposeDetails object before using it to create the new email.

Can we have the best of both worlds? If isPlainText is present, pick the appropriate body, and if it is not, error like the current API.

I had the same idea a few seconds ago. Yes. That will work.

Assignee: nobody → john
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

Additionally, the documentation says The compose format of an already opened compose window cannot be changed. Setting details.body, details.plainTextBody or details.isPlaintext will fail if the compose format of the compose window does not match. Use getComposeDetails(tabId) to get the current compose format., so perhaps the compose window should pick preferentially rather than it being taken from the message details?

I've also noticed that compose.newMessage has the same error. I assume a fix can solve all of these instances at once :-)

(In reply to siefkenj from comment #5)

I've also noticed that compose.newMessage has the same error. I assume a fix can solve all of these instances at once :-)

I mean compose.beginNew

isPlainText is dominant now and I think it even fixes another glitch, which sadly changes behaviour:

  1. if isPlainText is set to true and a body is present, the body is ignored now and a plain text composer is created. The old code picked html mode in this case.
  2. if isPlainText is set to false and a plainTextBody is present, the plainTextBody is ignored and an html composer is created. The old code picked also html in this case.

Are you able to build and test the patch?

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

Are you able to build and test the patch?
I've never build thunderbird before or applied a patch...is there a guide?

Also, Phabricator won't let me comment, but is there a reason for strict inequality testing against null? (e.g. details.body !== null on line 137) Won't this allow undefined to pass the check?

Also, Phabricator won't let me comment

Do you have a Phabricator account? It should work then, or I have to invite you. What is your username?

there a reason for strict inequality testing against null?

IIRC you cannot pass undefined into a WebExtension parameter. It is either of the specified type (string or boolean in this case) or null if not specified (for optional parameters). I need to distinguish between set (true/false/non-empty-string/empty-string) and unset. I guess I could use != as well, will check.

I've never build thunderbird before or applied a patch...is there a guide?

https://developer.thunderbird.net/thunderbird-development/getting-started
Once you have build TB successfully, install moz-phab

./mach install-moz-phab

and apply my patch in the comm folder

moz-phab patch D134648

and build again. If you need further help on this, reach out to me via our matrix chat channel(s):
https://developer.thunderbird.net/add-ons/community

I am TbSync there as well.

(In reply to siefkenj from comment #4)

Additionally, the documentation says The compose format of an already opened compose window cannot be changed. Setting details.body, details.plainTextBody or details.isPlaintext will fail if the compose format of the compose window does not match. Use getComposeDetails(tabId) to get the current compose format., so perhaps the compose window should pick preferentially rather than it being taken from the message details?

What curently throws, but should not according to your request:

  • plainTextBody and body in beginNew, but no isPlainText
    -> should create a composer in the users default format and pick the correct value from the provided body/plainTextBody properties
  • plainTextBody and body in update, but no isPlainText
    -> should pick the correct value from the provided body/plainTextBody properties according to the current format of the composer

What will continue to throw:

  • plainTextBody and/or isPlainText = true in an update for an html composer
  • body and/or isPlainText = false in an update for a plain text composer

Is that correct?

The patch works for me! I can now do browser.compose.setComposeDetails(7, await browser.compose.getComposeDetails(6)) from inside an extension context.

Attachment #9256757 - Attachment description: WIP: Bug 1747408 - Make compose.getComposeDetails() and compose.setComposeDetails() reflexive. → Bug 1747408 - Make compose.getComposeDetails() and compose.setComposeDetails() reflexive. r=mkmelin

This is the documentation for the new behaviour, taken from the ComposeDetails definition:

  • body : The HTML content of the message. Ignored by setComposeDetails, if used on a plain text composer. If only body is specified when used with beginNew, beginReply and similar functions, an HTML message will be created. The recommended usage is to always specify both (body and plainTextBody) and use isPlainText to select the compose format, or use the users default compose format.

  • plainTextBody : The plain text content of the message. Ignored by setComposeDetails, if used on an HTML composer. If only plainTextBody is specified when used with beginNew, beginReply and similar functions, a plain text message will be created. The recommended usage is to always specify both (body and plainTextBody) and use isPlainText to select the compose format, or use the users default compose format.

  • isPlainText : Wether the message is an HTML message or a plain text message. Can be used to specify the message format in beginNew, beginReply and similar functions. It is however not possible to change the compose format with setComposeDetails. Note: Using isPlainText together with a single but conflicting body type (e.g.: isPlainText = true and body is set but not plainTextBody) will cause an exception.

Regarding Comment 4: I did not alter the behaviour, that setComposeDetails() cannot change the format, because that would require a lot of core changes, as it is really not possible, even for Thunderbird itself. Creating a bug in core for that general capability might be a good idea, I can then pick up the feature for the WebExt API. But setComposeDetails() now ignores details.isPlainText and picks what it needs, either body or plainTextBody - instead of throwing. This could leave the user with an empty composer, if the developer only specified the "wrong" body type. But I decided to not throw in that case anymore, to keep the rules simple, which is just "body: Ignored by setComposeDetails, if used on a plain text composer." (and its counterpart).

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/0c0caa1e2745
Make compose.getComposeDetails() and compose.setComposeDetails() reflexive. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → 98 Branch

The patch does not apply cleanly to 91. I do not want to risk breaking add-ons in 91, so I set it to wontfix for ESR.

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