Build with Qt6

[doug1234]

Builds and the example runs with qt 6.6.0 on rhel9 with no new warnings. Currently using the qt5 compatibility library as was suggested in other pull requests.

Issue: The cursor is drawn farther to the right then it should be. This was not an issue for me with qt5. Looing into. Issue: Text is sized differently when selected. This makes an odd effect. This was not an issue for me with qt5.

TODO: Update documentation.

[doug1234]

Not sure how to update the CI to use qt6, the qt6 build of lxqt-build-tools and the appropriate python tools for qt6.

[yan12125]

Not sure how to update the CI to use qt6, the qt6 build of lxqt-build-tools and the appropriate python tools for qt6.

You can ignore it for now. It needs https://github.com/lxqt/lxqt-build-tools/pull/77 to be merged first.

[stefonarch]

Builds fine, just those warnings:

[ 82%] Building CXX object CMakeFiles/qtermwidget6.dir/lib/tools.cpp.o
/home/stef/git/doug1234/qtermwidget/lib/TerminalCharacterDecoder.cpp: In member function ‘virtual void Konsole::PlainTextDecoder::decodeLine(const Konsole::Character*, int, Konsole::LineProperty)’:
/home/stef/git/doug1234/qtermwidget/lib/TerminalCharacterDecoder.cpp:85:28: warning: comparing the result of pointer addition ‘(((const Konsole::Character*)characters) + ((sizetype)(((long unsigned int)i) * 16)))’ and NULL [-Waddress]
   85 |         if (characters + i == nullptr)
      |             ~~~~~~~~~~~~~~~^~~~~~~~~~
[ 84%] Building CXX object CMakeFiles/qtermwidget6.dir/lib/Vt102Emulation.cpp.o
[ 85%] Building CXX object CMakeFiles/qtermwidget6.dir/lib/moc_Emulation.cpp.o
[ 86%] Building CXX object CMakeFiles/qtermwidget6.dir/lib/moc_Filter.cpp.o
/home/stef/git/doug1234/qtermwidget/lib/Vt102Emulation.cpp: In member function ‘virtual void Konsole::Vt102Emulation::sendMouseEvent(int, int, int, int)’:
/home/stef/git/doug1234/qtermwidget/lib/Vt102Emulation.cpp:949:56: warning: ‘%d’ directive output may be truncated writing between 1 and 10 bytes into a region of size between 7 and 26 [-Wformat-truncation=]
  949 |         snprintf(command, sizeof(command), "\033[%d;%d;%dM", cb + 0x20, cx, cy);
      |                                                        ^~
/home/stef/git/doug1234/qtermwidget/lib/Vt102Emulation.cpp:949:44: note: directive argument in the range [1, 2147483647]
  949 |         snprintf(command, sizeof(command), "\033[%d;%d;%dM", cb + 0x20, cx, cy);
      |                                            ^~~~~~~~~~~~~~~~
/home/stef/git/doug1234/qtermwidget/lib/Vt102Emulation.cpp:949:17: note: ‘snprintf’ output between 9 and 37 bytes into a destination of size 32
  949 |         snprintf(command, sizeof(command), "\033[%d;%d;%dM", cb + 0x20, cx, cy);
      |         ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[marcusbritanicus]

Issue: The cursor is drawn farther to the right then it should be. This was not an issue for me with qt5. Looing into.

@doug1234 I took the strain to build konsole Qt6, and they seem to do this properly, I know not how. Since you're working on this, you might have a better idea. On first glance, I noticed that their TerminalDisplay class is much more sophisticated than the one we have here.

Would it help to port that code to QTermWidget?

[yan12125]

Builds fine, just those warnings:

Those warnings have been there for a long tine. I have a fix at https://github.com/lxqt/qtermwidget/commits/eliminate-warnings/, but I forgot why I didn't push it.

Would it help to port that code to QTermWidget?

That may help but will likely take much more efforts than porting to Qt 6, and thus it's out of scope for this PR in my opinion. A better place for such discussions can be https://github.com/lxqt/qterminal/issues/320

[marcusbritanicus]

@yan12125 I did have a look at it before I posted here. That thread seems be sleeping since 2018, and was rather unwilling to wake it up - I seriously disliked the direction it was taking.

I do have a vested interest in QTermWidget - we have two projects based on it (CoreTerminal and DesQ Term) and would like to keep our projects free from KDE parts. So I am willing to spend some time to port TerminalDisplay if that is of any help at all.

Edit: I will see if I can port it without much efforts.

[marcusbritanicus]

@doug1234 Fantastic work...!

Issue: The cursor is drawn farther to the right then it should be. This was not an issue for me with qt5. Looing into.

I think I fixed this issue. Following are the changes I made:

diff --git a/lib/TerminalDisplay.cpp b/lib/TerminalDisplay.cpp
index 3018f65..0ccfb75 100644
--- a/lib/TerminalDisplay.cpp
+++ b/lib/TerminalDisplay.cpp
@@ -291,6 +291,15 @@ void TerminalDisplay::setVTFont(const QFont& f)
   // Disabling kerning saves some computation when rendering text.
   font.setKerning(false);

+  // QFont::ForceIntegerMetrics has been removed.
+  // Set full hinting instead to ensure the letters are aligned properly.
+  font.setHintingPreference(QFont::PreferFullHinting);
+
+  // "Draw intense colors in bold font" feature needs to use different font weights. StyleName
+  // property, when set, doesn't allow weight changes. Since all properties (weight, stretch,
+  // italic, etc) are stored in QFont independently, in almost all cases styleName is not needed.
+  font.setStyleName(QString());
+
   QWidget::setFont(font);
   fontChange(font);
 }

@yan12125 Looks like porting of TerminalDisplay is not required.

PS: @doug1234 I have opened a PR in your repo with these changes.

[doug1234]

It looks great now thanks to help from @marcusbritanicus ! The only reason I can see not to merge is documentation since you don't want to update the ci here. I can tweak the documentation if you want but I am going to remove the draft tag now.

[yan12125]

Thank you both for awesome efforts! I should be able to have a look in coming days.

The only reason I can see not to merge is documentation since you don't want to update the ci here.

Yes almost. Just need https://github.com/lxqt/lxqt-build-tools/pull/77 and cleanup https://github.com/lxqt/qterminal/pull/1067

I seriously disliked the direction it was taking.

That's just a discussion. If there is a need to port Konsole codes in the future, feel free to propose alternative approaches.

[yan12125]

Not sure how to update the CI to use qt6, the qt6 build of lxqt-build-tools and the appropriate python tools for qt6.

I started testing Qt 6 CI in https://github.com/lxqt/qtermwidget/commits/qt6-ci/. The C++ part looks good, while the PyQt part may need some tricks.

[yan12125]

TODO: Update documentation.

What documentation needs updates?

Another question: is this an independent work or based on earlier pull requests in this repo? If the latter, it's better to give credits to authors of other pull requests.

[doug1234]

I definitely looked at the original branch and used it as a starting point for some things. Let me know how you would like me to give credit to the original author. I am willing to do it however you want.

[doug1234]

As far as documentation goes, I think we may need to change references to qt5 to qt6. Not sure if there is anything else or not. I can look into it if you want.

[yan12125]

I definitely looked at the original branch and used it as a starting point for some things. Let me know how you would like me to give credit to the original author. I am willing to do it however you want.

Thanks, mentioning links to earlier pull requests is good enough.

As far as documentation goes, I think we may need to change references to qt5 to qt6. Not sure if there is anything else or not. I can look into it if you want.

This can be either part of this pull request or in a later pull request - up to you. I don't consider it a blocker.

A real blocker is broken PyQt integration, as shown in https://github.com/lxqt/qtermwidget/actions/runs/8221585544/job/22482108500. I haven't had time to investigate it, though.

sip-wheel: pyproject.toml: 'tool.sip.project.qmake': specify a working qmake or add it to PATH

[marcusbritanicus]

Could that be because of this?

Not sure, but changing PyQt5 to PyQt6 would help?

[doug1234]

I definitely looked at the original branch and used it as a starting point for some things. Let me know how you would like me to give credit to the original author. I am willing to do it however you want.

Thanks, mentioning links to earlier pull requests is good enough.

Here is the link to the pull request: https://github.com/lxqt/qtermwidget/pull/515

As far as documentation goes, I think we may need to change references to qt5 to qt6. Not sure if there is anything else or not. I can look into it if you want.

I don't see anything more to do then change from qt 5 to qt 6 so I just did that.

[doug1234]

@yan12125 do you mind merging your qt6 CI branch into here? I think that would make it easier to test.

[yan12125]

Not sure, but changing PyQt5 to PyQt6 would help?

Nice catch! I updated it. Seems not enough, though.

do you mind merging your qt6 CI branch into here? I think that would make it easier to test.

Sure, updated.

[marcusbritanicus]

@yan12125 @doug1234 Another tiny issue (but it won't fix the ci issues)

Should this not be self.libraries.append('qtermwidget6')?

[marcusbritanicus]

@yan12125 I believe the ci problem is a bug in PyQt-Builder. It really requires the binary qmake to exist in the system (src). However, the qmake binary is named qmake6 in Qt6.

I've opened an issue with pyqt-builer here.

Until then, we can use a hack, something like sudo ln -s /usr/bin/qmake6 /usr/bin/qmake

[doug1234]

@yan12125 @doug1234 Another tiny issue (but it won't fix the ci issues)

Should this not be self.libraries.append('qtermwidget6')?

Good find. I made the change.

[doug1234]

We got a bit further now we are stuck at:

sip-wheel: /__w/qtermwidget/qtermwidget/pyqt/sip/qtermwidget.sip: line 50: column 23: 'QTextCodec' is undefined

Seems like PyQt6 doesn't support the compatibility stuff? Any thoughts @marcusbritanicus ?

[marcusbritanicus]

@doug1234 Ahh... I had forgotten about this. Sadly, PyQt6 does not provide Qt5 compatibility modules. Nor does PySide6.

I guess, we should move away from QTextCodec to QStringConverter.

Edit: I really do not see how we could do that easily..!!

[doug1234]

@doug1234 Ahh... I had forgotten about this. Sadly, PyQt6 does not provide Qt5 compatibility modules. Nor does PySide6.

I guess, we should move away from QTextCodec to QStringConverter.

Edit: I really do not see how we could do that easily..!!

Ok. I am up for doing that next week. Let me know how you think that should be done.

[marcusbritanicus]

I think the only place where we have used QTextCodec and QTextDecoder is in Emulation.cpp and Emulation.h. The other two files just call Emulation::setTextCodec(...) as I see it. For starters, we can use QStringDecoder in place of QTextDecoder. QTextCodec supported around 812 codecs and in my short test, all these are supported by QStringDecoder as well.

However, the more challenging task will be to get the encoding, i.e., we need a suitable replacement for QTextCodec::codecForLocale(). We may be able to use the original code in QTextCodec source. What are the problem that may bring, I am unsure. But it is not be undoable.

Once we have the QStringDecoder ready, it's trivial to perform the conversion (as given in the Qt6 docs):

QByteArray encodedString = "...";
auto toUtf16 = QStringDecoder(QStringDecoder::Utf8);
QString string = toUtf16(encodedString);

~Rest of the code should be unaffected, thankfully.~ Edit: We may need to use QStringEncoder to convert from Unicode to the encoding that was set. I had missed out the use of _codec->fromUnicode(...) in Vt102Emulation class.

[tsujan]

@yan12125, @doug1234, @marcusbritanicus This is only a question — I absolutely don't mean that there's a haste: Will the Qt6 port of QTerminal be ready before the next release, which will happen around Apr 15, 2024?

If the answer is "no" but you're sure that QTerminal will be ready in a few weeks after that date, we could delay the release. If not, we could release the Qt6 port of QTerminal separately, after LXQt 2.0.0 and whenever it's ready.

Please take your time and do your excellent work with no haste.

[marcusbritanicus]

This question has a rather strange answer: Strictly from the point of view of QTerminal, our current problem of QTextCodec is irrelevant. Unlike Konsole, QTerminal assumes that the input is UTF-8, and there is no setting to change it!! So, instead of breaking our heads on how to fix it, we can simply remove the allied code and make the release. While I am fairly certain that this is the case, once we start the work, we will be sure.

I think, you'll get a more concrete answer later this week. :)

Edit: But from the dev POV, it will take a bit of work to make the migration from QTextCodec to QStringCenverter. Since we'll be doing these changes without much reference (Konsole also uses QTextCodec), we'll have to fallback on maknig the changes as we see fit, and then test it a fair bit to see if we broke something. And if there is a piece of code that does not have a straight-forward alternative, it may take us a while to figure it out.

[doug1234]

@yan12125, @doug1234, @marcusbritanicus This is only a question — I absolutely don't mean that there's a haste: Will the Qt6 port of QTerminal be ready before the next release, which will happen around Apr 15, 2024?

If the answer is "no" but you're sure that QTerminal will be ready in a few weeks after that date, we could delay the release. If not, we could release the Qt6 port of QTerminal separately, after LXQt 2.0.0 and whenever it's ready.

Please take your time and do your excellent work with no haste.

I think we are getting close to the end. I am going to try to finish this up this week. I should know by the end of the week if I hit a snag.

[doug1234]

@marcusbritanicus I think I could grab the code in codecForLocale() to get the codec name and then use QStringDecoder. I will give that a shot but I think the tricky part will be testing that things work the same.

[marcusbritanicus]

@doug1234 Yes. that's what I thought as well.. To test, we could open a few documents which have utf-8 characters and test them against another terminal (say, Konsole/foot/alacritty/...) or an older version of QTerminal.

A fair warning: I'm not sure, but I think I saw some QGlobalData::instance(), and I've not yet investigated what the heck that is. It might, in the end, turn out to be some table that maps locales to encodings.

[yan12125]

From a quick search on Google and GitHub, it seems nobody is using QTermWidget::setTextCodec. It's okay to remove this public API for now. If someone still needs alternative encodings, a re-implementation using Qt 6 classes can be added later.

https://www.google.com/search?client=firefox-b-m&sca_upv=1&q=qtermwidget+%22setTextCodec%22

https://github.com/search?q=%22qtermwidget.h%22+%22setTextCodec%22&type=code https://github.com/search?q=import+qtermwidget+%22setTextCodec%22&type=code

[doug1234]

From a quick search on Google and GitHub, it seems nobody is using QTermWidget::setTextCodec. It's okay to remove this public API for now. If someone still needs alternative encodings, a re-implementation using Qt 6 classes can be added later.

https://www.google.com/search?client=firefox-b-m&sca_upv=1&q=qtermwidget+%22setTextCodec%22

https://github.com/search?q=%22qtermwidget.h%22+%22setTextCodec%22&type=code https://github.com/search?q=import+qtermwidget+%22setTextCodec%22&type=code

OK. I will take a look at it later today.

[doug1234]

Looks like QRegExp also needs to be upgraded for Qt6.

[doug1234]

CI build is now successful! The example runs and the regular expression search feature seems fine after the update and I see no other issues. However, I am not 100% on the codec change for unusual configurations. Please check my changes as much as possible.

[marcusbritanicus]

@yan12125 I believe the ci problem is a bug in PyQt-Builder. It really requires the binary qmake to exist in the system (src). However, the qmake binary is named qmake6 in Qt6.

I've opened an issue with pyqt-builer here.

Until then, we can use a hack, something like sudo ln -s /usr/bin/qmake6 /usr/bin/qmake

@doug1234 @yan12125 @tsujan In the meanwhile, we've figured out the other issue with pyqt and sip-wheel. Instead of making a symlink, we can pass the option --qmake=/usr/bin/qmake6 to the command:

CXXFLAGS="-I$PWD/../lib -I$PWD/../.build/lib" LDFLAGS="-L$PWD/../.build" sip-wheel --verbose --qmake=/usr/bin/qmake6

If you look at the log, you'll see that sip complains about [tool.sip.metadata] that needs to be upgraded. And when we were discussing the qmake issue, @philthompson10 from PyQt Builder, suggested a few changes to the pyproject.toml itself. While they're not critical, we can avoid potential issues others may face by making a few modifications. I"ll open a separate PR of it, so that it can be tested in its own time.

[marcusbritanicus]

CI build is now successful! The example runs and the regular expression search feature seems fine after the update and I see no other issues. However, I am not 100% on the codec change for unusual configurations. Please check my changes as much as possible.

The changes themselves look quite fine. But I do get a few artifacts when I tested it with QTerminal. I am not very sure if the issue is with qterminal itself. I am using the wip-qt6 branch.

https://github.com/lxqt/qtermwidget/assets/3350667/4972fdca-39ac-4152-8f35-ce9027b6e664

I'll revert the latest changes (codec and QRegExp) and then see if this still happens.

Update: @doug1234 I'm sorry, but it's the codec that's causing this issue. When I revert back to c4cedb9, the artefacts disappear.

[doug1234]

CI build is now successful! The example runs and the regular expression search feature seems fine after the update and I see no other issues. However, I am not 100% on the codec change for unusual configurations. Please check my changes as much as possible.

The changes themselves look quite fine. But I do get a few artifacts when I tested it with QTerminal. I am not very sure if the issue is with qterminal itself. I am using the wip-qt6 branch.

qterminal-artifacts.mp4 I'll revert the latest changes (codec and QRegExp) and then see if this still happens.

Update: @doug1234 I'm sorry, but it's the codec that's causing this issue. When I revert back to c4cedb9, the artefacts disappear.

Wow... it looks nothing like that for me. Any idea what is different on your end? I should have some time to look into the codex changes again later today.

[doug1234]

Actually after some more testing I get occasional oddities. I have a couple ideas. Let me see what happens.

[doug1234]

@marcusbritanicus I think I have fixed it. The issues I saw have gone away. However, I wasn't getting the same thing you were so please check it out when you get a chance.

[marcusbritanicus]

@marcusbritanicus I think I have fixed it. The issues I saw have gone away. However, I wasn't getting the same thing you were so please check it out when you get a chance.

@doug1234 I too can confirm that the issues are gone.

However, I'd like to do a few tests with random UTF-8 characters - text from asian languages, specifically. I'll do that and ping you.

Update: I was trying out several tests with random characters picked out from youtube titles. They mostly seem fine.

I think we can go ahead with merging this... :)

[marcusbritanicus]

A quick question: Version needs to be bumped to 2.0.0, right? Both in CMakeLists.txt and pyproject.toml

[stefonarch]

A quick question: Version needs to be bumped to 2.0.0, right? Both in CMakeLists.txt and pyproject.toml

yes

[doug1234]

I bumped the version to 2.0.0. Please give a final review @yan12125 as I think this is looking pretty good now.

[yan12125]

Thanks for the update. I tried non-BMP emoji, emoji flag sequence, and emoji variation selector, and there is no case working in Qt 5 but not in Qt 6. I think the implementation is good enough - will take a look this week.

[tsujan]

.... there is no case working in Qt 5 but not in Qt 6.

That's a Qt6 problem/regression I encountered years ago. It's visible in all Qt6 apps.

[tsujan]

Oh, sorry, I misread your comment! (Read "no case" as "one case".)

I meant that there are emojis which are shown correctly in Qt5 apps but not in Qt6 apps. Otherwise, I haven't tried the Qt6-based QTerminal yet.

[yan12125]

Thank you very much for the efforts! I will merge this along with https://github.com/lxqt/qterminal/pull/1067, after cleaning up remaining Qt 5 stuff in the latter.

That's a Qt6 problem/regression I encountered years ago. It's visible in all Qt6 apps.

Well, a case that breaks many Qt apos may be out of scope of this PR

[tsujan]

Well, a case that breaks many Qt apos may be out of scope of this PR

And that was my point: If there are some emojis that aren't shown correctly in the Qt6 version, the problem might be in Qt6 itself, not in QTerminal.

[yan12125]

Found an issue: after 0f2cbfeaecd2de38712b7784e50849d80fba1556, URLs are not underlined when hovered. I suspect changes in Filter.cpp break it, as that class is for filtering specific patterns (ex: URLs) from terminal contents.

Will the Qt6 port of QTerminal be ready before the next release, which will happen around Apr 15, 2024?

@tsujan It seems remaining work may not be trivial. I prefer to postpone Qt 6 port of qtermwidget to the next LXQt release.

[tsujan]

It seems remaining work may not be trivial.

I understand. It could be released whenever it's ready and tested.