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

Add Visual DICOM Browser | ⚠️ Authorship of integration commit changed #1165

Merged
merged 73 commits into from
Jan 19, 2024

Conversation

jcfr
Copy link
Member

@jcfr jcfr commented Jan 12, 2024

⚠️ Changes originally introduced in commit 8a0717d via this pull request have been removed from the main branch using force push. ⚠️

➡️ These changes have been replaced by the commit 88ff72b.

Rationale:

After performing a Squash & Merge operation in association with this pull request (#1165(, the primary authorship of the commit was inadvertently attributed to @jcfr instead of @Punzo, who contributed significantly to the changes.

To rectify this, a force push was applied to explicitly set @Punzo as the main author of the commit, reflecting their substantial contribution to the work.


This pull request introduces the ctkDICOMVisualBrowserWidget, which improves the threaded execution of various operations:

  • Filtering and navigation with thumbnails of local database and servers results
  • Import from file system to local database
  • Query/Retrieve from servers (DIMSE C-GET/C-MOVE )
  • Storage listener
  • Send (emits only a signal for the moment, requires external implementation)
  • Remove (only from local database, not from server)
  • Metadata exploration

Key Components:

  • Task pool manager supporting separate threads with a priority queue.
  • Core classes:
    • ctkAbstractJob
    • ctkAbstractScheduler
    • ctkAbstractWorker
  • DICOM/Core classes:
    • ctkDICOMEcho
    • ctkDICOMInserter
    • ctkDICOMInserterJob
    • ctkDICOMInserterWorker
    • ctkDICOMJob
    • ctkDICOMJobResponseSet
    • ctkDICOMQueryJob
    • ctkDICOMQueryWorker
    • ctkDICOMRetrieveJob
    • ctkDICOMRetrieveWorker
    • ctkDICOMScheduler
    • ctkDICOMServer
    • ctkDICOMStorageListener
    • ctkDICOMStorageListenerJob
    • ctkDICOMStorageListenerWorker
  • DICOM/Widgetclasses:
    • ctkDICOMPatientItemWidget
    • ctkDICOMSeriesItemWidget
    • ctkDICOMServerNodeWidget2
    • ctkDICOMStudyItemWidget

Related

Testing

Additionally, a CTK application named ctkDICOMVisualBrowser has been included for testing purposes (requires enabling CMake options examples and DICOM).

Testing can also be performed by running the visual DICOM browser from the Slicer Python console:

import os

server = ctk.ctkDICOMServer()
server.connectionName = "ConnectionName"
server.callingAETitle = "callingAETitle"
server.calledAETitle = "calledAETitle"
server.host = "host"
server.port = 11112
server.retrieveProtocol = ctk.ctkDICOMServer.CGET

DICOMDatabase = qt.QSettings().value(slicer.dicomDatabaseDirectorySettingsKey)
if not os.path.isdir(DICOMDatabase):
  os.makedirs(DICOMDatabase)  

browser = ctk.ctkDICOMVisualBrowserWidget()
browser.addServer(server)
browser.setDatabaseDirectory(DICOMDatabase)
browser.filteringPatientID = 1
browser.resize(slicer.util.mainWindow().size)
browser.show()
browser.onQueryPatient()

Similarly, here is how to test the scheduler from the Python console (e.g. query and retrieve):

server = ctk.ctkDICOMServer()
server.connectionName = "ConnectionName"
server.callingAETitle = "callingAETitle"
server.calledAETitle = "calledAETitle"
server.host = "host"
server.port = 11112
server.retrieveProtocol = ctk.ctkDICOMServer.CGET

scheduler = ctk.ctkDICOMScheduler()
scheduler.setDicomDatabase(slicer.dicomDatabase)
scheduler.addServer(server)

nDays = 30
endDate = qt.QDate().currentDate()
startDate = endDate.addDays(-nDays);
parameters = {
  "ID": "CT",
  #"Name": "name",
  #"Study": "description",
  #"Series": "description",
  #"Modalities": ["CT", "MR"],
  #"StartDate": startDate.toString("yyyyMMdd"),
  #"EndDate": endDate.toString("yyyyMMdd")
}

scheduler.setFilters(parameters)
# Use one of the following methods: all these methods run background processes!!!!
#scheduler.queryPatients()
#scheduler.queryStudies('patientID')
#scheduler.querySeries('patientID', 'studyInstanceUID')
#scheduler.queryInstances('patientID', 'studyInstanceUID', 'seriesInstanceUID')
#scheduler.retrieveStudy('patientID', 'studyInstanceUID')
#scheduler.retrieveSeries('patientID','studyInstanceUID', 'seriesInstanceUID')
#scheduler.retrieveSOPInstance('patientID', 'studyInstanceUID', 'seriesInstanceUID', 'SOPInstanceUID')
UML diagram Screenshot
VisualDICOMBrowserUML ScreenshotVisualDICOMBrowser
ScreenshotSettings

Video:

VideoVisuaDICOMBrowser.mp4

@jcfr jcfr changed the title Add dicom visual navigation Add Visual DICOM Browser Jan 12, 2024
@jcfr
Copy link
Member Author

jcfr commented Jan 12, 2024

Proposed commit title & message for Squash & Merge:

ENH: Add Visual DICOM Browser

Introduces the `ctkDICOMVisualBrowserWidget`, which improves the threaded
execution of the following operations:

- Filtering and navigation with thumbnails of local database and servers results
- Import from file system to local database
- Query/Retrieve from servers (DIMSE C-GET/C-MOVE )
- Storage listener
- Send (emits only a signal for the moment, requires external implementation)
- Remove (only from local database, not from server)
- Metadata exploration

In addition, the commit introduces the following classes:

* `Core` classes:
  * `ctkAbstractJob`
  * `ctkAbstractScheduler`
  * `ctkAbstractWorker`
* `DICOM/Core` classes:
  * `ctkDICOMEcho`
  * `ctkDICOMInserter`
  * `ctkDICOMInserterJob`
  * `ctkDICOMInserterWorker`
  * `ctkDICOMJob`
  * `ctkDICOMJobResponseSet`
  * `ctkDICOMQueryJob`
  * `ctkDICOMQueryWorker`
  * `ctkDICOMRetrieveJob`
  * `ctkDICOMRetrieveWorker`
  * `ctkDICOMScheduler`
  * `ctkDICOMServer`
  * `ctkDICOMStorageListener`
  * `ctkDICOMStorageListenerJob`
  * `ctkDICOMStorageListenerWorker`
* `DICOM/Widget`classes:
  * `ctkDICOMPatientItemWidget`
  * `ctkDICOMSeriesItemWidget`
  * `ctkDICOMServerNodeWidget2`
  * `ctkDICOMStudyItemWidget`

Co-authored-by: Andras Lasso <[email protected]>
Co-authored-by: Jean-Christophe Fillion-Robin <[email protected]>

cc: @lassoan @Punzo

@jcfr
Copy link
Member Author

jcfr commented Jan 12, 2024

Warnings to fix:

/path/to/CTK/Libs/DICOM/Widgets/ctkDICOMPatientItemWidget.cpp: In member function ‘void ctkDICOMPatientItemWidget::onSeriesItemClicked()’:
/path/to/CTK/Libs/DICOM/Widgets/ctkDICOMPatientItemWidget.cpp:791:35: warning: enum constant in boolean context [-Wint-in-bool-context]
  791 |       (Qt::ControlModifier || Qt::ShiftModifier))
      |                                   ^~~~~~~~~~~~~
