MoMLParser depends on ptolemy.actor but it doesn't have to

[cxbrooks]

Note: the issue was created automatically with bugzilla2github tool

Original bug ID: BZ#210 From: Thomas Huining Feng <huining.feng@gmail.com> Reported version: 7.1.devel CC: pt-dev@chess.eecs.berkeley.edu

[cxbrooks]

MoMLParser imports the following classes from ptolemy.actor:

ptolemy.actor.CompositeActor; ptolemy.actor.IOPort; ptolemy.actor.parameters.ParameterPort; ptolemy.actor.parameters.PortParameter; ptolemy.actor.parameters.SharedParameter;

To me, none of these imports is necessary if the design is right. For example:

1) CompositeActor is imported just to check whether a MoML string tries to add a director into an actor other than a composite actor. But if this is not allowed, then I think an exception should be thrown by the actor when the director is added, and there is no need for MoMLParser to check it.

2) IOPort is imported to handle the input and output properties in the MoML string that looks like <property "input"/> and <property "output"/>. We may want to add other port-specific properties in the future. So why not have the ports handle those properties?

3) ParameterPort and PortParameter should be added and removed together. I think they should override setContainer() to implement that functionality, but not require MoMLParser to handle it. There are other actors and attributes that do similar things, and we can't modify MoMLParser for each of those.

4) SharedParameter is imported so that MoMLParser adds such parameters to the set of validated parameters (whatever that means). But it seems we could define an interface for things that need validation, and have SharedParameter implement that interface.

[cxbrooks]

Disconnecting MoML from Actor is an interesting idea, but I'm not sure what it would buy us.

Other classes in the moml package depend on actor. Right now, the dependencies of moml are:

com.microstar.xml ptolemy.actor ptolemy.data ptolemy.kernel ptolemy.util

I'm all for decoupling code, but I'd like to know what we gain here.

One issue that has come up is using a parser other than com.microstar.xml.

Edward writes:

This is a good observation... Some comments:

bugzilla-daemon@ andrews.EECS.Berkeley.EDU wrote:

https://chess.eecs.berkeley.edu/bugzilla/show_bug.cgi?id=210

       Summary: MoMLParser depends on ptolemy.actor but it doesn't have
                to
       Product: Ptolemy II
       Version: 7.1.devel
      Platform: PC
    OS/Version: All
        Status: NEW
      Severity: enhancement
      Priority: P5
     Component: actors
    AssignedTo: cxh@ eecs.berkeley.edu
    ReportedBy: tfeng@ eecs.berkeley.edu
            CC: ptdevel@ chess.eecs.berkeley.edu

Estimated Hours: 0.0

MoMLParser imports the following classes from ptolemy.actor:

ptolemy.actor.CompositeActor; ptolemy.actor.IOPort; ptolemy.actor.parameters.ParameterPort; ptolemy.actor.parameters.PortParameter; ptolemy.actor.parameters.SharedParameter;

To me, none of these imports is necessary if the design is right. For example:

1) CompositeActor is imported just to check whether a MoML string tries to add a director into an actor other than a composite actor. But if this is not allowed, then I think an exception should be thrown by the actor when the director is added, and there is no need for MoMLParser to check it.

Agreed.

2) IOPort is imported to handle the input and output properties in the MoML string that looks like <property "input"/> and <property "output"/>. We may want to add other port-specific properties in the future. So why not have the ports handle those properties?

The problem is that these properties don't specify a class, so what should the MoMLParser do? About all it knows how to do is instantiate classes and set their container. I guess it could, perhaps, have some default property class, but then IOPort would need to discard the class once it had set it's input/output properties. This could be made to work...

3) ParameterPort and PortParameter should be added and removed together. I think they should override setContainer() to implement that functionality, but not require MoMLParser to handle it. There are other actors and attributes that do similar things, and we can't modify MoMLParser for each of those.

This is a good idea... Could be slightly tricky to implement, however... Have to be able to start with either a ParameterPort or a PortParameter, and make sure the other appears (or disappears, if it's being deleted). This is totally doable.

4) SharedParameter is imported so that MoMLParser adds such parameters to the set of validated parameters (whatever that means). But it seems we could define an interface for things that need validation, and have SharedParameter implement that interface.

The real issue here is a complexity issue. See the comment:

        // As an optimization, if there are multiple instances of
        // SharedParameter in the list that are shared, we only
        // validate the first of these. This prevents a square-law
        // increase in complexity, because each validation of an
        // instance of SharedParameter causes validation of all
        // its shared instances. EAL 9/10/06.

It would be important that any implementation avoid this square law complexity growth.

Edward