Hi --

I found another issue with MacroCommand -- the __init__ method doesn't call super(). The super() call is necessary to ensure that the Notifier.__init__ method is called, which provides the MacroCommand instance with a reference to the Facade.

A simple super(MacroCommand, self).__init__() should do the trick.

Thanks,
--Chris

Hi Toby --

Thanks for getting back to me.

I don't think it's a problem that subclasses of Facade have to override getInstance() -- it's convenient that they don't necessarily have to, but if it's more in line with the PureMVC standard, there's nothing wrong with forcing the override.

However, as I noted previously, the current implementation also allows for multiple instances of Facade, which is definitely a problem. The fix I mentioned will take care of it -- as it happens, it means we don't have to override getInstance() which I just take as a bonus.

Thoughts?

--Chris

Okay,

I discovered that my suggestion above didn't completely fix the problems. Unfortunately, several of the base classes provided by the PureMVC pattern directly reference the Facade base class, which prevents them from properly finding a derived ApplicationFacade() class for the custom application.

I think it boils down to two problematic methods: Facade.getInstance and Facade.__new__.

The problem with Facade.getInstance is that it's a staticmethod instead of a classmethod, so it creates an instance of Facade, instead of an instance of ApplicationFacade when the class that is actually being called from is ApplicationFacade.

The problem with Facade.__new__ is that it *does* use the cls argument for storing the singleton instance (cls.instance). This means that when an ApplicationFacade is created, it is stored at ApplicationFacade.instance, and when a Facade is created, it is stored at Facade.instance. Since several classes in the framework must reference the Facade class directly (they don't and can't know anything about a custom ApplicationFacade), the framework classes get the Facade instance, while custom usage gets the ApplicationFacade instance.

To fix this problem, here's what I suggest something like this on the Facade class:

:

class Facade(object, puremvc.interfaces.IFacade):
    #...

    def __new__(cls, *args, **kwargs):
        if not Facade.instance:
            Facade.instance = object.__new__(cls, *args, **kwargs)
            Facade.instance.initializeFacade()
        return Facade.instance

    @classmethod
    def getInstance(cls):
        if Facade.instance:
            return Facade.instance

        if cls is Facade:
            raise NotImplementedError('Cannot create instances of Facade directly.')

        return cls()

    #...

This approach requires that the application call ApplicationFacade.getInstance() prior to any other part of the framework calling Facade.getInstance(), but ensures that Facade.instance will always be of type ApplicationFacade. It also frees the user from having to override either of these methods.

What do you think?

Just to avoid any confusion, this problem is with the Python port, not the PHP port.

In the example I posted below, you don't actually have to pass in the class name -- that's done by Python for you, similar to the self variable on normal methods.

As for multiple calls, the way that the __new__ method is implemented, it will only return the existing instance, so you won't ever get multiple instances. (Which protects the Singleton assumption.)

Hi Cliff --

Thanks for the response -- just FYI, this is for the Python port, not the PHP port.

Is there anything more I should do to get this fixed?

Facade.getInstance currently has the following implementation (I have removed the docstring):

    @staticmethod
    def getInstance():
        return Facade()

This implementation requires that any subclass of Facade override the getInstance() method to make sure that the subclass is properly instantiated -- since Facade is designed to be subclassed, getInstance is effectively unusable.

Here's an alternative implementation:

    @classmethod
    def getInstance(cls):
        return cls()

This implementation removes the need for subclasses to override getInstance(), since cls will be the derived class, and the __new__ method will be called properly for both the Facade class and subclass.

Thoughts?

The MacroCommand.execute() method does not execute it's subCommands in FIFO order. Instead it executes them in LIFO order, due to the self.subCommands.pop() call. According to Python's documentation:

>>> help(list.pop)
Help on method_descriptor:

pop(...)
    L.pop([index]) -> item -- remove and return item at index (default last)

Changing the .pop() to .pop(0) causes the commands to be removed from the front of the list, providing FIFO ordering.

----

As a side question, I ran into this bug fairly early (In fact, I was writing a StartupCommand for my new application) -- how is it that other people haven't run into this problem?