Fix preview flicker

fixes #553
This line is not needed as the display will move
to the first slide by default.

.gitignore

@@ -48,4 +48,4 @@ package-lock.json
 tags
 test
 openlp-test-projectordb.sqlite
-Chromium_80.0.3987_(Linux_0.0.0)/
+*/test-results.xml

openlp/core/display/html/display.js

@@ -985,6 +985,29 @@ var Display = {
       Display._theme = theme;
     }
   },
+  /**
+   * Set background image, replaced when theme is updated/applied
+   * @param bg_color Colour behind the image
+   * @param image_path Image path
+   */
+  setBackgroundImage: function (bg_color, image_path) {
+    var targetElement = $(".slides > section")[0];
+    targetElement.setAttribute("data-background", "url('" + image_path + "')");
+    targetElement.setAttribute("data-background-size", "cover");
+    Reveal.sync();
+  },
+  /**
+   * Reset/reapply the theme
+   */
+  resetTheme: function () {
+    var targetElement = $(".slides > section")[0];
+    if (!targetElement) {
+      console.warn("Couldn't reset theme: No slides exist");
+      return;
+    }
+    Display.applyTheme(targetElement, targetElement.classList.contains("text-slides"));
+    Reveal.sync();
+  },
   /**
    * Apply the theme to the provided element
    * @param targetElement The target element to apply the theme (expected to be a <section> in the slides container)

openlp/core/display/window.py

@@ -202,6 +202,11 @@ class DisplayWindow(QtWidgets.QWidget, RegistryProperties, LogMixin):
         self.setGeometry(screen.display_geometry)
         self.screen_number = screen.number
 
+    def set_background_image(self, bg_color, image_path):
+        image_uri = image_path.as_uri()
+        self.run_javascript('Display.setBackgroundImage("{bg_color}", "{image}");'.format(bg_color=bg_color,
+                                                                                          image=image_uri))
+
     def set_single_image(self, bg_color, image_path):
         """
         :param str bg_color: Background color
@@ -415,6 +420,14 @@ class DisplayWindow(QtWidgets.QWidget, RegistryProperties, LogMixin):
         exported_theme = theme_copy.export_theme(is_js=True)
         self.run_javascript('Display.setTheme({theme});'.format(theme=exported_theme), is_sync=is_sync)
 
+    def reload_theme(self):
+        """
+        Applies the set theme
+        DO NOT use this when changing slides. Only use this if you need to force an update
+        to the current visible slides.
+        """
+        self.run_javascript('Display.resetTheme();')
+
     def get_video_types(self):
         """
         Get the types of videos playable by the embedded media player

openlp/core/ui/slidecontroller.py

@@ -868,6 +868,26 @@ class SlideController(QtWidgets.QWidget, LogMixin, RegistryProperties):
                 self.delay_spin_box.setValue(int(item.timed_slide_interval))
                 self.on_play_slides_once()
 
+    def set_background_image(self, bg_color, image_path):
+        """
+        Reload the theme on displays.
+        """
+        # Set theme for preview
+        self.preview_display.set_background_image(bg_color, image_path)
+        # Set theme for displays
+        for display in self.displays:
+            display.set_background_image(bg_color, image_path)
+
+    def reload_theme(self):
+        """
+        Reload the theme on displays.
+        """
+        # Set theme for preview
+        self.preview_display.reload_theme()
+        # Set theme for displays
+        for display in self.displays:
+            display.reload_theme()
+
     def _set_theme(self, service_item):
         """
         Set up the theme from the service item.
@@ -893,7 +913,6 @@ class SlideController(QtWidgets.QWidget, LogMixin, RegistryProperties):
         old_item = self.service_item
         # rest to allow the remote pick up verse 1 if large imaged
         self.selected_row = 0
-        self.preview_display.go_to_slide(0)
         # take a copy not a link to the servicemanager copy.
         self.service_item = copy.copy(service_item)
         if self.service_item.is_command():

openlp/core/ui/themeform.py

@@ -433,5 +433,6 @@ class ThemeForm(QtWidgets.QWizard, Ui_ThemeWizard, RegistryProperties):
             return
         # 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())
+        self.theme_manager.save_theme(self.theme)
+        self.theme_manager.save_preview(self.theme.theme_name, self.preview_box.save_screenshot())
         return QtWidgets.QDialog.accept(self)

openlp/core/ui/thememanager.py

@@ -353,6 +353,7 @@ class ThemeManager(QtWidgets.QWidget, RegistryBase, Ui_ThemeManager, LogMixin, R
         theme_data.theme_name = new_theme_name
         theme_data.extend_image_filename(self.theme_path)
         self.save_theme(theme_data, background_override=old_background)
+        self.update_preview_images([new_theme_name])
         self.load_themes()
 
     def on_edit_theme(self, field=None):
@@ -479,10 +480,9 @@ class ThemeManager(QtWidgets.QWidget, RegistryBase, Ui_ThemeManager, LogMixin, R
         self.application.set_busy_cursor()
         new_themes = []
         for file_path in file_paths:
-            new_themes.append(self.unzip_theme(file_path, self.theme_path))
+            new_themes.append(self.unzip_theme(file_path))
         self.settings.setValue('themes/last directory import', file_path.parent)
         self.update_preview_images(new_themes)
-        self.load_themes()
         self.application.set_normal_cursor()
 
     def load_first_time_themes(self):
@@ -494,7 +494,7 @@ class ThemeManager(QtWidgets.QWidget, RegistryBase, Ui_ThemeManager, LogMixin, R
         new_themes = []
         for theme_path in theme_paths:
             theme_path = self.theme_path / theme_path
-            new_themes.append(self.unzip_theme(theme_path, self.theme_path))
+            new_themes.append(self.unzip_theme(theme_path))
             delete_file(theme_path)
         # No themes have been found so create one
         if not theme_paths:
@@ -582,12 +582,11 @@ class ThemeManager(QtWidgets.QWidget, RegistryBase, Ui_ThemeManager, LogMixin, R
                                              defaultButton=QtWidgets.QMessageBox.No)
         return ret == QtWidgets.QMessageBox.Yes
 
-    def unzip_theme(self, file_path, directory_path):
+    def unzip_theme(self, file_path):
         """
         Unzip the theme, remove the preview file if stored. Generate a new preview file. Check the XML theme version
         and upgrade if necessary.
         :param Path file_path:
-        :param Path directory_path:
         """
         self.log_debug('Unzipping theme {name}'.format(name=file_path))
         file_xml = None
@@ -614,7 +613,7 @@ class ThemeManager(QtWidgets.QWidget, RegistryBase, Ui_ThemeManager, LogMixin, R
                     new_theme.load_theme(theme_zip.read(json_file[0]).decode("utf-8"))
                     theme_name = new_theme.theme_name
                     json_theme = True
-                theme_folder = directory_path / theme_name
+                theme_folder = self.theme_path / theme_name
                 if theme_folder.exists() and not self.over_write_message_box(theme_name):
                     abort_import = True
                     return
@@ -626,7 +625,7 @@ class ThemeManager(QtWidgets.QWidget, RegistryBase, Ui_ThemeManager, LogMixin, R
                     if split_name[-1] == '' or len(split_name) == 1:
                         # is directory or preview file
                         continue
-                    full_name = directory_path / zipped_file_rel_path
+                    full_name = self.theme_path / zipped_file_rel_path
                     create_paths(full_name.parent)
                     if zipped_file_rel_path.suffix.lower() == '.xml' or zipped_file_rel_path.suffix.lower() == '.json':
                         file_xml = str(theme_zip.read(zipped_file), 'utf-8')
@@ -643,12 +642,15 @@ class ThemeManager(QtWidgets.QWidget, RegistryBase, Ui_ThemeManager, LogMixin, R
                                                  'inaccessible or not a valid theme.').format(file_name=file_path))
         finally:
             if not abort_import:
-                # As all files are closed, we can create the Theme.
+                # TODO: remove XML handling after once the upgrade path from 2.4 is no longer required
-                if file_xml:
+                # As all files are closed, upgrade theme (xml to json) if needed.
-                    if json_theme:
+                if file_xml and not json_theme:
-                        self._create_theme_from_json(file_xml, self.theme_path)
+                    theme_path = self.theme_path / theme_name
-                    else:
+                    xml_file_paths = theme_path.glob('*.xml')
-                        self._create_theme_from_xml(file_xml, self.theme_path)
+                    for xml_file_path in xml_file_paths:
+                        xml_file_path.unlink()
+                    theme = self._create_theme_from_xml(file_xml, self.theme_path)
+                    self.save_theme(theme)
                 return theme_name
             else:
                 return None
@@ -668,7 +670,7 @@ class ThemeManager(QtWidgets.QWidget, RegistryBase, Ui_ThemeManager, LogMixin, R
             return False
         return True
 
-    def save_theme(self, theme, image=None, background_override=None):
+    def save_theme(self, theme, background_override=None):
         """
         Writes the theme to the disk and including the background image and thumbnail if necessary
 
@@ -700,15 +702,6 @@ class ThemeManager(QtWidgets.QWidget, RegistryBase, Ui_ThemeManager, LogMixin, R
                     shutil.copyfile(background_file, theme.background_filename)
                 except OSError:
                     self.log_exception('Failed to save theme image')
-        if image:
-            sample_path_name = self.theme_path / '{file_name}.png'.format(file_name=name)
-            if sample_path_name.exists():
-                sample_path_name.unlink()
-            image.save(str(sample_path_name), 'png')
-            thumb_path = self.thumb_path / '{name}.png'.format(name=name)
-            create_thumb(sample_path_name, thumb_path, False)
-        else:
-            self.update_preview_images([name])
 
     def save_preview(self, theme_name, preview_pixmap):
         """

openlp/plugins/images/lib/mediaitem.py

@@ -72,7 +72,8 @@ class ImageMediaItem(MediaManagerItem):
         self.add_group_form = AddGroupForm(self)
         self.fill_groups_combobox(self.choose_group_form.group_combobox)
         self.fill_groups_combobox(self.add_group_form.parent_group_combobox)
-        Registry().register_function('live_theme_changed', self.live_theme_changed)
+        Registry().register_function('live_theme_changed', self.on_display_changed)
+        Registry().register_function('slidecontroller_live_started', self.on_display_changed)
         # Allow DnD from the desktop.
         self.list_view.activateDnD()
 
@@ -663,9 +664,9 @@ class ImageMediaItem(MediaManagerItem):
         """
         self.reset_action.setVisible(False)
         self.reset_action_context.setVisible(False)
-        self.live_controller.display.reset_image()
+        self.live_controller.reload_theme()
 
-    def live_theme_changed(self):
+    def on_display_changed(self, service_item=None):
         """
         Triggered by the change of theme in the slide controller.
         """
@@ -686,13 +687,9 @@ class ImageMediaItem(MediaManagerItem):
                 return
             file_path = bitem.data(0, QtCore.Qt.UserRole).file_path
             if file_path.exists():
-                if self.live_controller.display.direct_image(str(file_path), background):
+                self.live_controller.set_background_image(background, file_path)
                 self.reset_action.setVisible(True)
                 self.reset_action_context.setVisible(True)
-                else:
-                    critical_error_message_box(
-                        UiStrings().LiveBGError,
-                        translate('ImagePlugin.MediaItem', 'There was no display item to amend.'))
             else:
                 critical_error_message_box(
                     UiStrings().LiveBGError,

tests/functional/openlp_core/ui/test_thememanager.py

@@ -309,19 +309,18 @@ def test_unzip_theme(registry):
     with patch('openlp.core.ui.thememanager.critical_error_message_box') \
             as mocked_critical_error_message_box:
         theme_manager = ThemeManager(None)
-        theme_manager._create_theme_from_xml = MagicMock()
         theme_manager.update_preview_images = MagicMock()
-        theme_manager.theme_path = None
+        theme_manager.theme_path = Path(mkdtemp())
-        folder_path = Path(mkdtemp())
         theme_file_path = RESOURCE_PATH / 'themes' / 'Moss_on_tree.otz'
 
         # WHEN: We try to unzip it
-        theme_manager.unzip_theme(theme_file_path, folder_path)
+        theme_manager.unzip_theme(theme_file_path)
 
-        # THEN: Files should be unpacked
+        # THEN: Files should be unpacked AND xml file should be upgraded to json
-        assert (folder_path / 'Moss on tree' / 'Moss on tree.xml').exists() is True
+        assert (theme_manager.theme_path / 'Moss on tree' / 'Moss on tree.xml').exists() is False
+        assert (theme_manager.theme_path / 'Moss on tree' / 'Moss on tree.json').exists() is True
         assert mocked_critical_error_message_box.call_count == 0, 'No errors should have happened'
-        shutil.rmtree(folder_path)
+        shutil.rmtree(theme_manager.theme_path)
 
 
 def test_unzip_theme_invalid_version(registry):
@@ -337,9 +336,10 @@ def test_unzip_theme_invalid_version(registry):
         mocked_zip_file.return_value = MagicMock(**{'namelist.return_value': [os.path.join('theme', 'theme.xml')]})
         mocked_getroot.return_value = MagicMock(**{'get.return_value': None})
         theme_manager = ThemeManager(None)
+        theme_manager.theme_path = Path('folder')
 
         # WHEN: unzip_theme is called
-        theme_manager.unzip_theme(Path('theme.file'), Path('folder'))
+        theme_manager.unzip_theme(Path('theme.file'))
 
         # THEN: The critical_error_message_box should have been called
         assert mocked_critical_error_message_box.call_count == 1, 'Should have been called once'

tests/functional/openlp_plugins/images/test_mediaitem.py

@@ -181,10 +181,52 @@ def test_on_reset_click(media_item):
     # WHEN: on_reset_click is called
     media_item.on_reset_click()
 
-    # THEN: the reset_action should be set visible, and the image should be reset
+    # THEN: the reset_action should be set invisible, and the image should be reset
     media_item.reset_action.setVisible.assert_called_with(False)
     media_item.reset_action_context.setVisible.assert_called_with(False)
-    media_item.live_controller.display.reset_image.assert_called_with()
+    media_item.live_controller.reload_theme.assert_called_with()
+
+
+def test_on_display_changed(media_item):
+    """
+    Test that on_display_changed() hides the reset background button
+    """
+    # GIVEN: A mocked version of reset_action
+    media_item.reset_action = MagicMock()
+    media_item.reset_action_context = MagicMock()
+
+    # WHEN: on_display_changed is called
+    media_item.on_display_changed()
+
+    # THEN: the reset_action should be set invisible
+    media_item.reset_action.setVisible.assert_called_with(False)
+    media_item.reset_action_context.setVisible.assert_called_with(False)
+
+
+@patch('openlp.plugins.images.lib.mediaitem.check_item_selected')
+@patch('openlp.plugins.images.lib.mediaitem.isinstance')
+@patch('openlp.plugins.images.lib.mediaitem.QtGui.QColor')
+@patch('openlp.plugins.images.lib.mediaitem.Path.exists')
+def test_on_replace_click(mocked_exists, mocked_qcolor, mocked_isinstance, mocked_check_item_selected, media_item):
+    """
+    Test that on_replace_click() actually sets the background
+    """
+    # GIVEN: A mocked version of reset_action, and a (faked) existing selected image file
+    media_item.reset_action = MagicMock()
+    media_item.reset_action_context = MagicMock()
+    media_item.list_view = MagicMock()
+    mocked_check_item_selected.return_value = True
+    mocked_isinstance.return_value = True
+    mocked_exists.return_value = True
+    mocked_qcolor.return_value = 'BackgroundColor'
+
+    # WHEN: on_replace_click is called
+    media_item.on_replace_click()
+
+    # THEN: the reset_action should be set visible, and the image should be set
+    media_item.reset_action.setVisible.assert_called_with(True)
+    media_item.reset_action_context.setVisible.assert_called_with(True)
+    media_item.live_controller.set_background_image.assert_called_with('BackgroundColor', ANY)
 
 
 @patch('openlp.plugins.images.lib.mediaitem.delete_file')

tests/js/test_display.js

@@ -769,6 +769,41 @@ describe("Display.setImageSlides", function () {
   });
 });
 
+describe("Display.setBackgroundImage and Display.resetTheme", function () {
+  beforeEach(function() {
+    document.body.innerHTML = "";
+    var slides_container = _createDiv({"class": "slides"});
+    Display._slidesContainer = slides_container;
+    var section = document.createElement("section");
+    Display._slidesContainer.appendChild(section);
+  });
+
+  it("should set the background image data and call sync once for set slides and again for set background", function () {
+    spyOn(Reveal, "sync");
+    spyOn(Reveal, "slide");
+
+    Display.setBackgroundImage("#fff", "/file/path");
+
+    expect($(".slides > section")[0].getAttribute("data-background")).toEqual("url('/file/path')");
+    expect(Reveal.sync).toHaveBeenCalledTimes(1);
+  });
+
+  it("should restore the background image to the theme", function () {
+    Display._theme = {
+      'background_type': BackgroundType.Image,
+      'background_filename': '/another/path'
+    };
+    $(".slides > section")[0].setAttribute("data-background", "/file/path");
+    spyOn(Reveal, "sync");
+    spyOn(Reveal, "slide");
+
+    Display.resetTheme();
+
+    expect($(".slides > section")[0].getAttribute("data-background")).toEqual("url('/another/path')");
+    expect(Reveal.sync).toHaveBeenCalledTimes(1);
+  });
+});
+
 describe("Display.setVideo", function () {
   beforeEach(function() {
     document.body.innerHTML = "";