WIP: Make Qt viewer widgets usable outside glue

[astrofrog]

I have been trying to think of the cleanest way to make Qt viewer widgets usable outside glue as if they were a normal Qt widget, or conversely, making an existing Qt widget usable in glue. There are several things I think we can do to make this easier:

• At the moment Qt data viewers have a different function signature than normal Qt widgets, because they have a non-optional session parameter. This PR currently changes session to no longer be a required positional argument but a keyword argument. Note that this preserves backward compatibility because I left it as the first argument. This is a minor change, but I think it would be cleaner this way since it means that if a widget class is used outside glue we don't have to initialize it with (None, parent).
• Currently DataViewer inherits from QMainWindow and ViewerBase. Now let's say that I have an existing QWidget class that I want to use in glue, call it MyWidget. Let's say I can't modify the code for MyWidget code. What would be useful is to then be able to create a sub-class where we add the wrapping code necessary for glue:

from somewhere import MyWidget

class MyGlueWidget(MyWidget)
    ...

However, the issue with that is that I can't re-use the stuff in DataViewer because DataViewer inherits from QMainWindow so I can't make MyGlueWidget inherit from DataViewer because MyGlueWidget already inherits from MyWidget which inherits from QWidget.

For this reason, I would like to propose that we actually take a lot of the methods/properties from DataViewer and make them into a mix-in class that can work with both QMainWindow or QWidget (and subclasses). That way, we could do:

class MyGlueWidget(MyWidget, DataViewerMixin):
    ...

and this will add all the conveniences that DataViewer adds. Of course, for now, we could still provide:

class DataViewer(QMainWindow, DataViewerMixin):
    ...

for backward-compatibility in glue. We could even call the mix-in class GlueViewerMixin which highlights that it is making it glue-compatible.

Note that of course there are still some methods the user would have to define, so something like:

class MyGlueViewer(QMainWindow, DataViewerMixin):
    LABEL = "Camembert Viewer"
    def add_data(self, data):
        ...
       return True

@ChrisBeaumont - what do you think? If you think this would make sense, I can make the changes and start writing up some docs on making QWidgets that can be used in and out of glue.

[astrofrog]

The mix-in class approach works nicely by the way, I've just tested it with a real example I was discussing at STScI with @ibusko

[astrofrog]

I've now implemented the mix-in class. With this, doing something like:

class MyGlueViewer(QMainWindow, DataViewerMixin):
    LABEL = "Camembert Viewer"
    def add_data(self, data):
        ...
       return True

as described works, out of the box.

[ChrisBeaumont]

For this reason, I would like to propose that we actually take a lot of the methods/properties from DataViewer and make them into a mix-in class that can work with both QMainWindow or QWidget

Extracting out functionality which is agnostic to widgets vs windows definitely sounds like a good idea.

IIUC the problem with the MyWidget scenario is that you can't have a class that inherits from both QMainWindow and QWidget, is that right? If so, how hard is it to to use composition rather than inheritance to plug MyWidget into glue?

class MyGlueViewer(DataViewer):
    def __init__():
        ...
        self.setCentralWidget(MyWidget())

At the moment Qt data viewers have a different function signature than normal Qt widgets, because they have a non-optional session parameter. This PR currently changes session to no longer be a required positional argument but a keyword argument

What features break if session is None?

[ChrisBeaumont]

I want to make sure that these code changes are motivated primarily by establishing the proper separation of concerns, and not by constraints due to inheritance mechanics in Qt. It sounds like data viewers implement a few classes of functionality, so maybe we should identify what those categories are and make the class hierarchy look more like that.

[astrofrog]

IIUC the problem with the MyWidget scenario is that you can't have a class that inherits from both QMainWindow and QWidget, is that right?

Yes, and that is solved by the use of the DataViewerMixin in this PR.

Composition is certainly another possibility, I hadn't thought of that.

Regarding session, the main change is simply that glue still expects it, but as a keyword argument. In the mix-in scenario, this means that if the class is not used inside glue, then session doesn't have to be passed. Of course, the widget would need to act differently if 'session' is not present.

But maybe your suggestion of composition would be cleaner because it truly isolates the custom widget inside the glue wrapper, which means no risk of clash on method names, etc. and the session issue becomes moot too because the original qwidget doesn't need to take a session argument.

So if we go with composition, we can simply close this issue. Let me test if it works in the use case I was looking at.

[astrofrog]

Yes, the composition all works fine for me. So in fact, I think we can close this PR. I plan to document this in #619 (unless this already exists - @ChrisBeaumont?)

[ChrisBeaumont]

Yes, the composition all works fine for me. So in fact, I think we can close this PR. I plan to document this in #619

Documenting the functionality that a glue viewer should implement (and/or a guide to explain how to integrate a Qt widget into Glue) definitely sounds like the first thing to solve. I imagine it could be the case that, once such a document exists, we can identify ways to decompose DataViewer into smaller, less-coupled interfaces. If so, and if that makes it easier to put things in/out of Glue, then defining some mixin classes sounds likes a good approach.

[astrofrog]

Ok, sounds good! Closing this for now, and working on the docs :)