Add dicom job list widget for logging jobs activity in the visual DICOM browser UI

[Punzo]

[lassoan]

Some suggestions (wishlist):

• Add column for job creation date/time.
• Add progress column (% completion)
• Add patient name as column (and maybe birth date and modality as well if easy to obtain)
• Make column names human-readable (with spaces) and translatable
• Study instance UID, series instance UID, SOP instance UID, Job UID do not contain directly useful information for the user and they take up a lot of space. I would even remove the DICOMLevel column and just merge it with the job class (as described above). Instead of polluting the table with these columns (making the table more complex, larger, and leaving less space for relevant information), it could be better to have a collapsible "Details" section (or popup) with a textbox that contains all the details of the job, including status, errors, etc. in plain text that can be easily copy-pasted for troubleshooting. If multiple jobs are selected then the details text would contain the concatenation of the information from all jobs.
• Record the job creation time, when execution started, and when it was completed (and display it in the job details text).
• Filter is usually at the top (see the old DICOM browser, the application log, etc.), but since we have some buttons on the right, I think we could have filtering options on the right. Maybe something similar to how the Windows event log works (although there filtering settings are in a popup, which is not very user friendly):

• It is not clear how "Show all" relates to filtering. It would be better to have all filtering options at the same place. Instead of "Show all" probably "Show completed" (turned off by default) would be more clear, and maybe we can add a "Reset" button next to the filter (that resets to the default filtering settings, i.e., show all tasks that are not completed).
• It is not clear if "Clear" means that it just removes successfully completed jobs, or also failed jobs, or it even stops all in-progress jobs and clears all scheduled jobs. Maybe a broom icon and change to "Clear completed" caption would make it clear.
• We should have a quick way of retrying all failed jobs (1-2 button clicks)

[Punzo]

Some suggestions (wishlist):

@lassoan thanks for the feedback.
I will apply them on Monday (tomorrow I am off).
The implementation should be straightforward.

[Punzo]

Fix also: Excessive logging in debug mode (from @lassoan).

Logging currently is at two levels:

1. DCMTK: it is a static level which can be changed at runtime. By default the the ctkDICOMScheduler set it to warning in its creator

Slicer set it at run time with the settings (detailed logging in the DICOM settings):
https://github.com/Slicer/Slicer/pull/7525/files#diff-522c86f7cb8b2b77df9d521eca44263ad00536c63f25182df3d75128dd09a16aR422-R427

2. CTK: each class can have static logger one, for example:

https://github.com/commontk/CTK/blob/master/Libs/DICOM/Core/ctkDICOMQuery.cpp#L51
https://github.com/commontk/CTK/blob/master/Libs/DICOM/Core/ctkDICOMQuery.cpp#L466

these logger classes have the logging level set at compilation as:
https://github.com/commontk/CTK/blob/master/Libs/Core/ctkLogger.cpp#L34-L38

I guess Andras issue is regarding (2). A fast fix could be setting warning or info level (instead of debug) if the project is set in debug configuration (in release mode currently is warning). I better fix could be to allow to change it at run time and have a slicer configuration in the settings.

[Punzo]

Fix also: Excessive logging in debug mode (from @lassoan).

Logging currently is at two levels:

1. DCMTK: it is a static level which can be changed at runtime. By default the the ctkDICOMScheduler set it to warning in its creator

Slicer set it at run time with the settings (detailed logging in the DICOM settings): https://github.com/Slicer/Slicer/pull/7525/files#diff-522c86f7cb8b2b77df9d521eca44263ad00536c63f25182df3d75128dd09a16aR422-R427

2. CTK: each class can have static logger one, for example:

https://github.com/commontk/CTK/blob/master/Libs/DICOM/Core/ctkDICOMQuery.cpp#L51 https://github.com/commontk/CTK/blob/master/Libs/DICOM/Core/ctkDICOMQuery.cpp#L466

these logger classes have the logging level set at compilation as: https://github.com/commontk/CTK/blob/master/Libs/Core/ctkLogger.cpp#L34-L38

I guess Andras issue is regarding (2). A fast fix could be setting warning or info level (instead of debug) if the project is set in debug configuration (in release mode currently is warning). I better fix could be to allow to change it at run time and have a slicer configuration in the settings.

@lassoan for the moment I have simply set the ctk LogLevel to Info for debug mode and Warning for release mode. In CTK logger.debug was the most used one. This highly reduces the number of logging outputs. Developers can always change it back locally. See commit:

3739cde

Moving forward, we either:

1. implement a proper logger pattern (e.g. DCMTK logger), where we could set the level globally at run time.
2. we move to use QLoggingCategory (https://doc.qt.io/qt-5/qloggingcategory.html) as already proposed by @jcfr ( see Update ctkLogger #1181 (comment))

[Punzo]

• (1) Add column for job creation date/time.

• (2) Add progress column (% completion)

• (3) Add patient name as column (and maybe birth date and modality as well if easy to obtain)

• (4) Make column names human-readable (with spaces) and translatable

• (5) Study instance UID, series instance UID, SOP instance UID, Job UID do not contain directly useful information for the user and they take up a lot of space. I would even remove the DICOMLevel column and just merge it with the job class (as described above). Instead of polluting the table with these columns (making the table more complex, larger, and leaving less space for relevant information), it could be better to have a collapsible "Details" section (or popup) with a textbox that contains all the details of the job, including status, errors, etc. in plain text that can be easily copy-pasted for troubleshooting. If multiple jobs are selected then the details text would contain the concatenation of the information from all jobs.

• (6) find a way to log DCMTK responses per job. i.e. instead of priting DCMTK logging to terminal, we need to have a stream for each Job (identified by the JobUID) to the ctkDICOMJobListWidget (which will be printed in the details)

• (7) Record the job creation time, when execution started, and when it was completed (and display it in the job details text).

• (8) Filter is usually at the top (see the old DICOM browser, the application log, etc.), but since we have some buttons on the right, I think we could have filtering options on the right. Maybe something similar to how the Windows event log works (although there filtering settings are in a popup, which is not very user friendly):

• (9) It is not clear how "Show all" relates to filtering. It would be better to have all filtering options at the same place. Instead of "Show all" probably "Show completed" (turned off by default) would be more clear, and maybe we can add a "Reset" button next to the filter (that resets to the default filtering settings, i.e., show all tasks that are not completed).

• (10) It is not clear if "Clear" means that it just removes successfully completed jobs, or also failed jobs, or it even stops all in-progress jobs and clears all scheduled jobs. Maybe a broom icon and change to "Clear completed" caption would make it clear.

We should have a quick way of retrying all failed jobs (1-2 button clicks)

[lassoan]

My issue (#1186) with logging for now is with logger.debug( "SQL worked!\n SQL: " + query.lastQuery()); code in ctkDICOMDatabase.cpp. Such messages are logged fairly frequently in cases when everything is fine. So, it is not a debug level message, but a trace level message. If setting to trace level fixes the superflous logging then it could be OK, but for performance reasons I would consider enabling them with #ifdef.

[Punzo]

My issue (#1186) with logging for now is with logger.debug( "SQL worked!\n SQL: " + query.lastQuery()); code in ctkDICOMDatabase.cpp. Such messages are logged fairly frequently in cases when everything is fine. So, it is not a debug level message, but a trace level message. If setting to trace level fixes the superflous logging then it could be OK, but for performance reasons I would consider enabling them with #ifdef.

I agree. I have set the majority of the loging from the database class as trace and switched back to debug the ctk log level for debug mode. b65c0f6

[Punzo]

Debug Logging discussion moved to #1186 and #1187

regarding this PR @lassoan I need only to finish (2 - progress bars) and (6 - stream DCMTK logging/errors per each job to the UI) from #1184 (comment)
ETA:
(2) tomorrow
(6) asap.

[Punzo]

@lassoan (2) progress bars done

See video:

2024-01-23.17-01-03.mp4

NOTE: I also have fixed:

1. some issues with CMOVE when using the new ctkDICOMStoregeListener
2. perfomances improvements: in some cases there were unnecessary re-renderings of the thumbnails.

I will try to have (6 - stream DCMTK logging/errors per each job to the UI) asap.

But I would consider to merge this PR this week, without (6). This PR is already very large and streaming the logging to the UI will be another large commit probably. It would be ideal to have this in Slicer before the project week. Please let me know if you agree @lassoan @jcfr

[Punzo]

(6 - stream DCMTK logging/errors per each job to the UI): this can be implemented by instantiating an Appender with a custom ErrorHandler to the SCU logger:
DCM_dcmnetLogger (name: "dcmtk.dcmnet")

See also:
https://support.dcmtk.org/redmine/projects/dcmtk/wiki/Howto_LogProgram#Redirecting-log-output-to-a-file

@lassoan @jcfr this will be done in a different PR (most likely next week at the project week) .

[lassoan]

Thank you it all looks good to me. I've just fixed a build error on Windows (a struct was forward-declared as class, causing a link error) and force-pushed it. I'll merge it now so that it can be included in tomorrow's Slicer Preview Release.