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

Replace deprecated parts of code #2419

Merged
merged 2 commits into from
Oct 26, 2018
Merged

Replace deprecated parts of code #2419

merged 2 commits into from
Oct 26, 2018

Conversation

brainplot
Copy link
Contributor

@brainplot brainplot commented Oct 25, 2018

Performing a dev build against the latest version of Qt fails because of some deprecated members.

Description

As of Qt 5.10, two functions and one class that were used in the code are deprecated.
Since this project supports Qt version 5.2 and higher, I replaced the two functions marked as deprecated with their updated versions and wrapped the new code in a compile-time check which enables it only if the project is being compiled against Qt 5.10 and up.

In the CSV import utility, the QSignalMapper class was used, which is entirely deprecated now. In place of it, lambdas can be used, which are part of the C++11 standard so I don't think that compile-time checks are required here since it's the version that the project is using, looking at the CMake scripts.
I removed the usage of the aforementioned class and used a lambda to achieve the same effect.

Motivation and context

I'm running Archlinux, which uses the latest version of Qt (5.11.2), and I wasn't able to compile the project with the -DWITH_DEV_BUILD flag.

How has this been tested?

Built with -DWITH_ASAN, -DWITH_TESTS and -DWITH_GUI_TESTS; test n.30, n.31 and n.32 failed but they failed before I made any change as well.
I manually tested the parts of code I changed: I auto-typed my password on a few sites; tried to re-import my database previously exported in .csv format and changed some columns around with the comboboxes. Everything worked as expected.

Types of changes

  • ✅ Bug fix (non-breaking change which fixes an issue)

Checklist:

  • ✅ I have read the CONTRIBUTING document. [REQUIRED]
  • ✅ My code follows the code style of this project. [REQUIRED]
  • ✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

Performing a dev build against the latest version of Qt failed
because of some deprecated members. They have been replaced according to
the Qt documentation.
Copy link
Member

@droidmonkey droidmonkey left a comment

Choose a reason for hiding this comment

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

It's not wrong, just difficult.

My only issue with this is that our CI will only be testing the original code on Linux (ie, the deprecated code) since we test with Qt 5.2. Windows CI will test the new code.

connect(combo, QOverload<int>::of(&QComboBox::currentIndexChanged), [=]{ comboChanged(combo, i); });
#else
connect(combo, static_cast<void(QComboBox::*)(int)>(&QComboBox::currentIndexChanged), [=]{ comboChanged(combo, i); });
#endif
Copy link
Member

Choose a reason for hiding this comment

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

This is very ugly, almost incomprehensible. I feel like this interaction is just not coded well to begin with due to the use of the QSignalMapper in the first place.

Copy link
Contributor Author

@brainplot brainplot Oct 25, 2018

Choose a reason for hiding this comment

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

To be honest I don't love it either, but there's no other way I know of I can tell the compiler to pick

void currentIndexChanged(int)

instead of

void currentIndexChanged(const QString&)

The QOverload<int>::of() syntax is a lot more readable in my opinion but it's only available since Qt 5.7.

Currently, there's no overload of the connect() method that can take a lambda if the SIGNAL() syntax is used. That's why I had to resort to that ugly syntax.

EDIT: if we break up that statement in this way, maybe it's a bit more readable.

#if QT_VERSION >= QT_VERSION_CHECK(5, 7, 0)
    constexpr auto onComboChange =
        QOverload<int>::of(&QComboBox::currentIndexChanged);
#else
    constexpr auto onComboChange =
        static_cast<void(QComboBox::*)(int)>(&QComboBox::currentIndexChanged);
#endif
    connect(combo, onComboChange, [=]{ comboChanged(combo, i); });

It's still ugly, but I think that QOverload<int>::of gives a strong hint about what the purpose of the code is.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, the AppImage build uses a more recent Qt version (5.10 I believe).

Copy link
Contributor Author

@brainplot brainplot Oct 26, 2018

Choose a reason for hiding this comment

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

@phoerious the code is still in my first version though. I haven't pushed my latest revision (i.e. the one where I extract the code that selects the right overload). Is it okay all the same?

@droidmonkey droidmonkey added this to the v2.4.0 milestone Oct 25, 2018
@brainplot
Copy link
Contributor Author

brainplot commented Oct 25, 2018

I understand the problem. However, I don't think to have changed the code in a very significant way (apart from the QSignalMapper thing, maybe).
I've only replaced a few method calls with new ones that, by contract, should do the same thing; so theoretically if the old code worked, the new code should work just as well.

But if you feel that this is too risky, feel free to reject this PR! :)

@droidmonkey
Copy link
Member

I don't have major heartburn with the changes, see what @phoerious thinks.

@droidmonkey droidmonkey requested a review from phoerious October 25, 2018 23:20
Q_OS_MACOS is now the only macro available to identify a machine running
macOS, the others are now deprecated.
See https://doc.qt.io/qt-5/qtglobal.html#Q_OS_OSX and
https://doc.qt.io/qt-5/qtglobal.html#Q_OS_MAC.
@brainplot
Copy link
Contributor Author

I replaced Q_OS_OSX and Q_OS_MAC with Q_OS_MACOS, as they're now both deprecated according to the Qt documentation. It also made the code base more consistent: only one macro is now used to identify an Apple machine instead of two, which could be a bit confusing.
I recommend switching to my first commit to see the actual reason I opened this PR :)

@droidmonkey
Copy link
Member

Good changes!

Copy link
Member

@phoerious phoerious left a comment

Choose a reason for hiding this comment

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

Looks good, ship it!

@phoerious phoerious merged commit f31d65b into keepassxreboot:develop Oct 26, 2018
@brainplot brainplot deleted the remove-deprecations branch October 26, 2018 13:25
droidmonkey added a commit that referenced this pull request Mar 19, 2019
- New Database Wizard [#1952]
- Advanced Search [#1797]
- Automatic update checker [#2648]
- KeeShare database synchronization [#2109, #1992, #2738, #2742, #2746, #2739]
- Improve favicon fetching; transition to Duck-Duck-Go [#2795, #2011, #2439]
- Remove KeePassHttp support [#1752]
- CLI: output info to stderr for easier scripting [#2558]
- CLI: Add --quiet option [#2507]
- CLI: Add create command [#2540]
- CLI: Add recursive listing of entries [#2345]
- CLI: Fix stdin/stdout encoding on Windows [#2425]
- SSH Agent: Support OpenSSH for Windows [#1994]
- macOS: TouchID Quick Unlock [#1851]
- macOS: Multiple improvements; include CLI in DMG [#2165, #2331, #2583]
- Linux: Prevent Klipper from storing secrets in clipboard [#1969]
- Linux: Use polling based file watching for NFS [#2171]
- Linux: Enable use of browser plugin in Snap build [#2802]
- TOTP QR Code Generator [#1167]
- High-DPI Scaling for 4k screens [#2404]
- Make keyboard shortcuts more consistent [#2431]
- Warn user if deleting referenced entries [#1744]
- Allow toolbar to be hidden and repositioned [#1819, #2357]
- Increase max allowed database timeout to 12 hours [#2173]
- Password generator uses existing password length by default [#2318]
- Improve alert message box button labels [#2376]
- Show message when a database merge makes no changes [#2551]
- Browser Integration Enhancements [#1497, #2253, #1904, #2232, #1850, #2218, #2391, #2396, #2542, #2622, #2637, #2790]
- Overall Code Improvements [#2316, #2284, #2351, #2402, #2410, #2419, #2422, #2443, #2491, #2506, #2610, #2667, #2709, #2731]
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