Fixes and enhs for the visual dicom browser

[Punzo]

I have adressed all items from:

Settings : #1162 (comment)
Patient Selection: #1162 (comment)

and:

last of Logic: #1162 (comment)
second of Filtering: #1162 (comment)

See also Slicer/Slicer#7676

[Punzo]

@lassoan

[jcfr]

Minor stylistic changes. Have not tested yet.

[Punzo]

Minor stylistic changes. Have not tested yet.

thanks Jc for the review. I have replied at the comments. I will apply them on Monday.

If you will have the chance to test any further feedback will be very welcome 😃

Also guys @jcfr @lassoan @pieper please note that commits c8ac922
e4093e2

introduce a new table in the database, so I have updated the database schema version.

and this commit cdda814

fix an issue that I found in the dicomDatabse, where patient metadata were inserted either with empty patientName or patientID. This created a lot of performances issues because in many methods is not actually allowed having either one of them empty (e.g. compositeID, etc....). Essentially in those cases the cache lists were broken. The commit ensure that insertPatient method uses the patientID and patientName from the uidsForDataSet method, which validates/modifies the patient ID and patientName.

[lassoan]

Also guys @jcfr @lassoan @pieper please note that commits c8ac922 e4093e2

introduce a new table in the database, so I have updated the database schema version.

Adding a new table just for storing multiple connection names for a patient is not worth it. It means extra complication when creating or deleting a patient (you need to create/remove extra records in another table) and even with this extra table, it is very limited and rigid what we can store. Instead, we could add a single new column in the patient table to store the list of allowed and denied servers (need to store the list of denied servers, too, so that we don't need to keep bugging the user asking about a server that the user previously already told not to use for that patient). Query/retrieve (and DICOM in general) is object-oriented anyway (e.g., you need to query each patient from the server), so it is very natural to use this kind of object-oriented interface, and this connections configuration data is small, therefore the performance should be as good (or even better) than with a relational database approach that uses a separate table.

The format of the field could be json, as we already use it (for specifying display column format). The new column name could be Connections and could store something like this: {"allow":["pacs1","pacs2"],"deny":["pacs5"]}. If you don't think we need a deny list for the current GUI implementation then we can just use {"allow":["pacs1","pacs2"]} for now. Using json is still worth it as it is more future-proof (if we need to add more per-patient connection configuration data then it can all go there), the syntax is self-explaining (no need to document what separator character is used, how to escape special characters, etc.), and we don't need to implement a parser.

[Punzo]

Also guys @jcfr @lassoan @pieper please note that commits c8ac922 e4093e2
introduce a new table in the database, so I have updated the database schema version.

Adding a new table just for storing multiple connection names for a patient is not worth it. It means extra complication when creating or deleting a patient (you need to create/remove extra records in another table) and even with this extra table, it is very limited and rigid what we can store. Instead, we could add a single new column in the patient table to store the list of allowed and denied servers (need to store the list of denied servers, too, so that we don't need to keep bugging the user asking about a server that the user previously already told not to use for that patient). Query/retrieve (and DICOM in general) is object-oriented anyway (e.g., you need to query each patient from the server), so it is very natural to use this kind of object-oriented interface, and this connections configuration data is small, therefore the performance should be as good (or even better) than with a relational database approach that uses a separate table.

The format of the field could be json, as we already use it (for specifying display column format). The new column name could be Connections and could store something like this: {"allow":["pacs1","pacs2"],"deny":["pacs5"]}. If you don't think we need a deny list for the current GUI implementation then we can just use {"allow":["pacs1","pacs2"]} for now. Using json is still worth it as it is more future-proof (if we need to add more per-patient connection configuration data then it can all go there), the syntax is self-explaining (no need to document what separator character is used, how to escape special characters, etc.), and we don't need to implement a parser.

I agree, I had the issue of the security warning been raised too many times. I will apply your proposed design next week.

[Punzo]

thanks Jc for the review. I have replied at the comments. I will apply them on Monda

@jcfr review applied in c86e441

[Punzo]

I agree, I had the issue of the security warning been raised too many times. I will apply your proposed design next week.

@lassoan done in 0c990d9

[Punzo]

Really impressive work Davide, lots of fixes and improvements. I've added a number of comments they are mostly all trivial.

thanks and thanks for the review. I will apply it asap.

done!

[lassoan]

Let us know when this is ready for review.

[Punzo]

Let us know when this is ready for review.

Ok, I'm planning to do a last round of manual testing tomorrow, in addition to the automated tests. I'll ping you when I have finished.

[Punzo]

@lassoan tested with Slicer/Slicer#7676. This PR should be ready, please let me know if you have any other feedback.

[lassoan]

It would be nice to fix these before merging:

• In dark mode, some fields are not visible at all (see Settings section):

• I don't understand this sentence: "One or more active servers have been marked with an unrecognized access level. A server is marked as such if it deviates from the cached source of patient data server stored in the local DICOM database." Since we have a good default value ("trusted" flag), and popups are annoying, I don't think we need the popup at all. If query of the patient from the server was not explicitly disabled or enabled then decide based on the "trusted" flag of the server.
• Make checkboxes in "Servers" list in patient section tri-state. If grayed out then it means that the default is used (trusted flag).
• It is odd that "Servers" list in patient section is in a collapsible section. Why not a simple label+widget?
• Query/Retrieve checkbox of the search icon is not clear. "Query/Retrieve from servers" would be more clear and the tooltip could mention servers, too.

[Punzo]

It would be nice to fix these before merging:

• In dark mode, some fields are not visible at all (see Settings section):

• I don't understand this sentence: "One or more active servers have been marked with an unrecognized access level. A server is marked as such if it deviates from the cached source of patient data server stored in the local DICOM database." Since we have a good default value ("trusted" flag), and popups are annoying, I don't think we need the popup at all. If query of the patient from the server was not explicitly disabled or enabled then decide based on the "trusted" flag of the server.
• Make checkboxes in "Servers" list in patient section tri-state. If grayed out then it means that the default is used (trusted flag).
• It is odd that "Servers" list in patient section is in a collapsible section. Why not a simple label+widget?
• Query/Retrieve checkbox of the search icon is not clear. "Query/Retrieve from servers" would be more clear and the tooltip could mention servers, too.

1. Regarding the color in dark mode, I have fixed it. But we should really move await from palette and use only the stylesheet approach in all the CTK classes (i.e. Roadmap list of issues/enhancements for Visual DICOM Browser #1162 (comment)). I can tackle it, we should also add @sjh26 in this loop.
2. I agree on the trusted servers behaviour/logic/UI, I have applied it.

Done!

Tomorrow I will do another set of manual tests. I will let you know when I have finished.

[Punzo]

1. Regarding the color in dark mode, I have fixed it. But we should really move await from palette and use only the stylesheet approach in all the CTK classes (i.e. Roadmap list of issues/enhancements for Visual DICOM Browser #1162 (comment)). I can tackle it, we should also add @sjh26 in this loop.

I still have to review all CTK classes that uses the palette approach, but ideally the solution should:

1. use one unique style file containing the stylesheet for all the CTK classes
2. change color of qt widgets in CTK dynamically with the Property Selector, e.g.: https://wiki.qt.io/Dynamic_Properties_and_Stylesheets
3. custom Slicer apps should be able to change the style by simply rewriting and reloading the style file

This is going to be addressed in another PR, since this PR is already too big. @sjh26, @lassoan @jcfr @pieper please let me know if you have any feedback/advice on this before I start any implementation. If necessary we can also do a videocall to discuss it.

[Punzo]

@lassoan I have done. For me it is good to go, please let me know if you have any further feedback.

[Punzo]

1. Regarding the color in dark mode, I have fixed it. But we should really move await from palette and use only the stylesheet approach in all the CTK classes (i.e. Roadmap list of issues/enhancements for Visual DICOM Browser #1162 (comment)). I can tackle it, we should also add @sjh26 in this loop.

I still have to review all CTK classes that uses the palette approach, but ideally the solution should:

1. use one unique style file containing the stylesheet for all the CTK classes
2. change color of qt widgets in CTK dynamically with the Property Selector, e.g.: https://wiki.qt.io/Dynamic_Properties_and_Stylesheets
3. custom Slicer apps should be able to change the style by simply rewriting and reloading the style file

This is going to be addressed in another PR, since this PR is already too big. @sjh26, @lassoan @jcfr @pieper please let me know if you have any feedback/advice on this before I start any implementation. If necessary we can also do a videocall to discuss it.

Provided a barebones example in Punzo#4

[lassoan]

Thank you, I've tested on Windows and everything worked nicely.

[Punzo]

Thank you, I've tested on Windows and everything worked nicely.

Thanks for testing and merging!