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. _______________________________________________ Mailing list: https://launchpad.net/~openlp-core Post to : [email protected] Unsubscribe : https://launchpad.net/~openlp-core More help : https://help.launchpad.net/ListHelp
