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 compilation for Qt6 #1023

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

Conversation

jporcher
Copy link

@jporcher jporcher commented Feb 3, 2022

Few changes to make code compile with Qt6. Did not test the changes as I'm not familiar with the classes modified. However, at least it compiles if one wants to play with it.

Copy link
Member

@lassoan lassoan left a comment

Choose a reason for hiding this comment

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

This is awesome, thanks a lot for your contribution. I've added a couple of comments, it would be great if you could update your pull request based on them (but if you don't have time for it then we'll eventually get to it when CTK will have Qt6 support officially).

Libs/Core/ctkUtils.cpp Outdated Show resolved Hide resolved
Libs/Core/ctkUtils.cpp Outdated Show resolved Hide resolved
Libs/Core/ctkUtils.cpp Outdated Show resolved Hide resolved
Libs/Widgets/ctkDoubleSpinBox.cpp Show resolved Hide resolved
Libs/Widgets/ctkDoubleSpinBox.cpp Outdated Show resolved Hide resolved
Libs/Widgets/ctkRangeSlider.cpp Outdated Show resolved Hide resolved
Libs/Widgets/ctkRangeSlider.cpp Outdated Show resolved Hide resolved
@jporcher
Copy link
Author

jporcher commented May 5, 2022

@lassoan Thank you for your comments, I updated the code and hopefully the pull request (I'm not a Git expert). Please let me know if it looks OK now!

@jporcher
Copy link
Author

jporcher commented May 5, 2022

@lassoan Let me know if there's any action expected from me to have this pull requested be merged to your master.

@lassoan
Copy link
Member

lassoan commented May 25, 2022

How did you build CTK with Qt6? I'm getting errors that Qt cannot be found when I'm trying to build with Qt6.

What did you set your CTK_QT_VERSION CMake variableto: 5 or 6?
Did you specify Qt5_DIR or Qt6_DIR variable?

There are lots of hardcoded references to Qt5 in the CMake files. They would need to be changed similarly to how it was done for VTK: Kitware/VTK@190059d

@jporcher
Copy link
Author

@lassoan Sorry, but I'm not using the original CMakeLists.txt, I've written my own.

In mine, I simply changed:
QT5_WRAP_CPP to QT_WRAP_CPP
QT5_WRAP_UI to QT_WRAP_UI
QT5_ADD_RESOURCES to QT_ADD_RESOURCES

Also had to set set(CMAKE_CXX_STANDARD 17)

And, as you mentioned, many variables had to be changed, if you set CTK_QT_VERSION to 6 then Qt5_DIR must be set to Qt${CTK_QT_VERSION}_DIR, find_package(Qt5Core) to find_package(Qt${CTK_QT_VERSION}Core)...etc

@lassoan
Copy link
Member

lassoan commented May 30, 2022

Please commit all your changes that were necessary to build with Qt6. We would then review those changes and either merge them or fix them up/generalize. Thank you very much!

@jporcher
Copy link
Author

As I mentioned, I did not use your CMakeLIsts.txt to build the library, so all the "changes" I did are commited. I have no more contribution available...

I gave a quick look at your code, there are many places where you do test CTK_QT_VERSION and thay all have to be reworked.

First main change is to lookup for Qt6 instead of Qt5, so in ctkMacroSetupQt.cmake:

if(CTK_QT_VERSION VERSION_GREATER "4")
...
else()
...
endif()

has to be changed to something like:

if(CTK_QT_VERSION VERSION_GREATER "5")
    cmake_minimum_required(VERSION 3.16)
    set(CMAKE_CXX_STANDARD 17)
    set(CMAKE_CXX_STANDARD_REQUIRED ON)
    find_package(Qt6)
    set(CTK_QT6_COMPONENTS Core Xml Concurrent Sql Test Multimedia)
    if(CTK_ENABLE_Widgets OR CTK_LIB_Widgets OR CTK_LIB_CommandLineModules/Frontend/QtGui OR CTK_BUILD_ALL OR CTK_BUILD_ALL_LIBRARIES)
      list(APPEND CTK_QT6_COMPONENTS Widgets OpenGL UiTools)
    endif()
    if(CTK_LIB_CommandLineModules/Frontend/QtWebKit OR CTK_BUILD_ALL OR CTK_BUILD_ALL_LIBRARIES)
      if(TARGET Qt6::WebKitWidgets)
        list(APPEND CTK_QT6_COMPONENTS WebKitWidgets)
      else()
        list(APPEND CTK_QT6_COMPONENTS WebEngineWidgets)
      endif()
    endif()
    if(CTK_LIB_XNAT/Core OR CTK_BUILD_ALL OR CTK_BUILD_ALL_LIBRARIES)
      list(APPEND CTK_QT6_COMPONENTS Script)
    endif()
    find_package(Qt6 COMPONENTS ${CTK_QT6_COMPONENTS} REQUIRED)

    mark_as_superbuild(Qt6_DIR) # Qt 6

    # XXX Backward compatible way
    if(DEFINED CMAKE_PREFIX_PATH)
      mark_as_superbuild(CMAKE_PREFIX_PATH) # Qt 6
    endif()
elseif(CTK_QT_VERSION VERSION_GREATER "4")
...
else()
...
endif()

Then all other places has to be reworked as well, like, for instance, in Libs\Visualization\VTK\Core\Testing\Cpp\CMakeLists.txt:

if(CTK_QT_VERSION VERSION_GREATER "4")
  QT5_WRAP_CPP(KIT_HELPER_SRCS ctkVTKObjectTestHelper.h)
  include_directories(${Qt5Widgets_INCLUDE_DIRS})
else()
  QT4_WRAP_CPP(KIT_HELPER_SRCS ctkVTKObjectTestHelper.h)
endif()

should be changed to:

if(CTK_QT_VERSION VERSION_GREATER "5")
  QT6_WRAP_CPP(KIT_HELPER_SRCS ctkVTKObjectTestHelper.h)
  include_directories(${Qt6Widgets_INCLUDE_DIRS})
elseif(CTK_QT_VERSION VERSION_GREATER "4")
  QT5_WRAP_CPP(KIT_HELPER_SRCS ctkVTKObjectTestHelper.h)
  include_directories(${Qt5Widgets_INCLUDE_DIRS})
else()
  QT4_WRAP_CPP(KIT_HELPER_SRCS ctkVTKObjectTestHelper.h)
endif()

etc....

You may also decide to use versionless commands, and then, you simply need to remove the Qt version suffix and use ${CTK_QT_VERSION} when needed. This would be more elegant and easy to maintain:

if(CTK_QT_VERSION VERSION_GREATER "4")
  QT_WRAP_CPP(KIT_HELPER_SRCS ctkVTKObjectTestHelper.h)
  include_directories(${Qt${CTK_QT_VERSION}Widgets_INCLUDE_DIRS})
else()
  QT4_WRAP_CPP(KIT_HELPER_SRCS ctkVTKObjectTestHelper.h)
endif()

@jamesobutler
Copy link
Contributor

@jporcher How did you build CTK without using the CMakeLists.txt provided at https://github.com/jporcher/CTK/blob/qt6_support/CMakeLists.txt? Can you commit the CMakeLists.txt that you used for your own project?

@jporcher
Copy link
Author

@jamesobutler That would be difficult as this CMakeLists.txt has dependencies on other files of my project, but I can paste you some part of it:

set (LIBNAME qtcommontk)

# This project compiles the external library qwt
PROJECT(${LIBNAME})

SET( CMAKE_DEBUG_POSTFIX ${SDE_DEBUG_FLAG} )

file( GLOB C_files
	Libs/Widgets/ctkBasePopupWidget.cpp
	Libs/Widgets/ctkDoubleRangeSlider.cpp
	Libs/Widgets/ctkDoubleRangeSliderEventPlayer.cpp
	Libs/Widgets/ctkDoubleRangeSliderEventTranslator.cpp
	Libs/Widgets/ctkDoubleSlider.cpp
	Libs/Widgets/ctkDoubleSpinBox.cpp
	Libs/Widgets/ctkPopupWidget.cpp
	Libs/Widgets/ctkRangeSlider.cpp
	Libs/Widgets/ctkRangeWidget.cpp
	Libs/Widgets/ctkSliderWidget.cpp
	Libs/Core/ctkUtils.cpp
	Libs/Core/ctkValueProxy.cpp
	Libs/Widgets/ctkWidgetsUtils.cpp
	../QtTesting-master/pqWidgetEventPlayer.cxx
	../QtTesting-master/pqWidgetEventTranslator.cxx )

file( GLOB H_files
	Libs/Widgets/ctkBasePopupWidget.h
	Libs/Widgets/ctkDoubleRangeSlider.h
	Libs/Widgets/ctkDoubleRangeSliderEventPlayer.h
	Libs/Widgets/ctkDoubleRangeSliderEventTranslator.h
	Libs/Widgets/ctkDoubleSlider.h
	Libs/Widgets/ctkDoubleSpinBox.h
	Libs/Widgets/ctkPopupWidget.h
	Libs/Widgets/ctkRangeSlider.h
	Libs/Widgets/ctkRangeWidget.h
	Libs/Widgets/ctkSliderWidget.h
	Libs/Core/ctkUtils.h
	Libs/Core/ctkValueProxy.h
	Libs/Widgets/ctkWidgetsUtils.h
	Libs/Widgets/ctkBasePopupWidget_p.h
	Libs/Widgets/ctkDoubleSpinBox_p.h
	Libs/Widgets/ctkPopupWidget_p.h
	../QtTesting-master/pqWidgetEventPlayer.h
	../QtTesting-master/pqWidgetEventTranslator.h )

file( GLOB UI_files Libs/Widgets/Resources/UI/ctkRangeWidget.ui Libs/Widgets/Resources/UI/ctkSliderWidget.ui )
file( GLOB QRC_files Libs/Widgets/Resources/ctkWidgets.qrc )

# We need to include all directories where the headers are
include_directories( Libs/Widgets )
include_directories( Libs/Core )
include_directories( ../QtTesting-master )
include_directories( sde )

SET(SDE3P_QTSDEVER 6)
SET(SDE3P_QTDIR "${CMAKE_CURRENT_LIST_DIR}/../../qt/6.2.2")
MESSAGE( "Set QTDIR=${SDE3P_QTDIR}" )

find_package(Qt${SDE3P_QTSDEVER}Core)
find_package(Qt${SDE3P_QTSDEVER}Gui)
find_package(Qt${SDE3P_QTSDEVER}Widgets)

set( pre_qt_wrap_sources ${C_files} )

QT_WRAP_CPP( MOC_FILES ${H_files})
list( APPEND C_files ${MOC_FILES} )

QT_WRAP_UI( UIC_SRCS ${UI_files} )
list( APPEND C_files ${UIC_SRCS} )

QT_ADD_RESOURCES( QRC_SRCS ${QRC_files} )
list( APPEND C_files ${QRC_SRCS} )

# C++17 is required by Qt6
add_definitions( "/Zc:__cplusplus" )
set(CMAKE_CXX_STANDARD 17)

# Library will depend on all source files
ADD_LIBRARY( ${LIBNAME} SHARED ${C_files} ${H_files} ${SRCS_CXX})

include_directories( ${QT_INCLUDE_DIR} )
include_directories( ${QT_INCLUDE_DIR}/QtCore )
include_directories( ${QT_INCLUDE_DIR}/QtGui )
include_directories( ${QT_INCLUDE_DIR}/QtWidgets )

@EricAtORS EricAtORS mentioned this pull request Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants