From b1aef9d0fd47d6ea4be793fdc89b231fbe549832 Mon Sep 17 00:00:00 2001 From: Mateus Meyer Jiacomelli Date: Mon, 12 Dec 2022 08:03:39 +0000 Subject: [PATCH] Fixing OpenLP crash (and increasing performance) caused when using Transpose API Endpoint --- openlp/core/api/versions/v2/plugins.py | 23 ++++------- tests/openlp_core/api/v2/test_plugins.py | 50 ++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 16 deletions(-) diff --git a/openlp/core/api/versions/v2/plugins.py b/openlp/core/api/versions/v2/plugins.py index f35648226..df395435b 100644 --- a/openlp/core/api/versions/v2/plugins.py +++ b/openlp/core/api/versions/v2/plugins.py @@ -28,7 +28,6 @@ from openlp.core.api.lib import login_required from openlp.core.lib.plugin import PluginStatus from openlp.core.common.registry import Registry from openlp.plugins.songs.lib import transpose_lyrics -from openlp.core.display.render import remove_tags log = logging.getLogger(__name__) @@ -154,22 +153,14 @@ def transpose(transpose_value): # make sure an service item is currently displayed and that it is a song if not current_item or current_item.name != 'songs': abort(400) - previous_pages = {} + live_item = current_item.to_dict() + live_item['id'] = str(current_item.unique_identifier) chord_song_text = '' - # re-create the song lyrics with OpenLP verse-tags to be able to transpose in one go so any keys are used - for raw_slide in current_item.slides: - verse_tag = raw_slide['verse'] - if verse_tag in previous_pages and previous_pages[verse_tag][0] == raw_slide: - pages = previous_pages[verse_tag][1] - else: - pages = current_item.renderer.format_slide(raw_slide['text'], current_item) - previous_pages[verse_tag] = (raw_slide, pages) - for page in pages: - chord_song_text += '---[Verse:{verse_tag}]---\n'.format(verse_tag=verse_tag) - chord_song_text += page - chord_song_text += '\n' - # remove formatting tags - chord_song_text = remove_tags(chord_song_text) + verse_index = 1 + for item in live_item['slides']: + chord_song_text += ('---[Verse:%d]---' % verse_index) + '\n' + chord_song_text += item['chords'] + '\n' + verse_index += 1 # transpose transposed_lyrics = transpose_lyrics(chord_song_text, transpose_value) # re-split into verses diff --git a/tests/openlp_core/api/v2/test_plugins.py b/tests/openlp_core/api/v2/test_plugins.py index 6a9a8a1e6..2f9673c5a 100644 --- a/tests/openlp_core/api/v2/test_plugins.py +++ b/tests/openlp_core/api/v2/test_plugins.py @@ -18,10 +18,16 @@ # You should have received a copy of the GNU General Public License # # along with this program. If not, see . # ########################################################################## +from pathlib import Path from unittest.mock import MagicMock from openlp.core.common.registry import Registry from openlp.core.common.enum import PluginStatus +from openlp.core.display.render import Renderer +from openlp.core.lib.serviceitem import ServiceItem +from openlp.core.state import State +from tests.openlp_core.lib.test_serviceitem import TEST_PATH as SERVICEITEM_TEST_PATH +from tests.utils import convert_file_service_item # Search options tests @@ -94,3 +100,47 @@ def test_plugin_songs_transpose_returns_plugin_exception(flask_client, settings) Registry().register('plugin_manager', MagicMock()) res = flask_client.get('/api/v2/plugins/songs/transpose-live-item/test') assert res.status_code == 400 + + +def test_plugin_songs_transpose_wont_call_renderer(flask_client, settings): + """ + Tests whether the transpose endpoint won't tries to use any Renderer method; the endpoint needs to operate using + already-primed caches (as that's what the /live-item endpoint does); also using Renderer from outside the Qt loop + causes it to crash. + + See https://gitlab.com/openlp/openlp/-/merge_requests/516 for some background on this. + """ + # GIVEN: A mocked plugin_manager, live_controller, renderer and a real service item with the internal slide cache + # filled + Registry().register('plugin_manager', MagicMock()) + renderer_mock = MagicMock() + # Mocking every Renderer attribute with a shared mock so we can know whether any property of it was called + renderer_mock_any_attr = MagicMock(name='Renderer attribute/call') + for name in dir(Renderer): + # Mocking every Renderer property + # skipping dunder/hide methods + if not name.startswith('_'): + setattr(renderer_mock, name, renderer_mock_any_attr) + # Mocking format_slides to return the correct data to ServiceItem + renderer_mock.format_slide.side_effect = lambda text, item: text + Registry().register('renderer', renderer_mock) + service_item = ServiceItem() + live_controller_mock = MagicMock() + live_controller_mock.service_item = service_item + Registry().register('live_controller', live_controller_mock) + # Using a real item to test this + line = convert_file_service_item(SERVICEITEM_TEST_PATH, 'serviceitem-song-linked-audio.osj') + State().load_settings() # needed to make the below method not fail + service_item.set_from_service(line, Path('/test/')) + # Trying to retrive the slides to trigger the cache prime process (as it's primed when using the service item first + # time in OpenLP) + service_item.rendered_slides + # Resetting the mocks to correctly compute illegal calls + renderer_mock_any_attr.reset_mock() + renderer_mock.format_slides.reset_mock() + + # WHEN: The endpoint is called + flask_client.get('/api/v2/plugins/songs/transpose-live-item/-1') + + # THEN: The renderer should not be called + renderer_mock_any_attr.assert_not_called()