/path/to/CTK/Libs/DICOM/Widgets/ctkDICOMPatientItemWidget.cpp:791:35: warning: enum constant in boolean context [-Wint-in-bool-context]
[ 89%] Building CXX object Libs/DICOM/Widgets/CMakeFiles/CTKDICOMWidgets.dir/ctkDICOMThumbnailListWidget.cpp.o
/path/to/CTK/Libs/DICOM/Widgets/ctkDICOMServerNodeWidget2.cpp: In member function ‘virtual bool QCenteredStyledItemDelegate::editorEvent(QEvent*, QAbstractItemModel*, const QStyleOptionViewItem&, const QModelIndex&)’:
/path/to/CTK/Libs/DICOM/Widgets/ctkDICOMServerNodeWidget2.cpp:171:59: warning: passing ‘Qt::CheckState’ chooses ‘int’ over ‘uint’ {aka ‘unsigned int’} [-Wsign-promo]
  171 |     return model->setData(index, state, Qt::CheckStateRole);
      |                                                           ^
/path/to/CTK/Libs/DICOM/Widgets/ctkDICOMServerNodeWidget2.cpp:171:59: warning:   in call to ‘QVariant::QVariant(int)’ [-Wsign-promo]
[ 89%] Building CXX object Libs/DICOM/Widgets/CMakeFiles/CTKDICOMWidgets.dir/ctkDICOMVisualBrowserWidget.cpp.o
/path/to/CTK/Libs/DICOM/Widgets/ctkDICOMServerNodeWidget2.cpp: In member function ‘void ctkDICOMServerNodeWidget2::saveSettings()’:
/path/to/CTK/Libs/DICOM/Widgets/ctkDICOMServerNodeWidget2.cpp:1001:91: warning: passing ‘bool’ chooses ‘int’ over ‘uint’ {aka ‘unsigned int’} [-Wsign-promo]
 1001 |   settings.setValue("DICOM/StorageEnabled", QString::number(this->storageListenerEnabled()));
      |                                                                                           ^
