1391020 - Quicktext No Longer Displays after Upgrade to Version 56.0b2 (32-bit)

Status: RESOLVED INVALID

(Thunderbird :: Message Compose Window, defect)

Description

[Alan Thompson]

Attachment: QuickText Display Issue.PNG

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.90 Safari/537.36

Steps to reproduce:

Upgraded to Thunderbird 56.0b2 (32-bit)


Actual results:

After upgrade, QuickText Add On no longer displays its contents, nor does the QuickText 
Tool Bar display in the Compose window.

Running Windows 10, 64-bit Version.

Comment 1

[Jorg K (CEST = GMT+2)]

Kent, who looks after QuickText these days? Have we found a solution yet?

This most likely suffers from wrong imports, like many other add-ons.
Not we need to use //gre/ for M-C imports and /// for C-C imports, see bug 1384218.

As you know, M-C have removed many interfaces, so it's likely that QuickText will be even more broken in TB 57 beta.
Next would be the nsILocalFile to nsIFile migration and the iterator migration, 'in' becomes 'of'.

Comment 2

[Kent James (:rkent)]

I also have not made any attempt to migrate ExQuilla post-52, and that will be a much bigger project.

What I hope to do soon is to start looking at these kinds of issues on ExQuilla first, maybe even put together a Wiki page that discusses known issues (since I got no takers on my request on tb-planning to take this on). Then I'll look at other extensions I am involved with, including this one. But I will feel no urgency until TB 59 beta is approaching.

:rkent

Comment 3

[Gary C Curtin]

So the addon installs, and you can create groups and templates, but you cannot save anything. Nothing is ever written to the templates.xml file.

Any templates you have created can be used in that session of TB, but after restarting TB all groups and templates have gone.

Comment 4

[monperrus]

> So the addon installs, and you can create groups and templates, but you cannot save anything. Nothing is ever written to the templates.xml file. Any templates you have created can be used in that session of TB, but after restarting TB all groups and templates have gone.

I confirm for TB 56.0b3. This makes Quicktext almost unusable. This is a major problem since there is no viable alternative.

