From 5793526bcb8d649b3f4303c461d931dd2b3b12a5 Mon Sep 17 00:00:00 2001 From: robbie jackson Date: Wed, 28 Apr 2021 07:10:27 +0000 Subject: [PATCH] Fix sequential presentations - proper fix --- .../presentations/lib/messagelistener.py | 23 +++++----- .../lib/presentationcontroller.py | 33 +++++++++++++- .../presentations/test_messagelistener.py | 2 +- .../test_presentationcontroller.py | 45 ++++++++++++++++++- 4 files changed, 89 insertions(+), 14 deletions(-) diff --git a/openlp/plugins/presentations/lib/messagelistener.py b/openlp/plugins/presentations/lib/messagelistener.py index 2e06515c1..6d75abf77 100644 --- a/openlp/plugins/presentations/lib/messagelistener.py +++ b/openlp/plugins/presentations/lib/messagelistener.py @@ -28,6 +28,7 @@ from openlp.core.common.registry import Registry from openlp.core.lib import ServiceItemContext from openlp.core.ui import HideMode from openlp.plugins.presentations.lib.pdfcontroller import PDF_CONTROLLER_FILETYPES +from openlp.plugins.presentations.lib.presentationcontroller import PresentationList log = logging.getLogger(__name__) @@ -49,21 +50,20 @@ class Controller(object): self.hide_mode = None log.info('{name} controller loaded'.format(name=live)) - def add_handler(self, controller, file, hide_mode, slide_no): + def add_handler(self, controller, file, hide_mode, slide_no, unique_id): """ Add a handler, which is an instance of a presentation and slidecontroller combination. If the slidecontroller has a display then load the presentation. """ log.debug('Live = {live}, add_handler {handler}'.format(live=self.is_live, handler=file)) self.controller = controller - if self.doc is not None: - self.shutdown() self.doc = self.controller.add_document(file) if not self.doc.load_presentation(): # Display error message to user # Inform slidecontroller that the action failed? self.doc.slidenumber = 0 return + PresentationList().add(self.doc, unique_id) self.doc.slidenumber = slide_no self.hide_mode = hide_mode log.debug('add_handler, slide_number: {slide:d}'.format(slide=slide_no)) @@ -205,15 +205,15 @@ class Controller(object): self.poll() return ret - def shutdown(self): + def shutdown(self, unique_id): """ Based on the handler passed at startup triggers slide show to shut down. """ log.debug('Live = {live}, shutdown'.format(live=self.is_live)) - if not self.doc: - return - self.doc.close_presentation() - self.doc = None + presentation_to_close = PresentationList().get_presentation_by_id(unique_id) + if presentation_to_close: + presentation_to_close.close_presentation() + PresentationList().remove(unique_id) def blank(self, hide_mode): """ @@ -365,7 +365,8 @@ class MessageListener(object): if self.handler is None: self.controller = controller else: - controller.add_handler(self.controllers[self.handler], file_path, hide_mode, message[3]) + controller.add_handler(self.controllers[self.handler], file_path, hide_mode, message[3], + message[0].unique_identifier) self.timer.start() def slide(self, message): @@ -443,10 +444,10 @@ class MessageListener(object): """ is_live = message[1] if is_live: - self.live_handler.shutdown() + self.live_handler.shutdown(message[0].unique_identifier) self.timer.stop() else: - self.preview_handler.shutdown() + self.preview_handler.shutdown(message[0].unique_identifier) def hide(self, message): """ diff --git a/openlp/plugins/presentations/lib/presentationcontroller.py b/openlp/plugins/presentations/lib/presentationcontroller.py index 73d9cc2a0..ca78c3ebe 100644 --- a/openlp/plugins/presentations/lib/presentationcontroller.py +++ b/openlp/plugins/presentations/lib/presentationcontroller.py @@ -24,7 +24,7 @@ from pathlib import Path from PyQt5 import QtCore -from openlp.core.common import md5_hash, sha256_file_hash +from openlp.core.common import Singleton, 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 @@ -386,6 +386,37 @@ class PresentationDocument(object): return self._sha256_file_hash +class PresentationList(metaclass=Singleton): + """ + This is a singleton class which maintains a list of instances for presentations + which have been started. + The document load_presentation() method is called several times - for example, when the + presentation files are being loaded into the library - but a document is included in this + PresentationList only when the presentation is actually displayed. + In this case the loading is initiated by a Registry 'presentation_start' event, the message + includes the service item, and the unique_identifier from the service item is used as the id + to differentiate the presentation document instances within this PresentationList. + The purpose of this is so that the 'presentation_stop' event, which also includes the service + item and its unique identifier, can result in the correct presentation being stopped. + This fixes issue #700 + """ + + def __init__(self): + self._presentations = {} + + def add(self, document, unique_id): + self._presentations[unique_id] = document + + def remove(self, unique_id): + del self._presentations[unique_id] + + def get_presentation_by_id(self, unique_id): + if unique_id in self._presentations: + return self._presentations[unique_id] + else: + return None + + class PresentationController(object): """ This class is used to control interactions with presentation applications by creating a runtime environment. diff --git a/tests/openlp_plugins/presentations/test_messagelistener.py b/tests/openlp_plugins/presentations/test_messagelistener.py index 86141c977..8ab1ed591 100644 --- a/tests/openlp_plugins/presentations/test_messagelistener.py +++ b/tests/openlp_plugins/presentations/test_messagelistener.py @@ -122,7 +122,7 @@ def test_add_handler_failure(): mocked_doc_controller.add_document.return_value = mocked_doc # WHEN: calling add_handler that fails - controller.add_handler(mocked_doc_controller, MagicMock(), True, 0) + controller.add_handler(mocked_doc_controller, MagicMock(), True, 0, "uuid") # THEN: slidenumber should be 0 assert controller.doc.slidenumber == 0, 'doc.slidenumber should be 0' diff --git a/tests/openlp_plugins/presentations/test_presentationcontroller.py b/tests/openlp_plugins/presentations/test_presentationcontroller.py index eb6a9f3b3..90ef03d04 100644 --- a/tests/openlp_plugins/presentations/test_presentationcontroller.py +++ b/tests/openlp_plugins/presentations/test_presentationcontroller.py @@ -26,7 +26,8 @@ import pytest from pathlib import Path from unittest.mock import MagicMock, call, patch -from openlp.plugins.presentations.lib.presentationcontroller import PresentationController, PresentationDocument +from openlp.plugins.presentations.lib.presentationcontroller import PresentationController, PresentationDocument, \ + PresentationList FOLDER_TO_PATCH = 'openlp.plugins.presentations.lib.presentationcontroller.PresentationDocument.get_thumbnail_folder' @@ -215,3 +216,45 @@ def test_load_presentation(get_thumbnail_folder): # THEN: load_presentation should return false assert result is False, "PresentationDocument.load_presentation should return false." + + +def test_presentation_list_is_singleton(): + """ + Test PresentationList is a singleton class + """ + # GIVEN: a PresentationList + presentation_list = PresentationList() + + # WHEN: I try to create another instance + presentation_list_2 = PresentationList() + + # THEN: I get the same instance returned + assert presentation_list_2 is presentation_list + + +def test_presentation_list_add_and_retrieve(document): + """ + Test adding a presentation document and later retrieving it + """ + # GIVEN: a fixture with a mocked document which is added to the PresentationList + PresentationList().add(document, "unique id") + + # WHEN: I retrieve the presentation document + retrieved_presentation = PresentationList().get_presentation_by_id("unique id") + + # THEN: I get the same instance returned + retrieved_presentation is document + + +def test_presentation_list_remove(document): + """ + Test removing a presentation document from the list + """ + # GIVEN: a fixture with a mocked document which is added to the PresentationList + PresentationList().add(document, "unique id") + + # WHEN: I remove the presentation document + PresentationList().remove("unique id") + + # THEN: That document shouldn't be in the list + PresentationList().get_presentation_by_id("unique id") is None