openlp-core team mailing list archive

[Tim Bentley <tim.bentley@xxxxxxxxx>]

Review: Needs Fixing

Looking good but a number of improvements required.
All merges need a test or two and something relevant to this would be good.

Diff comments:

> === modified file 'openlp/core/lib/serviceitem.py'
> --- openlp/core/lib/serviceitem.py	2016-05-05 03:57:04 +0000
> +++ openlp/core/lib/serviceitem.py	2016-05-17 21:10:14 +0000
> @@ -267,6 +267,9 @@
>                          'html': html_data.replace(' ', ' '),
>                          'verseTag': verse_tag
>                      })
> +            if hasattr(self, 'name') and ((self.name == 'songs') or (self.name == 'bibles')):

Tests like this for plugins should be avoided.
Please add a capability to add this feature and then test against that.

> +                display_at_end = "<br>" + (Settings().value(self.name + '/end character'))

the <br> will break paging as a theme has a set number of lines and if a page goes over it it will be put on a new page.  This can introduce new pages which only have the last page character.

> +                self._display_frames[(len(self._display_frames)) - 1]['html'] += display_at_end
>          elif self.service_item_type == ServiceItemType.Image or self.service_item_type == ServiceItemType.Command:
>              pass
>          else:
> 
> === modified file 'openlp/core/ui/slidecontroller.py'
> --- openlp/core/ui/slidecontroller.py	2016-05-05 03:57:04 +0000
> +++ openlp/core/ui/slidecontroller.py	2016-05-17 21:10:14 +0000
> @@ -1105,6 +1108,23 @@
>                  to_display = self.service_item.get_rendered_frame(row)
>                  if self.service_item.is_text():
>                      self.display.text(to_display, row != old_selected_row)
> +                    if (self.service_item.name == 'songs') or (self.service_item.name == 'bibles'):

Please use capabilities for this,

> +                        if Settings().value(self.service_item.name + '/info on end') == 1:

1 is a magic number, please create a class for this.
See ThemeLevel in commonn.__init__.py for an example.

> +                            if self.item_slide_count == 0:
> +                                self.display.footer(self.service_item.foot_text)
> +                            else:
> +                                self.display.footer('')
> +                        elif Settings().value(self.service_item.name + '/info on end') == 2:
> +                            if row == (len(self.service_item._display_frames)-1):
> +                                self.display.footer(self.service_item.foot_text)
> +                            else:
> +                                self.display.footer('')
> +                        else:
> +                            self.display.footer(self.service_item.foot_text)
> +                    elif self.service_item.foot_text:
> +                        self.display.footer(self.service_item.foot_text)
> +                    else:
> +                        self.display.footer('')
>                  else:
>                      if start:
>                          self.display.build_html(self.service_item, to_display)
> 
> === modified file 'openlp/plugins/bibles/lib/biblestab.py'
> --- openlp/plugins/bibles/lib/biblestab.py	2015-12-31 22:46:06 +0000
> +++ openlp/plugins/bibles/lib/biblestab.py	2016-05-17 21:10:14 +0000
> @@ -127,6 +127,25 @@
>          self.language_selection_combo_box.addItems(['', '', ''])
>          self.language_selection_layout.addWidget(self.language_selection_label)
>          self.language_selection_layout.addWidget(self.language_selection_combo_box)
> +

no need for blank lines

> +        self.end_settings_group_box = QtWidgets.QGroupBox(self.left_column)
> +        self.end_settings_group_box.setObjectName('end_settings_group_box')
> +        self.end_settings_layout = QtWidgets.QFormLayout(self.end_settings_group_box)
> +        self.end_settings_layout.setObjectName('end_settings_layout')
> +        self.end_character_label = QtWidgets.QLabel(self.end_settings_group_box)
> +        self.end_character_label.setObjectName('end_character_label')
> +        self.end_character_line_edit = QtWidgets.QLineEdit(self.end_settings_group_box)
> +        self.end_character_line_edit.setObjectName('end_character_line_edit')
> +        self.end_character_line_edit.setMaxLength(1)
> +        self.end_settings_layout.addRow(self.end_character_label, self.end_character_line_edit)
> +        self.info_on_end_label = QtWidgets.QLabel(self.end_settings_group_box)
> +        self.info_on_end_label.setObjectName('info_on_end_label')
> +        self.info_on_end_combo_box = QtWidgets.QComboBox(self.end_settings_group_box)
> +        self.info_on_end_combo_box.setObjectName('info_on_end_combo_box')
> +        self.info_on_end_combo_box.addItems(['', '', ''])
> +        self.end_settings_layout.addRow(self.info_on_end_label, self.info_on_end_combo_box)
> +
> +        self.right_layout.addWidget(self.end_settings_group_box)
>          self.right_layout.addWidget(self.language_selection_group_box)
>          self.left_layout.addStretch()
>          self.right_layout.addStretch()
> @@ -194,6 +214,12 @@
>              LanguageSelection.Application, translate('BiblesPlugin.BiblesTab', 'Application Language'))
>          self.language_selection_combo_box.setItemText(
>              LanguageSelection.English, translate('BiblesPlugin.BiblesTab', 'English'))
> +        self.end_settings_group_box.setTitle(translate('BiblesPlugin.BiblesTab', 'End slide behaviour'))
> +        self.end_character_label.setText(translate('BiblesPlugin.BiblesTab', 'Bible passage end character:'))
> +        self.info_on_end_label.setText(translate('BiblesPlugin.BiblesTab', 'Show copyright info on:'))
> +        self.info_on_end_combo_box.setItemText(0, translate('BiblesPlugin.BiblesTab', 'All slides (Recommended)'))

These text strings should be added to UiStrings and re-used across the plugins.  
Save translations

> +        self.info_on_end_combo_box.setItemText(1, translate('BiblesPlugin.BiblesTab', 'First shown slide only'))
> +        self.info_on_end_combo_box.setItemText(2, translate('BiblesPlugin.BiblesTab', 'Final slide of Bible passage only'))
>  
>      def on_bible_theme_combo_box_changed(self):
>          self.bible_theme = self.bible_theme_combo_box.currentText()


-- 
https://code.launchpad.net/~richie-the-g/openlp/sec-global-option/+merge/294977
Your team OpenLP Core is subscribed to branch lp:openlp.