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

New stream arg in tabview conflicts with sub classes of ImageView #44

Open
dylanmcreynolds opened this issue Dec 18, 2019 · 5 comments
Open
Assignees

Comments

@dylanmcreynolds
Copy link
Contributor

This was reported by Peter Ercias. The xi-cam.NCEM plugin cannot instatiate because of the stream kwarg introduced in cea06d1.

When the DunImageView instantiates, it complains that stream is unexpected.

File "c:\users\supervisor\utilities\xicam2\xi-cam.gui\xicam\gui\widgets\tabview.py", line 89, in dataChanged
    newwidget = self.widgetcls(itemdata, stream=self.stream, field=self.field, **self.kwargs)
   File "c:\users\supervisor\utilities\xicam2\xi-cam.ncem\xicam\NCEM\widgets\NCEMViewerPlugin.py", line 22, in __init__
    super(NCEMViewerPlugin, self).__init__(**kwargs)
   File "c:\users\supervisor\utilities\xicam2\xi-cam.gui\xicam\gui\widgets\dynimageview.py", line 7, in __init__
    super(DynImageView, self).__init__(*args, **kwargs)
 TypeError: __init__() got an unexpected keyword argument 'stream'
@ercius
Copy link

ercius commented Dec 18, 2019

A similar problem occurred with the metadataviewer. @ronpandolfi fixed it by removing the excludedkeys keyword before the super() call.

https://github.com/synchrotrons/Xi-cam.gui/blob/4274cffb277774b01608bbf82f7be1d2f4c3a255/xicam/gui/widgets/metadataview.py#L39

@ercius
Copy link

ercius commented Dec 18, 2019

My solution is to change the dynimageviewer.__init__ as follows:

class DynImageView(ImageView):
    def __init__(self, *args, **kwargs):
        if 'stream' in kwargs:
            del kwargs['stream']
        super(DynImageView, self).__init__(*args, **kwargs) 

The plugin continues to work in this case

@ihumphrey
Copy link
Contributor

That should work for NCEMViewerPlugin, but it I'm not sure it will work for FFTViewerPlugin and FourDImageView, since they don't inherit DynImageView. I'm thinking about what we can do about this (see notes that will follow).

@ihumphrey
Copy link
Contributor

Notes:

When instantiating the widgetcls passed to TabView, we pass the itemdata (which can be a NonDBHeader or BlueskyRun), a stream, a field, and any additional kwargs passed into TabView:

# TabView constructor for references
class TabView(QTabWidget):
    def __init__(
            self,
            catalogmodel: QStandardItemModel = None,
            selectionmodel: QItemSelectionModel = None,
            widgetcls=None,
            stream=None,
            field=None,
            bindings: List[tuple] = [],
            **kwargs,
    ):
# ...
# tabview.py:89
newwidget = self.widgetcls(itemdata, stream=self.stream, field=self.field, **self.kwargs)

If the widgetcls does not capture stream (or field) in its constructor, these will be passed as kwargs in the inheritance change. This can (as @ercius wrote above) cause a base class to fail to initialize due to unexpected arguments.

Here are some design questions I have regarding this:

  • Should any widgetcls passed into TabView be expected to handle stream and field (i.e. have stream and field as arguments in its __init__)?
  • Should a widgetcls inspect its kwargs and remove stream and/or field if they won't be used?
  • For widgetcls types that use mixins, how should the mixins handle stream and field if no mixin uses them (as an example, SAXSGUIPlugin passes a SAXSCalibrationViewer to a TabView)?
Example of this issue occurring for `NCEMViewerPlugin`, `FFTViewerPlugin`, `FourDImageView`.
class NCEMPlugin(GUIPlugin):
    # ....
    def __init__(self):
        # ....
        self.rawview = TabView(self.headermodel, self.selectionmodel, widgets.NCEMViewerPlugin, 'primary')
        self.fftview = TabView(self.headermodel, self.selectionmodel, widgets.FFTViewerPlugin, 'primary')
        self.fourDview = TabView(self.headermodel, self.selectionmodel, widgets.FourDImageView, 'primary')
class NCEMViewerPlugin(DynImageView, QWidgetPlugin):
    def __init__(self, header: NonDBHeader = None, field: str = 'primary', toolbar: QToolBar = None, *args, **kwargs):
    # ...
    super(NCEMViewerPlugin, self).__init__(**kwargs)
class DynImageView(ImageView):
    def __init__(self, *args, **kwargs):
        super(DynImageView, self).__init__(*args, **kwargs)
class ImageView(QtGui.QtWidget):
    def __init__(self, parent=None, name="ImageView", view=None, imageItem=None, *args):
class QWidgetPlugin(QWidget, IPlugin):
    isSingleton = False
    # no __init__, implicit QWidget construction

tacaswell pushed a commit to tacaswell/Xi-cam.gui that referenced this issue Dec 20, 2019
Use Splitter to make layout interactively configurable.
@ercius
Copy link

ercius commented Feb 13, 2020

For now Im going to fix this in my plugins by adding stream = 'primary' to the init class of the plugin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants