Code review comment for lp:~mjthompson/openlp/media_plugin

Revision history for this message
Martin Thompson (mjthompson) wrote :

On Wed, Jul 01, 2009 at 05:14:09AM -0000 or thereabouts, Tim Bentley wrote:
> Review: Needs Fixing
> Sorry but this will break the other plugins.
>
> adding self.ServiceItemIconName will be required by all plugins now
>

Should I add it to the other plugins then...?

> Also is incorrect.
> - service_item.addIcon(u':/media/media_image.png')
> 92 + service_item.addIcon(self.ServiceItemIconName)
>
> should be
> service_item.addIcon(u':/media/media_' + 'self.ShortPluginName + u'image.png')
> This does not work for bibles as the media_image for bibles is _verses instead of _bibles.

which would also fix this problem.

>
> Use of assert causes problems and rejections on a number of boxes can we have throws error instead.

OK.

Cheers,
Martin

« Back to merge proposal