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

BUG: Fix localization and inconsistent state in ctkModalityWidget #1080

Merged
merged 1 commit into from
Apr 12, 2023

Conversation

lassoan
Copy link
Member

@lassoan lassoan commented Apr 12, 2023

There were several issues with the imaging modality selector widget (ctkModalityWidget):

  • when imaging modality abbreviations (CT, MRI, XA, ...) were localized then the widget did not work at all, because the checkbox text was used to identify the checkboxes
  • widget state was stored redundantly in SelectedModalities and VisibleModalities variables and the checkbox properties; and these were often inconsistent

Fixed by using Qt object names to identify checkboxes; and removing SelectedModalities and VisibleModalities variables (store widget state in the checkbox properties).

@lassoan lassoan requested a review from pieper April 12, 2023 02:36
@lassoan lassoan self-assigned this Apr 12, 2023
@jcfr
Copy link
Member

jcfr commented Apr 12, 2023

Since I plan on finalizing #1005, let's fix the build-qt4 CI.

I target to wrap this up by end of next week, then we will tag CTK and move forward with removing Qt4 support and adding support for Qt6.

@jcfr
Copy link
Member

jcfr commented Apr 12, 2023

@jcfr
Copy link
Member

jcfr commented Apr 12, 2023

re: Qt Testing

Once we are done with Qt4 removal, we should evaluate if the overall of maintaining QtTesting player/translator for both CTK & Slicer is worth it.

@lassoan
Copy link
Member Author

lassoan commented Apr 12, 2023

Not using QSignalBlocker leads to lower-quality code and extra development effort, so this is a little bit annoying, but OK, let's do this one last time.

Once we are done with Qt4 removal, we should evaluate if the overall of maintaining QtTesting player/translator for both CTK & Slicer is worth it.

We are looking into refreshing/improving QtTesting for recording tutorials that can be replayed in different languages (because we cannot use English language screenshots in non-English tutorials). We would like to generate executable Python code instead of XML. Maybe we'll use the recorder (just fix and extend it as needed) and convert the XML to Python; or maybe we'll change it more significantly to create Python code directly.

Should this file be updated ? See https://github.com/commontk/CTK/blob/master/Libs/Widgets/Testing/Cpp/ctkModalityWidgetEventTranslatorPlayerTest1.xml

The test still passes.

@jcfr
Copy link
Member

jcfr commented Apr 12, 2023

QSignalBlocker

So that the cleaned up code uses QSignalBlocker, I suggest ...

#if QT_VERSION >= QT_VERSION_CHECK(5,3,0)
  QSignalBlocker blocker(this->AnyCheckBox);
#else
  bool wasBlocking = this->AnyCheckBox->blockSignals(false);
#endif

...

#if QT_VERSION < QT_VERSION_CHECK(5,3,0)
    this->blockSignals(wasBlocking)
    return;
#endif

There were several issues with the imaging modality selector widget (ctkModalityWidget):
- when imaging modality abbreviations (CT, MRI, XA, ...) were localized then the widget did not work at all, because the checkbox text was used to identify the checkboxes
- widget state was stored redundantly in SelectedModalities and VisibleModalities variables and the checkbox properties; and these were often inconsistent

Fixed by using Qt object names to identify checkboxes; and removing SelectedModalities and VisibleModalities variables (store widget state in the checkbox properties).
@lassoan lassoan force-pushed the fix-ctkModalityWidget branch from 6d4eb07 to d4977ad Compare April 12, 2023 04:01
@lassoan lassoan requested review from jcfr and removed request for jcfr and pieper April 12, 2023 04:02
@lassoan lassoan merged commit cd194ff into commontk:master Apr 12, 2023
@lassoan lassoan deleted the fix-ctkModalityWidget branch April 12, 2023 12:26
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.

2 participants