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 all Qt 5.15 deprecation warnings #7783

Merged
merged 18 commits into from
Jun 22, 2024

Conversation

c4rlo
Copy link
Contributor

@c4rlo c4rlo commented Apr 3, 2022

Fix all deprecation warnings when building against Qt 5.15. This is a first step towards being able to eventually upgrade to Qt 6 (see #7774 for discussion).

This now builds cleanly with -DWITH_XC_ALL=ON -DWITH_GUI_TESTS=ON -DWITH_DEV_BUILD=ON.

Since this is a sizeable change, I've done my best to structure this sensibly into a set of commits.

I've assumed here that the earliest Qt version we support is 5.9.5, as stated in INSTALL.md. I have adjusted the check in CMakeLists.txt accordingly (0a1e776). In some cases I've resorted to compile-time checks for the Qt version.

One particular annoyance is that QUrl::topLevelDomain() has been deprecated without direct replacement. I worked around that by accessing its functionality indirectly (9ed6095).

Testing strategy

Built with -DWITH_XC_ALL=ON -DWITH_GUI_TESTS=ON -DWITH_DEV_BUILD=ON -DCMAKE_BUILD_TYPE=Debug and ran all tests.

Did the same thing in an Ubuntu 18.04 container that has libbotan-kpxc-2-dev from ppa:phoerious/keepassxc installed in it. This one did cause some test failures (in TestCli::testClip() and TestCli::testInvalidDbFiles()), but those also failed against the 2.7.4 tag, so I feel okay about them.

Suggestion

In order to keep the code base deprecation-clean, it would be excellent if the automated tests (i.e. TeamCity) could be enhanced to build with -DWITH_XC_ALL=ON -DWITH_GUI_TESTS=ON -DWITH_DEV_BUILD=ON -DCMAKE_BUILD_TYPE=Debug and ideally against different Qt versions, including Qt 5.9.5 (assuming that we want to keep supporting it) and Qt 5.15 (the last Qt 5 version).

Type of change

  • ✅ Refactor (significant modification to existing code)

@c4rlo c4rlo changed the title Fix all Qt 5.15 deprecation warnings [WIP] Fix all Qt 5.15 deprecation warnings Apr 3, 2022
@codecov-commenter
Copy link

codecov-commenter commented Apr 3, 2022

Codecov Report

Attention: Patch coverage is 70.54545% with 81 lines in your changes missing coverage. Please review.

Project coverage is 64.85%. Comparing base (e6db2ce) to head (0914ce9).
Report is 1 commits behind head on develop.

Current head 0914ce9 differs from pull request most recent head 0115713

Please upload reports for the commit 0115713 to get more accurate results.

Files Patch % Lines
src/cli/DatabaseEdit.cpp 46.15% 7 Missing ⚠️
src/gui/entry/EntryModel.cpp 12.50% 7 Missing ⚠️
src/cli/Analyze.cpp 33.33% 6 Missing ⚠️
src/cli/Utils.cpp 62.50% 6 Missing ⚠️
src/gui/styles/base/BaseStyle.cpp 0.00% 6 Missing ⚠️
src/browser/BrowserEntryConfig.cpp 0.00% 4 Missing ⚠️
src/cli/Edit.cpp 33.33% 4 Missing ⚠️
src/core/PasswordHealth.cpp 0.00% 4 Missing ⚠️
src/cli/Add.cpp 40.00% 3 Missing ⚠️
src/cli/DatabaseCreate.cpp 72.73% 3 Missing ⚠️
... and 23 more
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #7783      +/-   ##
===========================================
+ Coverage    63.79%   64.85%   +1.06%     
===========================================
  Files          365      342      -23     
  Lines        44783    44416     -367     
===========================================
+ Hits         28568    28804     +236     
+ Misses       16215    15612     -603     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tenzap
Copy link
Contributor

tenzap commented Apr 4, 2022

FYI, with 2.7.0 (+ c33995e) it is still possible to build with Qt 5.5 (but not below)

@c4rlo
Copy link
Contributor Author

c4rlo commented Apr 4, 2022

FYI, with 2.7.0 (+ c33995e) it is still possible to build with Qt 5.5 (but not below)

Hmm, so what is the minimum Qt version the maintainers want to support? Currently, 5.9.5 is what's documented (in INSTALL.md).

@c4rlo c4rlo force-pushed the qt-modernize branch 6 times, most recently from e40c912 to 7e867bf Compare April 18, 2022 12:54
@c4rlo c4rlo changed the title [WIP] Fix all Qt 5.15 deprecation warnings Fix all Qt 5.15 deprecation warnings Apr 18, 2022
@c4rlo
Copy link
Contributor Author

c4rlo commented Apr 18, 2022

Ok, I think this is in a reasonable state now. Regarding the test failures:

Ubuntu failure

From the logs, this is:

[12:58:26]
[Step 3/3] FAIL!  : TestKdbx4Format::testCustomData() Compared values are not the same
[12:58:26]
[Step 3/3]    Loc: [/opt/buildagent/work/c401303cba1b4098/tests/TestKdbx4.cpp(471)]

That line is:

    QCOMPARE(*clonedEntry->customData(), *entry->customData());

Now, Entry::clone() calls CustomData::copyDataFrom(), which calls CustomData::updateLastModified(), which can modify the custom data if we're just on the cusp of a new second; so I suspect that might be a pre-existing bug (likely affecting only the tests) that happens occasionally.

Windows failure

Not much detail in the logs, just:

[13:01:48]
[Step 3/4] 1/3 Test #39: testgui ..........................***Failed   23.33 sec

Since I don't have a Windows build setup ready to go, I'd appreciate any hints or details.

@droidmonkey
Copy link
Member

You can ignore the gui failures, they are transient

@c4rlo
Copy link
Contributor Author

c4rlo commented Apr 18, 2022

Cool. My latest update just rebased against develop; good to see that both failures have gone away. From my POV, this now seems good to merge.

The CustomData failure is as I suspected, I'll open a separate PR for that.

c4rlo added a commit to c4rlo/keepassxc that referenced this pull request Apr 18, 2022
Otherwise, assertions in TestKdbx4::testCustomData() may fail on rare
occasions, because the customData in a cloned entry won't be identical
to its original, because of its potentially-updated LastModified
property.

Originally noticed in
keepassxreboot#7783 (comment).
droidmonkey pushed a commit that referenced this pull request Apr 18, 2022
Otherwise, assertions in TestKdbx4::testCustomData() may fail on rare
occasions, because the customData in a cloned entry won't be identical
to its original, because of its potentially-updated LastModified
property.

Originally noticed in
#7783 (comment).
@droidmonkey droidmonkey added build system pr: refactoring Pull request that refactors code labels May 1, 2022
t-h-e pushed a commit to t-h-e/keepassxc that referenced this pull request Sep 8, 2022
Otherwise, assertions in TestKdbx4::testCustomData() may fail on rare
occasions, because the customData in a cloned entry won't be identical
to its original, because of its potentially-updated LastModified
property.

Originally noticed in
keepassxreboot#7783 (comment).
src/CMakeLists.txt Outdated Show resolved Hide resolved
src/cli/Utils.h Outdated Show resolved Hide resolved
src/core/Global.h Outdated Show resolved Hide resolved
src/core/Tools.cpp Outdated Show resolved Hide resolved
src/core/Tools.cpp Outdated Show resolved Hide resolved
c4rlo added a commit to c4rlo/keepassxc that referenced this pull request Jun 2, 2024
It does not need to be a recursive mutex, as per
keepassxreboot#7783 (comment).
c4rlo added a commit to c4rlo/keepassxc that referenced this pull request Jun 2, 2024
It does not need to be a recursive mutex, as per
keepassxreboot#7783 (comment).
@c4rlo
Copy link
Contributor Author

c4rlo commented Jun 2, 2024

I'll merge this after 2.7.9 is bottled up, don't need any merge conflicts between branches.

I've just rebased this again so there shouldn't be any merge conflicts (but don't mind if you want to defer merge after 2.7.9 anyway).

@c4rlo c4rlo mentioned this pull request Jun 4, 2024
c4rlo added 18 commits June 22, 2024 00:03
qSort() is deprecated.
QDateTime::toString(Qt::DefaultLocaleShortDate) will go away in Qt 6;
replace it as recommended in the docs, introducing a Clock::toString()
helper function.

The replacement functionality we use already exists in Qt 5.9.5, our
minimum supported Qt version.
Replace it with

  QLocale::system().toString(..., QLocale::ShortFormat)

as recommended in the official docs.

The replacement already exists in Qt 5.9.5, our earliest supported Qt
version.
...instead of QDate(QDateTime) constructor, which is deprecated as of Qt
5.15.

Since QDateTime::startOfDay() is only available in Qt 5.14, we need to
use an #if.
QString::SplitBehavior (of which SkipEmptyParts is an element) was
deprecated in Qt 5.15.

Its designated replacement, Qt::SplitBehavior, was only added in Qt
5.14. Hence we need to use some preprocessor magic to keep supporting
older Qt versions (we require at minimum Qt 5.9.5).
It is deprecated as of Qt 5.15.
QSet::toList() is deprecated as of Qt 5.14.

Use the recommended replacement QSet::values() instead. It already
exists as of Qt 5.9.5, our minimum supported Qt version.
Both methods are deprecated since Qt 5.14.

The recommended replacement

  QSet(list.begin(), list.end())

is only available from Qt 5.14. Since we support Qt 5.9.5 or better,
provide our own 'Tools::asSet()' helper function for this purpose.
QHash::insertMulti is deprecated as of Qt 5.15.

QMultiHash already exists in Qt 5.9.5, our earliest supported Qt
version.
The overload we were using before has been deprecated in Qt 5.15. To get
the same behaviour as before, we need to use QProcess::splitCommand(),
which was only introduced in Qt 5.15, hence we need to use an #if for
the Qt version.
QProcess::pid() has been deprecated since Qt 5.15. Recommended
replacement QProcess::processId() exists since Qt 5.3.
The "HighQuality" version is deprecated as of Qt 5.14.

Since it's not immediately clear at which point in Qt version history it
becomes preferable to use the non-"HighQuality" one, we only do so in Qt
5.14 onwards.
QPalette::background() is deprecated as of Qt 5.13.

Its replacement, QtPalette::window(), is already available in Qt 5.9.5,
our minimum supported Qt version.
Instead of their deprecated aliases.
Qt 5.15 deprecates usage of non-'Qt::'-prefixed 'endl' and 'flush' I/O
manipulators.

Since the 'Qt::' versions are only introduced in Qt 5.14, we backport
them for older Qt versions (since we support down to Qt 5.9.5).
Instead of deprecated constructor QMutex(QMutex::RecursionMode), use
recommended replacement class QRecursiveMutex.

This deprecation was done only in the KDE branch of Qt, specifically in
https://invent.kde.org/qt/qt/qtbase/-/commit/6d5988831862f081404270bd7fa2f25bc836fdc9.
Since that commit is included in the Qt packaged for some Linux
distributions (e.g. Arch Linux:
https://archlinux.org/packages/extra/x86_64/qt5-base/), it seems worth
respecting this deprecation.

Since QRecursiveMutex was only introduced in Qt 5.14, whereas we need to
support Qt 5.9.5 or better, this involves some #ifdef magic.
Use non-deprecated QComboBox::sizeAdjustPolicy setting
It does not need to be a recursive mutex, as per
keepassxreboot#7783 (comment).
@droidmonkey droidmonkey merged commit 88b7624 into keepassxreboot:develop Jun 22, 2024
11 checks passed
c4rlo added a commit to c4rlo/keepassxc that referenced this pull request Jun 22, 2024
Addendum to 32ac59e post keepassxreboot#7783 merge
(needed due to subsequent commits).
c4rlo added a commit to c4rlo/keepassxc that referenced this pull request Jun 22, 2024
Similar to 7458712 from keepassxreboot#7783 (needed
due to subsequent commits).
pull bot pushed a commit to tigerwill90/keepassxc that referenced this pull request Jun 23, 2024
* Deprecated qSort() -> std::sort()
* Replace QDateTime::toString(Qt::DefaultLocaleShortDate) with Clock::toString()
* Replace QDateTime::toString(Qt::SystemLocaleShortDate) with QLocale::system().toString(..., QLocale::ShortFormat)
* Use QDateTime::startOfDay() instead of QDate(QDateTime) 
  Note: QDateTime::startOfDay() is only available in Qt 5.14, we need to guard it
* Replace QString::SkipEmptyParts with Qt::SkipEmptyParts
  Note: Its designated replacement, Qt::SplitBehavior, was only added in Qt 5.14.
* Don't call deprecated QFlags(nullptr) constructor
* QSet::{toList->values}
* Replace QList::toSet, QSet::fromList with Tools::asSet()
* QHash::insertMulti -> QMultiHash::insert
* QProcess::startDetached: non-deprecated overload
* QProcess::{pid->processId}
* QPainter::{HighQuality->}Antialiasing
* QPalette::{background->window}()
* Use Qt::{Background,Foreground}Role
* endl -> Qt::endl, flush -> Qt::flush
* Make YubiKey::s_interfaceMutex non-recursive
* OpenSSHKeyGenDialog: use non-deprecated QComboBox::sizeAdjustPolicy setting
BryanJacobs added a commit to BryanJacobs/keepassxc that referenced this pull request Jul 26, 2024
* upstream/develop: (57 commits)
  Fix typos in tooltips from EditEntryWidgetBrowser.ui
  Refactor: separate GUI sources from core sources
  Database key settings: fix UI bug
  tests: gui: Fix NULL dereference in GUI tests
  Docs: explain how to generate passwords with the browser extension (keepassxreboot#9242)
  Fix a couple more Qt 5.15 deprecation warnings (keepassxreboot#10953)
  Update URLs to Chrome Web Store page for `KeePassXC-Browser` extension
  Update browser extension icon states in documentation (keepassxreboot#10875)
  Fix сentering icon and text on buttons
  Fix backup file path substitution
  Verify USB listener callback handle
  Refactor Database Settings (keepassxreboot#9485)
  Fix all Qt 5.15 deprecation warnings (keepassxreboot#7783)
  Passkey importer: fix file picker parent
  Require Qt >= 5.12
  Passkeys: Fix showing correct username in the reports
  Show character count in password generator dialog (keepassxreboot#10940)
  Increase the time interval for window show workaround
  Snap: Remove $HOME access from keepassxc-proxy
  Correct libusb usage on FreeBSD (keepassxreboot#10736)
  ...

# Conflicts:
#	src/CMakeLists.txt
#	src/core/Database.cpp
#	src/core/Database.h
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build system cleanup pr: refactoring Pull request that refactors code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants