Add background_source to Theme and use it in the theme wizard

The theme wizard (while editing a theme) now changes both background_source and background_filename then sets filename to the new cache location on apply (also added the filename_source var to Theme). The new var is needed so when the user goes to change the theme again, they see the file they selected, not the cached one. This commit fixes the issue of changing theme backgrounds not deleting the cached file but deleting the original file.

openlp/core/lib/theme.py

@@ -169,6 +169,7 @@ class Theme(object):
         jsn = get_text_file_string(json_path)
         self.load_theme(jsn)
         self.background_filename = None
+        self.background_source = None
         self.version = 2
 
     def expand_json(self, var, prev=None):

openlp/core/ui/themeform.py

@@ -280,7 +280,7 @@ class ThemeForm(QtWidgets.QWizard, Ui_ThemeWizard, RegistryProperties):
         Run the wizard.
         """
         log.debug('Editing theme {name}'.format(name=self.theme.theme_name))
-        self.temp_background_filename = None
+        self.temp_background_filename = self.theme.background_source
         self.update_theme_allowed = False
         self.set_defaults()
         self.update_theme_allowed = True
@@ -325,11 +325,11 @@ class ThemeForm(QtWidgets.QWizard, Ui_ThemeWizard, RegistryProperties):
             self.setField('background_type', 1)
         elif self.theme.background_type == BackgroundType.to_string(BackgroundType.Image):
             self.image_color_button.color = self.theme.background_border_color
-            self.image_path_edit.path = self.theme.background_filename
+            self.image_path_edit.path = self.theme.background_source
             self.setField('background_type', 2)
         elif self.theme.background_type == BackgroundType.to_string(BackgroundType.Video):
             self.video_color_button.color = self.theme.background_border_color
-            self.video_path_edit.path = self.theme.background_filename
+            self.video_path_edit.path = self.theme.background_source
             self.setField('background_type', 4)
         elif self.theme.background_type == BackgroundType.to_string(BackgroundType.Stream):
             self.setField('background_type', 5)
@@ -467,6 +467,7 @@ class ThemeForm(QtWidgets.QWizard, Ui_ThemeWizard, RegistryProperties):
         :param pathlib.Path new_path: Path to the new image
         :rtype: None
         """
+        self.theme.background_source = new_path
         self.theme.background_filename = new_path
         self.set_background_page_values()
 
@@ -477,6 +478,7 @@ class ThemeForm(QtWidgets.QWizard, Ui_ThemeWizard, RegistryProperties):
         :param pathlib.Path new_path: Path to the new video
         :rtype: None
         """
+        self.theme.background_source = new_path
         self.theme.background_filename = new_path
         self.set_background_page_values()
 
@@ -553,14 +555,14 @@ class ThemeForm(QtWidgets.QWizard, Ui_ThemeWizard, RegistryProperties):
                 translate('OpenLP.ThemeWizard', 'Theme Name Invalid'),
                 translate('OpenLP.ThemeWizard', 'Invalid theme name. Please enter one.'))
             return
-        source_path = None
         destination_path = None
         if self.theme.background_type == BackgroundType.to_string(BackgroundType.Image) or \
            self.theme.background_type == BackgroundType.to_string(BackgroundType.Video):
             file_name = self.theme.background_filename.name
             destination_path = self.path / self.theme.theme_name / file_name
-            source_path = self.theme.background_filename
         if not self.edit_mode and not self.theme_manager.check_if_theme_exists(self.theme.theme_name):
             return
-        self.theme_manager.save_theme(self.theme, source_path, destination_path, self.preview_box.save_screenshot())
+        # Set the theme background to the cache location
+        self.theme.background_filename = destination_path
+        self.theme_manager.save_theme(self.theme, self.preview_box.save_screenshot())
         return QtWidgets.QDialog.accept(self)

openlp/core/ui/thememanager.py

@@ -325,14 +325,11 @@ class ThemeManager(QtWidgets.QWidget, RegistryBase, Ui_ThemeManager, LogMixin, R
         :param str new_theme_name: The new theme name of the theme
         :rtype: None
         """
-        destination_path = None
-        source_path = None
         if theme_data.background_type == 'image' or theme_data.background_type == 'video':
-            destination_path = self.theme_path / new_theme_name / theme_data.background_filename.name
+            theme_data.background_filename = self.theme_path / new_theme_name / theme_data.background_filename.name
-            source_path = theme_data.background_filename
         theme_data.theme_name = new_theme_name
         theme_data.extend_image_filename(self.theme_path)
-        self.save_theme(theme_data, source_path, destination_path)
+        self.save_theme(theme_data)
         self.load_themes()
 
     def on_edit_theme(self, field=None):
