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

@@ -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))
 

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

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()

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

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))

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):
     """