View Issue Details
|ID||Project||Category||View Status||Date Submitted||Last Update|
|0004360||Slicer4||Module DICOM||public||2017-04-06 10:41||2017-10-20 20:08|
|Product Version||Slicer 4.7.0|
|Target Version||Slicer 4.9.0||Fixed in Version||Slicer 4.9.0|
|Summary||0004360: Two popups after DICOM import|
After importing DICOM data into the database, two popups appear, which the user need to OK. Moreover, the two popups appear on different monitors if the DICOM browser was not on the primary screen (probably the parent of the popup message boxes are different, i.e. browser vs none).
This makes importing datasets inconvenient. I suggest consolidating the two popups into one, and possibly dialing down the size of the first popup so that people actually look at it.
|Tags||No tags attached.|
20170406_TwoDICOMImportPopups.jpg (511,642 bytes)
20170406_TwoDICOMImportPopups.jpg (511,642 bytes)
I see - yes, two dialogs is not good but didn't seem worth changing, but I didn't know about the multi-monitor issue. That is a pain.
This is slightly complicated by the issue that the import summary dialog is created by CTK, while the extension selection dialog is done by Slicer (this is why I didn't combine them in the first place).
There is already a property to turn off the ctk dialog. We could do this without changing the API. We could also change ctkDICOMBrowser to create the message string even when the dialog isn't shown and then expose that string as a property that can be used to combine it with the slicer level dialog.
Another option could be to change one or both dialogs into ctkMessageBox instances with the "Don't show this message again" option exposed.
Thanks for the feedback, Steve! I see, more complicated than I imagined for sure.
I like your suggestion about exposing the popup message as a string. If we can show both popups on one screen (created from Slicer using the same parent), then most of the inconvenience would be gone.
If the two popups can be made optionally not shown again, then I think it would be OK to keep them separate. For example I like being notified about the success of the import (number of patient/study/series imported), but I never care about the suggested extensions. I know I'm quite different from a typical user, but for me the best would actually be to only see any popup if importing failed.
In summary, making them 1) appear on the same screen, 2) optionally not shown, but keeping them separate would be a good solution I think.
I made the easiest small fix to address the first priority of making them appear on the same screen:
The second priority, making them expose the 'don't show again' checkbox (a.k.a."silencable"), is a little bigger step because it will have wide impact. Since the DICOMWidgets make use of the slicer.util.infoDisplay, which is ultimately a slicer.util.messageBox changing from a QMessageBox to a ctkMessageBox would change things in many places. Maybe it would be a change for the better, since all dialogs would be optionally able to be silenced.
Maybe the best solution would be to add (yet another) parameter to all the slicer.util.messageBox related methods so they could be optionally silenced with the default being that they are not slicencable for backwards compatibility.
For now Csaba can you try the latest commit and see if that gets us far enough? We should discuss the messageBox change on the list or at a hangout before making the bigger change.
Thank you, Steve! I'll test your change soon.
This is a good step forward. I don't have strong feelings toward making the dialogs silencable, so if for others the two popups are fine, then this issue can be closed.
Sounds good! Close this if the fix is workable for you.
It's not hard to change CTK or slicer.util but given the chances for unintended consequences we need to be sure it's strongly motivated.
Unfortunately the two dialogs still appear on separate screens, see new attachment.
(Cannot upload image. There's nothing much to see anyway, very similar to first screenshot above)
Hi Csaba -
I think this commit will fix it for you:
Can you test that on your build? If it works we can bump the CTK version in Slicer.
It works, Steve, thanks a lot!
Should the same change be made for line https://github.com/commontk/CTK/blob/master/Libs/DICOM/Widgets/ctkDICOMBrowser.cpp#L727 ?
@Csaba: yes, good catch! I committed that fix https://github.com/commontk/CTK/commit/6f7b8341008766ebb0e73a0c6bb9279fcc76837a
Slicer updated with latest CTK:
Great, thanks a lot, Steve! I'm closing the issue as this change fixes the most problematic aspect. Thanks again!
|2017-04-06 10:41||pinter||New Issue|
|2017-04-06 10:41||pinter||Status||new => assigned|
|2017-04-06 10:41||pinter||Assigned To||=> pieper|
|2017-04-06 10:46||pinter||File Added: 20170406_TwoDICOMImportPopups.jpg|
|2017-04-06 14:57||pieper||Relationship added||related to 0004146|
|2017-04-06 14:58||pieper||Note Added: 0014418|
|2017-04-06 14:58||pieper||Status||assigned => acknowledged|
|2017-04-06 15:15||pieper||Note Added: 0014419|
|2017-04-06 15:30||pinter||Note Added: 0014420|
|2017-04-07 10:48||pieper||Note Added: 0014421|
|2017-04-07 11:21||pinter||Note Added: 0014422|
|2017-04-07 11:23||pieper||Note Added: 0014423|
|2017-10-05 15:13||jcfr||Note Added: 0015257|
|2017-10-18 01:34||jcfr||Target Version||Slicer 4.7.0 => Slicer 4.9.0|
|2017-10-19 16:59||pinter||Note Added: 0015322|
|2017-10-19 17:46||pinter||Note Added: 0015323|
|2017-10-20 08:36||pieper||Note Added: 0015325|
|2017-10-20 13:26||pinter||Note Added: 0015327|
|2017-10-20 17:58||pieper||Note Added: 0015329|
|2017-10-20 17:58||pieper||Status||acknowledged => resolved|
|2017-10-20 17:58||pieper||Resolution||open => fixed|
|2017-10-20 20:08||pinter||Status||resolved => closed|
|2017-10-20 20:08||pinter||Fixed in Version||=> Slicer 4.9.0|
|2017-10-20 20:08||pinter||Note Added: 0015330|