/path/to/CTK/Libs/DICOM/Widgets/ctkDICOMServerNodeWidget2.cpp:1001:91: warning:   in call to ‘static QString QString::number(int, int)’ [-Wsign-promo]



/path/to/CTK/Libs/DICOM/Widgets/ctkDICOMVisualBrowserWidget.cpp: In member function ‘void ctkDICOMVisualBrowserWidgetPrivate::init()’:
/path/to/CTK/Libs/DICOM/Widgets/ctkDICOMVisualBrowserWidget.cpp:373:136: warning: passing ‘ctkDICOMVisualBrowserWidget::ImportDirectoryMode’ chooses ‘int’ over ‘uint’ {aka ‘unsigned int’} [-Wsign-promo]
  373 |   importDirectoryModeComboBox->addItem(ctkDICOMVisualBrowserWidget::tr("Add Link"), ctkDICOMVisualBrowserWidget::ImportDirectoryAddLink);
      |                                                                                                                                        ^
/path/to/CTK/Libs/DICOM/Widgets/ctkDICOMVisualBrowserWidget.cpp:373:136: warning:   in call to ‘QVariant::QVariant(int)’ [-Wsign-promo]
/path/to/CTK/Libs/DICOM/Widgets/ctkDICOMVisualBrowserWidget.cpp:374:129: warning: passing ‘ctkDICOMVisualBrowserWidget::ImportDirectoryMode’ chooses ‘int’ over ‘uint’ {aka ‘unsigned int’} [-Wsign-promo]
  374 |   importDirectoryModeComboBox->addItem(ctkDICOMVisualBrowserWidget::tr("Copy"), ctkDICOMVisualBrowserWidget::ImportDirectoryCopy);
      |                                                                                                                                 ^
/path/to/CTK/Libs/DICOM/Widgets/ctkDICOMVisualBrowserWidget.cpp:374:129: warning:   in call to ‘QVariant::QVariant(int)’ [-Wsign-promo]
/path/to/CTK/Libs/DICOM/Widgets/ctkDICOMVisualBrowserWidget.cpp:385:71: warning: passing ‘ctkDICOMVisualBrowserWidget::ImportDirectoryMode’ chooses ‘int’ over ‘uint’ {aka ‘unsigned int’} [-Wsign-promo]
  385 |         importDirectoryModeComboBox->findData(q->importDirectoryMode()));
      |                                                                       ^
/path/to/CTK/Libs/DICOM/Widgets/ctkDICOMVisualBrowserWidget.cpp:385:71: warning:   in call to ‘QVariant::QVariant(int)’ [-Wsign-promo]
[ 89%] Building CXX object Libs/DICOM/Widgets/CMakeFiles/CTKDICOMWidgets.dir/moc_ctkDICOMPatientItemWidget.cpp.o
/path/to/CTK/Libs/DICOM/Widgets/ctkDICOMVisualBrowserWidget.cpp: In member function ‘void ctkDICOMVisualBrowserWidget::setImportDirectoryMode(ctkDICOMVisualBrowserWidget::ImportDirectoryMode)’:
/path/to/CTK/Libs/DICOM/Widgets/ctkDICOMVisualBrowserWidget.cpp:1950:52: warning: passing ‘ctkDICOMVisualBrowserWidget::ImportDirectoryMode’ chooses ‘int’ over ‘uint’ {aka ‘unsigned int’} [-Wsign-promo]
 1950 |   comboBox->setCurrentIndex(comboBox->findData(mode));
      |                                                    ^
/path/to/CTK/Libs/DICOM/Widgets/ctkDICOMVisualBrowserWidget.cpp:1950:52: warning:   in call to ‘QVariant::QVariant(int)’ [-Wsign-promo]
/path/to/CTK/Libs/DICOM/Widgets/ctkDICOMVisualBrowserWidget.cpp: In member function ‘void ctkDICOMVisualBrowserWidget::setDatabaseDirectory(const QString&)’:
/path/to/CTK/Libs/DICOM/Widgets/ctkDICOMVisualBrowserWidget.cpp:1995:27: warning: catching polymorphic type ‘class std::exception’ by value [-Wcatch-value=]
 1995 |     catch (std::exception e)
      |                           ^
[ 92%] Building CXX object Libs/DICOM/Widgets/CMakeFiles/CTKDICOMWidgets.dir/moc_ctkDICOMSeriesItemWidget.cpp.o
/path/to/CTK/Libs/DICOM/Widgets/ctkDICOMVisualBrowserWidget.cpp: In member function ‘void ctkDICOMVisualBrowserWidget::exportSelectedItems(ctkDICOMModel::IndexType, QList<QWidget*>)’:
/path/to/CTK/Libs/DICOM/Widgets/ctkDICOMVisualBrowserWidget.cpp:3034:45: warning: ‘QFileDialog::DirectoryOnly’ is deprecated: Use setOption(ShowDirsOnly, true) instead [-Wdeprecated-declarations]
 3034 |   directoryDialog->setFileMode(QFileDialog::DirectoryOnly);
      |                                             ^~~~~~~~~~~~~
In file included from /home/jcfr/Support/Qt/5.15.2/gcc_64/include/QtWidgets/QFileDialog:1,
                 from /path/to/CTK/Libs/Widgets/ctkDirectoryButton.h:26,
                 from /path/to/CTK/Libs/DICOM/Widgets/ctkDICOMVisualBrowserWidget.cpp:39:
/home/jcfr/Support/Qt/5.15.2/gcc_64/include/QtWidgets/qfiledialog.h:84:21: note: declared here
   84 |                     DirectoryOnly Q_DECL_ENUMERATOR_DEPRECATED_X("Use setOption(ShowDirsOnly, true) instead")};
      |                     ^~~~~~~~~~~~~
/path/to/CTK/Libs/DICOM/Widgets/ctkDICOMVisualBrowserWidget.cpp:3034:45: warning: ‘QFileDialog::DirectoryOnly’ is deprecated: Use setOption(ShowDirsOnly, true) instead [-Wdeprecated-declarations]
 3034 |   directoryDialog->setFileMode(QFileDialog::DirectoryOnly);
      |                                             ^~~~~~~~~~~~~
In file included from /home/jcfr/Support/Qt/5.15.2/gcc_64/include/QtWidgets/QFileDialog:1,
                 from /path/to/CTK/Libs/Widgets/ctkDirectoryButton.h:26,
                 from /path/to/CTK/Libs/DICOM/Widgets/ctkDICOMVisualBrowserWidget.cpp:39:
/home/jcfr/Support/Qt/5.15.2/gcc_64/include/QtWidgets/qfiledialog.h:84:21: note: declared here
   84 |                     DirectoryOnly Q_DECL_ENUMERATOR_DEPRECATED_X("Use setOption(ShowDirsOnly, true) instead")};
      |                     ^~~~~~~~~~~~~

@jcfr jcfr force-pushed the addDICOMVisualNavigation-2 branch from c949a75 to 3ce3c67 Compare January 12, 2024 09:24
@Punzo

This comment was marked as duplicate.

@jcfr
Copy link
Member Author

jcfr commented Jan 12, 2024

Some of the tests are failing ... it would be great to fix these as well:

$ ctest -R ctkDI
Test project /home/jcfr/Projects/CTK-Qt5-build/CTK-build
      Start 182: ctkDICOMCoreTest1
 1/47 Test #182: ctkDICOMCoreTest1 ....................   Passed    0.04 sec

[...]

      Start 315: ctkDICOMApplicationTest1
