Minimal change for Qt6

69 views
Skip to first unread message

Fan Yang

unread,
Dec 3, 2021, 10:34:42 AM12/3/21
to scintilla-interest
Hi list,

I'm working on my project which uses scintilla, recently I made a fix for Qt6, please try the attached patch.
scintilla.qt6.patch

Neil Hodgson

unread,
Dec 3, 2021, 5:09:21 PM12/3/21
to Scintilla mailing list
Fan Yang:

> I'm working on my project which uses scintilla, recently I made a fix for Qt6, please try the attached patch.

For the includes, <QDesktopWidget> can be removed unconditionally as QDesktopWidget was dropped a while ago. Its also fine to add <QTextCodec>.

QFontMetricsF::horizontalAdvance isn’t available in Qt 4.x so it should be protected with #if and a fallback. Its also unclear if this will produce results that are sufficiently different to QFontMetricsF::width to cause any problems. Surface::Width which calls these is used for less common drawing like annotations and line numbers so a problem may not be so apparent and it may differ between platforms and their underlying text engines.

Replacing viewOptions with initViewItemOption appears less likely to cause problems and is adequately guarded.

Linking to core5compat appears to be needed just for QTextCodec but I couldn’t find a replacement for QTextCodec that would avoid the need for core5compat.

Overall, this doesn’t appear safe enough to include in the upcoming release 5.1.5 but it could be applied after this release with a backup for horizontalAdvance on 4.x.

Neil

dail8859

unread,
Dec 4, 2021, 9:55:41 AM12/4/21
to scintilla-interest
> Linking to core5compat appears to be needed just for QTextCodec but I couldn’t find a replacement for QTextCodec that would avoid the need for core5compat.

