From c0aa199ac3ccc6dd42ebd24d054dae07632dbd2a Mon Sep 17 00:00:00 2001 From: Daniel Date: Wed, 23 Oct 2019 22:54:29 +0000 Subject: [PATCH] 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 | 1 + openlp/core/ui/themeform.py | 14 ++- openlp/core/ui/thememanager.py | 25 ++-- .../functional/openlp_core/lib/test_theme.py | 2 +- .../openlp_core/ui/test_thememanager.py | 119 +++++++++++++++--- 5 files changed, 126 insertions(+), 35 deletions(-) diff --git a/openlp/core/lib/theme.py b/openlp/core/lib/theme.py index 356cbff64..bc3cbce5c 100644 --- a/openlp/core/lib/theme.py +++ b/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): diff --git a/openlp/core/ui/themeform.py b/openlp/core/ui/themeform.py index 7ff49784d..040aad980 100644 --- a/openlp/core/ui/themeform.py +++ b/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) diff --git a/openlp/core/ui/thememanager.py b/openlp/core/ui/thememanager.py index ca31466e2..2a447b7a9 100644 --- a/openlp/core/ui/thememanager.py +++ b/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 - source_path = theme_data.background_filename + theme_data.background_filename = self.theme_path / new_theme_name / theme_data.background_filename.name 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 Path image_destination_path: Where the Theme Image is to be saved to - :param image: The example image of the theme. Optionally. + :param image: The theme thumbnail. 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 self.old_background_image_path and image_destination_path != self.old_background_image_path: + if theme.background_source and theme.background_filename: + 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: diff --git a/tests/functional/openlp_core/lib/test_theme.py b/tests/functional/openlp_core/lib/test_theme.py index fb353171c..48b3e5367 100644 --- a/tests/functional/openlp_core/lib/test_theme.py +++ b/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' diff --git a/tests/functional/openlp_core/ui/test_thememanager.py b/tests/functional/openlp_core/ui/test_thememanager.py index 7f89d8099..230c3014e 100644 --- a/tests/functional/openlp_core/ui/test_thememanager.py +++ b/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 - file_path_1 = RESOURCE_PATH / 'church.jpg' - file_path_2 = RESOURCE_PATH / 'church2.jpg' - theme_manager.save_theme(mocked_theme, file_path_1, file_path_2) + # WHEN: Calling save_theme with both background paths to different images + theme_manager.save_theme(mocked_theme) - # 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()