Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert to pure WebExtensions #1

Closed
wants to merge 3 commits into from
Closed

Conversation

jobisoft
Copy link

This PR removes your Experiment and replaces it by WebExtension code.

To get the body, I use messages.getFull() and search for the first text/plain body part. I included a 3rd party library to parse email addresses. To make things simple, I converted the background to a module to be able to use ES6 modules.

@jobisoft
Copy link
Author

The onCommand event now has a second parameter to give you the active tab, so I could remove getCurrentWindow().

}
},
"name": "Copy Patch",
"description": "Copy email content to clipboard for application as patch",
"author": "Jan Kiszka",
"version": "2.1.3",
"version": "2.1.4",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Release will happen later.

parse5322.parseAddressList = parseAddressListSimple;
parse5322.parseFrom = parseFromSimple;
parse5322.parseSender = parseSenderSimple;
parse5322.parseReplyTo = parseReplyToSimple;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a valuable tool, but I'm not a fan of forking it here. We should probably make this addon an npm package so that its dependency can be pulled with standard tools.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have the bandwidth to do that, this is of course doable. I did not want to force you to having to build your add-on using a complex toolchain.

@jan-kiszka
Copy link
Owner

Thanks for the conversion. The changes look helpful, will test them in a minute.

However, I would rather like to do the conversion in smaller, logically separated commits. If you have the time, please factor out e.g. style changes, getCurrentWindow removal etc. Let me know if you have time to do that as well or if I should help out.

@jobisoft
Copy link
Author

jobisoft commented Aug 6, 2023

What style changes are you referring to? All I did was to move your legacy code into your background and include a 3rd party library. I do not see how that can be done in smaller steps and having a working add-on after each step. What part are you worried about in particular? I could add more explanatory comments, if that would help, but I do not have the bandwidth to redo this PR.

I have many other add-ons, where I have to support developers to move from legacy to WebExt: https://thundernest.github.io/add-on-reports/lost-tb102-to-tb115-worst-case.html

Thunderbird will move to stable monthly releases in 2024, and it is not plausible to expect add-on devs to release a new version for their Experiment add-ons every 4 weeks. So we have to get them away from Experiments where possible.

setTimeout(() => {
messenger.messageDisplayAction.setBadgeText(
{tabId: sender.tab.id, text: null});
{ tabId: sender.tab.id, text: null });
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These three are examples for style changes I was referring to.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will try to undo the automatic pretty format changes caused by Visual Studio Code tomorrow.

@jan-kiszka
Copy link
Owner

No problem, then I will rework the changes myself.

BTW, there is also https://addons.thunderbird.net/de/thunderbird/addon/toggle-line-wrap which is still using Experiments because TB lacks that feature natively (https://bugzilla.mozilla.org/show_bug.cgi?id=1587153). Just had to bump its version as well, and if I have to do that every other month soon, that would become rather unhandy.

@jan-kiszka
Copy link
Owner

Don't worry, I will rebuild the series to form a clean patch queue that can go into master.

@jobisoft
Copy link
Author

jobisoft commented Aug 7, 2023

I think toggle line wrap can be recreated as a pure WebExtension without using any internal Thunderbird stuff. Either by using a compose script to inject a css to toggle a line wrap visually and use onBeforeSend event to actually manipulate the outgoing plaintext mail to break at the desired places, or manipulate the DOM of the message while the user is typing.

@jan-kiszka
Copy link
Owner

I think toggle line wrap can be recreated as a pure WebExtension without using any internal Thunderbird stuff. Either by using a compose script to inject a css to toggle a line wrap visually and use onBeforeSend event to actually manipulate the outgoing plaintext mail to break at the desired places, or manipulate the DOM of the message while the user is typing.

Honestly, I would rather prefer this finally being addressed in TB because none of those solutions seems simple to me. So, if I have to invest time, that should go into a upstream patch. Let's follow up on that issue.

@@ -3,23 +3,20 @@
"applications": {
"gecko": {
"id": "[email protected]",
"strict_min_version": "78.4.0",
"strict_max_version": "102.*"
"strict_min_version": "102.0"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must become 106.0 due to using the tab parameter of the onCommand callback.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

@jobisoft jobisoft Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is documented in the 102 branch of the API as well:
https://webextension-api.thunderbird.net/en/102/commands.html#events

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was reading https://webextension-api.thunderbird.net/en/latest/commands.html - seems your documentation is outdated then. To which version of 102 was this added then exactly?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, 102.3.3 then.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But to be precise, it must be set to 102.4.0. So indeed a good catch! (102.3.3 does not exist in the list of allowed versions)

// Many thanks to Dominic Sayers and his documentation on the is_email function,
// http://code.google.com/p/isemail/ , which helped greatly in writing this parser.

export function parse5322(opts) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meanwhile I understood that this was a local modification on top of the original version. But we can do better, we can go upstream first (provided upstream is still merging): jackbearheart/email-addresses#68

}
return null;
}
// Compatibility layer for former experiment.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reads like as if there could be a better way. However, to my understanding, there is no way for the content script to access browser.messages directly. Therefore, we need such an interface, no?

@jan-kiszka
Copy link
Owner

#2 is supposed to carry everything of this PR, just broken up and slightly massaged.

@jan-kiszka jan-kiszka closed this Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants