openlp-core team mailing list archive

[Tim Bentley <tim.bentley@xxxxxxxxx>]

Review: Needs Fixing

Blank lines!
Will review tomorrow as I have bible study!

Diff comments:

> === modified file 'openlp/core/lib/serviceitem.py'
> --- openlp/core/lib/serviceitem.py	2017-05-17 20:06:45 +0000
> +++ openlp/core/lib/serviceitem.py	2017-06-12 18:37:29 +0000
> @@ -276,12 +277,9 @@
>          else:
>              log.error('Invalid value renderer: {item}'.format(item=self.service_item_type))
>          self.title = clean_tags(self.title)
> -        # The footer should never be None, but to be compatible with a few
> -        # nightly builds between 1.9.4 and 1.9.5, we have to correct this to
> -        # avoid tracebacks.
> -        if self.raw_footer is None:
> -            self.raw_footer = []
> -        self.foot_text = '<br>'.join([_f for _f in self.raw_footer if _f])
> +

No blank line!

> +        if not self.footer_html:
> +            self.footer_html = '<br>'.join([_f for _f in self.raw_footer if _f])
>  
>      def add_from_image(self, path, title, background=None, thumbnail=None):
>          """
> 
> === modified file 'openlp/plugins/songs/lib/mediaitem.py'
> --- openlp/plugins/songs/lib/mediaitem.py	2017-06-09 06:06:49 +0000
> +++ openlp/plugins/songs/lib/mediaitem.py	2017-06-12 18:37:29 +0000
> @@ -664,17 +658,47 @@
>              item.raw_footer.append("{text}: {authors}".format(text=AuthorType.Types[AuthorType.Translation],
>                                                                authors=create_separated_list(authors_translation)))
>          if song.copyright:
> -            if self.display_copyright_symbol:
> -                item.raw_footer.append("{symbol} {song}".format(symbol=SongStrings.CopyrightSymbol,
> -                                                                song=song.copyright))
> -            else:
> -                item.raw_footer.append(song.copyright)
> -        if self.display_songbook and song.songbook_entries:
> -            songbooks = [str(songbook_entry) for songbook_entry in song.songbook_entries]
> +            item.raw_footer.append("{symbol} {song}".format(symbol=SongStrings.CopyrightSymbol,
> +                                                            song=song.copyright))
> +        songbooks = [str(songbook_entry) for songbook_entry in song.songbook_entries]
> +        if song.songbook_entries:
>              item.raw_footer.append(", ".join(songbooks))
>          if Settings().value('core/ccli number'):
> -            item.raw_footer.append(translate('SongsPlugin.MediaItem',
> -                                             'CCLI License: ') + Settings().value('core/ccli number'))
> +            item.raw_footer.append(translate('SongsPlugin.MediaItem', 'CCLI License: ') +
> +                                   Settings().value('core/ccli number'))
> +
> +        footer_template = Settings().value('songs/footer template')
> +        # Keep this in sync with the list in songstab.py
> +        vars = {
> +            'title': song.title,
> +            'alternate_title': song.alternate_title,
> +            'authors_none_label': translate('OpenLP.Ui', 'Written by'),
> +            'authors_none': authors_none,
> +            'authors_words_label': AuthorType.Types[AuthorType.Words],
> +            'authors_words': authors_words,
> +            'authors_music_label': AuthorType.Types[AuthorType.Music],
> +            'authors_music': authors_music,
> +            'authors_words_music_label': AuthorType.Types[AuthorType.WordsAndMusic],
> +            'authors_words_music': authors_words_music,
> +            'authors_translation_label': AuthorType.Types[AuthorType.Translation],
> +            'authors_translation': authors_translation,
> +            'authors_words_all': authors_words + authors_words_music,
> +            'authors_music_all': authors_music + authors_words_music,
> +            'copyright': song.copyright,
> +            'songbook_entries': songbooks,
> +            'ccli_license': Settings().value('core/ccli number'),
> +            'ccli_license_label': translate('SongsPlugin.MediaItem', 'CCLI License'),
> +            'ccli_number': song.ccli_number,
> +            'topics': [topic.name for topic in song.topics]
> +        }
> +

hum

> +        try:
> +            item.footer_html = mako.template.Template(footer_template).render_unicode(**vars).replace('\n', '')
> +        except mako.exceptions.SyntaxException:
> +            log.error('Failed to render Song footer html:\n' + mako.exceptions.text_error_template().render())
> +            critical_error_message_box(message=translate('SongsPlugin.MediaItem',
> +                                                         'Failed to render Song footer html.\nSee log for details'))
> +
>          return authors_all
>  
>      def service_load(self, item):
> 
> === modified file 'openlp/plugins/songs/lib/songstab.py'
> --- openlp/plugins/songs/lib/songstab.py	2017-02-26 21:14:49 +0000
> +++ openlp/plugins/songs/lib/songstab.py	2017-06-12 18:37:29 +0000
> @@ -127,6 +128,55 @@
>          self.german_notation_radio_button.setText(translate('SongsPlugin.SongsTab', 'German') + ' (C-D-E-F-G-A-H)')
>          self.neolatin_notation_radio_button.setText(
>              translate('SongsPlugin.SongsTab', 'Neo-Latin') + ' (Do-Re-Mi-Fa-Sol-La-Si)')
> +        self.footer_group_box.setTitle(translate('SongsPlugin.SongsTab', 'Footer'))
> +

blank line

> +        # Keep this in sync with the list in mediaitem.py
> +        const = '<code>"{}"</code>'
> +        placeholders = [
> +            # placeholder, description, can be empty, is a list
> +            ['title', translate('SongsPlugin.SongsTab', 'Song Title'), False, False],
> +            ['alternate_title', translate('SongsPlugin.SongsTab', 'Alternate Title'), True, False],
> +            ['written_by', const.format(translate('SongsPlugin.SongsTab', 'Written By')), True, False],
> +            ['authors_none', translate('SongsPlugin.SongsTab', 'Authors when type is not set'), False, True],
> +            ['authors_words_label', const.format(AuthorType.Types[AuthorType.Words]), False, False],
> +            ['authors_words', translate('SongsPlugin.SongsTab', 'Authors (Type "Words")'), False, True],
> +            ['authors_music_label', const.format(AuthorType.Types[AuthorType.Music]), False, False],
> +            ['authors_music', translate('SongsPlugin.SongsTab', 'Authors (Type "Music")'), False, True],
> +            ['authors_words_music_label', const.format(AuthorType.Types[AuthorType.WordsAndMusic]), False, False],
> +            ['authors_words_music', translate('SongsPlugin.SongsTab', 'Authors (Type "Words and Music")'), False, True],
> +            ['authors_translation_label', const.format(AuthorType.Types[AuthorType.Translation]), False, False],
> +            ['authors_translation', translate('SongsPlugin.SongsTab', 'Authors (Type "Translation")'), False, True],
> +            ['authors_words_all', translate('SongsPlugin.SongsTab', 'Authors (Type "Words" & "Words and Music")'),
> +             False, True],
> +            ['authors_music_all', translate('SongsPlugin.SongsTab', 'Authors (Type "Music" & "Words and Music")'),
> +             False, True],
> +            ['copyright', translate('SongsPlugin.SongsTab', 'Copyright information'), True, False],
> +            ['songbook_entries', translate('SongsPlugin.SongsTab', 'Songbook Entries'), False, True],
> +            ['ccli_license', translate('SongsPlugin.SongsTab', 'CCLI License'), True, False],
> +            ['ccli_license_label', const.format(translate('SongsPlugin.SongsTab', 'CCLI License')), False, False],
> +            ['ccli_number', translate('SongsPlugin.SongsTab', 'Song CCLI Number'), True, False],
> +            ['topics', translate('SongsPlugin.SongsTab', 'Topics'), False, True],
> +        ]
> +

and again

> +        placeholder_info = '<table style="background: #eee">\n<tr><th><b>{ph}</b></th><th><b>{desc}</b></th></tr>\n'\
> +            .format(ph=translate('SongsPlugin.SongsTab', 'Placeholder'),
> +                    desc=translate('SongsPlugin.SongsTab', 'Description'))
> +        for placeholder in placeholders:
> +            placeholder_info += '<tr><td>{{{{{pl}}}}}</td><td>{des}{opt}</td></tr>\n'\
> +                                .format(pl=placeholder[0], des=placeholder[1],
> +                                        opt=('&nbsp;¹' if placeholder[2] else '') +
> +                                            ('&nbsp;²' if placeholder[3] else ''))
> +        placeholder_info += '</table>'
> +        placeholder_info += '\n<br/>¹ {}'.format(translate('SongsPlugin.SongsTab', 'can be empty'))
> +        placeholder_info += '\n<br/>² {}'.format(translate('SongsPlugin.SongsTab', 'list of entries, can be empty'))
> +        self.footer_placeholder_info.setHtml(placeholder_info)
> +        self.footer_placeholder_info.setReadOnly(True)
> +
> +        self.footer_info_label.setText(translate('SongsPlugin.SongsTab', 'How to use Footers:'))
> +        self.footer_desc_label.setText('{} (<a href="http://docs.makotemplates.org";>{}</a>):'
> +                                       .format(translate('SongsPlugin.SongsTab', 'Footer Template'),
> +                                               translate('SongsPlugin.SongsTab', 'Mako Syntax')))
> +        self.footer_reset_button.setText(translate('SongsPlugin.SongsTab', 'Reset Template'))
>  
>      def on_search_as_type_check_box_changed(self, check_state):
>          self.song_search = (check_state == QtCore.Qt.Checked)


-- 
https://code.launchpad.net/~sam92/openlp/bug-1695620/+merge/325514
Your team OpenLP Core is subscribed to branch lp:openlp.