← Back to team overview

openlp-core team mailing list archive

[Merge] lp:~phill-ridout/openlp/thumbnail_fixes into lp:openlp

 

Phill has proposed merging lp:~phill-ridout/openlp/thumbnail_fixes into lp:openlp.

Requested reviews:
  OpenLP Core (openlp-core)
Related bugs:
  Bug #1414360 in OpenLP: "2.1.2 Traceback on Presentations for MAC"
  https://bugs.launchpad.net/openlp/+bug/1414360
  Bug #1414585 in OpenLP: "Thumbnail of images isn't shown in stage view and remote control"
  https://bugs.launchpad.net/openlp/+bug/1414585

For more details, see:
https://code.launchpad.net/~phill-ridout/openlp/thumbnail_fixes/+merge/248033

Fixes #1414585 Thumbnail of images isn't shown in stage view and remote control
and #1414585 2.1.2 Traceback on Presentations for MAC


Add this to your merge proposal:
--------------------------------
lp:~phill-ridout/openlp/thumbnail_fixes (revision 2493)
[SUCCESS] http://ci.openlp.org/job/Branch-01-Pull/913/
[SUCCESS] http://ci.openlp.org/job/Branch-02-Functional-Tests/844/
[SUCCESS] http://ci.openlp.org/job/Branch-03-Interface-Tests/789/
[SUCCESS] http://ci.openlp.org/job/Branch-04a-Windows_Functional_Tests/700/
[SUCCESS] http://ci.openlp.org/job/Branch-04b-Windows_Interface_Tests/299/
[SUCCESS] http://ci.openlp.org/job/Branch-05a-Code_Analysis/441/
[SUCCESS] http://ci.openlp.org/job/Branch-05b-Test_Coverage/312/
-- 
Your team OpenLP Core is requested to review the proposed merge of lp:~phill-ridout/openlp/thumbnail_fixes into lp:openlp.
=== 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:
             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


Follow ups