v2: Port to Thunderbird stable 78.4+ then 82+, rewrite add-on from scratch, highlight.js

[Qeole]

Following Thunderbird's switch to MailExtensions, the add-on had to be ported as a "legacy" extension for v68. This proved to be a short relief, since the following stable, v78, does not support those anymore.

The new version of the add-on is a pure MailExtension that should be compatible with new Thunderbird versions. It relies on the messagesModify API that appeared in Thunderbird 82, but may be backported to version 78 in the future (see related bug).

See README.md from related branch for installation instructions.

Various parts of the add-on where refering to XUL overlays or chrome components, but none are available with MailExtensions. Instead of struggling to adapt these parts, the add-on was entirely rewritten from scratch, and with a different approach. We no longer parse and reformat the diffs: Instead, we embed and inject the highlight.js library that takes care of coloring the diff.

Consequences and main changes:

• We loose some accuracy, because we do not rely on "what part of the diff we are in". Concretely, all lines starting with + or - will be colored, even if in the commit log.

• We gain in stability, with less parsing susceptible to break.

• We loose the different modes, unified / context / side-by-side. The diffs are not rewritten, we simply add colors.

• We keep the ability to replace tabs and spaces, but loose the line numbers.

• It is not possible at this time to specify custom colors for the diffs, but we can pick a style amongst the hundred or so that are supported by highlight.js.

This PR is a draft because it is not complete yet. I intend to add back some kind of UI to deactivate coloring and toggle white spaces replacement from the message window. Posting mostly for trial and feedback.

Resolves: #93

[fbezdeka]

Thanks for keeping this plugin alive!

The following patch (on top of this branch) makes it work with Thunderbird 78.4, which was released some hours ago:

diff --git a/manifest.json b/manifest.json
index 2711d7d..a0690dd 100644
--- a/manifest.json
+++ b/manifest.json
@@ -3,7 +3,7 @@
   "applications": {
     "gecko": {
       "id": "{282C3C7A-15A8-4037-A30D-BBEB17FFC76B}",
-      "strict_min_version": "82.0a1"
+      "strict_min_version": "78.4"
     }
   },
   "name": "Colored Diffs",

[Qeole]

Thanks a lot for testing and for the patch!

I was wondering when 78.4 would be out. Ok, now it's been released, I must really find the time to finish this PR. I've made a lot of improvements locally, but I'm still working on some UI bits to disable/re-enable colors and whitespace-replacement without having to navigate to the options.

I'll try to have a version ready to publish this weekend.

[fbezdeka]

There is currently a problem after removing the plugin. If you try to reinstall the plugin after a removal, the installation itself seems to work, but the plugin remains "inactive".

I applied the following patch to diff.js to get better parsing results. Maybe this is "hot-patchable" after loading the scripts. Not sure... Maybe you like to play with it...

The following was addressed with that patch:

• Do not mark lines starting with "--" as removal. This may part of mail signatures so I changed the removal regex to match one "-" and following whitespaces only
• Mark lines starting with "index" as meta-strings. (Maybe this is the wrong class...)
• Allow lines starting with "@@" to be marked with the meta class even if something follows the closing "@@"
• Mark lines starting with "diff --git" as comments

--- diff_orig.js    2020-10-22 13:32:10.918791307 +0200
+++ diff.js 2020-10-22 13:36:59.591611350 +0200
@@ -13,10 +13,16 @@
     aliases: ['patch'],
     contains: [
       {
+        className: "meta-string", 
+        variants: [
+          {begin: /^index/, end: /$/ }
+        ]
+      },
+      {
         className: 'meta',
         relevance: 10,
         variants: [
-          {begin: /^@@ +\-\d+,\d+ +\+\d+,\d+ +@@$/},
+          {begin: /^@@ +\-\d+,\d+ +\+\d+,\d+ +@@/},
           {begin: /^\*\*\* +\d+,\d+ +\*\*\*\*$/},
           {begin: /^\-\-\- +\d+,\d+ +\-\-\-\-$/}
         ]
@@ -29,7 +35,8 @@
           {begin: /^\-{3}/, end: /$/},
           {begin: /^\*{3} /, end: /$/},
           {begin: /^\+{3}/, end: /$/},
-          {begin: /^\*{15}$/ }
+          {begin: /^\*{15}$/ },
+          {begin: /^diff --git/, end: /$/},
         ]
       },
       {
@@ -38,7 +45,7 @@
       },
       {
         className: 'deletion',
-        begin: '^\\-', end: '$'
+        begin: '^\\-\\s+', end: '$'
       },
       {
         className: 'addition',

[Qeole]

There is currently a problem after removing the plugin. If you try to reinstall the plugin after a removal, the installation itself seems to work, but the plugin remains "inactive".

Thanks, I'll have a look at it. Did you install and reinstall the plugin normally from the .xpi file? (Or did you use the Load Temporary Add-on feature from the add-on debugging section?) What happens exactly when the plugin is “inactive”, I take it nothing gets colorized? Does the add-on preferences selection UI work?

I applied the following patch to diff.js to get better parsing results. Maybe this is "hot-patchable" after loading the scripts. Not sure... Maybe you like to play with it...

The following was addressed with that patch:

• Do not mark lines starting with "--" as removal. This may part of mail signatures so I changed the removal regex to match one "-" and following whitespaces only

This one looks tricky, you can have double dashes in a variety of cases (diff on Markdown with itemized lists, diffs of diffs, merge conflict markers I think, ...). I'd rather exclude -- as single change than just prevent double dashes, but I'm not sure if that's doable for highlight.js.

• Mark lines starting with "index" as meta-strings. (Maybe this is the wrong class...)
• Allow lines starting with "@@" to be marked with the meta class even if something follows the closing "@@"
• Mark lines starting with "diff --git" as comments

These look like good (and generic) changes. I can have a look at hot-patching them, although I'm not sure if this is really something we want in the add-on. Have you considered submitting them upstream to the hljs library? Then we could simply pull them in the add-on?

[fbezdeka]

There is currently a problem after removing the plugin. If you try to reinstall the plugin after a removal, the installation itself seems to work, but the plugin remains "inactive".

Thanks, I'll have a look at it. Did you install and reinstall the plugin normally from the .xpi file? (Or did you use the Load Temporary Add-on feature from the add-on debugging section?) What happens exactly when the plugin is “inactive”, I take it nothing gets colorized? Does the add-on preferences selection UI work?

• I did not use the "load temporary add-on" feature
• Inactive: No error reported during installation, but no coloring of mails

After removing the add-on I had to restart Thunderbird before installing (a new test-version) again.

I applied the following patch to diff.js to get better parsing results. Maybe this is "hot-patchable" after loading the scripts. Not sure... Maybe you like to play with it... The following was addressed with that patch:

• Do not mark lines starting with "--" as removal. This may part of mail signatures so I changed the removal regex to match one "-" and following whitespaces only

This one looks tricky, you can have double dashes in a variety of cases (diff on Markdown with itemized lists, diffs of diffs, merge conflict markers I think, ...). I'd rather exclude -- as single change than just prevent double dashes, but I'm not sure if that's doable for highlight.js.

Right. Markdown seems a reasonable test-case. 99% of my mails are kernel patches, so I did not recognize it yet. To fix this properly we need a proper parser I guess. Unsure if highlight.js can help out here.

• Mark lines starting with "index" as meta-strings. (Maybe this is the wrong class...)
• Allow lines starting with "@@" to be marked with the meta class even if something follows the closing "@@"
• Mark lines starting with "diff --git" as comments

These look like good (and generic) changes. I can have a look at hot-patching them, although I'm not sure if this is really something we want in the add-on. Have you considered submitting them upstream to the hljs library? Then we could simply pull them in the add-on?

I can try to submit a PR. Let's see if they would accept it...

[Qeole]

Here is a version that looks clean enough to be pushed :tada:.

I could not reproduce the issue mentioned by @fbezdeka on re-installing the add-on.

I hit another issue, which is the add-on not working at all on v78.4, but that was with a profile downgraded from v83. With a clean profile, the add-on seems to work on v78.4 just fine.

No hot-patching of the hljs library at this time.

I'll push that version to master, and push the .xpi to addons.thunderbird.net. We can still do polishing or bug fixing if necessary after that.

[marxin]

@Qeole Thanks for working on that!

I've just installed the new plug-in from addons.thunderbird.net (version 2.0.0 according to Add-ons Manager) and I have TB 78.4 version. But the plug-in does not work for an email with a diff. I was using the old version some time ago, and it worked well. Any tips on how to debug that?

[fbezdeka]

@marxin, have you tried "uninstall plug-in" -> restart Thunderbird -> install plug-in again?

I was facing a similar problem when trying this patch but was unable to find the root-cause... Restarting Thunderbird after plugin removal helped, though.

[marxin]

@marxin, have you tried "uninstall plug-in" -> restart Thunderbird -> install plug-in again?

I've just tried that and it didn't help me.

It fails for me here:

uncaught exception: Object 2 ext-extensionScripts.js:161:12

in the following place:

[Qeole]

No idea where the error comes from :cry:. This is the same error I see after downgraded my TB profile, it's pretty generic and all I understand from it is “something in the script you register (or in registration itself?) isn't good”, but I haven't found what it complains about. I haven't managed to pin down the specific conditions for this to happen, either. For what it's worth I didn't see the issue on a clean 78.4 profile, or even after an add-on upgrade on v82 (Anyone who tries the dev versions of TB, do backup you profile or use a junk one, downgrading a profile is not possible - Well it is with the relevant command line option, but clearly not recommended).

I'll try to look some more into this. Please don't hesitate to file an issue next time, so that other people can track this as well :).

[marxin]

@Qeole Great, I've got it. The extension works for me in a New Window, but not in a new Tab: Is it a known limitation or a feature :) ? Anyway, thanks a lot for working on the plug-in!

[Qeole]

Certainly not a feature, but not something I knew either. I don't often use tabs, haven't tested with a new one. This needs more investigation... Happy you have at least the new window working!