Merge lp://qastaging/~joni-noplu/moovida/moovida-dvb into lp://qastaging/moovida

Hi Jonathan,
Support for DVB looks like an amazing feature (I recall it being discussed extensively but resources were never allocated for implementation).
I haven't tested your code, but I have a bunch of important remarks:

1) In case you didn't know, Moovida 1.x is not supported any longer by Fluendo Embedded (see http://www.moovida.com/blog/2010/03/26/moovida-update/ and an interesting thread on the developers' mailing list: http://lists.moovida.com/pipermail/developers-list/2010-March/001804.html). Basically that means that your best bet to get your work reviewed is to count on the community. However a large part of said community is now focused on the fork (temporarily named Elisa until a better name is agreed upon: https://launchpad.net/elisa), and the ongoing efforts are focused on:
  a) Fixing the outstanding issues in the 1.0 branch to keep it usable (no new features)
  b) Brainstorming and designing for the next version

2) Your patch mixes new features (the new gdvbd plugin) and unrelated fixes, I'd advise to submit each as separate merge requests, it will ease the life of potential reviewers.

3) For the reasons detailed in 1, you probably want to submit bug fixes against lp:elisa, you'll stand a better chance to have them reviewed and merged.

4) The fix for the amp plugin is already merged in lp:elisa, see http://bazaar.launchpad.net/~elisa-developers/elisa/1.0/revision/1616

5) A fix for bug #379409 (CSS values with no units) is already pending a review, see https://code.launchpad.net/~osomon/elisa/css_property_no_unit/+merge/26369

6) There is a bug report for the issue with teletext (bug #415031). It is in general a good practice to link to existing bug reports in the merge request. Again, this will make the life of the reviewer(s) much easier.

7) Your branch is missing the setup.py script for the new gdvbd plugin. If you haven't done so yet, I encourage you to go through the good practices detailed in the developers' guide at http://www.moovida.com/documentation/developers_guide/developers_guide.html

8) The plugin shouldn't be merged in elisa/moovida's tree: it is preferable to create a standalone project in Launchpad to host its code, track its issues separately and release it in an independent manner. This project should be declared as part of the Moovida Universe meta-project (https://launchpad.net/moovida-universe).

9) The change to the LIRC plugin seems unrelated, and in any case should be carefully explained, because it touches a sensitive piece of code.

Thanks for reading patiently my remarks! Some of them might sound picky, but please know that your effort is valued, as a former core developer on the project it is very rewarding to see such cool contributions coming in. If you're interested in the fork, drop by on irc://freenode.net/#elisa, there's always someone hanging around there.

Hi Oliver,

thanks for the reply.

1) I read that Fluendo switch to Banshee for Moovida 2.0, and searched
for a fork but didn't found it. Maybe you could announce it anywhere?

2) The fixes are unrelated agreed, but i couldn't write the plugin on
Ubuntu 10.04 with tons of exceptions in the output. Since i didn't know
u already have a fork with some bugs fixed i thought it could help.

3) true

4/5) true too, thanks for fixing it, your code for the css problem
running well here, should i make a comment about that in your merge
proposal?

6) see 8

7) I added a setup.py script, thanks for the hint.

8) Ok about that, i did only the plugin as a separate project
https://launchpad.net/elisa-gdvbd
The Problem is that using the plugin without the changes i made on the
video_player.py (teletext issue and deinterlacing support) isn't funny
(at least here in Germany where we have DVB streams interlaced and with
teletext). For the teletext issue i can publish a patch and relate it to
the bug, and for deinterlacing support i write a new bug report, is that
the correct solution?

9) Strange none else got issues with the lirc plugin (at least i found
no bug report). On my machine its not working without that change, maybe
its because i am using inputlirc
( http://www.realh.co.uk/dvbhowto/ar01s08.html ) instead of lirc, I'll
try to track the problem and write a bug report.

I'm pretty new to launchpad and bzr and i din't fully understand the way
development is working there, so thanks again for the help.

greets

joni

On Sun, 2010-05-30 at 17:13 +0000, Olivier Tilloy wrote:
> Hi Jonathan,
> Support for DVB looks like an amazing feature (I recall it being discussed extensively but resources were never allocated for implementation).
> I haven't tested your code, but I have a bunch of important remarks:
>
> 1) In case you didn't know, Moovida 1.x is not supported any longer by Fluendo Embedded (see http://www.moovida.com/blog/2010/03/26/moovida-update/ and an interesting thread on the developers' mailing list: http://lists.moovida.com/pipermail/developers-list/2010-March/001804.html). Basically that means that your best bet to get your work reviewed is to count on the community. However a large part of said community is now focused on the fork (temporarily named Elisa until a better name is agreed upon: https://launchpad.net/elisa), and the ongoing efforts are focused on:
> a) Fixing the outstanding issues in the 1.0 branch to keep it usable (no new features)
> b) Brainstorming and designing for the next version
>
> 2) Your patch mixes new features (the new gdvbd plugin) and unrelated fixes, I'd advise to submit each as separate merge requests, it will ease the life of potential reviewers.
>
> 3) For the reasons detailed in 1, you probably want to submit bug fixes against lp:elisa, you'll stand a better chance to have them reviewed and merged.
>
> 4) The fix for the amp plugin is already merged in lp:elisa, see http://bazaar.launchpad.net/~elisa-developers/elisa/1.0/revision/1616
>
> 5) A fix for bug #379409 (CSS values with no units) is already pending a review, see https://code.launchpad.net/~osomon/elisa/css_property_no_unit/+merge/26369
>
> 6)...

Another short question: Should i delete this proposal?

On Sun, 2010-05-30 at 17:13 +0000, Olivier Tilloy wrote:
> Hi Jonathan,
> Support for DVB looks like an amazing feature (I recall it being discussed extensively but resources were never allocated for implementation).
> I haven't tested your code, but I have a bunch of important remarks:
>
> 1) In case you didn't know, Moovida 1.x is not supported any longer by Fluendo Embedded (see http://www.moovida.com/blog/2010/03/26/moovida-update/ and an interesting thread on the developers' mailing list: http://lists.moovida.com/pipermail/developers-list/2010-March/001804.html). Basically that means that your best bet to get your work reviewed is to count on the community. However a large part of said community is now focused on the fork (temporarily named Elisa until a better name is agreed upon: https://launchpad.net/elisa), and the ongoing efforts are focused on:
> a) Fixing the outstanding issues in the 1.0 branch to keep it usable (no new features)
> b) Brainstorming and designing for the next version
>
> 2) Your patch mixes new features (the new gdvbd plugin) and unrelated fixes, I'd advise to submit each as separate merge requests, it will ease the life of potential reviewers.
>
> 3) For the reasons detailed in 1, you probably want to submit bug fixes against lp:elisa, you'll stand a better chance to have them reviewed and merged.
>
> 4) The fix for the amp plugin is already merged in lp:elisa, see http://bazaar.launchpad.net/~elisa-developers/elisa/1.0/revision/1616
>
> 5) A fix for bug #379409 (CSS values with no units) is already pending a review, see https://code.launchpad.net/~osomon/elisa/css_property_no_unit/+merge/26369
>
> 6) There is a bug report for the issue with teletext (bug #415031). It is in general a good practice to link to existing bug reports in the merge request. Again, this will make the life of the reviewer(s) much easier.
>
> 7) Your branch is missing the setup.py script for the new gdvbd plugin. If you haven't done so yet, I encourage you to go through the good practices detailed in the developers' guide at http://www.moovida.com/documentation/developers_guide/developers_guide.html
>
> 8) The plugin shouldn't be merged in elisa/moovida's tree: it is preferable to create a standalone project in Launchpad to host its code, track its issues separately and release it in an independent manner. This project should be declared as part of the Moovida Universe meta-project (https://launchpad.net/moovida-universe).
>
> 9) The change to the LIRC plugin seems unrelated, and in any case should be carefully explained, because it touches a sensitive piece of code.
>
> Thanks for reading patiently my remarks! Some of them might sound picky, but please know that your effort is valued, as a former core developer on the project it is very rewarding to see such cool contributions coming in. If you're interested in the fork, drop by on irc://freenode.net/#elisa, there's always someone hanging around there.

> 1) I read that Fluendo switch to Banshee for Moovida 2.0, and searched
> for a fork but didn't found it. Maybe you could announce it anywhere?

The reason it hasn't been announced very visibly is that we don't have anything to show off yet, merely a forked branch with a couple of patches in it.

> 4/5) true too, thanks for fixing it, your code for the css problem
> running well here, should i make a comment about that in your merge
> proposal?

Please do!

> 8) Ok about that, i did only the plugin as a separate project
> https://launchpad.net/elisa-gdvbd
> The Problem is that using the plugin without the changes i made on the
> video_player.py (teletext issue and deinterlacing support) isn't funny
> (at least here in Germany where we have DVB streams interlaced and with
> teletext). For the teletext issue i can publish a patch and relate it to
> the bug, and for deinterlacing support i write a new bug report, is that
> the correct solution?

Yes, definitely the way to go about those issues.

> 9) Strange none else got issues with the lirc plugin (at least i found
> no bug report). On my machine its not working without that change, maybe
> its because i am using inputlirc
> ( http://www.realh.co.uk/dvbhowto/ar01s08.html ) instead of lirc, I'll
> try to track the problem and write a bug report.

Cool. The LIRC plugin is a delicate beast, usually a fix for a set of configurations is likely to break for another set of configurations, which is why a change needs a solid justification, so your investigation will be welcome.

> I'm pretty new to launchpad and bzr and i din't fully understand the way
> development is working there, so thanks again for the help.

No problem! Thanks for your contributions.

> Another short question: Should i delete this proposal?

No need to do this, it can remain here for reference.