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

QT6 migration #386

Merged
merged 13 commits into from
Nov 7, 2022
Merged

QT6 migration #386

merged 13 commits into from
Nov 7, 2022

Conversation

zjeffer
Copy link
Collaborator

@zjeffer zjeffer commented Oct 1, 2022

Will fix #356.

TODO

  • Fix all build errors
  • Fix all warnings
  • Remove all unnecessary QT_VERSION_CHECKs
  • Test notes on all platforms
    • Linux (I tested ubuntu18 with Qt5.9.5, ArchLinux with Qt5.15.6, ArchLinux with Qt6.4.0)
    • macOS
    • Windows 10
    • Windows 11 (tested with Qt 6.4.0)
  • Fix font weight issue
  • Make backwards compatible down to Qt 5.9
  • Set QDataStream version to 5.6 for maximum compatibility
  • Fix resizing issue: the left pane seems to grow instead of the right pane when resizing.
  • QT6 builds with GitHub actions

3rdParty/qxt/qxtglobalshortcut_p.h Outdated Show resolved Hide resolved
src/main.cpp Outdated Show resolved Hide resolved
src/mainwindow.cpp Show resolved Hide resolved
src/mainwindow.cpp Show resolved Hide resolved
src/mainwindow.cpp Show resolved Hide resolved
src/mainwindow.cpp Outdated Show resolved Hide resolved
src/mainwindow.cpp Outdated Show resolved Hide resolved
src/styleeditorwindow.cpp Outdated Show resolved Hide resolved
src/updaterwindow.cpp Outdated Show resolved Hide resolved
src/updaterwindow.cpp Show resolved Hide resolved
src/notelistview.cpp Outdated Show resolved Hide resolved
src/dbmanager.cpp Outdated Show resolved Hide resolved
@zjeffer
Copy link
Collaborator Author

zjeffer commented Oct 13, 2022

@nuttyartist @bjorn PR is complete & commits are squashed. The code needs to be reviewed and the build needs to be tested on macOS & Windows.

@zjeffer zjeffer requested a review from bjorn October 13, 2022 21:20
Copy link
Collaborator

@bjorn bjorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left a few comments.

src/notelistview.cpp Outdated Show resolved Hide resolved
src/styleeditorwindow.cpp Outdated Show resolved Hide resolved
src/taglistdelegate.cpp Show resolved Hide resolved
3rdParty/qxt/qxtglobalshortcut.cpp Show resolved Hide resolved
@nuttyartist
Copy link
Owner

So, will we have one codebase for both Qt6 and Qt5?

@zjeffer
Copy link
Collaborator Author

zjeffer commented Oct 20, 2022

So, will we have one codebase for both Qt6 and Qt5?

In my opinion, that would be best. The changes between Qt 5 and 6 are not that large, judging by the relatively small amount of work I've had to do for this PR.

@zjeffer
Copy link
Collaborator Author

zjeffer commented Oct 21, 2022

I squashed commits & addressed the code review comments, now only needs to be tested on macOS and Windows.

@zjeffer
Copy link
Collaborator Author

zjeffer commented Oct 21, 2022

I fixed a problem with the windows build (this means we need a windows qt6 build check before we merge).

I also noticed that on both Windows and Linux, resizing works strangely. Whenever I start to resize, the left pane grows incredibly large, causing the middle and right panes to shrink down a lot. This happens on both Qt 5.15 and 6.4. Doesn't happen on Notes v2.0.0. Will investigate.

@nuttyartist
Copy link
Owner

Watching your updates. Let me know when you want me to test it.

@zjeffer
Copy link
Collaborator Author

zjeffer commented Oct 22, 2022

Not sure why, but the appveyor build fails because it can't find qmake. Is it trying to search for Qt5's qmake? I have 0 experience with appveyor so I'll need some help with this.

@guihkx
Copy link
Collaborator

guihkx commented Oct 22, 2022

I have added initial support for Qt 6 builds on GitHub Actions on my local qt6 branch (it's syncd with this PR's branch). Right now, only the Qt 6 build of Windows is failing:

https://github.com/guihkx/notes/actions/runs/3304734012

Feel free to incorporate my commits on this PR for your tests.

@nuttyartist
Copy link
Owner

Should we drop Appveyor in favor of GitHub Actions? We already do in Actions whatever we are doing in Appveyor right?

@zjeffer
Copy link
Collaborator Author

zjeffer commented Oct 23, 2022

Should we drop Appveyor in favor of GitHub Actions?

Yes, but I think we should also include 5.9 builds in windows.yml.

@nuttyartist Could you take a look at the latest windows qt6 build that fails? This is the error in framelesswindow.cpp (line 275): No member named 'getContentsMargins' in 'QMainWindow'. getContentsMargins is defined in QLayout, not in QMainWindow. What were you trying to do here?

@guihkx
Copy link
Collaborator

guihkx commented Oct 23, 2022

@nuttyartist:
Should we drop Appveyor in favor of GitHub Actions? We already do in Actions whatever we are doing in Appveyor right?

I suppose we can do that, yes.

@zjeffer
Yes, but I think we should also include 5.9 builds in windows.yml.

Hmm, is that really necessary, though? Aren't we only supporting 5.9 because of Ubuntu 18.08? 🤔

@zjeffer
Copy link
Collaborator Author

zjeffer commented Oct 23, 2022

Hmm, is that really necessary, though? Aren't we only supporting 5.9 because of Ubuntu 18.08?

Ah, of course. Is there no Windows version still using 5.9?

@guihkx
Copy link
Collaborator

guihkx commented Oct 23, 2022

As far as I know, the only way to install Qt on Windows is through the official installer provided by Qt (or via aqtinstall, but I'm not sure if that's a common thing to do).

So, different from Ubuntu (and Linux distros in general), on Windows it's very easy to upgrade to the latest Qt release.

@guihkx
Copy link
Collaborator

guihkx commented Oct 23, 2022

FWIW, you can also remove packaging/windows/Notes_Inno_Script.iss (unless @nuttyartist still needs it?).

@nuttyartist
Copy link
Owner

FWIW, you can also remove packaging/windows/Notes_Inno_Script.iss (unless @nuttyartist still needs it?).

Yes, you can remove it.

@zjeffer
Copy link
Collaborator Author

zjeffer commented Oct 29, 2022

@nuttyartist The appveyor build still runs for some reason. I guess you need to remove some kind of appveyor webhook?

This PR is now ready for testing.

@nuttyartist
Copy link
Owner

@nuttyartist The appveyor build still runs for some reason. I guess you need to remove some kind of appveyor webhook?

This PR is now ready for testing.

Yes, I removed the webhook.

Great, I'll test it soon.

@nuttyartist
Copy link
Owner

Works well on macOS, compiles on QT6.4 and Qt5.15.2. 👍
Will now test it on Windows.

@nuttyartist
Copy link
Owner

Works well on Windows as well 👍

@bjorn Any comments before I merge?

@nuttyartist
Copy link
Owner

Merging... @zjeffer Thank you so much! Great work.

@nuttyartist nuttyartist merged commit f7d5f4c into nuttyartist:dev Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Converting codebase to Qt 6
4 participants