openlp-core team mailing list archive

[Raoul Snyman <raoul@xxxxxxxxxxx>]

Review: Needs Fixing

Please see my comments.

Also, you need tests. http://wiki.openlp.org/Development:Unit_Tests

Diff comments:

> 
> === modified file 'openlp/core/lib/formattingtags.py'
> --- openlp/core/lib/formattingtags.py	2015-12-31 22:46:06 +0000
> +++ openlp/core/lib/formattingtags.py	2016-05-06 20:11:35 +0000
> @@ -22,10 +22,10 @@
>  """
>  Provide HTML Tag management and Formatting Tag access class
>  """
> -import json
> +import json, os

Please separate the imports. See http://wiki.openlp.org/Development:Coding_Standards#Imports

>  
> -from openlp.core.common import Settings
> -from openlp.core.lib import translate
> +from openlp.core.common import Settings, AppLocation
> +from openlp.core.lib import translate, theme, get_text_file_string

Rather import objects than entire modules. See the last section in "Imports": http://wiki.openlp.org/Development:Coding_Standards#Imports

Rather do this:

from openlp.core.lib import translate, get_text_file_string
from openlp.core.lib.theme import ThemeXML

>  
>  
>  class FormattingTags(object):
> 
> === modified file 'openlp/core/lib/theme.py'
> --- openlp/core/lib/theme.py	2015-12-31 22:46:06 +0000
> +++ openlp/core/lib/theme.py	2016-05-06 20:11:35 +0000
> @@ -318,6 +318,69 @@
>          element.appendChild(value)
>          background.appendChild(element)
>  
> +    def add_font2(self, name, color, size, override, fonttype='second', bold='False', italics='False',

Ugh, please don't call things "blah2", make them more descriptive: "add_second_font"

> +                 line_adjustment=0, xpos=0, ypos=0, width=0, height=0, outline='False', outline_color='#ffffff',
> +                 outline_pixel=2, shadow='False', shadow_color='#ffffff', shadow_pixel=5):
> +        """
> +        Add a Font.
> +
> +        :param name: The name of the font.
> +        :param color: The colour of the font.
> +        :param size: The size of the font.
> +        :param override: Whether or not to override the default positioning of the theme.
> +        :param fonttype: The type of font, ``main`` or ``footer``. Defaults to ``main``.
> +        :param bold:
> +        :param italics: The weight of then font Defaults to 50 Normal
> +        :param line_adjustment: Does the font render to italics Defaults to 0 Normal
> +        :param xpos: The X position of the text block.
> +        :param ypos: The Y position of the text block.
> +        :param width: The width of the text block.
> +        :param height: The height of the text block.
> +        :param outline: Whether or not to show an outline.
> +        :param outline_color: The colour of the outline.
> +        :param outline_pixel:  How big the Shadow is
> +        :param shadow: Whether or not to show a shadow.
> +        :param shadow_color: The colour of the shadow.
> +        :param shadow_pixel: How big the Shadow is
> +        """
> +        background = self.theme_xml.createElement('font')
> +        background.setAttribute('type', fonttype)
> +        self.theme.appendChild(background)
> +        # Create Font name element
> +        self.child_element(background, 'name', name)
> +        # Create Font color element
> +        self.child_element(background, 'color', str(color))
> +        # Create Proportion name element
> +        self.child_element(background, 'size', str(size))
> +        # Create weight name element
> +        self.child_element(background, 'bold', str(bold))
> +        # Create italics name element
> +        self.child_element(background, 'italics', str(italics))
> +        # Create indentation name element
> +        self.child_element(background, 'line_adjustment', str(line_adjustment))
> +        # Create Location element
> +        element = self.theme_xml.createElement('location')
> +        element.setAttribute('override', str(override))
> +        element.setAttribute('x', str(xpos))
> +        element.setAttribute('y', str(ypos))
> +        element.setAttribute('width', str(width))
> +        element.setAttribute('height', str(height))
> +        background.appendChild(element)
> +        # Shadow
> +        element = self.theme_xml.createElement('shadow')
> +        element.setAttribute('shadowColor', str(shadow_color))
> +        element.setAttribute('shadowSize', str(shadow_pixel))
> +        value = self.theme_xml.createTextNode(str(shadow))
> +        element.appendChild(value)
> +        background.appendChild(element)
> +        # Outline
> +        element = self.theme_xml.createElement('outline')
> +        element.setAttribute('outlineColor', str(outline_color))
> +        element.setAttribute('outlineSize', str(outline_pixel))
> +        value = self.theme_xml.createTextNode(str(outline))
> +        element.appendChild(value)
> +        background.appendChild(element)
> +
>      def add_display(self, horizontal, vertical, transition):
>          """
>          Add a Display options.
> @@ -427,6 +490,16 @@
>                      master = parent.tag
>                  if parent.tag == 'background':
>                      master = parent.tag
> +            if parent is not None:
> +                if parent.tag == 'font2':

Same thing here. "second_font"

> +                    master = parent.tag + '_' + parent.attrib['type']
> +                # set up Outline and Shadow Tags and move to font_main
> +                if parent.tag == 'display':
> +                    if element.tag.startswith('shadow') or element.tag.startswith('outline'):
> +                        self._create_attr('font_second', element.tag, element.text)
> +                    master = parent.tag
> +                if parent.tag == 'background':
> +                    master = parent.tag
>              if master:
>                  self._create_attr(master, element.tag, element.text)
>                  if element.attrib:
> @@ -533,6 +606,25 @@
>              self.font_main_shadow_color,
>              self.font_main_shadow_size
>          )
> +        self.add_font2(

Same as above.

> +            self.font_second_name,
> +            self.font_second_color,
> +            self.font_second_size,
> +            self.font_second_override, 'second',
> +            self.font_second_bold,
> +            self.font_second_italics,
> +            self.font_second_line_adjustment,
> +            self.font_second_x,
> +            self.font_second_y,
> +            self.font_second_width,
> +            self.font_second_height,
> +            self.font_second_outline,
> +            self.font_second_outline_color,
> +            self.font_second_outline_size,
> +            self.font_second_shadow,
> +            self.font_second_shadow_color,
> +            self.font_second_shadow_size
> +        )
>          self.add_font(
>              self.font_footer_name,
>              self.font_footer_color,
> 
> === modified file 'openlp/core/ui/themeform.py'
> --- openlp/core/ui/themeform.py	2016-04-17 18:57:03 +0000
> +++ openlp/core/ui/themeform.py	2016-05-06 20:11:35 +0000
> @@ -71,8 +72,11 @@
>          self.image_browse_button.clicked.connect(self.on_image_browse_button_clicked)
>          self.image_file_edit.editingFinished.connect(self.on_image_file_edit_editing_finished)
>          self.main_color_button.colorChanged.connect(self.on_main_color_changed)
> +        self.main_color_button_6.colorChanged.connect(self.on_second_color_changed)

color_button_6? Rename please.

>          self.outline_color_button.colorChanged.connect(self.on_outline_color_changed)
> +        self.outline_color_button_7.colorChanged.connect(self.on_second_outline_color_changed)

color_button_7? Rename please.

>          self.shadow_color_button.colorChanged.connect(self.on_shadow_color_changed)
> +        self.shadow_color_button_7.colorChanged.connect(self.on_second_shadow_color_changed)
>          self.outline_check_box.stateChanged.connect(self.on_outline_check_check_box_state_changed)
>          self.shadow_check_box.stateChanged.connect(self.on_shadow_check_check_box_state_changed)
>          self.footer_color_button.colorChanged.connect(self.on_footer_color_changed)
> @@ -123,6 +128,17 @@
>          self.main_area_page.registerField('shadow_color_button', self.shadow_color_button)
>          self.main_area_page.registerField('shadow_size_spin_box', self.shadow_size_spin_box)
>          self.main_area_page.registerField('footer_size_spin_box', self.footer_size_spin_box)
> +        self.second_area_page.registerField('main_size_spin_box_6', self.main_size_spin_box_6)

Rename

> +        self.second_area_page.registerField('main_color_button_6', self.main_color_button_6)
> +        self.second_area_page.registerField('line_spacing_spin_box_2', self.line_spacing_spin_box_2)

All of these

> +        self.second_area_page.registerField('outline_check_box_2', self.outline_check_box_2)
> +        self.second_area_page.registerField('outline_color_button_7', self.outline_color_button_7)
> +        self.second_area_page.registerField('outline_size_spin_box_7', self.outline_size_spin_box_7)
> +        self.second_area_page.registerField('shadow_check_box_2', self.shadow_check_box_2)
> +        self.second_area_page.registerField('main_bold_check_box_6', self.main_bold_check_box_6)
> +        self.second_area_page.registerField('main_italics_check_box_6', self.main_italics_check_box_6)
> +        self.second_area_page.registerField('shadow_color_button_7', self.shadow_color_button_7)
> +        self.second_area_page.registerField('shadow_size_spin_box_7', self.shadow_size_spin_box_7);
>          self.area_position_page.registerField('main_position_x', self.main_x_spin_box)
>          self.area_position_page.registerField('main_position_y', self.main_y_spin_box)
>          self.area_position_page.registerField('main_position_width', self.main_width_spin_box)
> @@ -337,6 +355,25 @@
>          self.setField('main_bold_check_box', self.theme.font_main_bold)
>          self.setField('main_italics_check_box', self.theme.font_main_italics)
>  
> +    def set_second_area_page_values(self):
> +        """
> +        Handle the display and state of the Main Area page.
> +        """
> +        self.second_font_combo_box.setCurrentFont(QtGui.QFont(self.theme.font_second_name))
> +
> +        self.setField('main_size_spin_box_6', self.theme.font_second_size)

Don't forget these ones

> +        self.main_color_button_6.color = self.theme.font_second_color;
> +        self.setField('line_spacing_spin_box_2', self.theme.font_second_line_adjustment)
> +        self.setField('outline_check_box_2', self.theme.font_second_outline)
> +        self.outline_color_button.color_7 = self.theme.font_second_outline_color
> +        self.setField('outline_size_spin_box_7', self.theme.font_second_outline_size)
> +        self.setField('shadow_check_box_2', self.theme.font_second_shadow)
> +        self.shadow_color_button_7.color = self.theme.font_second_shadow_color
> +        self.setField('shadow_size_spin_box_7', self.theme.font_second_shadow_size)
> +        self.setField('main_bold_check_box_6', self.theme.font_second_bold)
> +        self.setField('main_italics_check_box_6', self.theme.font_second_italics)
> +
> +
>      def set_footer_area_page_values(self):
>          """
>          Handle the display and state of the Footer Area page.
> @@ -484,6 +539,13 @@
>          self.theme.font_main_shadow_size = self.field('shadow_size_spin_box')
>          self.theme.font_main_bold = self.field('main_bold_check_box')
>          self.theme.font_main_italics = self.field('main_italics_check_box')
> +        self.theme.font_second_name = self.second_font_combo_box.currentFont().family()