QStringDecoder and QStringEncoder are the alternatives. A while back I took a shot at getting Qt6 support (which I am very interested in). I made some of the same changes as the patch attached but went the route of using QStringDecoder/Encoder. The code changes to support these classes are not just a one-to-one replacement so I was not confident at all that the changes I was making were correct (I don't understand the different types of encodings well enough).

Justin

Neil Hodgson

unread,
Dec 4, 2021, 6:34:35 PM12/4/21
to Scintilla mailing list
Justin:

> QStringDecoder and QStringEncoder are the alternatives...
> The code changes to support these classes are not just a one-to-one replacement

QTextCodec is used often in the Qt platform layer and instances are sometimes used both for encode and decode, such as the CaseFolder. Having 2 classes instead of 1 makes this more complex and the new classes aren’t available on Qt 5 so its simpler for now to use core5compat.

Backwards compatibility libraries are often poorly maintained so, eventually, there should be a compatibility shim. Perhaps based on the 2 class approach with a pair of (encode, decode) classes implemented by Scintilla that #if to call either QTextCodec or QStringDecoder / QStringEncoder.

Longer-term, I’d like to move encoding more into platform-independent Scintilla with almost all calls to platform layers being for UTF-8 Unicode.

Neil

dail8859

unread,
Dec 5, 2021, 10:45:41 AM12/5/21
to scintilla-interest
> Having 2 classes instead of 1 makes this more complex and the new classes aren’t available on Qt 5

Yes this is the exact issue I was discovering trying to make the code modifications.

> so its simpler for now to use core5compat.

Though not desirable, I do agree core5compat is probably the best approach for now so that new issues are not introduced into Scintilla.

Justin

Jason Haslam

unread,
Dec 5, 2021, 9:49:28 PM12/5/21
to scintilla...@googlegroups.com
QStringDecoder/QStringEncoder don’t even support encodings other than UTF-8/UTF-16/UTF-32 and the current “System” encoding on Windows. It sounds like they no longer plan to add the legacy codecs back in and the proposed path forward is to use ICU directly (https://bugreports.qt.io/browse/QTBUG-75665). It’s rather unfortunate for a text editor that wants to support any number of legacy encodings.

Jason


--
You received this message because you are subscribed to the Google Groups "scintilla-interest" group.
To unsubscribe from this group and stop receiving emails from it, send an email to scintilla-inter...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/scintilla-interest/864934f1-eeb4-4899-a494-6ff616009396n%40googlegroups.com.

Neil Hodgson

unread,
Dec 6, 2021, 8:09:04 PM12/6/21
to Scintilla mailing list
Jason:

It sounds like they no longer plan to add the legacy codecs back in and the proposed path forward is to use ICU directly (https://bugreports.qt.io/browse/QTBUG-75665).

   While UTF-8 is becoming more common, its nowhere near universal yet and Scintilla will still need to support old code and documents that are in other encodings. ICU (or libiconv) is a large extra dependency so I’d prefer to avoid that.

   It may be better to distribute encoding conversion data with platform-independent Scintilla at a cost of adding something like 200KB to the executable. That’s mostly for the 4 main East Asian encodings.

   Neil

dail8859

unread,
Dec 11, 2021, 12:59:19 PM12/11/21
to scintilla-interest
Building off of Fan Yang's original patch I've got another revision of the patch. Very similar but incorporating Neil's suggestions. This requires core5compat to keep the QTextCodec. I haven't tried building this on Qt4 but there should be the proper #ifdefs to keep it functional.

With this patch I can build using Qt 5.15.2 and Qt 6.2.2

However I ran into an crash which seems to be related to Scintilla 5.1.5 changing how API calls can discover the length of the returned buffer. In QByteArray ScintillaEdit::TextReturner(...) if I add 1 to the length returned then it works correctly (I was calling SCI_GETLEXERLANGUAGE). This change isn't included in the patch attached due to the weird thing is that Scintilla 5.1.5 seems to work fine on Qt 5 but with Qt 6 it crashes. Not sure if Qt 6 has some extra checks around running off the end of a QByteArray.
scintilla.qt6.v2.patch

Neil Hodgson

unread,
Dec 11, 2021, 10:03:24 PM12/11/21
to Scintilla mailing list
Justin:

> Building off of Fan Yang's original patch I've got another revision of the patch. Very similar but incorporating Neil's suggestions. This requires core5compat to keep the QTextCodec. I haven't tried building this on Qt4 but there should be the proper #ifdefs to keep it functional.

Committed with
https://sourceforge.net/p/scintilla/code/ci/1d7efaa790b28c68fdbc4708d92f9d939e3d454e/

> However I ran into an crash which seems to be related to Scintilla 5.1.5 changing how API calls can discover the length of the returned buffer. In QByteArray ScintillaEdit::TextReturner(...) if I add 1 to the length returned then it works correctly (I was calling SCI_GETLEXERLANGUAGE). This change isn't included in the patch attached due to the weird thing is that Scintilla 5.1.5 seems to work fine on Qt 5 but with Qt 6 it crashes. Not sure if Qt 6 has some extra checks around running off the end of a QByteArray.

It needs +1. Previous code may have worked for Qt 5 if there is an extra byte available (or non-critical) at the end of the allocation for QByteArray.
Committed similar with
https://sourceforge.net/p/scintilla/code/ci/9ec109b3283748ab0f2a064816ef5c1ce87925d4/

Also made some changes to improve huge (>2GB) file support on Qt and harmonize types to avoid warnings.
https://sourceforge.net/p/scintilla/code/ci/32f02df705890a0495c37b3dc2ba7f558222a497/
https://sourceforge.net/p/scintilla/code/ci/eba4d675774e41eaa65e59d6c2556033a59dbb40/
https://sourceforge.net/p/scintilla/code/ci/e3e08ca9d8a80fe98ef20f27417220e54ea6dee4/
https://sourceforge.net/p/scintilla/code/ci/0cf46843e0907225b4b26b0f12cbafa9eb8917ba/

These are for ScintillaEditBase which I am more concerned with. Similar changes could be made to ScintillaEdit if any of its users are interested in huge file support.

Neil

dail8859

unread,
Dec 13, 2021, 4:00:00 PM12/13/21
to scintilla-interest
Just pulled all the latest changes and everything builds fine for me with both Qt 5.15 and Qt 6.2. Thanks!

> Similar changes could be made to ScintillaEdit if any of its users are interested in huge file support.

Currently I don't have a need for huge file support so no pressure there.


Justin
Reply all
Reply to author
Forward
0 new messages