ENH: Add remove signal to DICOM browser #1164
Conversation
Co-authored-by: Andras Lasso <[email protected]>
Co-authored-by: Andras Lasso <[email protected]>
Co-authored-by: Andras Lasso <[email protected]>
There was a problem hiding this comment.
After fixing the few trivial issues (see comments inline), this pull request looks OK to me.
To fix the issue in Slicer, it is probably better not to rely on these signals (you can only hope that items are only deleted via removePatient/Study/Series methods of a specific database object). Instead, you may want to connect to the databaseChanged signal, which is emitted even when the database is changed via another connection.
These signals can be still useful for in-memory databases and maybe also also for getting notifications faster (caching may delay changes on disk).
Libs/DICOM/Core/ctkDICOMDatabase.cpp
Outdated
| @@ -3014,6 +3014,8 @@ bool ctkDICOMDatabase::removeSeries(const QString& seriesInstanceUID, bool clear | |||
|
|
|||
| d->resetLastInsertedValues(); | |||
|
|
|||
| emit seriesRemoved(); | |||
Libs/DICOM/Core/ctkDICOMDatabase.cpp
Outdated
|
|
||
| if(result) | ||
| { | ||
| emit studyRemoved(); |
Libs/DICOM/Core/ctkDICOMDatabase.cpp
Outdated
|
|
||
| if(result) | ||
| { | ||
| emit patientRemoved(); |
Libs/DICOM/Core/ctkDICOMDatabase.h
Outdated
| @@ -430,6 +430,15 @@ class CTK_DICOM_CORE_EXPORT ctkDICOMDatabase : public QObject | |||
| /// - instanceUID (unique) | |||
| void instanceAdded(QString); | |||
|
|
|||
| /// This signal is emitted when a patient is removed from CTK database. | |||
There was a problem hiding this comment.
This signal will not be triggered when a patient is removed from the database. It will only be triggered if it is deleted by calling removePatient method on this object. Unless it is an in-memory database, the database can be simultaneously accessed from via several ctkDICOMDatabase objects in the same process or other processes, so not all deletions will trigger these signals. Update the method documentation to make this clear.
There was a problem hiding this comment.
Modifications done
cpoetteCertis
force-pushed
the
cpoette/add-remove-signal-to-dicom-browser
branch
from
078744f to
013d9a4
Compare
|
@cpoetteCertis thanks for the contributions. Adding the signals makes sense. @jcfr it could make sense to add this commit 013d9a4 to #1165 as well |
The commit can indeed be cherry-picked cleanly and it has been integrated. |
|
Closing. This is superseded by #1183 |
In order to fix slicer's bug described in the following link:
Slicer/Slicer#7290
It is necessary to emit a signal when removing series from DICOM database