openlp-core team mailing list archive
-
openlp-core team
-
Mailing list archive
-
Message #25912
[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