Kent, you saved Quicktext once (see https://bugzilla.mozilla.org/show_bug.cgi?id=1209425#c69).

Would you save it now again? That would so useful!

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: quicktext-0.9.11.8.xpi

Does this one work? I changed nsILocalFile to nsIFile.

Comment 6

[Gary C Curtin]

(In reply to Jorg K (GMT+2) from comment #5)
> Created attachment 8915774 [details]
> quicktext-0.9.11.8.xpi
> 
> Does this one work? I changed nsILocalFile to nsIFile.

It made no difference on my installation.

Comment 7

[Jorg K (CEST = GMT+2)]

OK, what are the errors in the error console? Tools > Developer Tools > Error Console. Clear the errors before exercising the add-on, then copy/paste the errors to a text file and attach that here ("Attach File" above), or if less than 15 lines, paste them into a comment. I briefly looked at the add-on source code and from my recollection of things removed in mozilla56, I spotted the nsILocalFile usage. I also checked for problematic "import", but didn't see any.

Comment 8

[Gary C Curtin]

(In reply to Jorg K (GMT+2) from comment #7)
> OK, what are the errors in the error console? Tools > Developer Tools >
> Error Console. Clear the errors before exercising the add-on, then
> copy/paste the errors to a text file and attach that here ("Attach File"
> above), or if less than 15 lines, paste them into a comment. I briefly
> looked at the add-on source code and from my recollection of things removed
> in mozilla56, I spotted the nsILocalFile usage. I also checked for
> problematic "import", but didn't see any.

I guess it does not all apply, but it is short here is the whole thing:

While creating services from category 'profile-after-change', service for entry 'Notification Telemetry Service', contract ID '@mozilla.org/notificationTelemetryService;1' does not implement nsIObserver.
XML Parsing Error: no root element found
Location: moz-nullprincipal:{240200c5-a434-4c85-893a-265cc06a564c}
Line Number 1, Column 1:  {240200c5-a434-4c85-893a-265cc06a564c}:1:1
Use of Mutation Events is deprecated. Use MutationObserver instead.  calendar-widgets.xml:512:18
Please do not load stuff in the multimessage browser directly, use the SummaryFrameManager instead.  summaryFrameManager.js:84
Unknown property ‘user-select’.  Declaration dropped. reps.css:218:13
TypeError: parentDocShell.getDocShellEnumerator is not a function[Learn More]  tab.js:62:23
NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIScriptableUnicodeConverter.ConvertFromUnicode] wzQuicktext.js:613

Comment 9

[Jorg K (CEST = GMT+2)]

Thanks for that. Looks like the only lines relevant to QuickText are the last two.

    // Unicode
    if (true)
    {
      var converter = Components.classes["@mozilla.org/intl/scriptableunicodeconverter"].createInstance(Components.interfaces.nsIScriptableUnicodeConverter);
      converter.charset = "UTF-16";

      var chunk = converter.ConvertFromUnicode(aData);  <=== line 613

That's surprising since nsIScriptableUnicodeConverter is still available in mozilla56. But all the converters have been reimplemented in Rust, so maybe UTF-16 is no longer a valid charset name, but then it should have failed on line 611. Needs more looking into.

Comment 10

[Gary C Curtin]

While waiting for a fix, I add or edit texts and scripts in an older version of Thunderbird, then copy the scripts.xml and templates.xml files to my 57b profile. As long as I don't attempt to edit them on 57b, they do work as normal in the new email window.

Comment 11

[monperrus]

> As long as I don't attempt to edit them on 57b, they do work as normal in the new email window.

Thanks for the tip. One can indeed still use the old templates if one does not try to modify them in the UI.

I also confirm that one can modify templates.xml by hand using a text editor in UTF-16 (I've successfully used kate)

Comment 12

[monperrus]

Just upgraded to 57.0b1. Lost of my Quicktext templates, even without modifying them.

Comment 13

[Jorg K (CEST = GMT+2)]

Many add-ons stopped working in TB 57 beta, especially ones that haven't be prepared for that version.

Comment 14

[monperrus]

Good to know, thanks. It seems that backward compatibility is a hot topic now at Mozilla.

Comment 15

[monperrus]

I terribly miss Quicktext in my daily email life.

Would anyone be able to save us by adapting Quicktext to TB57?

Comment 16

[Gary C Curtin]

Yes, a sad state of affairs. I know it was not a lot to pay for the Pro version, but you would think that when you pay for something, there would be some support. It's like the developer just took the money and ran. And now nothing!

Comment 17

[John Bieling]

multiple usage of undeclared vars and usage of octal integers, which is easy to fix. But this I do not know how to fix:


    var dateTimeService = Components.classes["@mozilla.org/intl/scriptabledateformat;1"].getService(Components.interfaces.nsIScriptableDateFormat);
    var timeStamp = new Date();

    this.mData['DATE'].data['long'] = TrimString(dateTimeService.FormatDate("", dateTimeService.dateFormatLong, timeStamp.getFullYear(), timeStamp.getMonth()+1, timeStamp.getDate()));
    this.mData['DATE'].data['short'] = TrimString(dateTimeService.FormatDate("", dateTimeService.dateFormatShort, timeStamp.getFullYear(), timeStamp.getMonth()+1, timeStamp.getDate()));

    this.mData['TIME'].data['seconds'] = TrimString(dateTimeService.FormatTime("", dateTimeService.timeFormatSeconds, timeStamp.getHours(), timeStamp.getMinutes(), timeStamp.getSeconds()));
    this.mData['TIME'].data['noseconds'] = TrimString(dateTimeService.FormatTime("", dateTimeService.timeFormatNoSeconds, timeStamp.getHours(), timeStamp.getMinutes(), timeStamp.getSeconds()));

scriptabledateformat is no longer supported and muat be replaced by Intl.DateTimeFormat. Any help?

Comment 18

[Jorg K (CEST = GMT+2)]

I hope you're taking attachment 8915774 [details] as a starting point where I already made changes compared to 0.9.11.7 published at AMO. Looking at it now, I only changed nsILocalFile to nsIFile :-(

For examples on how to format dates, look at
https://dxr.mozilla.org/comm-central/search?q=new+Services.intl.DateTimeFormat&redirect=false

Comment 19

[John Bieling]

Yes, started with your version. 

I removed all parts using scriptabledateformat for now and currently fix all the undeclared vars errors. Testing against Daily.

Comment 20

[Jorg K (CEST = GMT+2)]

Daily TB 61? That doesn't support non-bootstrapped add-ons any more. Please use TB 60 beta and change the maxVersion in the RDF file to 60.

Comment 21

[John Bieling]

Attachment: quicktext-0.9.11.9.xpi

fixed all undeclared vars (which I could find), fixed octal integer usage and removed firstload screen, which talks about a pro version, which is no longer avail.

Still Needs to fix usage of nsIScriptableDateFormat - but is usably to test for more Errors.

Comment 22

[John Bieling]

@Tb61: that is strange. My daily is at 61.0a1 (2018-03-21) (64-bit) and there the add on works. hm. will look at that tomorrow.

Maybe users of the add on could test and report back? Stuff using the nsIScriptableDateFormat will still not work, not fixed yet.

Comment 23

[Jorg K (CEST = GMT+2)]

TB 61 works until the 2018-03-26 with non-bootstrapped add-ons, so your version is good.

Comment 24

[John Bieling]

Just tested with 60.0b2 and there a few more errors pop up... I report back tomorrow.

Comment 25

[John Bieling]

@Jorg: Do you have (now) any idea, why 

var chunk = converter.ConvertFromUnicode(aData);  <=== line 613 / your comment #9

does not work in Tb60, but (!!!) in Tb61?

Comment 26

[John Bieling]

Sorry for the noise, something is odd, line 613 is also not working in TB61. I investiagte ...

Comment 27

[John Bieling]

Attachment: quicktext-0.9.11.10.xpi

OK, I did a lot of testing and it is indeed the "UTF-16" charset. If I change that to UTF-8, the conversion succeeds and no other errors pop up. This is the only change I did from .9 to .10

Due to the changed charset, the written template.xml file looks different, but the read function can still handle it (even there UTF-16 is still used). During my tests, version .10 could read files created by .8 and version .8 could read files created by .10.

I do not know enough about charsets and encoding to understand that, but the addon works (nsIScriptableDateFormat still needs fixing).
	
Before using .10, backup your template.xml, just to make sure!

Comment 28

[Jorg K (CEST = GMT+2)]

Hmm, have you tried UTF-16BE or UFT-16LE? https://en.wikipedia.org/wiki/UTF-16
I think using UTF-16 is crazy, maybe is should be changed to UTF-8 consistently.

The date format stuff is pretty trivial if you look at the examples. Looks like you've got it close to working.

Comment 29

[John Bieling]

I fixed the scriptabledateformat issue by now.

Regarding the charset stuff:

Nope, UTF-16BE and UTF-16LE both do not work.

If I change the read method to UFT-8 as well, I can no longer read old UTF-16 templates.xml, the read data is empty and will erase the template file on next write. Not good.

CORRECTION: Using UTF-16 on read and UTF-8 on write does NOT work as desired. The addon is not crashing, but the read data on next read is garbage. Of course.

Plan A: get UTF-16 write working again (where is source of nsIScriptableUnicodeConverter ?)
Plan B: detect UTF-8 or UTF-16 on read and always write UTF-8

I think nsIScriptableUnicodeConverter is legacy. Any info if that is going to be droped soon? Should we think about using nsIConverterOutputStream? Or does that not make any difference?

This is the only remaining issue...

Comment 30

[John Bieling]

Attachment: quicktext-0.9.11.11.xpi

This version fixes the scriptabledateformat issue.

But it still has problems with the charset, do not use it to save data, Special chars will come back as garbage on next read.

Comment 31

[John Bieling]

This also does not work (reverse the read function using convertToByteArray:

var converter = Components.classes["@mozilla.org/intl/scriptableunicodeconverter"].createInstance(Components.interfaces.nsIScriptableUnicodeConverter);

converter.charset = "UTF-16";

var chunk = converter.convertToByteArray(aData, {});//converter.ConvertFromUnicode(aData);
var boStream = Components.classes["@mozilla.org/binaryoutputstream;1"].createInstance(Components.interfaces.nsIBinaryOutputStream);
boStream.setOutputStream(foStream);
boStream.writeByteArray(chunk, chunk.length);

Same error. Maybe ist worth filing a bug in scriptableunicodeconverter?

I will try a polyfill for convertToByteArray next...

Comment 32

[Jorg K (CEST = GMT+2)]

nsIScriptableUnicodeConverter is not deprecated so far, it's used in M-C and C-C. There are plans to "retire" it one day, see bug 1347877.

As I said in comment #9, all converters have need implemented in Rust, so uconv was replaced by encoding_rs in bug 1261841. So I'm pretty sure that the underlying code has slightly changed with respect to UTF-16.

One of the call sites is:
var tmp = biStream.readByteArray(biStream.available());
var converter = Components.classes["@mozilla.org/intl/scriptableunicodeconverter"].createInstance(Components.interfaces.nsIScriptableUnicodeConverter);
converter.charset = "UTF-16";
text = converter.convertFromByteArray(tmp, tmp.length);

Since the internal storage is UTF-16 anyway, converting to UTF-16 is really a no-op, so perhaps that's not supported any more. What happens when you do: |text = tmp|?

The code is here:
https://searchfox.org/mozilla-central/source/intl/uconv/nsScriptableUConv.cpp

Henri, has anything changed to UTF-16 processing?

Comment 33

[Jorg K (CEST = GMT+2)]

This looks like a bug in nsScriptableUConv.cpp.

mEncoder is only set for charsets which are not UTF-16 and then ConvertFromUnicode() errors out on

  if (!mEncoder)
    return NS_ERROR_FAILURE;

Is that intentional? Maybe we get a quicker answer from Masatoshi-san.

Comment 34

[John Bieling]

I have now a working alternative for convertFromByteArray, at least on Windows. I fixed the endianness, so it matches the original file:

//var converter = Components.classes["@mozilla.org/intl/scriptableunicodeconverter"].createInstance(Components.interfaces.nsIScriptableUnicodeConverter);
//converter.charset = "UTF-16"; 
//var chunk = converter.convertToByteArray(aData, {});

//Polyfill for convertToByteArray, which does not work with UTF-16 in TB60 (above code works with TB52)
let chunk = [];
for (let l=0; l<aData.length; l++) {
    let c = aData.charCodeAt(l);
    //fixed endianness
    chunk.push( c & 0xFF );
    chunk.push( (c >> 8) & 0xFF );
}

I still need to verify that on linux...

@Jorg; |text = tmp| gives different results, all the zeros of the (unused) upper 8bit of UTF-16 are removed, so no real UTF-16, also no BOM.

Comment 35

[John Bieling]

ahhhh, convertToByteArray, of course

Comment 36

[Masatoshi Kimura [:emk]]

(In reply to Jorg K (GMT+1) from comment #33)
> This looks like a bug in nsScriptableUConv.cpp.
> 
> mEncoder is only set for charsets which are not UTF-16 and then
> ConvertFromUnicode() errors out on
> 
>   if (!mEncoder)
>     return NS_ERROR_FAILURE;
> 
> Is that intentional? Maybe we get a quicker answer from Masatoshi-san.

It's simply because encoding_rs does not support encoders for UTF-16 encodings.

Comment 37

[Jorg K (CEST = GMT+2)]

That was quick, thanks! So we need to replace the use of nsIScriptableUnicodeConverter or move away from UTF-16. I guess you can still open UTF-16 files in Notepad++ but I'm not sure why those files use UTF-16 in the first place.

Comment 38

[John Bieling]

Attachment: quicktext-0.9.11.12.xpi

I think I am done. Tested this version with Linux (TB52) and Windows (TB52 & TB60) and I get no more errors.

It can read and write the original UTF-16 file Format. Read is done using nsIScriptableUnicodeConverter with charset UTF-16, write is done using the polyfill of comment #34.

Can anybody else confirm this?

How do we get this thru review and to AMO?

John

Comment 39

[Jorg K (CEST = GMT+2)]

And you're sure that your "polyfill" is correct? I don't think so. It might work for ASCII text or at best some 8bit characters. You produce two bytes for every character.

Try äöü€ß§°çñ abd some Asian: Japanese: テスト, Korean: 안녕하세요, Chinese: 中文語

Maybe you can detect what you read and support UTF-16 and UTF-8, but always write UTF-8.

Comment 40

[John Bieling]

I tried your chinese and it works. Try it yourself.

Comment 41

[John Bieling]

Also works with your entire comment.

Comment 42

[John Bieling]

I am not converting to UTF-16, I am chopping up 16bit charcodes in two 8 bit pairs, as convertToByteArray is doing. Maybe that helps?

Comment 43

[Jorg K (CEST = GMT+2)]

Yep, I'm convinced, thanks a lot for your help. As I wrote earlier (or on some mailing list), I'm not a user, so I can' tell.

Please contact Kent James on [e-mail], he has access to AMO for the add-on.

Comment 44

[Gary C Curtin]

I just added this to 60.0b1 and it works with my previous quick texts. I have been without them since last year, and it is great to be able to use the add-on again. What you guys have done in the past 24 hours is amazing. Thank you so much!

Comment 45

[John Bieling]

The real test is to make changes to one of your templates (or add one), save, restart TB and check if your templates are still fine.

Comment 46

[Jorg K (CEST = GMT+2)]

The credit goes to John for 99.99%, I only claim 0.01%, but I made 99.99% of the noise to get this fixed ;-)

Comment 47

[Gary C Curtin]

I went through my templates, and so far what I need does work. I recreated one or two and am satisfied. [[CURSOR]] doesn't work, but I can live with that.

Comment 48

[John Bieling]

[[CURSOR]] is not working, because finder.Find in the following code is returning null:

var finder = Components.classes["@mozilla.org/embedcomp/rangefind;1"].createInstance().QueryInterface(Components.interfaces.nsIFind);
finder.caseSensitive = true;
finder.findBackwards = false;

while ((foundRange = finder.Find("[[CURSOR]]", aSearchRange, startRange, endRange)) != null) {
...
}

run in TB52, it does not return null (but the range where [[CURSOR]] is at).

I did check the input vars (aSearchRange, startRange, endRange) and they appear to have the same values in TB52 and TB60, just the result of Find differs...

Did something change with nsIFind?

Comment 49

[Jorg K (CEST = GMT+2)]

Since you still appear to be making improvements ;-)

I don't like the date/time formatting that doesn't follow the locale that TB uses.
Can you use:
    options["DATE-long"] = { dateStyle: "long" };
    options["DATE-short"] = { dateStyle: "short" }; 
    options["TIME-seconds"] = { timeStyle: "long" };
    options["TIME-noseconds"] = { timeStyle: "short" }; 
      
    let fields = Object.keys(options);
    for (let i=0; i<fields.length; i++) {
        let field = fields[i];
        let fieldinfo = field.split("-");
	this.mData[fieldinfo[0]].data[fieldinfo[1]] = TrimString(new mozIntl.DateTimeFormat([], options[field]).format(timeStamp));

Find should still work as it's tested here:
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/windowcreator/test/test_nsFind.html#26

And the fine code hasn't changed in ages:
https://hg.mozilla.org/mozilla-central/log/tip/toolkit/components/find/nsFindService.cpp

What is "[[CURSOR]]"? A string you're looking for in the given range?

Comment 50

[John Bieling]

yes, "[[CURSOR]]" is a token the user can place in his template to let the cursor jump to.

While hacking the replacemnet for the date time stuff, I did try it on different systems with different locales and it always used the system locale, since I do not specify a specific one (like you). I sure can use your code, just want to understand the difference.

Isn't "Intl.DateTimeFormat" the way to go? What is "mozIntl"?

Comment 51

[John Bieling]

Just found this: https://groups.google.com/forum/#!topic/firefox-dev/dNkRQFDd7-w

Does this also apply to Thunderbird? Maybe update 

https://wiki.mozilla.org/Thunderbird/Add-ons_Guide_57

than? Because thats where I got Intl.DateTimeFormat from. But if this is already outdated, it should not be standing there.

The "good stuff" of mozIntl are the dateStyle and timeStyles than?

Comment 52

[Jorg K (CEST = GMT+2)]

Intl.DateTimeFormat in the implementation of ECMA-402, but mozIntl is the better stuff. We use it almost everywhere in TB now since it gives regionally adjusted formats.

new Services.intl.DateTimeFormat(...) will use the good stuff and that is mentioned in the Wiki, albeit not in detail.

So you should be using TrimString(new Services.intl.DateTimeFormat(undefined, ...). Sorry, that's different to what I said before.

Comment 53

[Gary C Curtin]

I am wondering why this is marked as Resolved/Fixed when [[CURSOR]] is not working?

Also, what will happen for those of us with the Pro version? Will it also be fixed, or will there be a refund?

Comment 54

[Jorg K (CEST = GMT+2)]

OK, strictly, the bug was invalid from the start since it is about an add-on, not Thunderbird.

Comment 55

[Jorg K (CEST = GMT+2)]

I see pull requests on Github, how about comment #52?

Comment 56

[John Bieling]

TypeError: Services.intl is undefined

on TB52.7

the current quicktext on AMO (0.9.11.7) is not compatible with 52, so if I insert your changes, TB52 users are left out.

I know that quicktext .7 does work with TB52, but index.rdf says otherwise, so users have to manually download .7 and install it by hand - they apparently have not done that till now, so I do not assume, they will do that in the future.

Second, Kent James could update the max version in AMO , but i have no feedback from him whatsoever.

So I will not include these changes now.

Comment 57

[John Bieling]

From the reviews on AMO it looks like, .7 is indeed not fully working with TB52, so increasing max version is not an option. Once .12 is avail on AMO, I can add your changes to the next version.

It still has to be discussed, what we do with the pro version. Do we do that here? Or on github? Or in MailDev?

Comment 58

[Jorg K (CEST = GMT+2)]

Right, the "modern" date formatting is not in TB 52. I can't answer the other questions, but I'm happy to assist with technical issues. As for Kent, e-mail him directly at [e-mail]. I'm sure he'll grant you AMO access.

Thanks again for taking this on. I'm not a user of Quicktext but I know it's important for many people.

Comment 59

[John Bieling]

I contacted him 7 days ago, maybe he is on holiday... I will try again later.

Comment 60

[John Bieling]

.12 has passed review.

https://addons.mozilla.org/de/thunderbird/addon/quicktext/

Comment 61

[John Bieling]

Attachment: quicktext-0.9.14-beta.xpi

Beta version of the next quicktext version, including all "Pro" features.

Comment 62

[John Bieling]

All the additional features of the former non-free "Quicktext Pro" have been merged into the latest Quicktext version (attached as 0.9.14).

It will be released to AMO soon, but I need a few beta tester first. It works for me, I tested all features (including SCRIPT support). The only tag still not working is [[CURSOR]].

Comment 63

[plieser]

Attachment: quicktext-0.9.15-beta.xpi

New beta version which fixes using custom format for date and time

Comment 64

[dobrin]

(In reply to plieser from comment #63)
> Created attachment 8980352 [details]
> quicktext-0.9.15-beta.xpi
> 
> New beta version which fixes using custom format for date and time

I tested quicktext-0.9.15-beta.xpi on Windows 10 Pro x64 with Thunderbird 52.8.0.
Formats for date and time are get correctly from the operating system settings.

Comment 65

[John Bieling]

Attachment: quicktext 1.1

Comment 66

[John Bieling]

Dear quicktext users, 

I would like to inform you, that github user bgeiring provided a patch which probably restores the CURSOR tag. Could you give it a try? We are looking for feedback, if this really solves the issue.

https://github.com/thundernest/quicktext/issues/27#issuecomment-448032612