@@ -648,14 +645,12 @@ class ThemeManager(QtWidgets.QWidget, RegistryBase, Ui_ThemeManager, LogMixin, R
             return False
         return True
 
-    def save_theme(self, theme, image_source_path=None, image_destination_path=None, image=None):
+    def save_theme(self, theme, image=None):
         """
-        Writes the theme to the disk and handles the background image if necessary
+        Writes the theme to the disk and including the background image and thumbnail if necessary
 
         :param Theme theme: The theme data object.
-        :param Path image_source_path: Where the theme image is currently located.
+        :param image: The theme thumbnail. Optionally.
-        :param Path image_destination_path: Where the Theme Image is to be saved to
-        :param image: The example image of the theme. Optionally.
         :rtype: None
         """
         name = theme.theme_name
@@ -667,12 +662,14 @@ class ThemeManager(QtWidgets.QWidget, RegistryBase, Ui_ThemeManager, LogMixin, R
             theme_path.write_text(theme_pretty)
         except OSError:
             self.log_exception('Saving theme to file failed')
-        if image_source_path and image_destination_path:
+        if theme.background_source and theme.background_filename:
-            if self.old_background_image_path and image_destination_path != self.old_background_image_path:
+            if self.old_background_image_path and theme.background_filename != self.old_background_image_path:
                 delete_file(self.old_background_image_path)
-            if image_source_path != image_destination_path:
+            if not theme.background_source.exists():
+                self.log_warning('Background does not exist, retaining cached background')
+            elif theme.background_source != theme.background_filename:
                 try:
-                    shutil.copyfile(image_source_path, image_destination_path)
+                    shutil.copyfile(theme.background_source, theme.background_filename)
                 except OSError:
                     self.log_exception('Failed to save theme image')
         if image:

tests/functional/openlp_core/lib/test_theme.py

@@ -181,4 +181,4 @@ class TestTheme(TestCase):
         assert 0 == theme.display_vertical_align, 'display_vertical_align should be 0'
         assert theme.font_footer_bold is False, 'font_footer_bold should be False'
         assert 'Arial' == theme.font_main_name, 'font_main_name should be "Arial"'
-        assert 48 == len(theme.__dict__), 'The theme should have 48 attributes'
+        assert 49 == len(theme.__dict__), 'The theme should have 49 attributes'

tests/functional/openlp_core/ui/test_thememanager.py

@@ -87,19 +87,22 @@ class TestThemeManager(TestCase):
         Test that we don't try to overwrite a theme background image with itself
         """
         # GIVEN: A new theme manager instance, with mocked builtins.open, copyfile,
-        #        theme, create_paths and thememanager-attributes.
+        #        theme, create_paths, thememanager-attributes and background
+        #       .filename path is the same as the source path.
         theme_manager = ThemeManager(None)
-        theme_manager.old_background_image = None
+        theme_manager.old_background_image_path = None
         theme_manager.update_preview_images = MagicMock()
         theme_manager.theme_path = MagicMock()
         mocked_theme = MagicMock()
         mocked_theme.theme_name = 'themename'
         mocked_theme.extract_formatted_xml = MagicMock()
         mocked_theme.extract_formatted_xml.return_value = 'fake_theme_xml'.encode()
-
-        # WHEN: Calling save_theme with path to the same image, but the path written slightly different
         file_path_1 = RESOURCE_PATH / 'church.jpg'
-        theme_manager.save_theme(mocked_theme, file_path_1, file_path_1)
+        mocked_theme.background_filename = file_path_1
+        mocked_theme.background_source = file_path_1
+
+        # WHEN: Calling save_theme with both background paths to the same image
+        theme_manager.save_theme(mocked_theme)
 
         # THEN: The mocked_copyfile should not have been called
         assert mocked_shutil.copyfile.called is False, 'copyfile should not be called'
@@ -111,30 +114,118 @@ class TestThemeManager(TestCase):
         Test that we do overwrite a theme background image when a new is submitted
         """
         # GIVEN: A new theme manager instance, with mocked builtins.open, copyfile,
-        #        theme, create_paths and thememanager-attributes.
+        #        theme, create_paths, thememanager-attributes and background
+        #       .filename path is the same as the source path.
         theme_manager = ThemeManager(None)
-        theme_manager.old_background_image = None
+        theme_manager.old_background_image_path = None
         theme_manager.update_preview_images = MagicMock()
         theme_manager.theme_path = MagicMock()
         mocked_theme = MagicMock()
         mocked_theme.theme_name = 'themename'
-        mocked_theme.filename = "filename"
+        mocked_theme.background_filename = RESOURCE_PATH / 'church.jpg'
+        mocked_theme.background_source = RESOURCE_PATH / 'church2.jpg'
 
-        # WHEN: Calling save_theme with path to different images
+        # WHEN: Calling save_theme with both background paths to different images
-        file_path_1 = RESOURCE_PATH / 'church.jpg'
+        theme_manager.save_theme(mocked_theme)
-        file_path_2 = RESOURCE_PATH / 'church2.jpg'
-        theme_manager.save_theme(mocked_theme, file_path_1, file_path_2)
 
-        # THEN: The mocked_copyfile should not have been called
+        # THEN: The mocked_copyfile should have been called
         assert mocked_shutil.copyfile.called is True, 'copyfile should be called'
 
+    @patch('openlp.core.ui.thememanager.shutil')
+    @patch('openlp.core.ui.thememanager.delete_file')
+    @patch('openlp.core.ui.thememanager.create_paths')
+    def test_save_theme_delete_old_image(self, mocked_create_paths, mocked_delete_file, mocked_shutil):
+        """
+        Test that we do delete a old theme background image when a new is submitted
+        """
+        # GIVEN: A new theme manager instance, with mocked builtins.open,
+        #        theme, create_paths, thememanager-attributes and background
+        #       .filename path is the same as the source path.
+        theme_manager = ThemeManager(None)
+        theme_manager.old_background_image_path = RESOURCE_PATH / 'old_church.png'
+        theme_manager.update_preview_images = MagicMock()
+        theme_manager.theme_path = MagicMock()
+        mocked_theme = MagicMock()
+        mocked_theme.theme_name = 'themename'
+        mocked_theme.background_filename = RESOURCE_PATH / 'church.jpg'
+        mocked_theme.background_source = RESOURCE_PATH / 'church2.jpg'
+
+        # WHEN: Calling save_theme with both background paths to different images
+        theme_manager.save_theme(mocked_theme)
+
+        # THEN: The mocked_delete_file should have been called to delete the old cached background
+        assert mocked_delete_file.called is True, 'delete_file should be called'
+
+    @patch.object(ThemeManager, 'log_exception')
+    @patch('openlp.core.ui.thememanager.delete_file')
+    @patch('openlp.core.ui.thememanager.create_paths')
+    def test_save_theme_missing_original(self, mocked_paths, mocked_delete, mocked_log_exception):
+        """
+        Test that we revert to the old theme background image if the source is missing
+        when changing the theme. (User doesn't change background but the original is
+        missing)
+        """
+        # GIVEN: A new theme manager instance, with invalid files. Setup as if the user
+        # has left the background the same, or reselected the same path.
+        # Not using resource dir because I could potentially copy a file
+        folder_path = Path(mkdtemp())
+        theme_manager = ThemeManager(None)
+        theme_manager.old_background_image_path = folder_path / 'old.png'
+        theme_manager.update_preview_images = MagicMock()
+        theme_manager.theme_path = MagicMock()
+        mocked_theme = MagicMock()
+        mocked_theme.theme_name = 'themename'
+        mocked_theme.background_filename = folder_path / 'old.png'
+        mocked_theme.background_source = folder_path / 'non_existent_original.png'
+
+        # WHEN: Calling save_theme with a invalid background_filename
+        # Although all filenames are considered invalid in this test,
+        # it is important it reverts to the old background path as this in reality is always
+        # valid unless someone has messed with the cache.
+        theme_manager.save_theme(mocked_theme)
+
+        # THEN: The old background should not have bee deleted
+        #       AND the filename should have been replaced with the old cached background
+        #       AND there is no exception
+        assert mocked_delete.called is False, 'delete_file should not be called'
+        assert mocked_theme.background_filename == theme_manager.old_background_image_path, \
+            'Background path should be reverted'
+        assert mocked_log_exception.called is False, \
+            'Should not have been an exception as the file wasn\'t changed'
+
+    @patch.object(ThemeManager, 'log_warning')
+    @patch('openlp.core.ui.thememanager.delete_file')
+    @patch('openlp.core.ui.thememanager.create_paths')
+    def test_save_theme_missing_new(self, mocked_paths, mocked_delete, mocked_log_warning):
+        """
+        Test that we log a warning if the new background is missing
+        """
+        # GIVEN: A new theme manager instance, with invalid files. Setup as if the user
+        # has changed the background to a invalid path.
+        # Not using resource dir because I could potentially copy a file
+        folder_path = Path(mkdtemp())
+        theme_manager = ThemeManager(None)
+        theme_manager.old_background_image_path = folder_path / 'old.png'
+        theme_manager.update_preview_images = MagicMock()
+        theme_manager.theme_path = MagicMock()
+        mocked_theme = MagicMock()
+        mocked_theme.theme_name = 'themename'
+        mocked_theme.background_filename = folder_path / 'new_cached.png'
+        mocked_theme.background_source = folder_path / 'new_original.png'
+
+        # WHEN: Calling save_theme with a invalid background_filename
+        theme_manager.save_theme(mocked_theme)
+
+        # THEN: A warning should have happened due to attempting to copy a missing file
+        mocked_log_warning.assert_called_once_with('Background does not exist, retaining cached background')
+
     def test_save_theme_special_char_name(self):
         """
         Test that we can save themes with special characters in the name
         """
         # GIVEN: A new theme manager instance, with mocked theme and thememanager-attributes.
         theme_manager = ThemeManager(None)
-        theme_manager.old_background_image = None
+        theme_manager.old_background_image_path = None
         theme_manager.update_preview_images = MagicMock()
         theme_manager.theme_path = Path(self.temp_folder)
         mocked_theme = MagicMock()