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

Drop Qt4 support #2716

Merged
merged 1 commit into from
Dec 15, 2018
Merged

Drop Qt4 support #2716

merged 1 commit into from
Dec 15, 2018

Conversation

SunBlack
Copy link
Contributor

@SunBlack SunBlack commented Dec 15, 2018

Fixes #2618

I didn't modernized any C++ code in this PR to keep PR small ;)

Copy link
Member

@taketwo taketwo left a comment

Choose a reason for hiding this comment

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

Thanks for pushing this on. Two points from my side:

  1. The (now removed) pcl_find_qt5.cmake script included the following Qt modules:
  • Qt5Core
  • Qt5Gui
  • Qt5Widgets
  • Qt5Concurrent
  • Qt5OpenGL
    With your change we only search for OpenGL and Widgets components. Could you please comment on the rest?
  1. Style: should we maybe use lowercase names for functions/macros, e.g. qt5_wrap_ui?

@SunBlack
Copy link
Contributor Author

Qt5Concurrent didn't seems to be used and Qt5Widgets has as dependency Qt5Gui (and Qt5Core) and QtGui has dependency to Qt5Core. Find script of Qt5 respect this, so we don't need to search explicitly for all dependencies. Furthermore: Imported targets can define dependencies, so if we call target_link_libraries with Qt5::Widgets, Qt5::Gui and Qt5::Core are implicit linked, too.

I will remove qt5_wrap_ui and other qt5_* calls later. Instead I will switch to CMAKE_AUTOUIC, ... This is just not part of this MR, because I have then to move ui files to another location (they have to be in same directory like cpp file) and change include paths from e.g. #include <pcl/.../ui_foo.h> to #include "ui_foo.h". See here

@taketwo
Copy link
Member

taketwo commented Dec 15, 2018

Thanks for clarifications!

This is just not part of this MR, because I have then to move ui files to another location

👍 makes reviewing easier

@taketwo taketwo merged commit 20dca9d into PointCloudLibrary:master Dec 15, 2018
@taketwo taketwo changed the title Drop support of Qt4, because support of Qt4 ended end of 2015 Drop support for Qt4 Dec 15, 2018
@SunBlack SunBlack deleted the drop_qt4 branch December 15, 2018 20:00
@SunBlack
Copy link
Contributor Author

SunBlack commented Dec 16, 2018

Just saw: We have to adjust Azure pipelines. PCL_QT_VERSION is not necessary anymore and I don't know why, but build servers don't find Qt5 (works on my machine, so no idea why it isn't working on Azure)

//Edit: Fixed in #2719

@taketwo taketwo changed the title Drop support for Qt4 Drop Qt4 support Jan 18, 2020
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.

Drop support of QT4
2 participants