Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QRegExp ports, compile plugins on Qt6, remove Qt6Core5Compat dependency #1748

Merged
merged 24 commits into from
Oct 24, 2024

Conversation

matterhorn103
Copy link
Contributor

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or

(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.

(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.

@matterhorn103
Copy link
Contributor Author

Combined with your work on it, this means everything in avogadrolibs is ported except avogadrolibs/avogadro/molequeue/client/client, which I'll tackle in the coming days. With QRegularExpression there's no such thing as different pattern syntaxes any more, so I propose just to take out the switch/case entirely. I might need your help with that.

The ports I've made are a bit of a mixed bag; some I just did the minimal changes to make it work but with the same strategies, even if there are easier or more idiomatic ways to do it with QRegularExpression; other times I redid things completely, such as with the new QRegularExpressionMatchIterator, which saves tedious while loops.

So I'd very much appreciate you looking over what I've done to check it matches the old code in outcome.

What I can say is that everything in this PR compiles against Qt5 successfully (though with -DBUILD_MOLEQUEUE=OFF).

avogadro/qtgui/richtextdelegate.cpp Dismissed Show dismissed Hide dismissed
avogadro/qtgui/richtextdelegate.cpp Dismissed Show dismissed Hide dismissed
avogadro/qtplugins/gamessinput/gamesshighlighter.cpp Fixed Show resolved Hide resolved
avogadro/qtplugins/symmetry/symmetrywidget.cpp Dismissed Show dismissed Hide dismissed
@ghutchis
Copy link
Member

Clearly I should have merged my work -- at least on the qt6 fixes. In principle, git will properly merge both PR.

I'll do a real review in a bit, but it looks like a great start towards qt6, thanks!

Copy link
Contributor

Here are the build results
Avogadro2.AppImage
macOS.dmg
Win64.exe
Artifacts will only be retained for 90 days.

@matterhorn103 matterhorn103 changed the title WIP: QRegExp ports QRegExp ports, compile plugins on Qt6, remove Qt6Core5Compat dependency Oct 24, 2024
@matterhorn103
Copy link
Contributor Author

With my commits today, all instances of QRegExp are ported.

Various qtplugins (apbs, coordinateeditor, cp2kinput, forcefield, gamessinput, svg) were being left out for the Qt6 build; these are now included again. Everything compiles fine.

Most of those were excluded because of the QRegExp stuff, but that is now solved. Any other issues that arose have also been solved. This means that plugin scripts now work!

svg was excluded preliminarily due to the changes to QSvg in Qt6, but this seems to have been done out of caution – the main difference seems to be that two classes were moved into the new QtSvgWidgets library, but we don't use those classes. When included, svg compiles fine.

The only things still being excluded are thus qtplugins/symmetry and qtplugins/qtaim.

The QRegExp instances in avogadrolibs/avogadro/molequeue have also been ported, which presumably breaks building MoleQueue itself. I don't view this as a problem for now as MoleQueue will no longer be buildable on Qt6 anyway, initially at least.

On my machine both Qt6 and Qt5 builds succeed, in conjunction with the corresponding pull requests to avogadroapp and OpenChemistry/openchemistry. As far as I can see, everything in both binaries works correctly, and all the menu options are now available in the Qt6 version, which wasn't the case till now. 😄

@avo-bot
Copy link

avo-bot commented Oct 24, 2024

This pull request has been mentioned on Avogadro Discussion. There might be relevant details there:

https://discuss.avogadro.cc/t/qt6-tracking-thread/5592/7

Copy link
Contributor

Here are the build results
macOS.dmg
Artifacts will only be retained for 90 days.

@ghutchis
Copy link
Member

Thanks, this is obviously a great step forward if we can adopt Qt6.

There are some errors still because the tests directory wasn't ported, e.g.
tests/qtgui/generichighlightertest.cpp

I'll look into the errors on Windows.

Copy link
Contributor

Here are the build results
macOS.dmg
Artifacts will only be retained for 90 days.

@matterhorn103
Copy link
Contributor Author

See if this helps with getting the tests to pass.

@ghutchis
Copy link
Member

I'm going to merge -- looks good so far.

@ghutchis ghutchis merged commit 5c6c81e into OpenChemistry:master Oct 24, 2024
20 of 21 checks passed
@ghutchis
Copy link
Member

Next time, can we break into smaller pull requests so we don't have identical commits on different branches (e.g., the AppImage changes)? That would be a big help -- although I realize the qt6 changes were kinda a pain.

@matterhorn103
Copy link
Contributor Author

Absolutely, though most of this ended up being pretty tied together. Do you ever do squash merge commits? Or is this less about the number of commits and more about the number of changes?

@ghutchis
Copy link
Member

ghutchis commented Oct 24, 2024

I sometimes do squash commits - but I couldn't do that in this case, because you pulled some of my commits on the molecular properties dialog, plus the AppImage changes.

The number of changes isn't so much the issue - I probably should have merged the initial work on the MolProp dialog since I knew you were working on qt6 fixes.

The challenge is when there are commits from different branches in one pull request. So it's usually easier to have smaller self-contained PR to merge.

Not a big deal though.

Copy link
Contributor

Here are the build results
Win64.exe
Artifacts will only be retained for 90 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants