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

setParentTimeConstraint should be part of the TimeProcess API #30

Closed
jcelerier opened this issue Feb 17, 2016 · 5 comments
Closed

setParentTimeConstraint should be part of the TimeProcess API #30

jcelerier opened this issue Feb 17, 2016 · 5 comments
Assignees
Milestone

Comments

@jcelerier
Copy link
Member

It is always required if one wants to do a remotely useful process.
TimeProcess interface should get the :

std::shared_ptr<TimeConstraint> mParent;
and
void setParentTimeConstraint(const shared_ptr<TimeConstraint>);

members since they are recreated in both JamomaTimeProcess and any other kind of process that one would like to make.

This way it wouldn't be needed to do :

JamomaTimeProcess* t = dynamic_cast<JamomaTimeProcess*>(timeProcess.get());
if (t)
{
    t->setParentTimeConstraint(shared_from_this());
}

but just

t->setParentTimeConstraint(shared_from_this());

and it would work with any kind of TimeProcess.

@jcelerier jcelerier changed the title setParentTimeConstraint should be part of the TimeConstraint API setParentTimeConstraint should be part of the TimeProcess API Feb 17, 2016
@jcelerier
Copy link
Member Author

Actually, the simplest API would be to just have std::shared_ptr<TimeConstraint> mParent; a public member, since it's equivalent to :

public:
  /*! get the parent #TimeConstraint
   \return std::shared_ptr<#TimeConstraint> */
  std::shared_ptr<TimeConstraint> getParentTimeConstraint() const
  { return mParent; }

  /*! set the parent #TimeConstraint
   \param shared_ptr<State> */
  void setParentTimeConstraint(const std::shared_ptr<TimeConstraint>& cst)
  { mParent = cst; }

private:
  std::shared_ptr<TimeConstraint> mParent;

@theod theod added this to the release/1.0 milestone Feb 17, 2016
@theod theod self-assigned this Feb 17, 2016
@theod
Copy link
Member

theod commented Feb 17, 2016

ok I get it !
I assigned it to me but if you need it don't hesitate to do it as I'll not be able to work on this this week

@jcelerier
Copy link
Member Author

I just commited it, the change was simple to do.
Also there was a small bug (that would cause crash upon play) : mOffsetState was used, but never created.

@theod
Copy link
Member

theod commented Feb 18, 2016

as i-score is building and I know understand well the change you I'm wondering to know if it is not a problem to allow to set the parent ourself.

until now this was made into TimeConstraint::addProcess and so we were sure that no absurdity can happen. now it seems possible to add a process into a constraint and then to change its parent afterward (which is a non sens). is there a way to avoid this ?

@jcelerier
Copy link
Member Author

On Thu, Feb 18, 2016 at 12:32 PM, theod [email protected] wrote:

now it seems possible to add a process into a constraint and then to
change its parent afterward (which is a non sens).

but this was already possible before because the "setParentConstraint"
method had to be implemented somewhere, in a subclass. And then somebody
could have been calling this setParentConstraint when he wanted.
The "standard"[1] way to restrict behaviour in C++ is to have a "normal
class" with all the attributes available,
and then have a "view" class (e.g. TimeProcessView ...) which doesn't have
the "set" part.

e.g.

class TimeProcessView
{
    TimeProcess& process; // This could also be a shared_ptr but I don't
think that it would be needed 90% of the time

    public:
        TimeProcessView(TimeProcess& p): // same
            process(p)
        {
        }

        auto getParentConstraint() const
        { return process.parent; }

        auto offset(const TimeValue& t)
        { return process.offset(t); }

        auto state()
        { return process.state(); }
};

and when a function / class shouldn't be able to modify a parent, it should
take / store a TimeProcessView instead of a TimeProcess.
But the point is : it is counter-productive to restrict the programmer
because if the programmer wants to break things he will be able to anyway.
The restrictions should be placed where they are meaningful, to give a
deeper semantic to the code, but maximum flexibility should be allowed at
the origin
because else it complexifies the client code for no gain.

[1]: http://www.drdobbs.com/a-view-to-a-string-part-i/184402064 and
http://www.drdobbs.com/cpp/c-view-objects/196513737

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

No branches or pull requests

2 participants