-
Notifications
You must be signed in to change notification settings - Fork 494
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
ENH: Impove DIMSE widget #994
Conversation
I didn't have a chance to try it but it looks good to me - thanks for the contribution 👍 |
It looks good, but since we still have the half of the window taken up by the "Search options" widget, the improvement is not that substantial. It seems that the current layout is inspired by OsiriX: This is not a good design (meaning of tabbed control for the input text is not unclear, number of displayable image modalities are limited, data sources always take up a lot of space, etc.). We don't need a complete rework of the GUI now (although it would be really nice), but it would be enough to:
I've checked how Weasis Query/Retrieve looks like and interestingly it is almost that same as the changes that I've outlined above: |
I do that |
// Get the study UID of the current series to be retrieved | ||
auto currentStudyAndSeriesUIDPair = std::find_if( d->StudyAndSeriesInstanceUIDPairList.begin(), d->StudyAndSeriesInstanceUIDPairList.end(), | ||
[&seriesUID]( const std::pair<QString, QString>& element ) { return element.second == seriesUID; } ); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should init studyUID = currentStudyAndSeriesUIDPair->first to be clearer
@lassoan one disadvantage of using a collapsible button for filters (as in the posted figure) is that you cannot combine multiple filters. So, I propose to add a spin box to be able to add filters. |
You can easily combine multiple filters, you can have as many input lines as you want, you just need to show one at each time and hide the others. It is true that with a combobox you need two clicks to switch, so you may add a |
b23cd60
to
9492f0f
Compare
@riep I've updated this branch with a change that moves the search settings to the "Query" collapsible section and make the "Retrieve" section collapsible, too. The user can switch between these two sections (when one section is opened the other is closed automatically), so there is enough space for everything without the need to rework the search option widgets. I've also updated tests that did not compile due to the changed API. I've tested the retrieve of selected series and it worked well. Please take a look, give it some testing and if you like it then I think we should merge this. Thanks for your contribution! |
Thanks Andras! I was very busy lately so this PR was delayed. I test it next Monday and let you know. |
This is nice. I have to double-check the code and check the code format. Is there a specific way to apply format in CTK? |
I don't think there is a format specification file. However, the amount of modified code is so little that it should not be hard at all to fix up any formatting issues manually.
Do you find the current progress bar too detailed or too high-level? |
ok, I would be nice to have a double progress bar. That could be part of another feature and PR. I do the code formatting and testing today. |
* Retrieve only selected series (previously, all series of the selected studies were retrieved) * Add collapsible button to hide query or retrieve section, to give more space for browsing query results
b23cd60
to
efe6d8c
Compare
It looks good to me and should be ready to be merged. |
fixes Retrieve a selection of dicom from a PACS #993