Code review comment for lp:~tdelaet/entertainer/fixes-for-future-branch

Revision history for this message
Matt Layman (mblayman) wrote :

Thomas,

Thanks very much for the branch. Just a few notes:

 1. I guess we don't explicitly state it anywhere, but the pyclutter-gst that we are using is the one that is packaged by Francesco Marella in his PPA (if I could figure out how to copy his packages to the Entertainer PPA, I would do that). That being said, we're a little behind on which version of pyclutter-gst is in use so your branch is a little ahead of the curve. After Ubuntu Lucid Lynx is released, I will probably try to update our pyclutter-gst and gtk, then your code will be very valuable.
 2. I'm about to abandon the future branch. It's an added level of complexity that the Entertainer team thought we would use, but it was a branch that lived on too long to suit the purpose we wanted. Rest assured that trunk is now up-to-date with all of future's code. Therefore, I would request that after you make some changes (which I'm about to mention), please submit for merging into trunk.
 3. The Entertainer developers decided to use copyright assignment so that we could do something in the future like change the license (to something like GPLv3). Because of that, in order for us to accept your code, you'd have to be okay assigning the copyright to Entertainer Developers. To do that, simply add yourself, alphabetically by first name, to the list of contributors at the bottom of the docs/COPYING file.
 4. Finally, a code review comment... You changed the method name to get_pipeline, which is the right thing to do for the API change, but you left the variable as self.playbin. I think this would lead to confusion in the future, and I would request that you change every instance of the self.playbin variable to self.pipeline.

Once these changes are made, I'll be more than happy to merge the branch (though I may not do it until some time after Lucid is released when I actually need the code).

Thanks!
Matt

review: Needs Fixing

« Back to merge proposal