From 01fac492e99d7aa24d5ff6db65a6d42def45555b Mon Sep 17 00:00:00 2001 From: Tomas Groth Date: Sat, 30 May 2020 20:57:29 +0000 Subject: [PATCH] A few image related fixes. * Use the image itself as a thumbnail if none is available, fixes #547 * Store the thumbnail path in service files, no matter if light or not. * Added convertion of image thumbnail to sha256 as part of DB upgrade. --- openlp/core/display/window.py | 7 +- openlp/core/lib/serviceitem.py | 117 ++++++++++++++---- openlp/core/ui/servicemanager.py | 4 +- openlp/plugins/bibles/lib/importers/http.py | 6 +- openlp/plugins/images/lib/upgrade.py | 11 ++ .../openlp_core/lib/test_serviceitem.py | 39 +++--- 6 files changed, 138 insertions(+), 46 deletions(-) diff --git a/openlp/core/display/window.py b/openlp/core/display/window.py index 0252848f9..acc00a9f6 100644 --- a/openlp/core/display/window.py +++ b/openlp/core/display/window.py @@ -319,7 +319,12 @@ class DisplayWindow(QtWidgets.QWidget, RegistryProperties, LogMixin): imagesr = copy.deepcopy(images) for image in imagesr: image['path'] = image['path'].as_uri() - image['thumbnail'] = image['thumbnail'].as_uri() + # Not all images has a dedicated thumbnail (such as images loaded from old or local servicefiles), + # in that case reuse the image + if image.get('thumbnail', None): + image['thumbnail'] = image['thumbnail'].as_uri() + else: + image['thumbnail'] = image['path'] json_images = json.dumps(imagesr) self.run_javascript('Display.setImageSlides({images});'.format(images=json_images)) diff --git a/openlp/core/lib/serviceitem.py b/openlp/core/lib/serviceitem.py index 2765a452f..4cdcbf826 100644 --- a/openlp/core/lib/serviceitem.py +++ b/openlp/core/lib/serviceitem.py @@ -28,7 +28,7 @@ import os import uuid from copy import deepcopy from pathlib import Path -from shutil import copytree, copy +from shutil import copytree, copy, move from PyQt5 import QtGui @@ -104,7 +104,7 @@ class ServiceItem(RegistryProperties): self.auto_play_slides_loop = False self.timed_slide_interval = 0 self.will_auto_start = False - self.has_original_files = True + self.has_original_file_path = True self.sha256_file_hash = None self.stored_filename = None self._new_item() @@ -341,7 +341,7 @@ class ServiceItem(RegistryProperties): self.sha256_file_hash = sha256_file_hash(file_location) self.stored_filename = '{hash}{ext}'.format(hash=self.sha256_file_hash, ext=os.path.splitext(file_name)[1]) # Update image path to match servicemanager location if file was loaded from service - if image and not self.has_original_files and self.name == 'presentations': + if image and self.name == 'presentations': image = AppLocation.get_section_data_path(self.name) / 'thumbnails' / self.sha256_file_hash / \ ntpath.basename(image) self.slides.append({'title': file_name, 'image': image, 'path': path, 'display_title': display_title, @@ -396,16 +396,37 @@ class ServiceItem(RegistryProperties): elif self.service_item_type == ServiceItemType.Image: if lite_save: for slide in self.slides: - service_data.append({'title': slide['title'], 'path': slide['path'], + # When saving a service that originated from openlp 2.4 thumbnail might not be available + if 'thumbnail' in slide: + image_path = slide['thumbnail'] + else: + # Check if (by chance) the thumbnails for this image is available on this machine + test_thumb = AppLocation.get_section_data_path(self.name) / 'thumbnails' / stored_filename + if test_thumb.exists(): + image_path = test_thumb + else: + image_path = None + service_data.append({'title': slide['title'], 'image': image_path, 'path': slide['path'], 'file_hash': slide['file_hash']}) else: for slide in self.slides: - image_path = slide['thumbnail'].relative_to(AppLocation().get_data_path()) + # When saving a service that originated from openlp 2.4 thumbnail might not be available + if 'thumbnail' in slide: + image_path = slide['thumbnail'].relative_to(AppLocation().get_data_path()) + else: + # Check if (by chance) the thumbnails for this image is available on this machine + test_thumb = AppLocation.get_section_data_path(self.name) / 'thumbnails' / stored_filename + if test_thumb.exists(): + image_path = test_thumb + else: + image_path = None service_data.append({'title': slide['title'], 'image': image_path, 'file_hash': slide['file_hash']}) elif self.service_item_type == ServiceItemType.Command: for slide in self.slides: if isinstance(slide['image'], QtGui.QIcon): image = 'clapperboard' + elif lite_save: + image = slide['image'] else: image = slide['image'].relative_to(AppLocation().get_data_path()) service_data.append({'title': slide['title'], 'image': image, 'path': slide['path'], @@ -453,10 +474,10 @@ class ServiceItem(RegistryProperties): self.timed_slide_interval = header.get('timed_slide_interval', 0) self.will_auto_start = header.get('will_auto_start', False) self.processor = header.get('processor', None) - self.has_original_files = True + self.has_original_file_path = True self.metadata = header.get('item_meta_data', []) self.sha256_file_hash = header.get('sha256_file_hash', None) - self.stored_filename = header.get('stored_filename', self.title) + self.stored_filename = header.get('stored_filename', None) if 'background_audio' in header and State().check_preconditions('media'): self.background_audio = [] for file_path in header['background_audio']: @@ -478,7 +499,7 @@ class ServiceItem(RegistryProperties): settings_section = service_item['serviceitem']['header']['name'] background = QtGui.QColor(self.settings.value(settings_section + '/background color')) if path: - self.has_original_files = False + self.has_original_file_path = False for text_image in service_item['serviceitem']['data']: file_hash = None thumbnail = None @@ -487,29 +508,61 @@ class ServiceItem(RegistryProperties): file_hash = text_image['file_hash'] file_path = path / '{base}{ext}'.format(base=file_hash, ext=os.path.splitext(text)[1]) thumbnail = AppLocation.get_data_path() / text_image['image'] - # copy thumbnail for servicemanager path + # copy thumbnail from servicemanager path copy(path / 'thumbnails' / os.path.basename(text_image['image']), AppLocation.get_section_data_path(self.name) / 'thumbnails') else: - text = text_image - file_path = path / text - self.add_from_image(file_path, text, background, thumbnail=thumbnail, file_hash=file_hash) + org_file_path = path / text_image + # rename the extracted file so that it follows the sha256 based approach of openlp 3 + self.sha256_file_hash = sha256_file_hash(org_file_path) + new_file = '{hash}{ext}'.format(hash=self.sha256_file_hash, ext=os.path.splitext(text_image)[1]) + file_path = path / new_file + move(org_file_path, file_path) + # Check if (by chance) the thumbnails for this image is available on this machine + test_thumb = AppLocation.get_section_data_path(self.name) / 'thumbnails' / new_file + if test_thumb.exists(): + thumbnail = test_thumb + self.add_from_image(file_path, text_image, background, thumbnail=thumbnail, file_hash=file_hash) else: for text_image in service_item['serviceitem']['data']: file_hash = None text = text_image['title'] + thumbnail = None if version >= 3: + file_path = text_image['path'] file_hash = text_image['file_hash'] - self.add_from_image(text_image['path'], text, background, file_hash=file_hash) + thumbnail = AppLocation.get_data_path() / text_image['image'] + else: + file_path = Path(text_image['path']) + # Check if (by chance) the thumbnails for this image is available on this machine + file_hash = sha256_file_hash(file_path) + new_file = '{hash}{ext}'.format(hash=file_hash, ext=os.path.splitext(file_path)[1]) + test_thumb = AppLocation.get_section_data_path(self.name) / 'thumbnails' / new_file + if test_thumb.exists(): + thumbnail = test_thumb + self.add_from_image(file_path, text, background, thumbnail=thumbnail, file_hash=file_hash) elif self.service_item_type == ServiceItemType.Command: + if version < 3: + # If this is an old servicefile with files included, we need to rename the bundled files to match + # the new sha256 based scheme + if path: + file_path = Path(path) / self.title + self.sha256_file_hash = sha256_file_hash(file_path) + new_file = path / '{hash}{ext}'.format(hash=self.sha256_file_hash, + ext=os.path.splitext(self.title)[1]) + move(file_path, new_file) + else: + file_path = Path(service_item['serviceitem']['data'][0]['path']) / self.title + self.sha256_file_hash = sha256_file_hash(file_path) + # Loop over the slides for text_image in service_item['serviceitem']['data']: if not self.title: self.title = text_image['title'] if self.is_capable(ItemCapabilities.IsOptical) or self.is_capable(ItemCapabilities.CanStream): - self.has_original_files = False + self.has_original_file_path = False self.add_from_command(text_image['path'], text_image['title'], text_image['image']) elif path: - self.has_original_files = False + self.has_original_file_path = False # Copy any bundled thumbnails into the plugin thumbnail folder if version >= 3 and os.path.exists(path / self.sha256_file_hash) and \ os.path.isdir(path / self.sha256_file_hash): @@ -520,14 +573,28 @@ class ServiceItem(RegistryProperties): except FileExistsError: # Files already exists, just skip pass - if text_image['image'] == 'clapperboard': - text_image['image'] = UiIcons().clapperboard - self.add_from_command(path, text_image['title'], text_image['image'], - text_image.get('display_title', ''), text_image.get('notes', ''), - file_hash=self.sha256_file_hash) + if text_image['image'] in ['clapperboard', ':/media/slidecontroller_multimedia.png']: + image_path = UiIcons().clapperboard + elif version < 3: + # convert the thumbnail path to new sha256 based + new_file = '{hash}{ext}'.format(hash=self.sha256_file_hash, + ext=os.path.splitext(text_image['image'])[1]) + image_path = AppLocation.get_section_data_path(self.name) / 'thumbnails' / \ + self.sha256_file_hash / os.path.split(text_image['image'])[1] + else: + image_path = text_image['image'] + self.add_from_command(path, text_image['title'], image_path, text_image.get('display_title', ''), + text_image.get('notes', ''), file_hash=self.sha256_file_hash) else: - self.add_from_command(Path(text_image['path']), text_image['title'], text_image['image'], - file_hash=self.sha256_file_hash) + if text_image['image'] in ['clapperboard', ':/media/slidecontroller_multimedia.png']: + image_path = UiIcons().clapperboard + elif version < 3: + # convert the thumbnail path to new sha256 based + image_path = AppLocation.get_section_data_path(self.name) / 'thumbnails' / \ + self.sha256_file_hash / os.path.split(text_image['image'])[1] + else: + image_path = text_image['image'] + self.add_from_command(Path(text_image['path']), str(text_image['title']), image_path) self._new_item() def get_display_title(self): @@ -668,7 +735,7 @@ class ServiceItem(RegistryProperties): return '' if self.is_image() or self.is_capable(ItemCapabilities.IsOptical): path_from = frame['path'] - elif self.is_command() and not self.has_original_files and self.sha256_file_hash: + elif self.is_command() and not self.has_original_file_path and self.sha256_file_hash: path_from = os.path.join(frame['path'], self.stored_filename) else: path_from = os.path.join(frame['path'], frame['title']) @@ -760,7 +827,7 @@ class ServiceItem(RegistryProperties): self.is_valid = False break else: - if self.has_original_files: + if self.has_original_file_path: file_name = Path(slide['path']) / slide['title'] else: file_name = Path(slide['path']) / self.stored_filename @@ -780,6 +847,6 @@ class ServiceItem(RegistryProperties): if self.is_capable(ItemCapabilities.HasThumbnails): if self.is_command() and self.slides: return os.path.dirname(self.slides[0]['image']) - elif self.is_image() and self.slides: + elif self.is_image() and self.slides and 'thumbnail' in self.slides[0]: return os.path.dirname(self.slides[0]['thumbnail']) return None diff --git a/openlp/core/ui/servicemanager.py b/openlp/core/ui/servicemanager.py index 1950c9ebe..9f83907fb 100644 --- a/openlp/core/ui/servicemanager.py +++ b/openlp/core/ui/servicemanager.py @@ -565,6 +565,8 @@ class ServiceManager(QtWidgets.QWidget, RegistryBase, Ui_ServiceManager, LogMixi # For items that has thumbnails, add them to the list if item['service_item'].is_capable(ItemCapabilities.HasThumbnails): thumbnail_path = item['service_item'].get_thumbnail_path() + if not thumbnail_path: + continue thumbnail_path_parent = Path(thumbnail_path).parent if item['service_item'].is_command(): # Run through everything in the thumbnail folder and add pictures @@ -1268,7 +1270,7 @@ class ServiceManager(QtWidgets.QWidget, RegistryBase, Ui_ServiceManager, LogMixi for item in self.service_items: item['order'] = count count += 1 - if not item['service_item'].has_original_files: + if not item['service_item'].has_original_file_path: self.service_has_all_original_files = False # Repaint the screen self.service_manager_list.clear() diff --git a/openlp/plugins/bibles/lib/importers/http.py b/openlp/plugins/bibles/lib/importers/http.py index 28b589fd9..9d93a0e29 100644 --- a/openlp/plugins/bibles/lib/importers/http.py +++ b/openlp/plugins/bibles/lib/importers/http.py @@ -364,8 +364,8 @@ class BGExtract(RegistryProperties): books = [] for book in content: td_element = book.find('td', {'class': 'book-name'}) - span_element = td_element.find('span', {'class': 'collapse-icon'}) - book_name = span_element.next_sibling.strip() + strings = [text for text in td_element.stripped_strings] + book_name = strings[0] if book_name: books.append(book_name) return books @@ -381,7 +381,7 @@ class BGExtract(RegistryProperties): soup = get_soup_for_bible_ref(bible_url) if not soup: return None - bible_select = soup.find('select', {'class': 'search-dropdown'}) + bible_select = soup.find('select', {'class': 'search-translation-select'}) if not bible_select: log.debug('No select tags found - did site change?') return None diff --git a/openlp/plugins/images/lib/upgrade.py b/openlp/plugins/images/lib/upgrade.py index ef4620ca0..89cd6e019 100644 --- a/openlp/plugins/images/lib/upgrade.py +++ b/openlp/plugins/images/lib/upgrade.py @@ -23,6 +23,7 @@ The :mod:`upgrade` module provides the migration path for the OLP Paths database """ import json import logging +import shutil from pathlib import Path from sqlalchemy import Column, Table, types @@ -81,8 +82,18 @@ def upgrade_3(session, metadata): op.add_column('image_filenames', Column('file_hash', types.Unicode(128))) conn = op.get_bind() results = conn.execute('SELECT * FROM image_filenames') + thumb_path = AppLocation.get_data_path() / 'images' / 'thumbnails' for row in results.fetchall(): file_path = json.loads(row.file_path, cls=OpenLPJSONDecoder) hash = sha256_file_hash(file_path) sql = 'UPDATE image_filenames SET file_hash = \'{hash}\' WHERE id = {id}'.format(hash=hash, id=row.id) conn.execute(sql) + # rename thumbnail to use file hash + ext = file_path.suffix.lower() + old_thumb = thumb_path / '{name:d}{ext}'.format(name=row.id, ext=ext) + new_thumb = thumb_path / '{name:s}{ext}'.format(name=hash, ext=ext) + try: + shutil.move(old_thumb, new_thumb) + except OSError: + log.exception('Failed in renaming image thumb from {oldt} to {newt}'.format(oldt=old_thumb, + newt=new_thumb)) diff --git a/tests/functional/openlp_core/lib/test_serviceitem.py b/tests/functional/openlp_core/lib/test_serviceitem.py index effde5cd9..bf5083636 100644 --- a/tests/functional/openlp_core/lib/test_serviceitem.py +++ b/tests/functional/openlp_core/lib/test_serviceitem.py @@ -122,9 +122,9 @@ def test_service_item_load_image_from_service(state_media, settings): """ # GIVEN: A new service item and a mocked add icon function image_name = 'image_1.jpg' - test_file = TEST_PATH / image_name - frame_array = {'path': test_file, 'title': image_name, 'file_hash': 'abcd'} - + fake_hash = 'abcd' + extracted_file = Path(TEST_PATH) / '{base}{ext}'.format(base=fake_hash, ext=os.path.splitext(image_name)[1]) + frame_array = {'path': extracted_file, 'title': image_name, 'file_hash': fake_hash} service_item = ServiceItem(None) service_item.add_icon = MagicMock() @@ -132,17 +132,18 @@ def test_service_item_load_image_from_service(state_media, settings): line = convert_file_service_item(TEST_PATH, 'serviceitem_image_1.osj') with patch('openlp.core.ui.servicemanager.os.path.exists') as mocked_exists,\ patch('openlp.core.lib.serviceitem.AppLocation.get_section_data_path') as mocked_get_section_data_path,\ - patch('openlp.core.lib.serviceitem.sha256_file_hash') as mocked_sha256_file_hash: - mocked_sha256_file_hash.return_value = 'abcd' + patch('openlp.core.lib.serviceitem.sha256_file_hash') as mocked_sha256_file_hash,\ + patch('openlp.core.lib.serviceitem.move'): + mocked_sha256_file_hash.return_value = fake_hash mocked_exists.return_value = True mocked_get_section_data_path.return_value = Path('/path/') service_item.set_from_service(line, TEST_PATH) # THEN: We should get back a valid service item assert service_item.is_valid is True, 'The new service item should be valid' - assert test_file == service_item.get_rendered_frame(0), 'The first frame should match the path to the image' + assert extracted_file == service_item.get_rendered_frame(0), 'The first frame should match the path to the image' assert frame_array == service_item.get_frames()[0], 'The return should match frame array1' - assert test_file == service_item.get_frame_path(0), \ + assert extracted_file == service_item.get_frame_path(0), \ 'The frame path should match the full path to the image' assert image_name == service_item.get_frame_title(0), 'The frame title should match the image name' assert image_name == service_item.get_display_title(), 'The display title should match the first image name' @@ -168,8 +169,8 @@ def test_service_item_load_image_from_local_service(mocked_get_section_data_path mocked_exists.return_value = True image_name1 = 'image_1.jpg' image_name2 = 'image_2.jpg' - test_file1 = os.path.join('/home', 'openlp', image_name1) - test_file2 = os.path.join('/home', 'openlp', image_name2) + test_file1 = Path('/home/openlp') / image_name1 + test_file2 = Path('/home/openlp') / image_name2 frame_array1 = {'path': test_file1, 'title': image_name1, 'file_hash': 'abcd'} frame_array2 = {'path': test_file2, 'title': image_name2, 'file_hash': 'abcd'} service_item = ServiceItem(None) @@ -196,9 +197,9 @@ def test_service_item_load_image_from_local_service(mocked_get_section_data_path 'The Second frame should match the path to the image' assert frame_array1 == service_item.get_frames()[0], 'The return should match the frame array1' assert frame_array2 == service_item2.get_frames()[0], 'The return should match the frame array2' - assert test_file1 == str(service_item.get_frame_path(0)), \ + assert test_file1 == service_item.get_frame_path(0), \ 'The frame path should match the full path to the image' - assert test_file2 == str(service_item2.get_frame_path(0)), \ + assert test_file2 == service_item2.get_frame_path(0), \ 'The frame path should match the full path to the image' assert image_name1 == service_item.get_frame_title(0), 'The 1st frame title should match the image name' assert image_name2 == service_item2.get_frame_title(0), 'The 2nd frame title should match the image name' @@ -221,16 +222,19 @@ def test_add_from_command_for_a_presentation(): """ # GIVEN: A service item, a mocked icon and presentation data service_item = ServiceItem(None) + service_item.name = 'presentations' presentation_name = 'test.pptx' - image = MagicMock() + image = Path('thumbnails/abcd/slide1.png') display_title = 'DisplayTitle' notes = 'Note1\nNote2\n' frame = {'title': presentation_name, 'image': image, 'path': TEST_PATH, 'display_title': display_title, 'notes': notes, 'thumbnail': image} # WHEN: adding presentation to service_item - with patch('openlp.core.lib.serviceitem.sha256_file_hash') as mocked_sha256_file_hash: + with patch('openlp.core.lib.serviceitem.sha256_file_hash') as mocked_sha256_file_hash,\ + patch('openlp.core.lib.serviceitem.AppLocation.get_section_data_path') as mocked_get_section_data_path: mocked_sha256_file_hash.return_value = 'abcd' + mocked_get_section_data_path.return_value = Path('.') service_item.add_from_command(TEST_PATH, presentation_name, image, display_title, notes) # THEN: verify that it is setup as a Command and that the frame data matches @@ -244,14 +248,17 @@ def test_add_from_command_without_display_title_and_notes(): """ # GIVEN: A new service item, a mocked icon and image data service_item = ServiceItem(None) + service_item.name = 'presentations' image_name = 'test.img' - image = MagicMock() + image = Path('thumbnails/abcd/slide1.png') frame = {'title': image_name, 'image': image, 'path': TEST_PATH, 'display_title': None, 'notes': None, 'thumbnail': image} # WHEN: adding image to service_item - with patch('openlp.core.lib.serviceitem.sha256_file_hash') as mocked_sha256_file_hash: + with patch('openlp.core.lib.serviceitem.sha256_file_hash') as mocked_sha256_file_hash,\ + patch('openlp.core.lib.serviceitem.AppLocation.get_section_data_path') as mocked_get_section_data_path: mocked_sha256_file_hash.return_value = 'abcd' + mocked_get_section_data_path.return_value = Path('.') service_item.add_from_command(TEST_PATH, image_name, image) # THEN: verify that it is setup as a Command and that the frame data matches @@ -259,7 +266,7 @@ def test_add_from_command_without_display_title_and_notes(): assert service_item.get_frames()[0] == frame, 'Frames should match' -@patch(u'openlp.core.lib.serviceitem.ServiceItem.image_manager') +@patch('openlp.core.lib.serviceitem.ServiceItem.image_manager') @patch('openlp.core.lib.serviceitem.AppLocation.get_section_data_path') def test_add_from_command_for_a_presentation_thumb(mocked_get_section_data_path, mocked_image_manager): """