47/47 Test #315: ctkDICOMApplicationTest1 .............   Passed    0.23 sec

81% tests passed, 9 tests failed out of 47

[...]


Total Test time (real) =  32.83 sec

The following tests FAILED:
	195 - ctkDICOMEchoTest1 (Failed)
	199 - ctkDICOMQueryTest2 (Failed)
	201 - ctkDICOMRetrieveTest2 (Failed)
	202 - ctkDICOMSchedulerTest1 (Failed)
	203 - ctkDICOMTesterTest1 (Failed)
	204 - ctkDICOMTesterTest2 (Failed)
	216 - ctkDICOMAppWidgetTest1 (SEGFAULT)
	217 - ctkDICOMBrowserTest (Subprocess aborted)
	305 - ctkDICOMHostTest1 (Failed)

@Punzo
Copy link
Contributor

Punzo commented Jan 12, 2024

Some of the tests are failing ... it would be great to fix these as well:

$ ctest -R ctkDI
Test project /home/jcfr/Projects/CTK-Qt5-build/CTK-build
      Start 182: ctkDICOMCoreTest1
 1/47 Test #182: ctkDICOMCoreTest1 ....................   Passed    0.04 sec

[...]

      Start 315: ctkDICOMApplicationTest1
47/47 Test #315: ctkDICOMApplicationTest1 .............   Passed    0.23 sec

81% tests passed, 9 tests failed out of 47

[...]


Total Test time (real) =  32.83 sec

The following tests FAILED:
	195 - ctkDICOMEchoTest1 (Failed)
	199 - ctkDICOMQueryTest2 (Failed)
	201 - ctkDICOMRetrieveTest2 (Failed)
	202 - ctkDICOMSchedulerTest1 (Failed)
	203 - ctkDICOMTesterTest1 (Failed)
	204 - ctkDICOMTesterTest2 (Failed)
	216 - ctkDICOMAppWidgetTest1 (SEGFAULT)
	217 - ctkDICOMBrowserTest (Subprocess aborted)
	305 - ctkDICOMHostTest1 (Failed)

ok I am running them and double-checking.

It may also be related to the second part of #1142 (comment). Reporting here:

Note/question for @jcfr: To run the test locally, I had to follow these steps:

  • I configured CTK in a CTK-Build folder as follows:
CTK_BUILD_ALL                    OFF
CTK_BUILD_ALL_APPS               OFF
CTK_BUILD_ALL_LIBRARIES          OFF
CTK_BUILD_ALL_PLUGINS            OFF
CTK_BUILD_EXAMPLES               ON
CTK_ENABLE_DICOM                 ON
CTK_ENABLE_DICOMApplicationHos   OFF
CTK_ENABLE_PluginFramework       OFF
CTK_ENABLE_Python_Wrapping       OFF
CTK_ENABLE_Widgets               ON
  • Ran make -j 20 in CTK-Build

  • Manually ran make install in CTK-Build to generate DCMTK binaries in CTK-Build/CMakeExternals/Install/bin

  • Added CTK-Build/CMakeExternals/Install/lib to my LD_LIBRARY_PATH environment variable

  • Then I could run the test in CTK-Build/CTK-build folder (ctest)

I might be missing something, but it would be helpful if the superbuild could automatically install DCMTK (eliminating the need for steps 3 and 4). Please let me know if this is a bug or if I am overlooking something.

@Punzo
Copy link
Contributor

Punzo commented Jan 12, 2024

@jcfr I confirm with the ld library path fixed manually, only these tests fails:

he following tests FAILED:
167 - ctkDICOMSchedulerTest1 (Failed)
181 - ctkDICOMAppWidgetTest1 (SEGFAULT)
182 - ctkDICOMBrowserTest (Subprocess aborted)

I am going to fix them now.

