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

Fix build deprecation warnings #714

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

zjeffer
Copy link
Collaborator

@zjeffer zjeffer commented Oct 20, 2024

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
Copy link
Collaborator Author

zjeffer commented Oct 20, 2024

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
Copy link
Owner

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
Copy link
Collaborator Author

zjeffer commented Oct 20, 2024

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
Copy link
Collaborator

guihkx commented Oct 20, 2024

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
Copy link
Collaborator Author

zjeffer commented Oct 20, 2024

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
Copy link
Owner

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
Copy link
Collaborator

guihkx commented Oct 22, 2024

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
Copy link
Collaborator Author

zjeffer commented Oct 22, 2024

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.

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.

3 participants