Merge lp:~tomasgroth/openlp/presentation-load-speedup into lp:openlp

[Tomas Groth]

WIP!
Fundamental change to how service files are storing data.

WIP!
Fundamental change to how service files are storing data.
When we add a file to a service file, instead of adding it with its name,
we should rename it so its name reflects the sha256 checksum of the
filecontent. So is a file is called "church-presentation.ppt" it is saved
as "505911856c191c5a7c4a0dc4858a8627e21b7171a1d9189bd8bb39090eac982f.ppt",
and no path/folders will be included. The new filename will also be added
to the service-item data. Doing this will avoid any problems with paths,
and if any files has the same hash it will be because they are the same, so
no duplicate content.
Avoiding folders from included files also allow us to use folders for
something else, namely thumbnails (for presentations). So reusing the the
example above, we could create a folder in the servicefile named
"505911856c191c5a7c4a0dc4858a8627e21b7171a1d9189bd8bb39090eac982f" which
contains all the thumbnails (and slide notes and titles), which can then be
reused when loading the servicefile, thereby skipping the step of
recreating the thumbnails (and thereby fixing bug #1677037).

[Raoul Snyman (raoul-snyman)]

Linux tests failed, please see https://ci.openlp.io/job/MP-02-Linux_Tests/170/ for more details

[lp:~tomasgroth/openlp/presentation-load-speedup]

2829. By Tomas Groth on 2019-05-27

A few fixes

[Phill (phill-ridout)]

Just a comment on implementation.

[lp:~tomasgroth/openlp/presentation-load-speedup]

2830. By Tomas Groth on 2019-06-18

merge trunk

2831. By Tomas Groth on 2019-06-20

Added preliminary support for saving image thumbnails in servicefiles.

[Raoul Snyman (raoul-snyman)]

Just a question about os.path vs Path

review: Needs Information

[lp:~tomasgroth/openlp/presentation-load-speedup]

2832. By Tomas Groth on 2019-06-21

Make image plugin use hashes for id and thumbnails

2833. By Tomas Groth on 2019-06-26

merge trunk

2834. By Tomas Groth on 2019-07-02

merge trunk

2835. By Tomas Groth on 2019-07-02

More work on saving and loading images to servicefile with new file hashes.

2836. By Tomas Groth on 2019-07-04

minor stuff

2837. By Tomas Groth on 2019-07-04

merge trunk

2838. By Tomas Groth on 2019-07-16

Loading presentation thumbnails from servicefiles now works. Also introduced a new service file header attribute: stored_filename.

2839. By Tomas Groth on 2019-07-16

merge trunk

2840. By Tomas Groth on 2019-07-18

Change tests to match new implementation.

2841. By Tomas Groth on 2019-09-15

merge trunk

Unmerged revisions

2841. By Tomas Groth on 2019-09-15

merge trunk

2840. By Tomas Groth on 2019-07-18

Change tests to match new implementation.

2839. By Tomas Groth on 2019-07-16

merge trunk

2838. By Tomas Groth on 2019-07-16

Loading presentation thumbnails from servicefiles now works. Also introduced a new service file header attribute: stored_filename.

2837. By Tomas Groth on 2019-07-04

merge trunk

2836. By Tomas Groth on 2019-07-04

minor stuff

2835. By Tomas Groth on 2019-07-02

More work on saving and loading images to servicefile with new file hashes.

2834. By Tomas Groth on 2019-07-02

merge trunk

2833. By Tomas Groth on 2019-06-26

merge trunk

2832. By Tomas Groth on 2019-06-21

Make image plugin use hashes for id and thumbnails

=== modified file 'openlp/core/common/__init__.py'
--- openlp/core/common/__init__.py	2019-07-27 06:37:16 +0000
+++ openlp/core/common/__init__.py	2019-09-15 09:05:37 +0000
@@ -282,6 +282,21 @@
     return hash_value
 
 
+def sha256_file_hash(filename):
+    """
+    Returns the hashed output of sha256 on the file content using Python3 hashlib
+
+    :param filename: Name of the file to hash
+    :returns: str
+    """
+    log.debug('sha256_hash(filename="{filename}")'.format(filename=filename))
+    hash_obj = hashlib.sha256()
+    with open(filename, 'rb') as f:
+        for chunk in iter(lambda: f.read(65536), b''):
+            hash_obj.update(chunk)
+    return hash_obj.hexdigest()
+
+
 def qmd5_hash(salt=None, data=None):
     """
     Returns the hashed output of MD5Sum on salt, data

=== modified file 'openlp/core/lib/serviceitem.py'
--- openlp/core/lib/serviceitem.py	2019-08-30 21:32:26 +0000
+++ openlp/core/lib/serviceitem.py	2019-09-15 09:05:37 +0000
@@ -30,11 +30,12 @@
 import uuid
 from copy import deepcopy
 from pathlib import Path
+from shutil import copytree
 
 from PyQt5 import QtGui
 
 from openlp.core.state import State
-from openlp.core.common import md5_hash
+from openlp.core.common import sha256_file_hash
 from openlp.core.common.applocation import AppLocation
 from openlp.core.common.i18n import translate
 from openlp.core.common.mixins import RegistryProperties
@@ -113,6 +114,8 @@
         self.timed_slide_interval = 0
         self.will_auto_start = False
         self.has_original_files = True
+        self.sha256_file_hash = None
+        self.stored_filename = None
         self._new_item()
         self.metadata = []
 
@@ -238,7 +241,7 @@
                     self._print_slides.append(slide)
         return self._print_slides
 
-    def add_from_image(self, path, title, background=None, thumbnail=None):
+    def add_from_image(self, path, title, background=None, thumbnail=None, file_hash=None):
         """
         Add an image slide to the service item.
 
@@ -250,7 +253,9 @@
         if background:
             self.image_border = background
         self.service_item_type = ServiceItemType.Image
-        slide = {'title': title, 'path': path}
+        if not file_hash:
+            file_hash = sha256_file_hash(path)
+        slide = {'title': title, 'path': path, 'file_hash': file_hash}
         if thumbnail:
             slide['thumbnail'] = thumbnail
         self.slides.append(slide)
@@ -274,7 +279,7 @@
         self.slides.append({'title': title, 'text': text, 'verse': verse_tag})
         self._new_item()
 
-    def add_from_command(self, path, file_name, image, display_title=None, notes=None):
+    def add_from_command(self, path, file_name, image, display_title=None, notes=None, file_hash=None):
         """
         Add a slide from a command.
 
@@ -283,18 +288,23 @@
         :param image: The command of/for the slide.
         :param display_title: Title to show in gui/webinterface, optional.
         :param notes: Notes to show in the webinteface, optional.
+        :param file_hash: Sha256 hash checksum of the file.
         """
         self.service_item_type = ServiceItemType.Command
         # If the item should have a display title but this frame doesn't have one, we make one up
         if self.is_capable(ItemCapabilities.HasDisplayTitle) and not display_title:
             display_title = translate('OpenLP.ServiceItem',
                                       '[slide {frame:d}]').format(frame=len(self.slides) + 1)
+        if file_hash:
+            self.sha256_file_hash = file_hash
+        else:
+            file_location = Path(path) / file_name
+            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':
-            file_location = os.path.join(path, file_name)
-            file_location_hash = md5_hash(file_location.encode('utf-8'))
-            image = os.path.join(AppLocation.get_section_data_path(self.name), 'thumbnails', file_location_hash,
-                                 ntpath.basename(image))  # TODO: Pathlib
+            image = os.path.join(str(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,
                             'notes': notes, 'thumbnail': image})
         # if self.is_capable(ItemCapabilities.HasThumbnails):
@@ -305,6 +315,10 @@
         """
         This method returns some text which can be saved into the service file to represent this item.
         """
+        if self.sha256_file_hash:
+            stored_filename = '{hash}{ext}'.format(hash=self.sha256_file_hash, ext=os.path.splitext(self.title)[1])
+        else:
+            stored_filename = None
         service_header = {
             'name': self.name,
             'plugin': self.name,
@@ -329,7 +343,9 @@
             'theme_overwritten': self.theme_overwritten,
             'will_auto_start': self.will_auto_start,
             'processor': self.processor,
-            'metadata': self.metadata
+            'metadata': self.metadata,
+            'sha256_file_hash': self.sha256_file_hash,
+            'stored_filename': stored_filename
         }
         service_data = []
         if self.service_item_type == ServiceItemType.Text:
@@ -341,12 +357,16 @@
         elif self.service_item_type == ServiceItemType.Image:
             if lite_save:
                 for slide in self.slides:
-                    service_data.append({'title': slide['title'], 'path': slide['path']})
+                    service_data.append({'title': slide['title'], 'path': slide['path'],
+                                         'file_hash': slide['file_hash']})
             else:
-                service_data = [slide['title'] for slide in self.slides]
+                for slide in self.slides:
+                    image_path = os.path.relpath(slide['thumbnail'], AppLocation().get_data_path())
+                    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:
-                service_data.append({'title': slide['title'], 'image': slide['image'], 'path': slide['path'],
+                image_path = os.path.relpath(slide['image'], AppLocation().get_data_path())
+                service_data.append({'title': slide['title'], 'image': image_path, 'path': slide['path'],
                                      'display_title': slide['display_title'], 'notes': slide['notes']})
         return {'header': service_header, 'data': service_data}
 
@@ -357,7 +377,7 @@
         self._display_slides = []
         self._rendered_slides = []
 
-    def set_from_service(self, service_item, path=None):
+    def set_from_service(self, service_item, path=None, version=2):
         """
         This method takes a service item from a saved service file (passed from the ServiceManager) and extracts the
         data actually required.
@@ -365,6 +385,7 @@
         :param service_item: The item to extract data from.
         :param path: Defaults to *None*. This is the service manager path for things which have their files saved
             with them or None when the saved service is lite and the original file paths need to be preserved.
+        :param version: Format version of the data.
         """
         log.debug('set_from_service called with path {path}'.format(path=path))
         header = service_item['serviceitem']['header']
@@ -392,6 +413,8 @@
         self.processor = header.get('processor', None)
         self.has_original_files = 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)
         if 'background_audio' in header and State().check_preconditions('media'):
             self.background_audio = []
             for file_path in header['background_audio']:
@@ -412,11 +435,24 @@
             if path:
                 self.has_original_files = False
                 for text_image in service_item['serviceitem']['data']:
-                    file_path = path / text_image
-                    self.add_from_image(file_path, text_image, background)
+                    file_hash = None
+                    if version >= 3:
+                        text = text_image['title']
+                        file_hash = text_image['file_hash']
+                        file_path = path / '{base}{ext}'.format(base=file_hash, ext=os.path.splitext(text)[1])
+                    else:
+                        text = text_image
+                        file_path = path / text
+                    self.add_from_image(file_path, text, background, file_hash=file_hash)
             else:
                 for text_image in service_item['serviceitem']['data']:
-                    self.add_from_image(text_image['path'], text_image['title'], background)
+                    file_hash = None
+                    if version >= 3:
+                        text = text_image['title']
+                        file_hash = text_image['file_hash']
+                    else:
+                        text = text_image
+                    self.add_from_image(text_image['path'], text, background, file_hash=file_hash)
         elif self.service_item_type == ServiceItemType.Command:
             for text_image in service_item['serviceitem']['data']:
                 if not self.title:
@@ -426,10 +462,22 @@
                     self.add_from_command(text_image['path'], text_image['title'], text_image['image'])
                 elif path:
                     self.has_original_files = 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):
+                        try:
+                            copytree(path / self.sha256_file_hash,
+                                     AppLocation.get_section_data_path(self.name) / 'thumbnails' /
+                                     self.sha256_file_hash)
+                        except FileExistsError:
+                            # Files already exists, just skip
+                            pass
                     self.add_from_command(path, text_image['title'], text_image['image'],
-                                          text_image.get('display_title', ''), text_image.get('notes', ''))
+                                          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'])
+                    self.add_from_command(Path(text_image['path']), text_image['title'], text_image['image'],
+                                          file_hash=self.sha256_file_hash)
         self._new_item()
 
     def get_display_title(self):
@@ -565,6 +613,8 @@
                 return ''
         if self.is_image() or self.is_capable(ItemCapabilities.IsOptical):
             path_from = frame['path']
+        elif self.is_command():
+            path_from = os.path.join(frame['path'], self.stored_filename)
         else:
             path_from = os.path.join(frame['path'], frame['title'])
         if isinstance(path_from, str):
@@ -645,7 +695,7 @@
                         self.is_valid = False
                         break
                 else:
-                    file_name = os.path.join(slide['path'], slide['title'])
+                    file_name = os.path.join(slide['path'], self.stored_filename)
                     if not os.path.exists(file_name):
                         self.is_valid = False
                         break
@@ -654,3 +704,14 @@
                         if file_suffix.lower() not in suffixes:
                             self.is_valid = False
                             break
+
+    def get_thumbnail_path(self):
+        """
+        Returns the thumbnail folder. Should only be used for items that support thumbnails.
+        """
+        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:
+                return os.path.dirname(self.slides[0]['thumbnail'])
+        return None

=== modified file 'openlp/core/ui/servicemanager.py'
--- openlp/core/ui/servicemanager.py	2019-08-30 11:28:10 +0000
+++ openlp/core/ui/servicemanager.py	2019-09-15 09:05:37 +0000
@@ -34,8 +34,8 @@
 
 from PyQt5 import QtCore, QtGui, QtWidgets
 
+from openlp.core.common import ThemeLevel, delete_file, sha256_file_hash
 from openlp.core.state import State
-from openlp.core.common import ThemeLevel, delete_file
 from openlp.core.common.actions import ActionList, CategoryOrder
 from openlp.core.common.applocation import AppLocation
 from openlp.core.common.i18n import UiStrings, format_time, translate
@@ -43,10 +43,10 @@
 from openlp.core.common.mixins import LogMixin, RegistryProperties
 from openlp.core.common.registry import Registry, RegistryBase
 from openlp.core.common.settings import Settings
-from openlp.core.lib import build_icon
+from openlp.core.lib import build_icon, ItemCapabilities
 from openlp.core.lib.exceptions import ValidationError
 from openlp.core.lib.plugin import PluginStatus
-from openlp.core.lib.serviceitem import ItemCapabilities, ServiceItem, ServiceItemType
+from openlp.core.lib.serviceitem import ServiceItem, ServiceItemType
 from openlp.core.lib.ui import create_widget_action, critical_error_message_box, find_and_set_in_combo_box
 from openlp.core.ui.icons import UiIcons
 from openlp.core.ui.media import AUDIO_EXT, VIDEO_EXT
@@ -331,6 +331,7 @@
         self._service_path = None
         self.service_has_all_original_files = True
         self.list_double_clicked = False
+        self.servicefile_version = None
 
     def bootstrap_initialise(self):
         """
@@ -516,9 +517,15 @@
         :return: service array
         """
         service = []
+        # Regarding openlp-servicefile-version:
+        #  1: OpenLP 1? Not used.
+        #  2: OpenLP 2 (default when loading a service file without openlp-servicefile-version)
+        #  3: The new format introduced in OpenLP 3.0.
+        # Note that the servicefile-version numbering is not expected to follow the OpenLP version numbering.
         core = {
             'lite-service': self._save_lite,
-            'service-theme': self.service_theme
+            'service-theme': self.service_theme,
+            'openlp-servicefile-version': 3
         }
         service.append({'openlp_core': core})
         return service
@@ -536,16 +543,38 @@
             if item['service_item'].uses_file():
                 for frame in item['service_item'].get_frames():
                     path_from = item['service_item'].get_frame_path(frame=frame)
-                    if path_from in write_list or path_from in missing_list:
+                    path_from_path = Path(path_from)
+                    if item['service_item'].stored_filename:
+                        sha256_file_name = item['service_item'].stored_filename
+                    else:
+                        sha256_file_name = sha256_file_hash(path_from_path) + os.path.splitext(path_from)[1]
+                    path_from_tuple = (path_from_path, sha256_file_name)
+                    if path_from_tuple in write_list or path_from_path in missing_list:
                         continue
                     if not os.path.exists(path_from):
-                        missing_list.append(Path(path_from))
+                        missing_list.append(path_from_path)
                     else:
-                        write_list.append(Path(path_from))
+                        write_list.append(path_from_tuple)
+                # For items that has thumbnails, add them to the list
+                if item['service_item'].is_capable(ItemCapabilities.HasThumbnails):
+                    if item['service_item'].is_command():
+                        thumbnail_path = item['service_item'].get_thumbnail_path()
+                        # Run through everything in the thumbnail folder and add pictures
+                        thumbnail_path_parent = Path(thumbnail_path).parent
+                        for filename in os.listdir(thumbnail_path):
+                            # Skip non-pictures
+                            if os.path.splitext(filename)[1] not in ['.png', '.jpg']:
+                                continue
+                            filename_path = Path(thumbnail_path) / Path(filename)
+                            # Create a thumbnail path in the zip/service file
+                            service_path = filename_path.relative_to(thumbnail_path_parent)
+                            write_list.append((filename_path, service_path))
+                    # TODO: For images only copy the relevant thumbnails, not all thumbnails, skipped for now
             for audio_path in item['service_item'].background_audio:
-                if audio_path in write_list:
+                audio_path_tuple = (audio_path, audio_path)
+                if audio_path_tuple in write_list:
                     continue
-                write_list.append(audio_path)
+                write_list.append(audio_path_tuple)
         return write_list, missing_list
 
     def save_file(self):
@@ -567,7 +596,6 @@
 
         if not self._save_lite:
             write_list, missing_list = self.get_write_file_list()
-
             if missing_list:
                 self.application.set_normal_cursor()
                 title = translate('OpenLP.ServiceManager', 'Service File(s) Missing')
@@ -594,8 +622,8 @@
         service_content = json.dumps(service, cls=OpenLPJSONEncoder)
         service_content_size = len(bytes(service_content, encoding='utf-8'))
         total_size = service_content_size
-        for file_item in write_list:
-            total_size += file_item.stat().st_size
+        for local_file_item, zip_file_item in write_list:
+            total_size += local_file_item.stat().st_size
         self.log_debug('ServiceManager.save_file - ZIP contents size is %i bytes' % total_size)
         self.main_window.display_progress_bar(total_size)
         try:
@@ -605,9 +633,9 @@
                 zip_file.writestr('service_data.osj', service_content)
                 self.main_window.increment_progress_bar(service_content_size)
                 # Finally add all the listed media files.
-                for write_path in write_list:
-                    zip_file.write(write_path, write_path)
-                    self.main_window.increment_progress_bar(write_path.stat().st_size)
+                for local_file_item, zip_file_item in write_list:
+                    zip_file.write(str(local_file_item), str(zip_file_item))
+                    self.main_window.increment_progress_bar(local_file_item.stat().st_size)
                 with suppress(FileNotFoundError):
                     file_path.unlink()
                 os.link(temp_file.name, file_path)
@@ -696,11 +724,30 @@
         service_data = None
         self.application.set_busy_cursor()
         try:
-            with zipfile.ZipFile(file_path) as zip_file:
+            # TODO: figure out a way to use the presentation thumbnails from the service file
+            with zipfile.ZipFile(str(file_path)) as zip_file:
                 compressed_size = 0
                 for zip_info in zip_file.infolist():
                     compressed_size += zip_info.compress_size
                 self.main_window.display_progress_bar(compressed_size)
+                # First find the osj-file to find out how to handle the file
+                for zip_info in zip_file.infolist():
+                    # The json file has been called 'service_data.osj' since OpenLP 3.0
+                    if zip_info.filename == 'service_data.osj' or zip_info.filename.endswith('osj'):
+                        with zip_file.open(zip_info, 'r') as json_file:
+                            service_data = json_file.read()
+                        break
+                if service_data:
+                    items = json.loads(service_data, cls=OpenLPJSONDecoder)
+                else:
+                    raise ValidationError(msg='No service data found')
+                # Extract the service file version
+                for item in items:
+                    if 'openlp_core' in item:
+                        item = item['openlp_core']
+                        self.servicefile_version = item.get('openlp-servicefile-version', 2)
+                        break
+                print('service format version: %d' % self.servicefile_version)
                 for zip_info in zip_file.infolist():
                     self.log_debug('Extract file: {name}'.format(name=zip_info.filename))
                     # The json file has been called 'service_data.osj' since OpenLP 3.0
@@ -708,19 +755,19 @@
                         with zip_file.open(zip_info, 'r') as json_file:
                             service_data = json_file.read()
                     else:
-                        zip_info.filename = os.path.basename(zip_info.filename)
-                        zip_file.extract(zip_info, self.service_path)
+                        # Service files from earlier versions than 3 expects that all files are extracted
+                        # into the root of the service folder.
+                        if self.servicefile_version and self.servicefile_version < 3:
+                            zip_info.filename = os.path.basename(zip_info.filename.replace('/', os.path.sep))
+                        zip_file.extract(zip_info, str(self.service_path))
                     self.main_window.increment_progress_bar(zip_info.compress_size)
-            if service_data:
-                items = json.loads(service_data, cls=OpenLPJSONDecoder)
+                # Handle the content
                 self.new_file()
                 self.process_service_items(items)
                 self.set_file_name(file_path)
                 self.main_window.add_recent_file(file_path)
                 self.set_modified(False)
                 Settings().setValue('servicemanager/last file', file_path)
-            else:
-                raise ValidationError(msg='No service data found')
         except (NameError, OSError, ValidationError, zipfile.BadZipFile):
             self.application.set_normal_cursor()
             self.log_exception('Problem loading service file {name}'.format(name=file_path))
@@ -752,9 +799,9 @@
                         self.service_theme = theme
             else:
                 if self._save_lite:
-                    service_item.set_from_service(item)
+                    service_item.set_from_service(item, version=self.servicefile_version)
                 else:
-                    service_item.set_from_service(item, self.service_path)
+                    service_item.set_from_service(item, self.service_path, self.servicefile_version)
                 service_item.validate_item(self.suffixes)
                 if service_item.is_capable(ItemCapabilities.OnLoadUpdate):
                     new_item = Registry().get(service_item.name).service_load(service_item)
@@ -1259,11 +1306,12 @@
         """
         Empties the service_path of temporary files on system exit.
         """
-        for file_path in self.service_path.iterdir():
-            delete_file(file_path)
-        audio_path = self.service_path / 'audio'
-        if audio_path.exists():
-            shutil.rmtree(audio_path, True)
+        for file_name in os.listdir(self.service_path):
+            file_path = Path(self.service_path, file_name)
+            if os.path.isdir(file_path):
+                shutil.rmtree(file_path, True)
+            else:
+                delete_file(file_path)
 
     def on_theme_combo_box_selected(self, current_index):
         """

=== modified file 'openlp/plugins/images/lib/db.py'
--- openlp/plugins/images/lib/db.py	2019-04-13 13:00:22 +0000
+++ openlp/plugins/images/lib/db.py	2019-09-15 09:05:37 +0000
@@ -65,6 +65,7 @@
             * id
             * group_id
             * file_path
+            * file_hash
     """
     session, metadata = init_db(url)
 
@@ -79,7 +80,8 @@
     image_filenames_table = Table('image_filenames', metadata,
                                   Column('id', types.Integer(), primary_key=True),
                                   Column('group_id', types.Integer(), ForeignKey('image_groups.id'), default=None),
-                                  Column('file_path', PathType(), nullable=False)
+                                  Column('file_path', PathType(), nullable=False),
+                                  Column('file_hash', types.Unicode(128), nullable=False)
                                   )
 
     mapper(ImageGroups, image_groups_table)

=== modified file 'openlp/plugins/images/lib/mediaitem.py'
--- openlp/plugins/images/lib/mediaitem.py	2019-05-23 19:33:46 +0000
+++ openlp/plugins/images/lib/mediaitem.py	2019-09-15 09:05:37 +0000
@@ -25,7 +25,7 @@
 
 from PyQt5 import QtCore, QtGui, QtWidgets
 
-from openlp.core.common import delete_file, get_images_filter
+from openlp.core.common import delete_file, get_images_filter, sha256_file_hash
 from openlp.core.common.applocation import AppLocation
 from openlp.core.common.i18n import UiStrings, get_natural_key, translate
 from openlp.core.common.path import create_paths
@@ -347,7 +347,7 @@
         :rtype: Path
         """
         ext = image.file_path.suffix.lower()
-        return self.service_path / '{name:d}{ext}'.format(name=image.id, ext=ext)
+        return self.service_path / '{name:s}{ext}'.format(name=image.file_hash, ext=ext)
 
     def load_full_list(self, images, initial_load=False, open_group=None):
         """
@@ -492,6 +492,7 @@
             image_file = ImageFilenames()
             image_file.group_id = group_id
             image_file.file_path = image_path
+            image_file.file_hash = sha256_file_hash(image_path)
             self.manager.save_object(image_file)
             self.main_window.increment_progress_bar()
         if reload_list and image_paths:

=== modified file 'openlp/plugins/images/lib/upgrade.py'
--- openlp/plugins/images/lib/upgrade.py	2019-05-22 06:47:00 +0000
+++ openlp/plugins/images/lib/upgrade.py	2019-09-15 09:05:37 +0000
@@ -26,16 +26,17 @@
 import logging
 from pathlib import Path
 
-from sqlalchemy import Column, Table
+from sqlalchemy import Column, Table, types
 
+from openlp.core.common import sha256_file_hash
 from openlp.core.common.applocation import AppLocation
 from openlp.core.common.db import drop_columns
-from openlp.core.common.json import OpenLPJSONEncoder
+from openlp.core.common.json import OpenLPJSONEncoder, OpenLPJSONDecoder
 from openlp.core.lib.db import PathType, get_upgrade_op
 
 
 log = logging.getLogger(__name__)
-__version__ = 2
+__version__ = 3
 
 
 def upgrade_1(session, metadata):
@@ -68,3 +69,21 @@
         else:
             op.drop_constraint('image_filenames', 'foreignkey')
             op.drop_column('image_filenames', 'filenames')
+
+
+def upgrade_3(session, metadata):
+    """
+    Version 3 upgrade - add sha256 hash
+    """
+    log.debug('Starting upgrade_3 for adding sha256 hashes')
+    old_table = Table('image_filenames', metadata, autoload=True)
+    if 'file_hash' not in [col.name for col in old_table.c.values()]:
+        op = get_upgrade_op(session)
+        op.add_column('image_filenames', Column('file_hash', types.Unicode(128)))
+        conn = op.get_bind()
+        results = conn.execute('SELECT * FROM image_filenames')
+        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)

