Closed Bug 1456988 Opened 2 years ago Closed 2 years ago

Make dictionaries compatible by default. (Thunderbird 60 made my native language dictionary incompatible.)

Categories

(Toolkit :: Add-ons Manager, defect, major)

60 Branch
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
thunderbird_esr60 + ---
firefox63 --- fixed

People

(Reporter: kevinberaca, Assigned: mkmelin)

References

()

Details

(Whiteboard: [wanted for Thunderbird 60])

User Story

https://support.mozilla.org/en-US/questions/1217316
https://discourse.mozilla.org/t/dictionaries-will-break-if-60-0-ships-soon/
https://discourse.mozilla.org/t/greek-spelling-or-spellchecking-dictionary/

Attachments

(2 files, 3 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180323154952

Steps to reproduce:

Automatically updated to TB 60 beta


Actual results:

The Romanian spelling dictionary became incompatible


Expected results:

The Romanian dictionary should have kept working.

I say that because in Firefox, for example, I'm using the exact same dictionary. When FF switched to webextensions, I remember it saying something like "this is going to be incompatible with the new standard etc.", but after updating to Quantum, it was still there, even though it hasn't been updated since 2013 (https://addons.mozilla.org/en-US/firefox/addon/romanian-spellchecking-diction/).

In an e-mail app, typing is the main thing you do so no matter what other updates or improvements you make, if the main thing isn't working properly then nothing else matters. I don't think I make many spelling mistakes, but once in a while it comes in very handy.

While doing various tests, I found the config editor setting to bypass this restriction (extensions.strictCompatibility - false), but for most people who aren't tech savvy, this isn't really an option and, if things happen like with Firefox, even this setting might not work after a while.

Thankfully I had the time to figure things out, but I think this is an oversight that needs to be addressed.

Thanks in advance.
Blocks: tb60found
Component: Untriaged → General
Can we just auto update the Thunderbird dictionaries to be compatible with 60? 
And those that are restartless, <em:type>64</em:type>, to be compatible with latest?


Philipp, what do you think?
Flags: needinfo?(philipp)
Update server side, if that wasn't clear.
It might be possible, I haven't done a such thing though. The problem is that we'd need to do this compatibility bump on every beta release, which can be a fair amount of work. If we can fix this client side to ignore strict compatibility that would be preferable. TheOne would know the implications.
Flags: needinfo?(philipp) → needinfo?(awagner)
Yes, making dictionaries compatible by default sounds like the right approach. I am not aware of any implications, but that doesn't mean there aren't any.
Flags: needinfo?(awagner)
We would need this soon. How do we make it happen?
I face the problem here with 60.0b6 (64-bit) when I add Français dictionnary.
Philipp, is there someone other than you that can handle this?
Flags: needinfo?(philipp)
For information, I worked arround the problem by allowing Thunderbird to install plugins even they are not explicitely told to be compatible with 60. This is done by editing the "extensions.strictCompatibility" setting to "false". I temporarily set this for the dictionnary, and then got back to the default "true" value.
User Story: (updated)
I'm certain there is someone else who can do this, then again I don't think I signed up for doing it. IIUC, making dictionaries compatible by default requires a m-c patch that would allow for this. This is an engineering change I'd rather have someone else handle. I don't know that code very well.

Manually bumping all the dictionaries on AMO seems like a lot of work given it will need QA and such, so the first option sounds more lucrative to me.
Flags: needinfo?(philipp)
To clarify, I wasn't suggesting you (fallen) should take this on - quite the opposite.  

I am just searching for the person/solution so we can have this in place when Thunderbird 60.0 is released - because we aren't making significant progress thus far, we typically we don't get fast progress on m-c patches, and most most of the dictionaries I see haven't had updates in recent years. (and compat wasn't done for firefox 60 or earlier, so we might assume it's not important to them)

Eyeballing the list at https://addons.mozilla.org/en-US/thunderbird/tag/dictionary?sort=users it's hard to know many users might be affected (the stats include firefox users).  Assuming more Thunderbird than Firefox users would want a dictionary (I don't know that's true), my guess is at least half a million Thunderbird users are affected.
Summary: Thunderbird 60 made my native language dictionary incompatible → Make dictionaries compatible by default. (Thunderbird 60 made my native language dictionary incompatible.)
Whiteboard: [wanted for Thunderbird 60]
Taking a stab that this needs to go to  Toolkit, Add-ons Manager
Component: General → Add-ons Manager
Product: Thunderbird → Toolkit
Version: 60 → 60 Branch
Dictionaries are already default compatible in gecko 60.
They are? Mind you we're settting extensions.strictCompatibility true in Thunderbird since we're still supporting old style add-ons - but those now generally need an update to work.
Compat by default only applies if strictCompatibility is not enabled. I'm not sure what legacy extensions have to do with setting strictCompatibility, though. We stopped setting that by default years before there was any idea of WebExtensions.
This is a request that dictionaries would still be compatible by default even when extensions.strictCompatibility is set to true
We set strictCompatibility to avoid that add-ons not "made fit" for Gecko 60 would do damage in TB. Since there have been a plethora of interface changes in M-C between 52 and 60, most add-ons would need some changes anyway.
Attached patch bug1456988_dict_compat.patch (obsolete) — Splinter Review
Untested (and probably incomplete)
(In reply to Magnus Melin from comment #17)
> This is a request that dictionaries would still be compatible by default
> even when extensions.strictCompatibility is set to true

I might accept a patch to do that for 60, since that's an ESR. But in general, we're trying to remove these kinds of special cases, and we're going to be removing support for legacy dictionaries in favor of WebExtension dictionaries (which set a maxversion of "*" by default), anyway.
I admit I'm not very knowledgeable when it comes to the inner workings of these kinds of things, but why not solve this like Firefox did - make a separate dictionary category and leave that always compatible?
https://imgur.com/a/3oYawDP

Like I mentioned at the beginning, even when Firefox made the switch to only support web extensions, this is why dictionaries remained compatible and, because of the separate category, this permanent compatibility didn't affect other addons.


Also, again, pardon my lack of knowledge, but isn't a dictionary basically a text list which typed words get compared to?
I get that sometime in the future, leaving dictionaries as legacy extensions, even in a separate category, might somehow cause some compatibility issues with other WebExtension features, but this doesn't seem to be the case for Firefox. At least not yet.

Furthermore, I get that living in an internet age, everything changes and gets updated more and more frequently, so addons shouldn't be seen as exceptions to that, but languages tend to evolve at a comparatively much slower pace, so dictionaries are usually seen as these monolithic things that don't really get modified very often (except when dictionary companies try to be "hip" and add new slang words).

Anyway, TL:DR, that's my suggestion. Separate dictionaries into a totally different category from normal addons like Firefox has done. Even if this might be more technically difficult to implement, it could be a more long-term solution.
[11:11] <marcoagpinto> guys?!
[11:11] <marcoagpinto> what's up with spellers in TB 60?
[11:11] <marcoagpinto> a guy reported for a tab "dictionaries" to be added and keep legacy spellers
[11:12] <marcoagpinto> I really don't know how to convert my speller
[11:41] <darktrojan> I still have my dictionary in 61
[11:42] <darktrojan> hmm, it doesn't appear to *work* though
[11:43] <%wsmwk> marcoagpinto: just change the max version afaik
[11:43] <marcoagpinto> wsmwk: I always change the max version
[11:43] <marcoagpinto> :)
[11:43] <marcoagpinto> but TB doesn't accept *
[11:43] <marcoagpinto> :)
[11:43] <marcoagpinto> FF accepts *
[11:47] <%wsmwk> hmm, then maybe that's why we are broken.  you should write detailed comments in the bug report
See Also: → 1451097
Attached patch bug1456988_dict_compat.patch (obsolete) — Splinter Review
For Thunderbird, I tested with a profile created with verstion 52, and installed a dictionary there - the swedish one - https://addons.mozilla.org/en-US/thunderbird/addon/g%C3%B6rans-hemmasnickrade-ordli/?src=ss  - which according to install.rdf has <em:maxVersion>50.0</em:maxVersion>

After that, install the latest Thunderbird version and start with that profile.

Without the patch, now that Thunderbird has strict compatibility turned on by default the dictionary was disabled. With the patch it's all ok.

I've been testing it with trunk. Since tb nightlies has strict compatibility off, also patched it locally to set extensions.strictCompatibility true in mail/app/profile/all-thunderbird.js
Assignee: nobody → mkmelin+mozilla
Attachment #8975357 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8989017 - Flags: review?(kmaglione+bmo)
Attachment #8989017 - Flags: feedback?(jorgk)
Comment on attachment 8989017 [details] [diff] [review]
bug1456988_dict_compat.patch

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

::: toolkit/mozapps/extensions/internal/XPIInstall.jsm
@@ +716,5 @@
>             " in install manifest");
>        continue;
>      }
> +    if (addon.type == "webextension-dictionary") {
> +      targetApp.maxVersion = "*";

I don't think this is necessary if you're also making the compat-checking changes.

@@ +2601,3 @@
>      let ignoreMaxVersion = false;
> +    // Ignore strict compatibility for dictionaries by default.
> +    let ignoreStrictCompat = (this.addon.type == "webextension-dictionary");

I don't think this is quite the right approach. I think you want something like:

  (!AddonManager.strictCompatibility || this.addon.type == "webextension-dictionary")

so that we just ignore the AddonManager strict compatibility setting for dictionaries, but still respect the add-on-specific one.

You'll also need to make a similar change here:

https://searchfox.org/mozilla-central/rev/e45edf9ebc1f8e89686cace12c60085af7a4cdc5/toolkit/mozapps/extensions/internal/XPIDatabase.jsm#487-490

Really, we should probably just make the strictCompatibility pref type-specific... but that's probably a different bug.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +2575,5 @@
>      let addons = await Promise.all(
>        Array.from(XPIStates.sideLoadedAddons.keys(),
>                   id => this.getAddonByID(id)));
>  
> +    return addons.filter(addon => (addon && addon.seen === false &&

This seems unrelated.
Attachment #8989017 - Flags: review?(kmaglione+bmo) → review-
Comment on attachment 8989017 [details] [diff] [review]
bug1456988_dict_compat.patch

I'll try the final version.
Attachment #8989017 - Flags: feedback?(jorgk)
This works too.
Attachment #8989046 - Flags: review?(kmaglione+bmo)
Attachment #8989017 - Attachment is obsolete: true
kmag: ping on the review
I think the condition in XPIDatabase.jsm can be optimized a bit...

    if (this.type in COMPATIBLE_BY_DEFAULT_TYPES &&
        !this.strictCompatibility &&
        (!AddonManager.strictCompatibility || this.type == "webextension-dictionary")) {
Comment on attachment 8989046 [details] [diff] [review]
bug1456988_dict_compat.patch

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

Sorry, I didn't notice this had been updated.

r=me with these changes.

::: toolkit/mozapps/extensions/internal/XPIDatabase.jsm
@@ +490,3 @@
>      if (this.type in COMPATIBLE_BY_DEFAULT_TYPES &&
> +        ((!AddonManager.strictCompatibility && !this.strictCompatibility) ||
> +         (!this.strictCompatibility && this.type == "webextension-dictionary"))) {

This can be written as:

  if (this.type in COMPATIBLE_BY_DEFAULT_TYPES &&
      !this.strictCompatibility &&
      (!AddonManager.strictCompatibility ||
       this.type == "webextension-dictionary"))

::: toolkit/mozapps/extensions/internal/XPIInstall.jsm
@@ +2598,4 @@
>      let ignoreMaxVersion = false;
> +    // Ignore strict compatibility for dictionaries by default.
> +    let ignoreStrictCompat = (!AddonManager.strictCompatibility ||
> +                              this.addon.type == "webextension-dictionary");

This can just be `= this.addon.type == "webextension-dictionary"`. The !strictCompatibility case gets caught on the next line.
Attachment #8989046 - Flags: review?(kmaglione+bmo) → review+
Thanks for moving this along. Just in time to land for building on Thursday.
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/integration/mozilla-inbound/rev/205d60219ab8
don't use strict compatibility for dictionaries. r=kmag
(In reply to Kris Maglione [:kmag] from comment #29)
> This can just be `= this.addon.type == "webextension-dictionary"`. The
> !strictCompatibility case gets caught on the next line.

Right, that was in my original patch. 

Thanx for the review! Landed with comments addressed, and a needed adjustment in test_strictcompatibility.js which a try push showed
https://hg.mozilla.org/mozilla-central/rev/205d60219ab8
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Uplift for Thunderbird60.
Flags: needinfo?(jorgk)
I've been waiting for you ;-) - It's on the list of bugs to uplift already.
Flags: needinfo?(jorgk)
Magnus, that patch doesn't apply to mozilla-esr60 at all. All hunks failed. Why do you think it would apply after three M-C revisions? :-(
Flags: needinfo?(mkmelin+mozilla)
Attached patch Mozilla60 version (obsolete) — Splinter Review
This is the mozilla60 version. Sadly test_strictcompatibility.js has changed completely. Kris and Magnus, can you please check the test changes.

I'm pretty sure about changing the last argument to 'true' in these
-  do_check_compat_status(true, [true, false, false, false, false, false, false], run_test_1);
+  do_check_compat_status(true, [true, false, false, false, false, false, true], run_test_1);

but I'm unsure about
-    Assert.ok(!a7.strictCompatibility);
+    Assert.ok(a7.strictCompatibility);

I'd like to avoid landing something and having to follow up. Also, I don't see a way to do a try run for mozilla60 stuff. (Besides, the original author should really worry about that and not dump it into the lap of the code manager for the release.)
Attachment #8992146 - Flags: review?(kmaglione+bmo)
Attachment #8992146 - Flags: feedback?(mkmelin+mozilla)
Attachment #8992146 - Flags: review?(kmaglione+bmo) → review+
Checked the test changes with Kris on IRC. Reverting one hunk.
Attachment #8992146 - Attachment is obsolete: true
Attachment #8992146 - Flags: feedback?(mkmelin+mozilla)
Attachment #8992220 - Flags: review+
Test failure:
TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_strictcompatibility.js | do_check_compat_status/< - [do_check_compat_status/< : 145] false == true

That's this line when testing the dictionary:
  Assert.equal(a7.isCompatible, aAddonCompat[6]);

In the five sub-tests run, we always pass 'true' as aAddonCompat[6], but the test shows, that the add-on isn't compatible as expected.

Sadly it's hard to tell which of the sub-tests fails since the info strings are not printed.

Check https://taskcluster-artifacts.net/CU3bygYbSpOwfKwb86Nsew/0/public/logs/live_backing.log and search for "false == true".

I've come to the end of my sheriffing duties, so I'll refer this back to the original author Magnus.
Flags: needinfo?(mkmelin+mozilla)
"webextension-dictionary" gives zero hits on mozilla-esr60:
https://dxr.mozilla.org/mozilla-esr60/search?q=webextension-dictionary&redirect=false

So the patch just appears to be wrong for mozilla60 and therefore not working which the test detects :-(
This
https://dxr.mozilla.org/mozilla-esr60/rev/dd52b41d2b775e5c7261ce52795268b7670635fc/toolkit/mozapps/extensions/nsBlocklistService.js#1243
const types = ["extension", "theme", "locale", "dictionary", "service"];

together with
https://dxr.mozilla.org/mozilla-esr60/search?q=addon.type+path%3Atoolkit%2Fmozapps%2Fextensions&redirect=false

makes me think that it could/should be
  this.addon.type == "dictionary"
  this.type == "dictionary"
instead if
  this.addon.type == "webextension-dictionary"
  this.type == "webextension-dictionary"
in mozilla60.

Kris?
Flags: needinfo?(kmaglione+bmo)
Correct.
Flags: needinfo?(kmaglione+bmo)
See Also: → 1481900
See Also: → 1506372
See Also: → 1482801
Sancus,  can we do something from the ATN end to bump these min versions to be more than 5.  It really is poor when we are basically offering dictionaries that can not be installed.
Flags: needinfo?(sancus)
I don't think we can, no, there's nothing to bump because ATN does not have any concept of compatibility for dictionaries. I assume they rely on what is in the file, and so those files need to be updated. Or this should be fixed client side, which sounds like the correct solution. Thunderbird on ATN has a "*" version nowadays so it should be possible to upload dictionaries with maxVersion "*" in install.rdf.
Flags: needinfo?(sancus)
(In reply to Andrei Hajdukewycz [:sancus] from comment #47)
> I don't think we can, no, there's nothing to bump because ATN does not have
> any concept of compatibility for dictionaries. I assume they rely on what is
> in the file, and so those files need to be updated. Or this should be fixed
> client side, which sounds like the correct solution. Thunderbird on ATN has
> a "*" version nowadays so it should be possible to upload dictionaries with
> maxVersion "*" in install.rdf.

@sancus:
Could you confirm the "*" version? It was added then removed, was it added again?

Does this mean that in my next GB speller update I can add "*" as the maximum version?

Thanks,
@Sancus
Who do I need to see about getting appropriate access to fix the things on ATN.  Just upload fixed dictionaries where necessary.  I am no addon author but there does not appear to be much skill required to edit the install RDF file and just replace all the min version numbers with *
(In reply to Marco A.G.Pinto [:marcoagpinto] from comment #48)
> @sancus:
> Could you confirm the "*" version? It was added then removed, was it added
> again?
> 
> Does this mean that in my next GB speller update I can add "*" as the
> maximum version?

https://addons.thunderbird.net/en-US/thunderbird/pages/appversions/ * is there, it should work.(In reply to Matt from comment #49)

> @Sancus
> Who do I need to see about getting appropriate access to fix the things on
> ATN.  Just upload fixed dictionaries where necessary.  I am no addon author
> but there does not appear to be much skill required to edit the install RDF
> file and just replace all the min version numbers with *

Well, if you're volunteering to maintain dictionary compatibility, we can probably get you added as a developer to them... admins can upload versions for any add-on, but we can't give out admin for something like this. I'm not sure if there is a lower level of access that would allow it.

Maybe Philipp knows?
Flags: needinfo?(philipp)
(In reply to Matt from comment #49)
> ... and just replace all the min version numbers with *
Let's do the *max* version number, please.

afaik the lowest level would be staff reviewer, but that also give access to a bunch of other things. I'm not sure this is still relevant now that we have wx dicts though.

Flags: needinfo?(philipp)

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #52)

afaik the lowest level would be staff reviewer, but that also give access to a bunch of other things. I'm not sure this is still relevant now that we have wx dicts though.

Yeah it's not relevant anymore.

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