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 Qt4/C++98 build #1005

Merged
merged 28 commits into from
Jul 13, 2023
Merged

Fix Qt4/C++98 build #1005

merged 28 commits into from
Jul 13, 2023

Conversation

jcfr
Copy link
Member

@jcfr jcfr commented Nov 4, 2021

Notes regarding Qt4 build:

  • ctkDICOMQueryRetrieveWidget class will not be available Update: The class is now available when building against Qt4
  • Setting of field format using Json attribute in ctkDICOMTableView is not supported
  • Setting PBR mode in ctkVTKSurfaceMaterialPropertyWidget is not available when building against VTK8. Attempting to set the Metallic or Roughness property will display a warning.

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.

Thanks for all this work. It will be great to have a clean status of Qt4/Cxx98 support before we get rid of them for good. I've added a number of trivial comments that would be nice to address.

CMakeLists.txt Outdated Show resolved Hide resolved
Libs/DICOM/Widgets/CMakeLists.txt Outdated Show resolved Hide resolved
Libs/DICOM/Widgets/Plugins/CMakeLists.txt Outdated Show resolved Hide resolved
Libs/DICOM/Widgets/Plugins/ctkDICOMWidgetsPlugins.h Outdated Show resolved Hide resolved
Libs/DICOM/Widgets/ctkDICOMBrowser.cpp Outdated Show resolved Hide resolved
Libs/DICOM/Widgets/ctkDICOMBrowser.cpp Outdated Show resolved Hide resolved
Libs/DICOM/Widgets/ctkDICOMBrowser.cpp Outdated Show resolved Hide resolved
Libs/DICOM/Widgets/ctkDICOMBrowser.cpp Outdated Show resolved Hide resolved
Libs/DICOM/Widgets/ctkDICOMBrowser.cpp Outdated Show resolved Hide resolved
@jcfr jcfr mentioned this pull request Nov 22, 2022
1 task
@jamesobutler
Copy link
Contributor

@jcfr Do you think you would be able to finalize the last changes based on the comments here? Then per your comment #947 (comment), we can proceed with removing Qt4 support (#1006) and proceed supporting Qt5 and Qt6.

@jamesobutler
Copy link
Contributor

CircleCI checks passed. The CommitCheck status check reminded me we could probably change to the GitHub action like we use in the Slicer repo https://github.com/Slicer/Slicer/blob/main/.github/workflows/commit-message.yml

@jcfr
Copy link
Member Author

jcfr commented Mar 4, 2023

Re: commit check
Switching to github workflow makes sense

Re: status
I still need to finalize the 3 commits by Re working the history and testing... this will be ready next week

@lassoan
Copy link
Member

lassoan commented Apr 5, 2023

I assume Qt4 support is no longer needed. I'm closing this to clean up the pull request list.

@lassoan lassoan closed this Apr 5, 2023
@jamesobutler
Copy link
Contributor

@lassoan It appeared @jcfr had plans (see #947 (comment)) to make Qt4 work one last time before it was to be removed in #1006. Then the goal was to enable Qt 6 support.

@lassoan
Copy link
Member

lassoan commented Apr 5, 2023

Thanks for the note. I closed the pull request because I saw was open since November 2021. I did not notice that the last update was relatively recent. The branch is still there, so @jcfr can still reopen the pull request, but it is hard to imagine a scenario where investing into better Qt4 support makes sense (especially if it delays the transition to Qt6).

@jcfr
Copy link
Member Author

jcfr commented Apr 13, 2023

The branch has been updated and is in its final round of QA.

Currently building locally CTK with all options enabled against Qt4/VTK8 and Qt5/VTK9, I will follow up once this is completed.

jcfr added 5 commits July 11, 2023 02:05
…ates

This commit fixes build errors like the following:

  /path/to/CTK/Libs/DICOM/Core/ctkDICOMQuery.cpp:243:28: error: ‘>>’ should be ‘> >’ within a nested template argument list
   QList<QPair<QString,QString>> ctkDICOMQuery::studyAndSeriesInstanceUIDQueried()const
                              ^
jcfr added 19 commits July 11, 2023 02:06
…nObject

This commit exclude the parsing of "formatForField" and instead reported
the warning indicating that the specified format string failed to be
decoded from json.
This commit updates the implementation to account for changes introduced
in PR commontk#1002 (ENH: Add API to select items in the DICOM browser)
Conditionally select ITK version based on selected CMAKE_CXX_STANDARD.
If CMAKE_CXX_STANDARD is C++98, set revision tag to v4.13.3 as it is
the latest version supporting it.
…= 8.90

Adapted from CTK@48e4a3f38 (COMP: Add support for building against VTK >= 8.90)
This commit re-introduces the QVTKWidget implementation inadvertently
removed in 48e4a3f (COMP: Add support for building against VTK >= 8.90).
This commit fixes a regression introduced in 58f4038 (ENH: Add access
to render window in ctkVTKChartView)
…VTK8/Qt4

This commit fixes a regression introduced in 0a1f9c1 (ENH: Add
qImageToVTKImageData to ctkVTKWidgetsUtils) by reporting a warning
if a 4-component image is being converted.
… VTK8/Qt4

This commit fixes a regression introduced in 7b48c48 (ENH: Add support
for PBR material properties in material property widgets) by reporting
a warning if Metallic or Roughness properties are set.
For example:

[...]
-- CTK_LIB_Core_WITH_BFD_SHARED is OFF
-- CTK_LIB_Core_WITH_BFD_STATIC is OFF
-- CTK_LIB_Visualization/VTK/Widgets_USE_TRANSFER_FUNCTION_CHARTS is ON
[...]
Since C++98 doesn't support the auto keyword or lambdas, the following
workarounds are implemented:

1. The type of the currentStudyAndSeriesUIDPair variable is explicitly
   specified as StudyAndSeriesInstanceUIDPairList::iterator.

2. The macro Q_FOREACH macro is used instead of the "for-each loop".

3. A struct called FindBySeriesUID is introduced to be used a functor
   representing the predicate used in std::find_if.

4. Ensure compatibility with across C++ standard using QList, QPair
   and qMakePair
Fixes the following error:

  /path/to/CTK/Libs/DICOM/Core/ctkDICOMDatabase.h: In member function ‘void ctkDICOMBrowser::onRepairAction()’:
  /path/to/CTK/Libs/DICOM/Core/ctkDICOMDatabase.h:425:8: error: ‘void ctkDICOMDatabase::databaseChanged()’ is protected
     void databaseChanged();
          ^
  /path/to/CTK/Libs/DICOM/Widgets/ctkDICOMBrowser.cpp:928:37: error: within this context
     d->DICOMDatabase->databaseChanged();
                                       ^
…th QChar

This commit fixes errors like the following introduced
in 08461b3 (ENH: Improve Python autocomplete list ordering)

Error:

  /path/to/CTK/Libs/Scripting/Python/Widgets/ctkPythonConsole.cpp:379:18: error: conversion from ‘const char [2]’ to ‘QChar’ is ambiguous
       if (s1[0] != "_" && s2[0] == "_")
                    ^
@jcfr
Copy link
Member Author

jcfr commented Jul 12, 2023

I plan on merging this tomorrow (July 13th), then I will proceed with:

  • creating tag
  • updating the README to explain that Qt4 is being removed and that active development is continuing for Qt5 and Qt6.

This means that support for C++98, Qt4 and associated compatibility macros will be removed.

Cc: @jamesobutler @finetjul @lassoan @pieper

@jcfr jcfr merged commit 72d830f into commontk:master Jul 13, 2023
3 checks passed
@jcfr jcfr deleted the fix-qt4-build branch July 13, 2023 17:56
@lassoan
Copy link
Member

lassoan commented Jul 13, 2023

Awesome, thank you, it'll be great that we won't need to worry about qt4 compatibility anymore.

@jcfr
Copy link
Member Author

jcfr commented Jul 13, 2023

To follow-up, I just updated the README1

Next:

  • Move Release Process from Making_a_Release to https://github.com/commontk/CTK/wiki
  • Create release + update README indicating Qt4 and Python 2.7 supports are being removed.
  • Setup schedule for (automatically?) releasing on a quarterly basis
  • Migrate others pages to the wiki
  • Create a resources release to organize PDFs, slides, etc .. currently on the wiki

Footnotes

  1. https://github.com/commontk/CTK#readme

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