Remove support for qt5, fix build warnings in qt 6.8.0

[zjeffer]

Fixes lots of deprecation warnings I encountered with Qt 6.8.0

One more warning is present, but there's a comment next to it:

notes/src/mainwindow.cpp

Lines 3945 to 3948 in 75c876e

	QApplication::setActiveWindow(
	this); // TODO: The docs say this function is deprecated but it's the only one
	// that works in returning the user input from m_editorSettingsWidget
	// Qt::Popup

Why doesn't QWidget::activateWindow() work? I don't really know what this part of the code actually does.

[zjeffer]

Ugh, looks like Qt5 doesn't build because it doesn't have some of the newer functions :(
I might have to use ifdefs again around every call of those functions? Would be kind of ugly though.

As an aside, how much longer will we support Qt5 for newer builds? Qt5 5.15 LTS support has already ended May 2023, and extended lifetime support for Qt subscribers will end May 2025: https://www.qt.io/blog/qt-5.15-extended-support-for-subscription-license-holders

[nuttyartist]

To be honest, it kinda sucks that Qt 6 supports only Windows 10+ (from a specific build). Some people still uses old versions. I'm kinda split on this, but I'm leaning toward still supporting Qt 5 cause it still makes up about 20% of the downloads: https://tooomm.github.io/github-release-stats/?username=nuttyartist&repository=notes (maybe even more since I don't count the bundling of the Windows setup build).

What do you guys think?

[zjeffer]

I wasn't aware Qt6 is unsupported on versions of Windows before 10, that's annoying...

At some point we do have to make a decision to stop supporting Qt5, though. Windows 10 is already almost a decade old.
Down the line it'll get harder and harder to ensure Qt5 still works, unless we put ifdefs everywhere.

In my opinion users who are still on Qt5 should be recommended to upgrade to Qt6 if possible, and otherwise stay on the last Notes version that is supported on their system. Maybe we should implement some way to block automatic updates on Qt5 if the users' OS doesn't support Qt6?

[guihkx]

how much longer will we support Qt5 for newer builds?

I have no strong opinion on this matter, because I barely touch the C++/QML code, so the maintenance burden of Qt 5 won't affect me as much. All the work needed for working Qt 5 builds is already done, and it's unlikely that major changes will be needed going forward.

That being said, I think we should still provide a Qt 5 version, but only because of Ubuntu 20.04, which is still supported by Canonical (for 5 more months...) and can't run the Qt 6 version.

Any user running a Windows or macOS version that can't run Qt 6, is deliberately running an operating system officially unsupported by Microsoft and Apple, so they are essentially "own their own", in my opinion.

I'm kinda split on this, but I'm leaning toward still supporting Qt 5 cause it still makes up about 20% of the downloads

I'm also not sure that's a valid indicative of how much Qt 5 is used in favor of Qt 6.

I suspect that most people downloading Notes don't even know what Qt is. lol

They are probably downloading the Qt 5 version because GitHub releases are listed alphabetically, and that's the first option they see...

And we contribute to that, too. We don't explain the differences between the two Qt versions anywhere.

I might have to use ifdefs again around every call of those functions? Would be kind of ugly though.

Down the line it'll get harder and harder to ensure Qt5 still works, unless we put ifdefs everywhere.

I agree, but we already do that plenty, anyway:

$ git grep QT_VERSION_CHECK | wc -l
65

¯\_(ツ)_/¯

[zjeffer]

Down the line it'll get harder and harder to ensure Qt5 still works, unless we put ifdefs everywhere.

I agree, but we already do that plenty, anyway:
$ git grep QT_VERSION_CHECK | wc -l
65

Indeed, I think a lot of them are mine from when I initially added qt6 support: #386. This was a pain to add and makes the code as a whole less readable. Removing qt5 support would clean it up nicely ;)

[nuttyartist]

Alrighty, you both got some good points. I guess it's time to move to Qt 6 and streamline the development (that's what I did with Daino Notes as well). Should we move to Qt 6 now or wait the 5 months @guihkx mentioned?

P.S. There are initiative to make Qt 6 work on older OSes (https://github.com/crystalidea/qt6windows7, for example).

[guihkx]

Should we move to Qt 6 now or wait the 5 months @guihkx mentioned?

Alternatively, we could maintain a 2.3.x branch (in a best-effort manner), where the Qt 5 version would still build, but we would only push bug fixes there.

Then, if @zjeffer wants to, this PR could be renamed and we'd remove all the Qt 5 stuff (one big commit for that is fine by me). I can help with the CI/CD part, if necessary.

P.S. There are initiative to make Qt 6 work on older OSes (https://github.com/crystalidea/qt6windows7, for example).

That looks interesting, but it seems that they only provide qtbase, and no qtdeclarative (for the QML stuff), so I think there's no use for us...

[zjeffer]

Alternatively, we could maintain a 2.3.x branch (in a best-effort manner), where the Qt 5 version would still build, but we would only push bug fixes there.

Then, if @zjeffer wants to, this PR could be renamed and we'd remove all the Qt 5 stuff (one big commit for that is fine by me). I can help with the CI/CD part, if necessary.

Sounds good to me. I think the branch should be called 'qt5', though, so it's clear what its purpose is.

[zjeffer]

Qt5 support has now been removed in the latest commits. I also created the qt5 branch, branched off from master.

@guihkx Can you take care of the CI/CD?

[guihkx]

@guihkx Can you take care of the CI/CD?

Sure.

[guihkx]

Need to update the documentation as well.

At the end we should also squash those commits into just two: One for the Qt 6.8 warning fixes, and one for the Qt 5 removal.

[guihkx]

Forgot to update the minimum system requirements when building on Windows.

I also added a commit message about the Qt 5 removal, so let me know if it's good enough.

[zjeffer]

LGTM 👍

[nuttyartist]

Thanks, @zjeffer! Testing...

[nuttyartist]

I added a fix to macOS frameless window not showing up correctly due to recent changes in Qt 6.8 (https://codereview.qt-project.org/c/qt/qtbase/+/561465). But there's still an issue with the notesList for some reason. I'm out-of-time today, but I'll try to get back to it soon (or if someone can take a stab at it).

[guihkx]

Apologies for the force-push, but I had to rebase against master to fix a conflict after I merged #717.

I added a fix to macOS frameless window not showing up correctly due to recent changes in Qt 6.8

IMO that work should go into a separate PR. This one fixes some compiler warnings and removes Qt 5 support.

We also don't ship Notes with Qt 6.8.0 yet (though I'm planning to do that), so maybe that fix could wait a bit...

But that's up to you, of course. I still left your commit here after the rebase (I fixed the linting errors as well).

[nuttyartist]

Alrighty, feel free to remove the commit and merge, I'll open a new PR for it.

[guihkx]

@nuttyartist I suggest you add a branch protection rule for the qt5 branch, to avoid its accidental deletion and force-pushes...

[nuttyartist]

Just added. Looks good? (at the bottom I also selected "Block force pushes").

[guihkx]

Yup, looks good!

[nuttyartist]

Yup, looks good!

I notice it says "Applies to 0 targets", maybe it's incorrect?

[guihkx]

I'm not sure, maybe try removing the quotation marks from the name pattern of the branch?

[nuttyartist]

I'm not sure, maybe try removing the quotation marks from the name pattern of the branch?

Worked, thanks (: