Fixing OpenLP crash (and increasing performance) caused when using Transpose API Endpoint

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
+        verse_index = 1
-        for raw_slide in current_item.slides:
+        for item in live_item['slides']:
-            verse_tag = raw_slide['verse']
+            chord_song_text += ('---[Verse:%d]---' % verse_index) + '\n'
-            if verse_tag in previous_pages and previous_pages[verse_tag][0] == raw_slide:
+            chord_song_text += item['chords'] + '\n'
-                pages = previous_pages[verse_tag][1]
+            verse_index += 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)
         # transpose
         transposed_lyrics = transpose_lyrics(chord_song_text, transpose_value)
         # re-split into verses

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 <https://www.gnu.org/licenses/>. #
 ##########################################################################
+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()