openlp-core team mailing list archive

[Raoul Snyman <raoul.snyman@xxxxxxxxxxxxxxxxxxxxxxxx>]

Review: Approve
Watch your spaces, and preferably name your counter something interesting:

  for i in range (0 ,  self.desktop().numScreens()):
      screens.insert(i, (i+1, self.desktop().availableGeometry(i+1)))
      log.info(u'Screen %d found with resolution %s', i+1, self.desktop().availableGeometry(i+1))

  should be:

  for screen in range(0, self.desktop().numScreens()):
      screens.insert(screen, (screen + 1, self.desktop().availableGeometry(screen + 1)))
      log.info(u'Screen %d found with resolution %s', screen + 1, self.desktop().availableGeometry(screen + 1))

XML is flexible, please don't use "color1" and "color2" as they undescriptive. If it's a single background colour, "color" will suffice, if it's a gradient, "startColor" and "endColor."

  if background_type == u'color':
      background.setAttribute(u'mode', u'opaque')
      background.setAttribute(u'type', u'color')
      color = self.theme_xml.createElement(u'color')
      # do the rest of your "color" stuff here
  elif background_type == u'gradient':
      background.setAttribute(u'mode', u'opaque')
      background.setAttribute(u'type', u'gradient')
      startColor = self.theme_xml.createElement(u'startColor')
      endColor = self.theme_xml.createElement(u'endColor')
      # do the rest of your "gradient" stuff here
  ...

Underscores are used to separate words in variables and parameters:

  def add_font(self, fontname, fontcolor, fontproportion, override, fonttype=u'main', xpos=0, ypos=0 ,width=0, height=0)

  should read:

  def add_font(self, font_name, font_color, font_proportion, override, font_type=u'main', xpos=0, ypos=0, width=0, height=0)

  however, since we're dealing with a font (as denoted by the method name "add_font"), you can drop the "font" in the parameter names (and since "type" is a reserved word, we append the _):

  def add_font(self, name, color, proportion, override, type_=u'main', xpos=0, ypos=0, width=0, height=0)

Spaces:

  bbox=self._render_lines_unaligned(lines, False, (x,y))
  => bbox = self._render_lines_unaligned(lines, False, (x, y))

  bbox=self._render_lines_unaligned(lines1, True, (x,y) )
  => bbox = self._render_lines_unaligned(lines1, True, (x, y))

  (plenty other places)
-- 
https://code.launchpad.net/~trb143/openlp/ThemeManager2/+merge/5460
Your team openlp.org Core is subscribed to branch lp:openlp.