search

commontk / CTK

A set of common support code for medical imaging, surgical navigation, and related purposes.
https://commontk.org
Apache License 2.0
827 stars 480 forks source link

visual dicom browser enhs and bugs fixes #1191

Closed Punzo closed 3 months ago

Punzo commented 4 months ago

@lassoan @jcfr

I am plannig to use this branch https://github.com/Punzo/CTK/tree/visualDICOMBrowserENH for dev regarding to https://github.com/commontk/CTK/issues/1162

The current commit fix a couple of breaking bugs:

  1. the proxy server option in the servers settings UI was cleared at startup
  2. the sorting of the study widgets was broken when studies had the same date. This caused that some study widgets were not added to the layout.
  3. fix and clean jobs handling (stopping, setting the status, etc...)

It would be nice to merge this PR and update Slicer as well.

Punzo commented 4 months ago

Thanks for working on this. I feel that the whole job stopping mechanism is quite fragile. See more details in inline comments.

Hei Andras, thanks for reviewing. The jobs stop commit was still a draft, but thanks for the feedback. I incorporated your feedback as well in the final version of the commit. Please have a look at https://github.com/commontk/CTK/pull/1191/commits/a3245ff1b77ac97dfcd8d557b3a8f0d3e185cb83

I have tested and I could not reproduce any crash anymore.

Punzo commented 4 months ago

Thank you Davide, this is so much better! Still, there are a few things that would be nice to clarify - see comments inline.

thank you for reviewing!!! I have replied to the comments and applied some in https://github.com/commontk/CTK/pull/1191/commits/3af864d4f2c58186fb07eb1622f766c8119ba86f

Punzo commented 4 months ago

Thanks for the update.

you are welcome!

I've added a couple of more comments inline.

ok I have applied them see https://github.com/commontk/CTK/pull/1191/commits/045cb41959b7b5b2ba2832b8d2562faef94d85ae

In addition to that, it would be nice if you could document meaning of each state in the abstract job header file.

Done!

Punzo commented 3 months ago

Thank you, the changes look good to me.

thanks for reviewing and merging. Slicer PR is at https://github.com/Slicer/Slicer/pull/7650