=== modified file 'openlp/plugins/presentations/lib/mediaitem.py'
--- openlp/plugins/presentations/lib/mediaitem.py	2019-06-30 23:28:38 +0000
+++ openlp/plugins/presentations/lib/mediaitem.py	2019-09-15 09:05:37 +0000
@@ -359,7 +359,8 @@
                             note = ''
                             if notes and len(notes) >= i:
                                 note = notes[i - 1]
-                            service_item.add_from_command(str(path), file_name, str(thumbnail_path), title, note)
+                            service_item.add_from_command(str(path), file_name, str(thumbnail_path), title, note,
+                                                          doc.get_sha256_file_hash())
                             i += 1
                             thumbnail_path = doc.get_thumbnail_path(i, True)
                         doc.close_presentation()

=== modified file 'openlp/plugins/presentations/lib/presentationcontroller.py'
--- openlp/plugins/presentations/lib/presentationcontroller.py	2019-06-21 20:53:42 +0000
+++ openlp/plugins/presentations/lib/presentationcontroller.py	2019-09-15 09:05:37 +0000
@@ -25,12 +25,12 @@
 
 from PyQt5 import QtCore
 
-from openlp.core.common import md5_hash
+from openlp.core.common import md5_hash, sha256_file_hash
 from openlp.core.common.applocation import AppLocation
 from openlp.core.common.path import create_paths
 from openlp.core.common.registry import Registry
 from openlp.core.common.settings import Settings
-from openlp.core.lib import create_thumb, validate_thumb
+from openlp.core.lib import create_thumb
 
 
 log = logging.getLogger(__name__)
@@ -98,6 +98,7 @@
         :rtype: None
         """
         self.controller = controller
+        self._sha256_file_hash = None
         self._setup(document_path)
 
     def _setup(self, document_path):
@@ -146,6 +147,12 @@
         #       get_temp_folder and PresentationPluginapp_startup is removed
         if Settings().value('presentations/thumbnail_scheme') == 'md5':
             folder = md5_hash(bytes(self.file_path))
+        elif Settings().value('presentations/thumbnail_scheme') == 'sha256file':
+            if self._sha256_file_hash:
+                folder = self._sha256_file_hash
+            else:
+                self._sha256_file_hash = sha256_file_hash(self.file_path)
+                folder = self._sha256_file_hash
         else:
             folder = self.file_path.name
         return Path(self.controller.thumbnail_folder, folder)
@@ -161,13 +168,20 @@
         #       get_thumbnail_folder and PresentationPluginapp_startup is removed
         if Settings().value('presentations/thumbnail_scheme') == 'md5':
             folder = md5_hash(bytes(self.file_path))
+        elif Settings().value('presentations/thumbnail_scheme') == 'sha256file':
+            if self._sha256_file_hash:
+                folder = self._sha256_file_hash
+            else:
+                self._sha256_file_hash = sha256_file_hash(self.file_path)
+                folder = self._sha256_file_hash
         else:
             folder = self.file_path.name
         return Path(self.controller.temp_folder, folder)
 
     def check_thumbnails(self):
         """
-        Check that the last thumbnail image exists and is valid and are more recent than the powerpoint file.
+        Check that the last thumbnail image exists and is valid. It is not checked if presentation file is newer than
+        thumbnail since the path is based on the file hash, so if it exists it is by definition up to date.
 
         :return: If the thumbnail is valid
         :rtype: bool
@@ -175,7 +189,7 @@
         last_image_path = self.get_thumbnail_path(self.get_slide_count(), True)
         if not (last_image_path and last_image_path.is_file()):
             return False
-        return validate_thumb(Path(self.file_path), Path(last_image_path))
+        return True
 
     def close_presentation(self):
         """
@@ -360,6 +374,17 @@
                 notes_path = self.get_thumbnail_folder() / 'slideNotes{number:d}.txt'.format(number=slide_no)
                 notes_path.write_text(note)
 
+    def get_sha256_file_hash(self):
+        """
+        Returns the sha256 file hash for the file.
+
+        :return: The sha256 file hash
+        :rtype: str
+        """
+        if not self._sha256_file_hash:
+            self._sha256_file_hash = sha256_file_hash(self.file_path)
+        return self._sha256_file_hash
+
 
 class PresentationController(object):
     """

=== modified file 'openlp/plugins/presentations/presentationplugin.py'
--- openlp/plugins/presentations/presentationplugin.py	2019-05-18 04:56:15 +0000
+++ openlp/plugins/presentations/presentationplugin.py	2019-09-15 09:05:37 +0000
@@ -158,7 +158,7 @@
         for path in presentation_paths:
             self.media_item.clean_up_thumbnails(path, clean_for_update=True)
         self.media_item.list_view.clear()