And these ones

> +        self.theme.font_second_size = self.field('main_size_spin_box_6')
> +        self.theme.font_second_line_adjustment = self.field('line_spacing_spin_box_2')
> +        self.theme.font_second_outline_size = self.field('outline_size_spin_box_7')
> +        self.theme.font_second_shadow_size = self.field('shadow_size_spin_box_7')
> +        self.theme.font_second_bold = self.field('main_bold_check_box_6')
> +        self.theme.font_second_italics = self.field('main_italics_check_box_6')
>          # footer page
>          self.theme.font_footer_name = self.footer_font_combo_box.currentFont().family()
>          self.theme.font_footer_size = self.field('footer_size_spin_box')
> 
> === modified file 'openlp/core/ui/themewizard.py'
> --- openlp/core/ui/themewizard.py	2016-04-17 18:57:03 +0000
> +++ openlp/core/ui/themewizard.py	2016-05-06 20:11:35 +0000
> @@ -217,6 +217,90 @@
>          self.shadow_layout.addWidget(self.shadow_size_spin_box)
>          self.main_area_layout.addRow(self.shadow_check_box, self.shadow_layout)
>          theme_wizard.addPage(self.main_area_page)
> +        # Second Area Page

Lots of renaming down here too.

> +
> +        self.second_area_page = QtWidgets.QWizardPage()
> +        self.second_area_page.setObjectName('second_area_page')
> +        self.second_area_layout_7 = QtWidgets.QFormLayout(self.second_area_page)
> +        self.second_area_layout_7.setObjectName('second_area_layout_7')
> +        self.second_font_label = QtWidgets.QLabel(self.second_area_page)
> +        self.second_font_label.setObjectName('main_font_label_2')
> +        self.second_font_combo_box = QtWidgets.QFontComboBox(self.second_area_page)
> +        self.second_font_combo_box.setObjectName('second_font_combo_box')
> +        self.second_area_layout_7.addRow(self.second_font_label, self.second_font_combo_box)
> +        self.second_color_label = QtWidgets.QLabel(self.second_area_page)
> +        self.second_color_label.setObjectName('main_color_label_2')
> +        self.second_properties_layout = QtWidgets.QHBoxLayout()
> +        self.second_properties_layout.setObjectName('second_properties_layout')
> +        self.main_color_button_6 = ColorButton(self.second_area_page)
> +        self.main_color_button_6.setObjectName('main_color_button_6')
> +        self.second_properties_layout.addWidget(self.main_color_button_6)
> +        self.second_properties_layout.addSpacing(20)
> +        self.main_bold_check_box_6 = QtWidgets.QCheckBox(self.second_area_page)
> +        self.main_bold_check_box_6.setObjectName('main_bold_check_box_6')
> +        self.second_properties_layout.addWidget(self.main_bold_check_box_6)
> +        self.second_properties_layout.addSpacing(20)
> +        self.main_italics_check_box_6 = QtWidgets.QCheckBox(self.second_area_page)
> +        self.main_italics_check_box_6.setObjectName('main_italics_check_box_6')
> +        self.second_properties_layout.addWidget(self.main_italics_check_box_6)
> +        self.second_area_layout_7.addRow(self.second_color_label, self.second_properties_layout)
> +        self.second_size_label = QtWidgets.QLabel(self.second_area_page)
> +        self.second_size_label.setObjectName('main_size_label')
> +        self.main_size_layout_6 = QtWidgets.QHBoxLayout()
> +        self.main_size_layout_6.setObjectName('second_size_layout')
> +        self.main_size_spin_box_6 = QtWidgets.QSpinBox(self.second_area_page)
> +        self.main_size_spin_box_6.setMaximum(999)
> +        self.main_size_spin_box_6.setValue(16)
> +        self.main_size_spin_box_6.setObjectName('main_size_spin_box_6')
> +        self.main_size_layout_6.addWidget(self.main_size_spin_box_6)
> +        self.main_line_count_label_6 = QtWidgets.QLabel(self.second_area_page)
> +        self.main_line_count_label_6.setObjectName('main_line_count_label')
> +        self.main_size_layout_6.addWidget(self.main_line_count_label_6)
> +        self.second_area_layout_7.addRow(self.second_size_label, self.main_size_layout_6)
> +
> +        self.line_spacing_label_2 = QtWidgets.QLabel(self.second_area_page)
> +        self.line_spacing_label_2.setObjectName('line_spacing_label')
> +        self.line_spacing_spin_box_2 = QtWidgets.QSpinBox(self.second_area_page)
> +        self.line_spacing_spin_box_2.setMinimum(-250)
> +        self.line_spacing_spin_box_2.setMaximum(250)
> +        self.line_spacing_spin_box_2.setObjectName('line_spacing_spin_box_2')
> +        self.second_area_layout_7.addRow(self.line_spacing_label_2, self.line_spacing_spin_box_2)
> +        self.outline_check_box_2 = QtWidgets.QCheckBox(self.second_area_page)
> +        self.outline_check_box_2.setObjectName('outline_check_box_2')
> +        self.outline_layout_7 = QtWidgets.QHBoxLayout()
> +        self.outline_layout_7.setObjectName('outline_layout_7')
> +        self.outline_color_button_7 = ColorButton(self.second_area_page)
> +        self.outline_color_button_7.setEnabled(False)
> +        self.outline_color_button_7.setObjectName('outline_color_button_7')
> +        self.outline_layout_7.addWidget(self.outline_color_button_7)
> +        self.outline_layout_7.addSpacing(20)
> +        self.outline_size_label_7 = QtWidgets.QLabel(self.second_area_page)
> +        self.outline_size_label_7.setObjectName('outline_size_label')
> +        self.outline_layout_7.addWidget(self.outline_size_label_7)
> +        self.outline_size_spin_box_7 = QtWidgets.QSpinBox(self.second_area_page)
> +        self.outline_size_spin_box_7.setEnabled(False)
> +        self.outline_size_spin_box_7.setObjectName('outline_size_spin_box_7')
> +        self.outline_layout_7.addWidget(self.outline_size_spin_box_7)
> +        self.second_area_layout_7.addRow(self.outline_check_box_2, self.outline_layout_7)
> +        self.shadow_check_box_2 = QtWidgets.QCheckBox(self.second_area_page)
> +        self.shadow_check_box_2.setObjectName('shadow_check_box_2')
> +        self.shadow_layout_7 = QtWidgets.QHBoxLayout()
> +        self.shadow_layout_7.setObjectName('shadow_layout_7')
> +        self.shadow_color_button_7 = ColorButton(self.second_area_page)
> +        self.shadow_color_button_7.setEnabled(False)
> +        self.shadow_color_button_7.setObjectName('shadow_color_button_7')
> +        self.shadow_layout_7.addWidget(self.shadow_color_button_7)
> +        self.shadow_layout_7.addSpacing(20)
> +        self.shadow_size_label_7 = QtWidgets.QLabel(self.second_area_page)
> +        self.shadow_size_label_7.setObjectName('shadow_size_label')
> +        self.shadow_layout_7.addWidget(self.shadow_size_label_7)
> +        self.shadow_size_spin_box_7 = QtWidgets.QSpinBox(self.second_area_page)
> +        self.shadow_size_spin_box_7.setEnabled(False)
> +        self.shadow_size_spin_box_7.setObjectName('shadow_size_spin_box_7')
> +        self.shadow_layout_7.addWidget(self.shadow_size_spin_box_7)
> +        self.second_area_layout_7.addRow(self.shadow_check_box_2, self.shadow_layout_7)
> +        theme_wizard.addPage(self.second_area_page)
> +
>          # Footer Area Page
>          self.footer_area_page = QtWidgets.QWizardPage()
>          self.footer_area_page.setObjectName('footer_area_page')
> @@ -365,8 +449,12 @@
>          self.background_combo_box.currentIndexChanged.connect(self.background_stack.setCurrentIndex)
>          self.outline_check_box.toggled.connect(self.outline_color_button.setEnabled)
>          self.outline_check_box.toggled.connect(self.outline_size_spin_box.setEnabled)
> +        self.outline_check_box_2.toggled.connect(self.outline_color_button_7.setEnabled)

Don't forget to refactor these names.

> +        self.outline_check_box_2.toggled.connect(self.outline_size_spin_box_7.setEnabled)
>          self.shadow_check_box.toggled.connect(self.shadow_color_button.setEnabled)
>          self.shadow_check_box.toggled.connect(self.shadow_size_spin_box.setEnabled)
> +        self.shadow_check_box_2.toggled.connect(self.shadow_color_button_7.setEnabled)
> +        self.shadow_check_box_2.toggled.connect(self.shadow_size_spin_box_7.setEnabled)
>          self.main_position_check_box.toggled.connect(self.main_x_spin_box.setDisabled)
>          self.main_position_check_box.toggled.connect(self.main_y_spin_box.setDisabled)
>          self.main_position_check_box.toggled.connect(self.main_width_spin_box.setDisabled)
> 
> === modified file 'openlp/plugins/songs/forms/editverseform.py'
> --- openlp/plugins/songs/forms/editverseform.py	2016-01-09 16:26:14 +0000
> +++ openlp/plugins/songs/forms/editverseform.py	2016-05-06 20:11:35 +0000
> @@ -169,3 +170,77 @@
>          if not text.startswith('---['):
>              text = '---[%s:1]---\n%s' % (VerseType.translated_names[VerseType.Verse], text)
>          return text
> +
> +    def on_format_button_clicked(self):
> +        text = self.verse_text_edit.toPlainText()
> +        position = self.verse_text_edit.textCursor().position()
> +        length = len(text)
> +        textarray = text.splitlines()
> +        textarray2 = []

