openlp-core team mailing list archive

[Raoul Snyman <raoul.snyman@xxxxxxxxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing
23 from renderer import Renderer
24 from openlp.core.lib import ThemeLevel
25 (deleted)
26 log = logging.getLogger(__name__)

Please don't delete that open line.


94 def updateCapability(self, capability, State=False):
95     self.capability_state[capability] = State

Please use a lowercase "s" for "state".


94 def updateCapability(self, capability, State=False):

97 def getCapability(self, capability):

ServiceItem is not a Qt object, please rename these methods.


68 capabilities = [
69     u'allows_preview',
70     u'allows_edit',
71     u'allows_maintain',
72     u'requires_media'
73 ]

Rather make this an "enumeration". It is more accurate and less prone to typing mistakes.

class ItemCapabilities(object):
    AllowsPreview = 1
    AllowsEdit = 2
    AllowsMaintain = 3
    RequiresMedia = 4

This also makes your life easier, because you don't need to maintain a dict, you can maintain a simple list, and if that capability is not in the list, that service item doesn't have that capability - once again lessening the probability of bugs being introduced.

A simple test would be:

  if ItemCapabilities.AllowsEdit in self.capabilities:
      # do something


+ self.config.set_config(u'monitor display', self.MonitorDisplay)

What is "monitor display" and self.MonitorDisplay? If it's a boolean, rather call it "display monitor" and self.DisplayMonitor, as these make more sense. "monitor display" sounds like it could be which monitor to put the display screen on.


Please also just watch your indentation... line wraps should be indented 4 spaces.
-- 
https://code.launchpad.net/~trb143/openlp/bugs1/+merge/22722
Your team OpenLP Core is subscribed to branch lp:openlp.