-        Settings().setValue('presentations/thumbnail_scheme', 'md5')
+        Settings().setValue('presentations/thumbnail_scheme', 'sha256file')
         self.media_item.validate_and_load(presentation_paths)
 
     @staticmethod

=== modified file 'tests/functional/openlp_core/lib/test_serviceitem.py'
--- tests/functional/openlp_core/lib/test_serviceitem.py	2019-05-22 06:47:00 +0000
+++ tests/functional/openlp_core/lib/test_serviceitem.py	2019-09-15 09:05:37 +0000
@@ -135,17 +135,19 @@
         assert 'Slide 2' == service_item.get_frame_title(1), '"Slide 2" has been returned as the title'
         assert '' == service_item.get_frame_title(2), 'Blank has been returned as the title of slide 3'
 
-    def test_service_item_load_image_from_service(self):
+    @patch('openlp.core.lib.serviceitem.sha256_file_hash')
+    def test_service_item_load_image_from_service(self, mocked_sha256_file_hash):
         """
         Test the Service Item - adding an image from a saved service
         """
         # 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}
+        frame_array = {'path': test_file, 'title': image_name, 'file_hash': 'fake_file_hash'}
 
         service_item = ServiceItem(None)
         service_item.add_icon = MagicMock()
+        mocked_sha256_file_hash.return_value = 'fake_file_hash'
 
         # WHEN: adding an image from a saved Service and mocked exists
         line = convert_file_service_item(TEST_PATH, 'serviceitem_image_1.osj')
@@ -174,9 +176,11 @@
         assert service_item.is_capable(ItemCapabilities.CanAppend) is True, \
             'This service item should be able to have new items added to it'
 
+    @patch('openlp.core.lib.serviceitem.sha256_file_hash')
     @patch('openlp.core.lib.serviceitem.os.path.exists')
     @patch('openlp.core.lib.serviceitem.AppLocation.get_section_data_path')
-    def test_service_item_load_image_from_local_service(self, mocked_get_section_data_path, mocked_exists):
+    def test_service_item_load_image_from_local_service(self, mocked_get_section_data_path, mocked_exists,
+                                                        mocked_sha256_file_hash):
         """
         Test the Service Item - adding an image from a saved local service
         """
@@ -187,12 +191,13 @@
         image_name2 = 'image_2.jpg'
         test_file1 = os.path.join('/home', 'openlp', image_name1)
         test_file2 = os.path.join('/home', 'openlp', image_name2)
-        frame_array1 = {'path': test_file1, 'title': image_name1}
-        frame_array2 = {'path': test_file2, 'title': image_name2}
+        frame_array1 = {'path': test_file1, 'file_hash': 'fake_file_hash', 'title': {'path': test_file1, 'title': image_name1}}
+        frame_array2 = {'path': test_file2, 'file_hash': 'fake_file_hash', 'title': {'path': test_file2, 'title': image_name2}}
         service_item = ServiceItem(None)
         service_item.add_icon = MagicMock()
         service_item2 = ServiceItem(None)
         service_item2.add_icon = MagicMock()
+        mocked_sha256_file_hash.return_value = 'fake_file_hash'
 
         # WHEN: adding an image from a saved Service and mocked exists
         line = convert_file_service_item(TEST_PATH, 'serviceitem_image_2.osj')
@@ -215,8 +220,8 @@
                 'The frame path should match the full path to the image'
             assert test_file2 == str(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'
+        assert image_name1 == service_item.get_frame_title(0)['title'], 'The 1st frame title should match the image name'
+        assert image_name2 == service_item2.get_frame_title(0)['title'], 'The 2nd frame title should match the image name'
         assert service_item.name == service_item.title.lower(), \
             'The plugin name should match the display title, as there are > 1 Images'
         assert service_item.is_image() is True, 'This service item should be of an "image" type'
@@ -229,7 +234,8 @@
         assert service_item.is_capable(ItemCapabilities.CanAppend) is True, \
             'This service item should be able to have new items added to it'
 
-    def test_add_from_command_for_a_presentation(self):
+    @patch('openlp.core.lib.serviceitem.sha256_file_hash')
+    def test_add_from_command_for_a_presentation(self, mocked_sha256_file_hash):
         """
         Test the Service Item - adding a presentation
         """
@@ -249,7 +255,8 @@
         assert service_item.service_item_type == ServiceItemType.Command, 'It should be a Command'
         assert service_item.get_frames()[0] == frame, 'Frames should match'
 
-    def test_add_from_command_without_display_title_and_notes(self):
+    @patch('openlp.core.lib.serviceitem.sha256_file_hash')
+    def test_add_from_comamnd_without_display_title_and_notes(self, mocked_sha256_file_hash):
         """
         Test the Service Item - add from command, but not presentation
         """
@@ -267,13 +274,16 @@
         assert service_item.service_item_type == ServiceItemType.Command, 'It should be a Command'
         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(self, mocked_get_section_data_path, mocked_image_manager):
+    @patch('openlp.core.lib.serviceitem.sha256_file_hash')
+    def test_add_from_command_for_a_presentation_thumb(self, mocked_sha256_file_hash, mocked_get_section_data_path,
+                                                       mocked_image_manager):
         """
         Test the Service Item - adding a presentation, updating the thumb path & adding the thumb to image_manager
         """
         # GIVEN: A service item, a mocked AppLocation and presentation data
+        mocked_sha256_file_hash.return_value = 'fake_file_hash'
         mocked_get_section_data_path.return_value = Path('mocked') / 'section' / 'path'
         service_item = ServiceItem(None)
         service_item.add_capability(ItemCapabilities.HasThumbnails)
@@ -283,8 +293,7 @@
         thumb = Path('tmp') / 'test' / 'thumb.png'
         display_title = 'DisplayTitle'
         notes = 'Note1\nNote2\n'
-        expected_thumb_path = Path('mocked') / 'section' / 'path' / 'thumbnails' / \
-            md5_hash(str(TEST_PATH / presentation_name).encode('utf8')) / 'thumb.png'
+        expected_thumb_path = Path('mocked') / 'section' / 'path' / 'thumbnails' / 'fake_file_hash' / 'thumb.png'
         frame = {'title': presentation_name, 'image': str(expected_thumb_path), 'path': str(TEST_PATH),
                  'display_title': display_title, 'notes': notes, 'thumbnail': str(expected_thumb_path)}
 
@@ -296,7 +305,8 @@
         assert service_item.get_frames()[0] == frame, 'Frames should match'
         # assert 1 == mocked_image_manager.add_image.call_count, 'image_manager should be used'
 
-    def test_service_item_load_optical_media_from_service(self):
+    @patch('openlp.core.lib.serviceitem.sha256_file_hash')
+    def test_service_item_load_optical_media_from_service(self, mocked_sha256_file_hash):
         """
         Test the Service Item - load an optical media item
         """

=== modified file 'tests/functional/openlp_plugins/images/test_lib.py'
--- tests/functional/openlp_plugins/images/test_lib.py	2019-05-22 06:47:00 +0000
+++ tests/functional/openlp_plugins/images/test_lib.py	2019-09-15 09:05:37 +0000
@@ -101,8 +101,9 @@
         assert self.media_item.manager.save_object.call_count == 0, \
             'The save_object() method should not have been called'
 
+    @patch('openlp.plugins.images.lib.mediaitem.sha256_file_hash')
     @patch('openlp.plugins.images.lib.mediaitem.ImageMediaItem.load_full_list')
-    def test_save_new_images_list_single_image_with_reload(self, mocked_load_full_list):
+    def test_save_new_images_list_single_image_with_reload(self, mocked_load_full_list, mocked_sha256_file_hash):
         """
         Test that the save_new_images_list() calls load_full_list() when reload_list is set to True
         """
@@ -110,6 +111,7 @@
         image_list = [Path('test_image.jpg')]
         ImageFilenames.file_path = None
         self.media_item.manager = MagicMock()
+        mocked_sha256_file_hash.return_value = 'fake_file_hash'
 
         # WHEN: We run save_new_images_list with reload_list=True
         self.media_item.save_new_images_list(image_list, reload_list=True)
@@ -120,14 +122,16 @@
         # CLEANUP: Remove added attribute from ImageFilenames
         delattr(ImageFilenames, 'file_path')
 
+    @patch('openlp.plugins.images.lib.mediaitem.sha256_file_hash')
     @patch('openlp.plugins.images.lib.mediaitem.ImageMediaItem.load_full_list')
-    def test_save_new_images_list_single_image_without_reload(self, mocked_load_full_list):
+    def test_save_new_images_list_single_image_without_reload(self, mocked_load_full_list, mocked_sha256_file_hash):
         """
         Test that the save_new_images_list() doesn't call load_full_list() when reload_list is set to False
         """
         # GIVEN: A list with 1 image and a mocked out manager
         image_list = [Path('test_image.jpg')]
         self.media_item.manager = MagicMock()
+        mocked_sha256_file_hash.return_value = 'fake_file_hash'
 
         # WHEN: We run save_new_images_list with reload_list=False
         self.media_item.save_new_images_list(image_list, reload_list=False)
@@ -135,14 +139,16 @@
         # THEN: load_full_list() should not have been called
         assert mocked_load_full_list.call_count == 0, 'load_full_list() should not have been called'
 
+    @patch('openlp.plugins.images.lib.mediaitem.sha256_file_hash')
     @patch('openlp.plugins.images.lib.mediaitem.ImageMediaItem.load_full_list')
-    def test_save_new_images_list_multiple_images(self, mocked_load_full_list):
+    def test_save_new_images_list_multiple_images(self, mocked_load_full_list, mocked_sha256_file_hash):
         """
         Test that the save_new_images_list() saves all images in the list
         """
         # GIVEN: A list with 3 images
         image_list = [Path('test_image_1.jpg'), Path('test_image_2.jpg'), Path('test_image_3.jpg')]
         self.media_item.manager = MagicMock()
+        mocked_sha256_file_hash.return_value = 'fake_file_hash'
 
         # WHEN: We run save_new_images_list with the list of 3 images
         self.media_item.save_new_images_list(image_list, reload_list=False)
@@ -151,14 +157,16 @@
         assert self.media_item.manager.save_object.call_count == 3, \
             'load_full_list() should have been called three times'
 
+    @patch('openlp.plugins.images.lib.mediaitem.sha256_file_hash')
     @patch('openlp.plugins.images.lib.mediaitem.ImageMediaItem.load_full_list')
-    def test_save_new_images_list_other_objects_in_list(self, mocked_load_full_list):
+    def test_save_new_images_list_other_objects_in_list(self, mocked_load_full_list, mocked_sha256_file_hash):
         """
         Test that the save_new_images_list() ignores everything in the provided list except strings
         """
         # GIVEN: A list with images and objects
         image_list = [Path('test_image_1.jpg'), None, True, ImageFilenames(), Path('test_image_2.jpg')]
         self.media_item.manager = MagicMock()
+        mocked_sha256_file_hash.return_value = 'fake_file_hash'
 
         # WHEN: We run save_new_images_list with the list of images and objects
         self.media_item.save_new_images_list(image_list, reload_list=False)
@@ -217,12 +225,15 @@
             returned_object1 = ImageFilenames()
             returned_object1.id = 1
             returned_object1.file_path = Path('/', 'tmp', 'test_file_1.jpg')
+            returned_object1.file_hash = 'fake_file_hash'
             returned_object2 = ImageFilenames()
             returned_object2.id = 2
             returned_object2.file_path = Path('/', 'tmp', 'test_file_2.jpg')
+            returned_object2.file_hash = 'fake_file_hash'
             returned_object3 = ImageFilenames()
             returned_object3.id = 3
             returned_object3.file_path = Path('/', 'tmp', 'test_file_3.jpg')
+            returned_object3.file_hash = 'fake_file_hash'
             return [returned_object1, returned_object2, returned_object3]
         if args[1] == ImageGroups and args[2]:
             # Change the parent_id that is matched so we don't get into an endless loop
@@ -233,9 +244,10 @@
             return [returned_object1]
         return []
 
+    @patch('openlp.plugins.images.lib.mediaitem.sha256_file_hash')
     @patch('openlp.plugins.images.lib.mediaitem.delete_file')
     @patch('openlp.plugins.images.lib.mediaitem.check_item_selected')
-    def test_on_delete_click(self, mocked_check_item_selected, mocked_delete_file):
+    def test_on_delete_click(self, mocked_check_item_selected, mocked_delete_file, mocked_sha256_file_hash):
         """
         Test that on_delete_click() works
         """
@@ -245,6 +257,7 @@
         test_image.id = 1
         test_image.group_id = 1
         test_image.file_path = Path('imagefile.png')
+        test_image.file_hash = 'fake_file_hash'
         self.media_item.manager = MagicMock()
         self.media_item.service_path = Path()
         self.media_item.list_view = MagicMock()
@@ -252,6 +265,7 @@
         mocked_row_item.data.return_value = test_image
         mocked_row_item.text.return_value = ''
         self.media_item.list_view.selectedItems.return_value = [mocked_row_item]
+        mocked_sha256_file_hash.return_value = 'fake_file_hash'
 
         # WHEN: Calling on_delete_click
         self.media_item.on_delete_click()

=== modified file 'tests/interfaces/openlp_core/widgets/test_views.py'
--- tests/interfaces/openlp_core/widgets/test_views.py	2019-04-13 13:00:22 +0000
+++ tests/interfaces/openlp_core/widgets/test_views.py	2019-09-15 09:05:37 +0000
@@ -79,7 +79,8 @@
         # THEN: The number of the current item should be -1.
         assert self.preview_widget.current_slide_number() == -1, 'The slide number should be -1.'
 
-    def test_replace_service_item(self):
+    @patch('openlp.core.lib.serviceitem.sha256_file_hash')
+    def test_replace_service_item(self, mocked_sha256_file_hash):
         """
         Test item counts and current number with a service item.
         """
@@ -94,7 +95,8 @@
         assert self.preview_widget.slide_count() == 2, 'The slide count should be 2.'
         assert self.preview_widget.current_slide_number() == 1, 'The current slide number should  be 1.'
 
-    def test_change_slide(self):
+    @patch('openlp.core.lib.serviceitem.sha256_file_hash')
+    def test_change_slide(self, mocked_sha256_file_hash):
         """
         Test the change_slide method.
         """