Code review comment for lp:~joni-noplu/moovida/moovida-dvb

Revision history for this message
Jonathan Rauprich (joni-noplu) wrote :

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

6) see 8

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

8) Ok about that, i did only the plugin as a separate project
The Problem is that using the plugin without the changes i made on the (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
( ) 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.



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 and an interesting thread on the developers' mailing list: 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:, 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
> 5) A fix for bug #379409 (CSS values with no units) is already pending a review, see
> 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 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
> 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 (
> 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://, there's always someone hanging around there.

« Back to merge proposal