Request for advice on direction of refactoring the Python API
#1
As many know I've been refactoring the Python Addon code to allow the implementation of addons in multiple programming languages. I've run into an issue I could use some feedback on ... but the explanation is somewhat involved.

In short, the existing Python Addon interface is built around some issues in the structure of the core windowing hierarchy (found in xbmc/guilib and xbmc/xbmc). To compensate for these the Python addon system developer has done a few things I'd like to undo.

To be a little more specific, the Python classes are implemented using a dual hierarchy: one that participates and extends the core windowing hierarchy (I'll call this the Python GUIWindow extentions), and one associated set of "classes" that are presented to Python as the (majority of) the xbmcgui module (the Python API classes). This solution got the job done but, given the SWIG code generator and the fact that I'm trying to implement a programming-language-neutral/native API, I've tried to simplify and reduce the amount of code that needs to be maintained going forward.

Though I made about 3 false starts (I'd call them "refactors" but that would probably be to generous) I think I've come up with a pretty nice solution that significantly reduces the number of classes and the amount of code yet gets the job done .... but only to a certain point.

If you've followed me so far then please read on; I need to get into more details.

The Python GUIWindow extentions extend the main window hierarchy at a two points: CGUIWindow and CGUIMediaWindow, while the main Python hierarchy is a single uniform hierarchy.

Code:
Python API                    Partial       CGUIWindow
                                       Core            |
            Window                    Window     ------^------+---------------+-------
               |                     Hierarchy         |      |               |
       +-------^------+                                |      v               v
       |              |                                |  CGUIDialog   CGUIMediaWindow
   WindowXML      WindowDialog      ___________________|______________________|_________
       |                                               |                      |
WindowXMLDialog                      Python           v                      v
                                       GUI       CGUIPythonWindow      CGUIPythonWindowXML
                                   Extentions          |                      |
                                                       v                      v
                                              CGUIPythonWindowDialog     CGUIPythonWindowXMLDialog

Some things should be obvious from this. First, the Python API (and also the core API) is discriminated in multiple orthogonal directions. That is, the class discriminator for the root "window" class (that is, what makes subclasses of a parent classes distinct) is not the same for all of the children. In the Python GUI Extention there are two discriminators: Dialog/Non-Dialog and XML-Window/non-XML-Window. A selection was made that the primary discrimination would be XML/non-XML (Note, these two classes are at the top of the Python GUI Extentions) and secondarily we would discriminate by Dialog/non-Dialog.

This selection is the cause of the specialized message hack required in ApplicationMessager which is accompanied by the comment (though the alternate selection would have been worse):

Code:
// TODO: This is ugly - really these python dialogs should just be normal XBMC dialogs

Ideally we would have a diamond in the hierarchy to handle this: E.g.: a CGUIPythonWindowXMLDialog is both a CGUIPythonWindow and a CGUIPythonWindowDialog. However, this requires some accounting for in the core windowing hierarchy.

Secondly, in trying to come up with a native API that mimics the current Python API (the "programming language neutral" API I've been working on), there's no reason to keep these as two separate heirarchies.

Solving this without multiplying classes and replicating code is the source of my several false starts. I have a solution that gets me ALMOST all the way, but not quite.

First, I have "Window" class as the root of the new native, language-neutral API who's parent is dynamic (AH! the hackery you can derrive using C++ templates!). That is, effectively (but not quite):

Code:
Window<P> : public P(extends CGUIWindow)
                                          |
                           +--------------^-------------------+
                           |                                  |
              WindowDialog:Window<CGUIWindow>     WindowXML:Window<CGUIMediaWindow>

This seems to work but I'm not sure how to do the last step. Ideally I want WindowXMLDialog to inherit from WindowXML but have TWO parent classes. CGUIMediaWindow and CGUIDialog.

So ... I'm stuck. I can hack in a WindowXMLDialog that inherits from WindowXML:Window<CGUIMediaWindow> and then make it act like a dialog but that feels too much like a hacked workaround.

I think the correct fix is to make CGUIWindow a virtual base class of both CGUIDialog and CGUIMediaWindow. This would allow me to redo the hierarchy completely and in a more intuitive manner:

Code:
CGUIWindow
                             |
     CGUIDialog            Window
       |    |                |                   CGUIMediaWindow
       |    |                |                         |
       |    |   -------------^-----------+  +----------+
       |    v   |                        |  |
       |  WindowDialog                 WindowXML
       +------------------------------+    |
                                      v    v
                                     WindowXMLDialog

The problem with this solution is that there is way too much downcasting from CGUIWindow through CGUIDialog (downcasting is not allowed with virtual base classes so another means would be required to get a CGUIDialog* from a CGUIWindow* (like adding a virtual AsDialog method to CGUIWindow)), plus the initialization of a virtual base class needs to be accounted for in the child classes of the classes that declare the base class as virtual. Both these issues have easy resolution but do far I've been able to isolate my changes to additions, this would create a large number of diffs in xbmc/xbmc and xbmc/guilibs.

So ... after all that ... any suggestions?

Jim
Reply
#2
First off, WindowXMLDialog should not (IMO) be derived from GUIMediaWindow at all. It's not the same thing, nor should it be.

Secondly, I think it might be of benefit to check exactly what is different between GUIDialog and GUIWindow - effectively all a dialog is is a window that is shown on top of other windows with or without focus. One option may be to push the dialog-isation stuff into GUIWindow directly and throw GUIDialog away.

How much would that simplify things?
Always read the XBMC online-manual, FAQ and search the forum before posting.
Do not e-mail XBMC-Team members directly asking for support. Read/follow the forum rules.
For troubleshooting and bug reporting please make sure you read this first.


Image
Reply
#3
jmarshall Wrote:First off, WindowXMLDialog should not (IMO) be derived from GUIMediaWindow at all. It's not the same thing, nor should it be.

Interesting. Should WindowXML be inherited from CGUIMediaWindow? Currently WindowXML in Python works in conjunction with CGUIPythonWndowXML which inherits from CGUIMediaWindow. Collapsing these into one hierarchy would make WindowXML inherit from CGUIMediaWindow.

By extension, since (if?) WindowXMLDialog inherits from WindowXML (like it does as presented to Python, as well as in the parallel hierarchy CGUIPythonWindowXMLDialog inherits from CGUIPythonWindowXML), then they are related indirectly.

I've been trying to get the API into a form like the last diagram I had in the original post (The 4 classes beginning with Window* are the new API and the CGUI* classes are the core classes). Is that wrong?

jmarshall Wrote:Secondly, I think it might be of benefit to check exactly what is different between GUIDialog and GUIWindow - effectively all a dialog is is a window that is shown on top of other windows with or without focus. One option may be to push the dialog-isation stuff into GUIWindow directly and throw GUIDialog away.

How much would that simplify things?

I'm under the impression that would be a more dramatic change. I don't think there is a conceptual issue with leaving them distinct. It's just that when a set of subclasses have multiple discriminators there's always the possibility that lower in the hierarchy you'll want to bring them back together again. In order to do that you really need that first level to declare the base class as a virtual base class (in C++ at least).

In any case I moved ahead with inheriting WindowXMLDialog from WindowXML<CGUIMediaWindow> because it presents in Python through SWIG with the identical relationships as the current API and required no changes to the existing core classes. It might not be the best criteria but it allows me to move forward until I get more buy-in (which I hope I'll get once it's working).

You can see all of these relationships in the refactored API here:

xbmc/xbmc/lib/libPython/module/AddonWindow.h

in:

git://github.com/jimfcarroll/xbmc-multi-language-addons.git
Reply
#4
Basically GUIPythonWindowXMLDialog needs only the xml loading bits from GUIPythonWindowXML- everything else in there (all the GUIMediaWindow stuff) is not needed. I'm not sure how easy it would be to remedy this - GUIPythonWindowXML overrides some functions in GUIWindow to do the path changing shenanigans to load XML files.

The good news is that this is one big hack that doesn't really do what it advertises. Reason is that even if we change the path to load the skin XML files from, much of the rest of the information we use to load the skin (includes, colors, default controls etc.) are still taken from the current skin context (g_skinInfo). Instead, we should actually have a single skin context for the addon itself that is referenced from each GUIPythonWindowXML*. There's a patch up on trac that tackles some of this - I'm waiting until Dharma is out the door before seeing whether it can be cleaned up into something that works well - ideally we'd get rid of the global. Doing this *might* move most of the XML loading stuff down into guilib, which would then mean that GUIPythonWindowXML/Dialog then isn't really that much different than a standard media window/dialog.

Cheers,
Jonathan
Always read the XBMC online-manual, FAQ and search the forum before posting.
Do not e-mail XBMC-Team members directly asking for support. Read/follow the forum rules.
For troubleshooting and bug reporting please make sure you read this first.


Image
Reply

Logout Mark Read Team Forum Stats Members Help
Request for advice on direction of refactoring the Python API0