-
Notifications
You must be signed in to change notification settings - Fork 514
COMP: Fix Qt >= 6.7 QCheckBox stateChanged -> checkStateChanged #1322
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
COMP: Fix Qt >= 6.7 QCheckBox stateChanged -> checkStateChanged #1322
Conversation
|
Thanks for the contribution 🙏 Since you are working on this ... do you think you could apply a similar change to all use of the deprecated signal in the codebase? |
I thought I had caught all usages of |
I suggest to set the connection in the cpp file instead of the ui file. This should allow to conditionally connect. |
|
EDIT: changed Qt version check to 6.7, since this is the actual version that |
893bc25 to
d08eabd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR updates the CTK codebase to use Qt's new checkStateChanged signal instead of the deprecated stateChanged signal for QCheckBox, with conditional compilation based on Qt version 6.7 or later. The changes also migrate to Qt's new connection syntax (function pointers) for better compile-time type checking.
- Adds Qt version checks (
QT_VERSION >= QT_VERSION_CHECK(6, 7, 0)) to conditionally use the appropriate signal - Migrates from old-style SIGNAL/SLOT macros to new-style function pointer syntax for Qt 6.7+
- Removes UI file connection in favor of programmatic connection to support version-dependent logic
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| Libs/Widgets/ctkModalityWidget.cpp | Adds conditional compilation to use checkStateChanged signal with new connection syntax for Qt 6.7+, while maintaining backward compatibility with stateChanged for older versions |
| Libs/DICOM/Widgets/ctkDICOMServerNodeWidget2.cpp | Updates storage checkbox connection to use checkStateChanged signal with new connection syntax for Qt 6.7+, preserving old-style connection for older versions |
| Libs/DICOM/Widgets/ctkDICOMAppWidget.cpp | Adds version-conditional connection for auto-play checkbox, using checkStateChanged for Qt 6.7+ and stateChanged for older versions, both with new connection syntax |
| Libs/DICOM/Widgets/Resources/UI/ctkDICOMAppWidget.ui | Removes the auto-play checkbox connection from UI file to allow programmatic connection with version-specific signal selection |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| connect(d->AutoPlayCheckbox, &QCheckBox::checkStateChanged, this, &ctkDICOMAppWidget::onAutoPlayCheckboxStateChanged); | ||
| #else | ||
| connect(d->AutoPlayCheckbox, &QCheckBox::stateChanged, this, &ctkDICOMAppWidget::onAutoPlayCheckboxStateChanged); |
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent indentation style. The code within the #if/#else blocks uses extra indentation (4 spaces), which is inconsistent with the rest of the file and with the style used in the other modified files (ctkModalityWidget.cpp and ctkDICOMServerNodeWidget2.cpp) where the code is not indented relative to the preprocessor directives. For consistency, remove the extra indentation.
| connect(d->AutoPlayCheckbox, &QCheckBox::checkStateChanged, this, &ctkDICOMAppWidget::onAutoPlayCheckboxStateChanged); | |
| #else | |
| connect(d->AutoPlayCheckbox, &QCheckBox::stateChanged, this, &ctkDICOMAppWidget::onAutoPlayCheckboxStateChanged); | |
| connect(d->AutoPlayCheckbox, &QCheckBox::checkStateChanged, this, &ctkDICOMAppWidget::onAutoPlayCheckboxStateChanged); | |
| #else | |
| connect(d->AutoPlayCheckbox, &QCheckBox::stateChanged, this, &ctkDICOMAppWidget::onAutoPlayCheckboxStateChanged); |
| this->setupUi(q); | ||
|
|
||
| this->AnyCheckBox->setTristate(true); | ||
| #if QT_VERSION >= QT_VERSION_CHECK(6, 7, 0) |
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR description states "Starting with Qt 6.9, the stateChanged signal in QCheckBox has been deprecated," but the version check here uses QT_VERSION_CHECK(6, 7, 0). Please verify the correct Qt version where stateChanged was deprecated and checkStateChanged was introduced, and ensure the version check is accurate. If Qt 6.7 is correct, the PR description should be updated to reflect this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The QCheckBox::checkState exists since 6.7, the 6.9 in the original description is incorrect and was updated.
|
|
||
| q->readSettings(); | ||
|
|
||
| #if QT_VERSION >= QT_VERSION_CHECK(6, 7, 0) |
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR description states "Starting with Qt 6.9, the stateChanged signal in QCheckBox has been deprecated," but the version check here uses QT_VERSION_CHECK(6, 7, 0). Please verify the correct Qt version where stateChanged was deprecated and checkStateChanged was introduced, and ensure the version check is accurate. If Qt 6.7 is correct, the PR description should be updated to reflect this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The QCheckBox::checkState exists since 6.7, the 6.9 in the original description is incorrect and was updated.
|
|
||
| d->ThumbnailsWidget->setThumbnailSize( | ||
| QSize(d->ThumbnailWidthSlider->value(), d->ThumbnailWidthSlider->value())); | ||
|
|
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR description states "Starting with Qt 6.9, the stateChanged signal in QCheckBox has been deprecated," but the version check here uses QT_VERSION_CHECK(6,7,0). Please verify the correct Qt version where stateChanged was deprecated and checkStateChanged was introduced, and ensure the version check is accurate. If Qt 6.7 is correct, the PR description should be updated to reflect this.
| // Starting with Qt 6.7, the stateChanged signal in QCheckBox is deprecated in favor of checkStateChanged. |
| d->ThumbnailsWidget->setThumbnailSize( | ||
| QSize(d->ThumbnailWidthSlider->value(), d->ThumbnailWidthSlider->value())); | ||
|
|
||
| #if QT_VERSION >= QT_VERSION_CHECK(6,7,0) |
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent spacing in QT_VERSION_CHECK macro. This file uses QT_VERSION_CHECK(6,7,0) without spaces after commas, while the other modified files (ctkModalityWidget.cpp and ctkDICOMServerNodeWidget2.cpp) use QT_VERSION_CHECK(6, 7, 0) with spaces. For consistency, use the same spacing convention across all modified files.
| #if QT_VERSION >= QT_VERSION_CHECK(6,7,0) | |
| #if QT_VERSION >= QT_VERSION_CHECK(6, 7, 0) |
Use the recommended checkStateChanged signal instead of the somewhat ambiguous stateChanged signal in Qt versions where the new signal is available.
c96fc1e to
3ba6daf
Compare
lassoan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| d->ThumbnailsWidget->setThumbnailSize( | ||
| QSize(d->ThumbnailWidthSlider->value(), d->ThumbnailWidthSlider->value())); | ||
|
|
||
| #if QT_VERSION >= QT_VERSION_CHECK(6,7,0) |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent spacing in version check. Use QT_VERSION_CHECK(6, 7, 0) with spaces after commas to match the style used in the other files (ctkModalityWidget.cpp and ctkDICOMServerNodeWidget2.cpp).
| #if QT_VERSION >= QT_VERSION_CHECK(6,7,0) | |
| #if QT_VERSION >= QT_VERSION_CHECK(6, 7, 0) |
| #if QT_VERSION >= QT_VERSION_CHECK(6,7,0) | ||
| connect(d->AutoPlayCheckbox, &QCheckBox::checkStateChanged, this, &ctkDICOMAppWidget::onAutoPlayCheckboxStateChanged); | ||
| #else | ||
| connect(d->AutoPlayCheckbox, &QCheckBox::stateChanged, this, &ctkDICOMAppWidget::onAutoPlayCheckboxStateChanged); |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent indentation. The connect statement should use 2 spaces for indentation to match the project style, not 4 spaces.
| connect(d->AutoPlayCheckbox, &QCheckBox::stateChanged, this, &ctkDICOMAppWidget::onAutoPlayCheckboxStateChanged); | |
| connect(d->AutoPlayCheckbox, &QCheckBox::stateChanged, this, &ctkDICOMAppWidget::onAutoPlayCheckboxStateChanged); |
Starting with Qt 6.7 (corrected, was incorrect 6.9), the
stateChangedsignal inQCheckBoxhas been deprecated in favor of thecheckStateChangedsignal; this pull request applies this change also in CTK code (and updates it to the new connection syntax, which can be checked during compile time).