If you can have a look how to fix the installation of DCMTK, it would be great (I don't want to mess up the cmake for projects relying on CTK).

@jcfr
Copy link
Member Author

jcfr commented Jan 12, 2024

re: LD_LIBRARY_PATH + lookup binaries

Thanks for the details.
I will look into fixing the build-system to automate this.

re: UML & testing scripts

I updated the main description based of #1165 (comment) to include the updated diagram & testing scripts

@Punzo

This comment was marked as outdated.

@jcfr
Copy link
Member Author

jcfr commented Jan 13, 2024

Thanks @Punzo 🙏 , your changes were helpful to move forward.

Fixes and improvement to the DICOM tests are being integrated in the following pull requests:

Once there are integrated, we will rebase this one and ensure all tests are still passing 💯 🚀

Thanks for your patience ⏳

@lassoan
Copy link
Member

lassoan commented Jan 15, 2024

Thank you @jcfr for workong on this a lot! What is the status of this now? Can I help with anything to get this integrated?

@jcfr jcfr force-pushed the addDICOMVisualNavigation-2 branch from eaeaf7b to 3a98e65 Compare January 16, 2024 01:11
@jcfr
Copy link
Member Author

jcfr commented Jan 16, 2024

Proposed plan:

@lassoan
Copy link
Member

lassoan commented Jan 16, 2024

Sounds good, thank you!

@Punzo
Copy link
Contributor

Punzo commented Jan 16, 2024

@jcfr can you please add this commit jcfr#2 before merging? thanks in advance!

@jcfr jcfr force-pushed the addDICOMVisualNavigation-2 branch 2 times, most recently from c44c296 to 3d86f96 Compare January 17, 2024 11:12
@jcfr jcfr force-pushed the addDICOMVisualNavigation-2 branch from 7843549 to 3e025b8 Compare January 18, 2024 22:49
@Punzo
Copy link
Contributor

Punzo commented Jan 18, 2024

@Punzo Could you perform a last round of test ?

sure! done and everything looks good. Thanks a lot for your hard work on the cleaning, I appreciate it.

  • Consider reviewing the commit Simplify copy of JobResponseSet introducing clone() (one of the most recent)

Checked, looks good to me. I have also tested with valgrind/massif and the memory usage/deallocation works as expected.

  • Note that ctkAbstractScheduler was renamed to ctkJobScheduler as it is not abstract anymore. You may want to update the diagram
  • While the class ctkDICOMWorker but is not fulfilling any purpose, we could keep it around ...

I agree, I removed it in 6afcfe5

and here the new UML

ctkVisualDICOMBrowser

ah that's great!!!

Punzo and others added 2 commits January 18, 2024 18:36
As a step toward automating styling in CTK & Slicer, the most common
styling practices accross CTK & Slicer code base have been applied.
@jcfr jcfr merged commit 8a0717d into commontk:master Jan 19, 2024
3 checks passed
@jcfr jcfr deleted the addDICOMVisualNavigation-2 branch January 19, 2024 06:28
jcfr added a commit that referenced this pull request Jan 19, 2024
Introduces the `ctkDICOMVisualBrowserWidget`, which improves the threaded
execution of the following operations:

- Filtering and navigation with thumbnails of local database and servers results
- Import from file system to local database
- Query/Retrieve from servers (DIMSE C-GET/C-MOVE )
- Storage listener
- Send (emits only a signal for the moment, requires external implementation)
- Remove (only from local database, not from server)
- Metadata exploration

In addition, the commit introduces the following classes:

* `Core` classes:
  * `ctkAbstractJob`
  * `ctkAbstractScheduler`
  * `ctkAbstractWorker`
* `DICOM/Core` classes:
  * `ctkDICOMEcho`
  * `ctkDICOMInserter`
  * `ctkDICOMInserterJob`
  * `ctkDICOMInserterWorker`
  * `ctkDICOMJob`
  * `ctkDICOMJobResponseSet`
  * `ctkDICOMQueryJob`
  * `ctkDICOMQueryWorker`
  * `ctkDICOMRetrieveJob`
  * `ctkDICOMRetrieveWorker`
  * `ctkDICOMScheduler`
  * `ctkDICOMServer`
  * `ctkDICOMStorageListener`
  * `ctkDICOMStorageListenerJob`
  * `ctkDICOMStorageListenerWorker`
* `DICOM/Widget`classes:
  * `ctkDICOMPatientItemWidget`
  * `ctkDICOMSeriesItemWidget`
  * `ctkDICOMServerNodeWidget2`
  * `ctkDICOMStudyItemWidget`

Co-authored-by: Andras Lasso <[email protected]>
Co-authored-by: Jean-Christophe Fillion-Robin <[email protected]>
@jcfr jcfr changed the title Add Visual DICOM Browser Add Visual DICOM Browser | ⚠️ Authorship of integration commit changed Jan 19, 2024
@lassoan
Copy link
Member

lassoan commented Jan 19, 2024

🍾 Amazing job guys, really looking forward to seeing this working in Slicer!

jcfr added a commit to jcfr/CTK that referenced this pull request Apr 2, 2024
Remove setting of unsupported and unused `placeholderText` property from
`ctkDICOMVisualBrowserWidget.ui`. Since the property is set to its default
value, removing it will not impact how the widget is rendered.

For reference, it was originally introduced in 88ff72b ("ENH: Add Visual
DICOM Browser (commontk#1165)", 2024-01-19)
jcfr added a commit to jcfr/CTK that referenced this pull request Apr 2, 2024
Remove setting of unsupported and unused `placeholderText` property from
`ctkDICOMVisualBrowserWidget.ui`. Since the property is set to its default
value, removing it will not impact how the widget is rendered. For
reference, the use of `placeholderText` was originally introduced in
88ff72b ("ENH: Add Visual DICOM Browser (commontk#1165)", 2024-01-19)

Conditionally use `Qt::MatchRegularExpression` introduced in Qt 5.15.
For reference, its use was originally introduced in 888cdd9 ("ENH: Add
ctkDICOMJobListWidget for logging jobs activity in the UI", 2024-01-18)
jcfr added a commit to jcfr/CTK that referenced this pull request Apr 3, 2024
This commit resolves compilation issues encountered when building the DICOMWidget
against Qt 5.12.8. The changes include:

1. Removed the setting of the unsupported and unused `placeholderText` property
   from `ctkDICOMVisualBrowserWidget.ui`. This property's removal does not affect
   widget rendering. The removal addresses a compilation error introduced in commit
   88ff72b ("ENH: Add Visual DICOM Browser (commontk#1165)", 2024-01-19):

    ```
    /path/to/CTK/Libs/DICOM/Core/ctkDICOMStorageListener.cpp:71:18: error: ‘acceptAssociations’ is not a member of ‘DcmSCP’
       71 |   return DcmSCP::acceptAssociations();
          |                  ^~~~~~~~~~~~~~~~~~
    ```

2. Introduced conditional utilization of `Qt::MatchRegularExpression`, available
   in Qt 5.15, to resolve a compilation error introduced in
   commit 888cdd9 ("ENH: Add ctkDICOMJobListWidget for logging jobs activity in the UI", 2024-01-18):

    ```
    /path/to/CTK/Libs/DICOM/Widgets/ctkDICOMJobListWidget.cpp:405:69: error: ‘MatchRegularExpression’ is not a member of ‘Qt’
    405 |   QList<QStandardItem*> list = this->findItems(tr("completed"), Qt::MatchRegularExpression, Columns::Status);
        |                                                                     ^~~~~~~~~~~~~~~~~~~~~~
    ```
jcfr added a commit that referenced this pull request Apr 3, 2024
This commit resolves compilation issues encountered when building the DICOMWidget
against Qt 5.12.8. The changes include:

1. Removed the setting of the unsupported and unused `placeholderText` property
   from `ctkDICOMVisualBrowserWidget.ui`. This property's removal does not affect
   widget rendering. The removal addresses a compilation error introduced in commit
   88ff72b ("ENH: Add Visual DICOM Browser (#1165)", 2024-01-19):

    ```
    /path/to/CTK/Libs/DICOM/Core/ctkDICOMStorageListener.cpp:71:18: error: ‘acceptAssociations’ is not a member of ‘DcmSCP’
       71 |   return DcmSCP::acceptAssociations();
          |                  ^~~~~~~~~~~~~~~~~~
    ```

2. Introduced conditional utilization of `Qt::MatchRegularExpression`, available
   in Qt 5.15, to resolve a compilation error introduced in
   commit 888cdd9 ("ENH: Add ctkDICOMJobListWidget for logging jobs activity in the UI", 2024-01-18):

    ```
    /path/to/CTK/Libs/DICOM/Widgets/ctkDICOMJobListWidget.cpp:405:69: error: ‘MatchRegularExpression’ is not a member of ‘Qt’
    405 |   QList<QStandardItem*> list = this->findItems(tr("completed"), Qt::MatchRegularExpression, Columns::Status);
        |                                                                     ^~~~~~~~~~~~~~~~~~~~~~
    ```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants