openlp-core team mailing list archive

[Raoul Snyman <raoul@xxxxxxxxxxx>]

Review: Needs Fixing



Diff comments:

> === modified file 'openlp/plugins/bibles/lib/__init__.py'
> --- openlp/plugins/bibles/lib/__init__.py	2015-01-22 13:19:10 +0000
> +++ openlp/plugins/bibles/lib/__init__.py	2015-01-29 18:35:27 +0000
> @@ -178,7 +178,7 @@
>      default_separators = [
>          '|'.join([
>              translate('BiblesPlugin', ':', 'Verse identifier e.g. Genesis 1 : 1 = Genesis Chapter 1 Verse 1'),
> -            translate('BiblesPlugin', 'v','Verse identifier e.g. Genesis 1 v 1 = Genesis Chapter 1 Verse 1'),
> +            translate('BiblesPlugin', 'v', 'Verse identifier e.g. Genesis 1 v 1 = Genesis Chapter 1 Verse 1'),
>              translate('BiblesPlugin', 'V', 'Verse identifier e.g. Genesis 1 V 1 = Genesis Chapter 1 Verse 1'),
>              translate('BiblesPlugin', 'verse', 'Verse identifier e.g. Genesis 1 verse 1 = Genesis Chapter 1 Verse 1'),
>              translate('BiblesPlugin', 'verses',
> 
> === modified file 'openlp/plugins/images/lib/mediaitem.py'
> --- openlp/plugins/images/lib/mediaitem.py	2015-01-24 18:24:51 +0000
> +++ openlp/plugins/images/lib/mediaitem.py	2015-01-29 18:35:27 +0000
> @@ -314,6 +314,16 @@
>                  return True
>          return return_value
>  
> +    def generate_thumbnail_path(self, image):
> +        """
> +        Generate a path to the thumbnail
> +
> +        :param imageFile: An instance of fImageFileNames
> +        :return: A path to the thumbnail of type str
> +        """
> +        ext = os.path.splitext(image.filename)[1].lower()
> +        return os.path.join(self.service_path, '{}{}'.format(str(image.id), ext))
> +
>      def load_full_list(self, images, initial_load=False, open_group=None):
>          """
>          Replace the list of images and groups in the interface.
> @@ -338,8 +348,7 @@
>          for imageFile in images:

image_file, not imageFile, please.

>              log.debug('Loading image: %s', imageFile.filename)
>              filename = os.path.split(imageFile.filename)[1]
> -            ext = os.path.splitext(imageFile.filename)[1].lower()
> -            thumb = os.path.join(self.service_path, "%s%s" % (str(imageFile.id), ext))
> +            thumb = self.generate_thumbnail_path(imageFile)
>              if not os.path.exists(imageFile.filename):
>                  icon = build_icon(':/general/general_delete.png')
>              else:
> @@ -549,24 +558,24 @@
>          # force a nonexistent theme
>          service_item.theme = -1
>          missing_items_file_names = []
> -        images_file_names = []
> +        images = []
>          # Expand groups to images
>          for bitem in items:
>              if isinstance(bitem.data(0, QtCore.Qt.UserRole), ImageGroups) or bitem.data(0, QtCore.Qt.UserRole) is None:
>                  for index in range(0, bitem.childCount()):
>                      if isinstance(bitem.child(index).data(0, QtCore.Qt.UserRole), ImageFilenames):
> -                        images_file_names.append(bitem.child(index).data(0, QtCore.Qt.UserRole).filename)
> +                        images.append(bitem.child(index).data(0, QtCore.Qt.UserRole))
>              elif isinstance(bitem.data(0, QtCore.Qt.UserRole), ImageFilenames):
> -                images_file_names.append(bitem.data(0, QtCore.Qt.UserRole).filename)
> +                images.append(bitem.data(0, QtCore.Qt.UserRole))
>          # Don't try to display empty groups
> -        if not images_file_names:
> +        if not images:
>              return False
>          # Find missing files
> -        for filename in images_file_names:
> -            if not os.path.exists(filename):
> -                missing_items_file_names.append(filename)
> +        for image in images:
> +            if not os.path.exists(image.filename):
> +                missing_items_file_names.append(image.filename)
>          # We cannot continue, as all images do not exist.
> -        if not images_file_names:
> +        if not images:
>              if not remote:
>                  critical_error_message_box(
>                      translate('ImagePlugin.MediaItem', 'Missing Image(s)'),
> @@ -582,9 +591,10 @@
>                  QtGui.QMessageBox.No:
>              return False
>          # Continue with the existing images.
> -        for filename in images_file_names:
> -            name = os.path.split(filename)[1]
> -            service_item.add_from_image(filename, name, background, os.path.join(self.service_path, name))
> +        for image in images:
> +            name = os.path.split(image.filename)[1]
> +            thumbnail = self.generate_thumbnail_path(image)
> +            service_item.add_from_image(image.filename, name, background, thumbnail)
>          return True
>  
>      def check_group_exists(self, new_group):
> 
> === modified file 'openlp/plugins/presentations/presentationplugin.py'
> --- openlp/plugins/presentations/presentationplugin.py	2015-01-22 17:31:00 +0000
> +++ openlp/plugins/presentations/presentationplugin.py	2015-01-29 18:35:27 +0000
> @@ -143,7 +143,10 @@
>          super().app_startup()
>          files_from_config = Settings().value('presentations/presentations files')
>          for file in files_from_config:
> -            self.media_item.clean_up_thumbnails(file)
> +            try:
> +                self.media_item.clean_up_thumbnails(file)
> +            except AttributeError:
> +                pass
>          self.media_item.list_view.clear()
>          Settings().setValue('presentations/thumbnail_scheme', 'md5')
>          self.media_item.validate_and_load(files_from_config)
> 
> === modified file 'tests/functional/openlp_core_utils/test_utils.py'
> --- tests/functional/openlp_core_utils/test_utils.py	2015-01-19 08:34:29 +0000
> +++ tests/functional/openlp_core_utils/test_utils.py	2015-01-29 18:35:27 +0000
> @@ -25,7 +25,7 @@
>  import os
>  from unittest import TestCase
>  
> -from openlp.core.utils import clean_filename, get_filesystem_encoding, get_locale_key, \
> +from openlp.core.utils import clean_filename, delete_file, get_filesystem_encoding, get_locale_key, \
>      get_natural_key, split_filename, _get_user_agent, get_web_page, get_uno_instance, add_actions
>  from tests.functional import MagicMock, patch
>  
> @@ -184,6 +184,59 @@
>          # THEN: The file name should be cleaned.
>          self.assertEqual(wanted_name, result, 'The file name should not contain any special characters.')
>  
> +    def delete_file_no_path_test(self):
> +        """"
> +        Test the delete_file function when called with out a valid path
> +        """
> +        # GIVEN: A blank path
> +        # WEHN: Calling delete_file
> +        result = delete_file('')
> +
> +        # THEN: delete_file should return False
> +        self.assertFalse(result, "delete_file should return False when called with ''")
> +
> +    def delete_file_path_success_test(self):
> +        """"
> +        Test the delete_file function when it successfully deletes a file
> +        """
> +        # GIVEN: A mocked os which returns True when os.path.exists is called
> +        with patch('openlp.core.utils.os', **{'path.exists.return_value': False}):
> +
> +            # WHEN: Calling delete_file with a file path
> +            result = delete_file('path/file.ext')
> +
> +            # THEN: delete_file should return True
> +            self.assertTrue(result, 'delete_file should return True when it successfully deletes a file')
> +
> +    def delete_file_path_no_file_exists_test(self):
> +        """"
> +        Test the delete_file function when the file to remove does not exist
> +        """
> +        # GIVEN: A mocked os which returns False when os.path.exists is called
> +        with patch('openlp.core.utils.os', **{'path.exists.return_value': False}):
> +
> +            # WHEN: Calling delete_file with a file path
> +            result = delete_file('path/file.ext')
> +
> +            # THEN: delete_file should return True
> +            self.assertTrue(result, 'delete_file should return True when the file doesnt exist')
> +
> +    def delete_file_path_exception_test(self):
> +        """"
> +        Test the delete_file function when os.remove raises an exception
> +        """
> +        # GIVEN: A mocked os which returns True when os.path.exists is called and raises an OSError when os.remove is
> +        #       called.
> +        with patch('openlp.core.utils.os', **{'path.exists.return_value': True, 'path.exists.side_effect': OSError}), \
> +                patch('openlp.core.utils.log') as mocked_log:
> +
> +            # WHEN: Calling delete_file with a file path
> +            result = delete_file('path/file.ext')
> +
> +            # THEN: delete_file should log and exception and return False
> +            self.assertEqual(mocked_log.exception.call_count, 1)
> +            self.assertFalse(result, 'delete_file should return False when os.remove raises an OSError')
> +
>      def get_locale_key_test(self):
>          """
>          Test the get_locale_key(string) function
> 


-- 
https://code.launchpad.net/~phill-ridout/openlp/thumbnail_fixes/+merge/248033
Your team OpenLP Core is subscribed to branch lp:openlp.