No, please!

> +        counter = 0
> +        slot_counter = -1
> +        slot_counter2 = 0
> +        slot = 0
> +        line2 = ""

You're fond of this, aren't you?

> +        newtext = ""
> +        for line in textarray:
> +            counter += len(line)+1
> +            line2 = line.replace(" ","")
> +            newtext += line+'\n'
> +            if len(line2) == 0 or line2.find('[---]')>-1 or line2.find('---[')>-1 or line2.find('{/secondTheme}')>-1:
> +                if counter >= position:
> +                    if slot_counter == -1:
> +                        slot_counter = slot
> +                textarray2.append(newtext)
> +                newtext = ""
> +                slot += 1
> +        textarray2.append(newtext)
> +        new = textarray2[slot_counter]

Decent variable names are not your strong point.

> +        slot_counter2 = slot_counter +1

We really need to get you out of this habit.

> +        match =''.join(textarray2[slot_counter2:])
> +        match.replace(" ","")
> +        match.replace("\n","")
> +        if new.find("{secondTheme}")>-1:

You need spaces. http://wiki.openlp.org/Development:Coding_Standards#Whitespace

> +            if new.find("{/secondTheme}")>-1:
> +                new = new.replace("{/secondTheme}\n","\n")
> +                new = new.replace("{secondTheme}","")
> +                new = new.replace("{/secondTheme}","")
> +        else:
> +            if new.endswith('[---]\n'):
> +                new = new.replace('[---]',"")
> +                if new.find('---[')>-1:
> +                    temp = "{secondTheme}" + new[:new.find('---[')] + "{/secondTheme}" + new[new.find('---['):]
> +                else:
> +                    temp = "{secondTheme}" + new + "{/secondTheme}"
> +                temp = temp.replace("\n\n{/secondTheme}","{/secondTheme}\n[---]\n")
> +                new = temp
> +            else:
> +                if new.startswith('[---]'):
> +                        temp = "[---]" + "{secondTheme}" + new + "{/secondTheme}"
> +                        temp = temp.replace("\n\n{/secondTheme}","{/secondTheme}\n\n")
> +                        new = temp
> +                else:
> +                    if match == '':
> +                        if new.find('---[')>-1:
> +                            temp = "{secondTheme}" + new[:new.find('---[')] + "{/secondTheme}\n" + new[new.find('---['):]
> +                        else:
> +                            temp = "{secondTheme}" + new + "{/secondTheme}"
> +                        temp = temp.replace("\n\n{/secondTheme}","{/secondTheme}")
> +                        temp = temp.replace("\n{/secondTheme}","{/secondTheme}")
> +                        temp = temp.replace("\n\n---[","\n---[")
> +                        new = temp
> +                    else:
> +                        if new.find('---[')>-1:
> +                            temp = "{secondTheme}" + new[:new.find('---[')] + "{/secondTheme}\n" + new[new.find('---['):]
> +                        else:
> +                            temp = "{secondTheme}" + new + "{/secondTheme}"
> +                        temp = temp.replace("\n\n{/secondTheme}","{/secondTheme}\n\n")
> +                        temp = temp.replace("\n{/secondTheme}","{/secondTheme}\n")
> +                        temp = temp.replace("\n\n---[","\n---[")
> +                        new = temp
> +        textarray2[slot_counter] = new
> +        self.verse_text_edit.setPlainText(''.join(textarray2))
> +        textCursor = self.verse_text_edit.textCursor()

Please read our coding standards carefully. http://wiki.openlp.org/Development:Coding_Standards#Naming_Conventions

> +        textCursor.setPosition(position)
> +        self.verse_text_edit.setTextCursor(textCursor)
> +        self.verse_text_edit.setFocus()
> 
> === modified file 'resources/forms/themewizard.ui'
> --- resources/forms/themewizard.ui	2010-12-27 21:53:02 +0000
> +++ resources/forms/themewizard.ui	2016-05-06 20:11:35 +0000
> @@ -1229,6 +1719,7 @@
>   </widget>
>   <resources>
>    <include location="../images/openlp-2.qrc"/>
> +  <include location="../../../../../.designer/images/openlp-2.qrc"/>

Please don't change things like this

>   </resources>
>   <connections>
>    <connection>


-- 
https://code.launchpad.net/~openlp-w/openlp/secondTheme/+merge/294047
Your team OpenLP Core is subscribed to branch lp:openlp.