openlp-core team mailing list archive

[Raoul Snyman <raoul@xxxxxxxxxxx>]

Review: Needs Fixing

A few small fixes and then you should be good to go.

Diff comments:

> 
> === modified file 'openlp/plugins/bibles/forms/bibleimportform.py'
> --- openlp/plugins/bibles/forms/bibleimportform.py	2016-08-29 16:11:09 +0000
> +++ openlp/plugins/bibles/forms/bibleimportform.py	2016-12-04 03:28:05 +0000
> @@ -717,6 +725,7 @@
>          self.license_details_page.registerField('license_version', self.version_name_edit)
>          self.license_details_page.registerField('license_copyright', self.copyright_edit)
>          self.license_details_page.registerField('license_permissions', self.permissions_edit)
> +        self.license_details_page.registerField("license_full_license", self.full_license_edit, "plainText")
>  

Our standard is single quotes.

>      def set_defaults(self):
>          """
> 
> === modified file 'openlp/plugins/bibles/forms/editbibleform.py'
> --- openlp/plugins/bibles/forms/editbibleform.py	2016-05-21 08:31:24 +0000
> +++ openlp/plugins/bibles/forms/editbibleform.py	2016-12-04 03:28:05 +0000
> @@ -61,10 +61,32 @@
>          """
>          log.debug('Load Bible')
>          self.bible = bible
> -        self.version_name_edit.setText(self.manager.get_meta_data(self.bible, 'name').value)
> -        self.copyright_edit.setText(self.manager.get_meta_data(self.bible, 'copyright').value)
> -        self.permissions_edit.setText(self.manager.get_meta_data(self.bible, 'permissions').value)
>          book_name_language = self.manager.get_meta_data(self.bible, 'book_name_language')
> +        """
> +        Try loading the metadata, if the field does not exist in the metadata, continue executing the code,
> +        missing fields will be created on "self.accept" (save).
> +        """

This is not really the right way to do this. Each "get_meta_data" function returns None if the row doesn't exist in the database, so change your exceptions to this:

meta = self.manager.get_meta_data(self.bible, 'name')
if meta:
    self.version_name_edit.setText(meta.value)

And so on and so forth. It's 3 lines each instead of 4, and it's clearer to see what the code is doing and WHY.

> +        try:
> +            self.version_name_edit.setText(self.manager.get_meta_data(self.bible, 'name').value)
> +        except AttributeError:
> +            pass
> +        try:
> +            self.copyright_edit.setText(self.manager.get_meta_data(self.bible, 'copyright').value)
> +        except AttributeError:
> +            pass
> +        try:
> +            self.permissions_edit.setText(self.manager.get_meta_data(self.bible, 'permissions').value)
> +        except AttributeError:
> +            pass
> +        try:
> +            self.full_license_edit.setPlainText(self.manager.get_meta_data(self.bible, 'full_license').value)
> +        except AttributeError:
> +            pass
> +        # Set placeholder texts for the fields.
> +        self.version_name_edit.setPlaceholderText(UiStrings().RequiredShowInFooter)
> +        self.copyright_edit.setPlaceholderText(UiStrings().RequiredShowInFooter)
> +        self.permissions_edit.setPlaceholderText(UiStrings().OptionalShowInFooter)
> +        self.full_license_edit.setPlaceholderText(UiStrings().OptionalHideInFooter)
>          if book_name_language and book_name_language.value != 'None':
>              self.language_selection_combo_box.setCurrentIndex(int(book_name_language.value) + 1)
>          self.books = {}
> 
> === modified file 'openlp/plugins/songs/songsplugin.py'
> --- openlp/plugins/songs/songsplugin.py	2016-09-19 18:51:48 +0000
> +++ openlp/plugins/songs/songsplugin.py	2016-12-04 03:28:05 +0000
> @@ -60,6 +60,7 @@
>      'songs/add song from service': True,
>      'songs/display songbar': True,
>      'songs/display songbook': False,
> +    'songs/display written by': False,

What's the previous behaviour? If previously the "Written by" used to always be displayed, then this should be True, not False. We cannot change the default behaviour of OpenLP from what people are expecting when we introduce a new feature.

>      'songs/display copyright symbol': False,
>      'songs/last directory import': '',
>      'songs/last directory export': '',


-- 
https://code.launchpad.net/~suutari-olli/openlp/add-bible-license-field/+merge/312426
Your team OpenLP Core is subscribed to branch lp:openlp.