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

Revision history for this message
Raoul Snyman (raoul-snyman) wrote :

Please don't use this, it throws Eric4 out (and is incorrect Python):

  assert 0, 'This fn needs to be defined by the plugin'

Rather use:

  raise NotImplementedError(u'This function needs to be defined by the plugin')

Rather return None, and do a check on the other side... "image is not None":

264 + except:
265 + log.info("Can't generate video preview for some reason");
266 + import sys
267 + print sys.exc_info()
268 + return QtGui.QImage()

This should be in a docstring:
43 # self.ListViewWithDnD_class - there is a base list class with DnD assigned to it (openlp.core.lib.BaseListWithDnD())
44 # each plugin needs to inherit a class from this and pass that *class* (not an instance) to here
45 # via the ListViewWithDnD_class member
46 + # self.ServiceItemIconName - string referring to an icon file or a resource icon
47 + # self.PreviewFunction - a function which returns a QImage to represent the item (a preview usually) - no scaling required - that's done later
48 + # If this fn is not defined, a default will be used (treat the filename as an image)
49 +
50 # The assumption is that given that at least two plugins are of the form
51 # "text with an icon" then all this will help
52 # even for plugins of another sort, the setup of the right-click menu, common toolbar

review: Needs Fixing

« Back to merge proposal