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

Don't use deprecated Qt5 functions #5086

Closed
wants to merge 13 commits into from
Closed

Don't use deprecated Qt5 functions #5086

wants to merge 13 commits into from

Conversation

enp2s0
Copy link
Contributor

@enp2s0 enp2s0 commented Jul 21, 2019

This PR removes a number of deprecated QT functions.
QColor::dark() becomes QColor::darker()
QColor::light() becomes QColor::lighter()
QFontMetrics::width() becomes QFontMetrics::horizontalAdvance()
QDir objects can no longer have their paths set via operator=, use QDir::setPath() instead

I still get compiler warnings about obsolete QSignalMapper but I don't know enough Qt to remove them.

@enp2s0
Copy link
Contributor Author

enp2s0 commented Jul 21, 2019

This PR builds fine on my machine (Arch Linux, x86-64). However, it fails with a compilation error on travis and circleci...

@PhysSong
Copy link
Member

QFontMetrics::width() becomes QFontMetrics::horizontalAdvance()

The change was introduced in Qt 5.11, but the latest Qt version that's officially supported is 5.9. I think you should either revert the change or use #define horizontalAdvance width with a Qt version check.

@enp2s0
Copy link
Contributor Author

enp2s0 commented Jul 21, 2019

The #define seems really ugly to me, so I'll just revert it for now. Once Qt 5.9 support is dropped (or when the latest version of Qt fully removes width(), we can do a new PR to remove it.

@PhysSong
Copy link
Member

I still get compiler warnings about obsolete QSignalMapper but I don't know enough Qt to remove them.

I think it can be replaced with functor-based connections. In other words, you can connect signals to lambda functions in Qt5. See https://doc.qt.io/qt-5/qobject.html#connect-4 and https://doc.qt.io/qt-5/qobject.html#connect-5 for details.
If you're not familiar with the technique, you can leave that part as-is.

@enp2s0
Copy link
Contributor Author

enp2s0 commented Jul 25, 2019

OK, QSignalMapper has been removed.

Copy link
Member

@PhysSong PhysSong left a comment

Choose a reason for hiding this comment

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

Everything is good, except unrelated whitespace changes. If your editor keeps doing it, I can manually do them before merging.

@enp2s0
Copy link
Contributor Author

enp2s0 commented Jul 27, 2019

Sorry about that, my editor was misconfigured and I didnt notice until after commit+push. Shouldnt happen again.

@PhysSong
Copy link
Member

I found that src/tracks/InstrumentTrack.cpp and src/tracks/SampleTrack.cpp also uses deprecated QSignalMapper. You can use an override of QMenu::addAction which supports lambda functions, but it requires Qt 5.6. To support old Qt(our Windows builds on Travis CI still uses 5.4), you need to create a action without a signal-slot connection and then connect to a lambda function.

@lukas-w
Copy link
Member

lukas-w commented Aug 27, 2019

Superseded by #5156.

@lukas-w lukas-w closed this Aug 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants