Skip to content
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: Create new view to embed DICOM browser in main widget #1061

Closed

Conversation

@Sunderlandkyl
Copy link

commented Dec 13, 2018

Accomplished through the use of the new view factory qSlicerSingletonViewFactory.
Both C++ and Python classes can register any singleton QWidget by calling factory->setWidget(QWidget*) and factory->setTagName(std::string).

The DICOM browser layout is registered as a single view which spans the whole widget.
Navigating to the DICOM browser module will change to this new layout and hide the data probe, while navigating to a different widget, loading a series (with browser persistence off), or clicking the new "Close" button, will restore the previous layout and settings.

See discussion: https://discourse.slicer.org/t/dicom-browser-is-stuck-behind-main-window-after-dicom-import/4826/24

image

Copy link
Contributor

left a comment

Nice work, thank you very much. A couple of small issues/questions are added inline.

Base/QTGUI/qSlicerSingletonViewFactory.cxx Outdated Show resolved Hide resolved
Base/QTGUI/qSlicerSingletonViewFactory.h Outdated Show resolved Hide resolved
Modules/Scripted/DICOM/DICOM.py Outdated Show resolved Hide resolved

layoutManager = slicer.app.layoutManager()
layoutManager.registerViewFactory(self.viewFactory)
self.DICOM_BROWSER_LAYOUT_ID = 2200

This comment has been minimized.

Copy link
@lassoan

lassoan Dec 14, 2018

Contributor

It would be better to define this layout ID as a constant outside of the class so that you don't need to instantiate a class to access it.

This comment has been minimized.

Copy link
@jcfr

jcfr Dec 14, 2018

Member

the layout base id approach

For layout id, I am wondering if we should allocate 100 slots per extension or module or domain, and come up with hash function returning an integer and multiply it by 100:

unsigned int ComputeLayoutBaseId(const std::string& domain)

That way for any given module or extension, we could be sure to have a unique base ID and we would avoid clash.

layout specific to a module/extension would then be indexed relative to their layout base id.

explicit registry

An other approach would be to allocate layout base id and keep track of assignment in a shared document.

This comment has been minimized.

Copy link
@lassoan

lassoan Dec 14, 2018

Contributor

Choosing to use integer layout identifiers was a mistake. We cannot correct it in Slicer-4.10.x but we should switch to string IDs in Slicer5. String IDs can may be prefixed with modulename+"." as we do it for singleton tags and node attributes. We may remain somewhat backward compatible by having SetLayoutID(int) method that maps known layout integer IDs to the new string IDs.

Modules/Scripted/DICOM/DICOM.py Outdated Show resolved Hide resolved
@Sunderlandkyl

This comment has been minimized.

Copy link
Author

commented Dec 14, 2018

Would it make sense to allow each instance of qSlicerSingletonViewFactory to create multiple different types of widgets?

We could call qSlicerSingletonViewFactory::addWidget(QWidget* name, QString tag):

Ex.

factory->addWidget(widget1, "widget1");
factory->addWidget(widget2, "widget3");
factory->addWidget(widget3, "widget3");

Rather than creating a factory for each one, a module may want to create a single factory holding all of the desired singleton view widgets.

@lassoan

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2018

Storing multiple widgets in a factory would complicate the factory and it would not simplify other code parts much, so I think the code is good as is. Do you have any specific use case in mind that would be simplified by multiple widgets storage?

@Sunderlandkyl

This comment has been minimized.

Copy link
Author

commented Dec 14, 2018

No, I can't think of any specific use cases.

Next question: should the option be provided to continue to use the existing pop-up window? (Using a checkbox/QSettings)

@lassoan

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2018

should the option be provided to continue to use the existing pop-up window? (Using a checkbox/QSettings)

Since the current behavior is not good (stuck behind on MacOS) and we plan to make further improvements, which may be more complicated if we want to keep both modes working, I don't think we need the compatibility option.

@pieper

This comment has been minimized.

Copy link
Member

commented Dec 14, 2018

Since the current behavior is not good (stuck behind on MacOS) and we plan to make further improvements, which may be more complicated if we want to keep both modes working, I don't think we need the compatibility option

There was a user/use case where people wanted the dicom browser on one monitor in persistent mode while interacting in the main monitory. I haven't been following this discussion closely but would that still be supported in your new design?

@lassoan

This comment has been minimized.

Copy link
Contributor

commented Dec 15, 2018

Multi-monitor support would come with multi-monitor support of view layouts. Modules could also display a browser in a popup.

@Sunderlandkyl Sunderlandkyl force-pushed the Sunderlandkyl:dicom_browser_view branch from c748f8c to 8c8a3f1 Dec 17, 2018
@jcfr

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

The qSlicerSingletonViewFactory is useful in itself, if there are no consensus regarding the update of the DICOM module, I suggest we split the PR and move forward with the integration of qSlicerSingletonViewFactory

@Sunderlandkyl

This comment has been minimized.

Copy link
Author

commented Jan 16, 2019

Sounds good, I'll create a separate pull request for qSlicerSingletonViewFactory.

@Sunderlandkyl Sunderlandkyl force-pushed the Sunderlandkyl:dicom_browser_view branch from 8c8a3f1 to 1ade23b Jul 4, 2019
@Sunderlandkyl

This comment has been minimized.

Copy link
Author

commented Jul 4, 2019

I've rebased this change onto the latest master.

@@ -81,6 +81,8 @@ class DICOMDetailsBase(VTKObservationMixin, SizePositionSettingsMixin):
"""

widgetType = None
loadingFinished = qt.Signal()

This comment has been minimized.

Copy link
@jcfr

jcfr Jul 5, 2019

Member

Are these signals used ? If yes, it may be worth documenting both loadingFinished and closed in the class description

@lassoan

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2019

I've pushed a commit on my branch that would be a step towards the final GUI that you talked about (with a subject hierarchy tree on the left): https://github.com/lassoan/Slicer/tree/dicom_browser_view

@Sunderlandkyl Sunderlandkyl force-pushed the Sunderlandkyl:dicom_browser_view branch from 1ade23b to ed60266 Aug 15, 2019
@Sunderlandkyl

This comment has been minimized.

Copy link
Author

commented Aug 15, 2019

Updated this pull request to include the changes that you made @lassoan, as well as moved the browser settings (database directory and table density) to the left panel. Also made some fixes to the qSlicerSingletonViewFactory class.

Here's what it looks like now:
image

@Sunderlandkyl Sunderlandkyl force-pushed the Sunderlandkyl:dicom_browser_view branch from ed60266 to 4fea742 Aug 15, 2019
@lassoan

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

Thank you, this will be awesome!

Few minor things (except the first one, only fix those that are not hard to implement):

  • Tables and menu are shown twice
  • Could we hide the node IDs in "Loaded data" tree?
  • Could you collapse "Browser settings" by default?
  • Change title of the collapsible section that contains the "Loaded data" tree from "DICOM database" to something else. Maybe just "Data"? Or put the data tree outside of collapsible sections?
  • Move out "Show DICOM Browser" from the collapsible section.
  • Make "Show DICOM Browser" a toggle button (it would remain pushed as long as the browser is visible and released when the DICOM browser gets hidden)
  • Move "Horizontal" and "Browser persistent" to Browser settings section (they are not used frequently, so it would be better if they did not clutter the GUI)
@jcfr

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

I also wonder if the "close" button at the bottom should be removed

@lassoan

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

If the toggle button is made prominent enough then I agree that we don't really need the Close button.

@Sunderlandkyl

This comment has been minimized.

Copy link
Author

commented Aug 15, 2019

Great, thanks for the feedback!

The double table issue is already fixed, I just forgot to update the image, and the "Browser settings" groupbox is already closed by default.

I'll make the rest of the changes, including removing the close button, and update the pull request.

@Sunderlandkyl Sunderlandkyl force-pushed the Sunderlandkyl:dicom_browser_view branch from 4fea742 to 36e1677 Aug 16, 2019
@Sunderlandkyl

This comment has been minimized.

Copy link
Author

commented Aug 16, 2019

Ok, changes made. Here is the new layout:

image

@lassoan

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

This looks awesome. I'm just wondering if we should change the label of the browser toggle button to "Show DICOM Database", since that is the window title of the browser. Also, since it is a toggle button, I think we can keep displaying the "Show" verb (regardless the button is pressed or not).

@cpinter

This comment has been minimized.

Copy link

commented Aug 16, 2019

I like it! I always wonder if there is a toggle button that says show/hide, how people would interpret it. What you did Kyle is what I usually do as well, just not sure if it's the right way. I mean that when it's pushed, and it says Hide, then would the user think that it would hide, or that hide was activated.

@cpinter

This comment has been minimized.

Copy link

commented Aug 16, 2019

Right, @lassoan had the same idea :)

@Sunderlandkyl

This comment has been minimized.

Copy link
Author

commented Aug 16, 2019

@lassoan "Show DICOM Database" title case or "Show DICOM database" sentence case? (Or do we consider the DICOM Database a proper noun?)

@lassoan

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

It should probably have the same capitalization as the window title of the browser. Maybe "DICOM database" title and "Show DICOM database browser" button text.

The DICOM browser layout is registered as a single view which spans the whole widget.
Navigating to the DICOM browser module will change to this new layout and hide the data probe, while navigating to a different widget, loading a series (with browser persistence off), or clicking the new "Close" button, will restore the previous layout and settings.
@Sunderlandkyl Sunderlandkyl force-pushed the Sunderlandkyl:dicom_browser_view branch from 36e1677 to e93cc2c Aug 16, 2019
@Sunderlandkyl

This comment has been minimized.

Copy link
Author

commented Aug 16, 2019

OK, sounds good. Title/button text changed.

@lassoan

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

This works really well! Merged r28447.

@lassoan lassoan closed this Aug 19, 2019
@cpinter

This comment has been minimized.

Copy link

commented Aug 19, 2019

Great :) Looking forward to using it!

@Sunderlandkyl Sunderlandkyl deleted the Sunderlandkyl:dicom_browser_view branch Aug 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.