Fix performance regression with Songbook search

The problem was that in each iteration the database was accessed (the song object).
Fixed this by loading all neccessary information directly in the query.

bzr-revno: 2655
Fixes: https://launchpad.net/bugs/1552563

openlp/plugins/songs/lib/mediaitem.py

@@ -21,7 +21,6 @@
 ###############################################################################
 
 import logging
-import re
 import os
 import shutil
 
@@ -207,9 +206,11 @@ class SongMediaItem(MediaManagerItem):
             search_keywords = search_keywords.rpartition(' ')
             search_book = search_keywords[0] + '%'
             search_entry = search_keywords[2] + '%'
-            search_results = (self.plugin.manager.session.query(SongBookEntry)
+            search_results = (self.plugin.manager.session.query(SongBookEntry.entry, Book.name, Song.title, Song.id)
+                              .join(Song)
                               .join(Book)
-                              .filter(Book.name.like(search_book), SongBookEntry.entry.like(search_entry)).all())
+                              .filter(Book.name.like(search_book), SongBookEntry.entry.like(search_entry),
+                                      Song.temporary.is_(False)).all())
             self.display_results_book(search_results)
         elif search_type == SongSearch.Themes:
             log.debug('Theme Search')
@@ -313,23 +314,20 @@ class SongMediaItem(MediaManagerItem):
         """
         Display the song search results in the media manager list, grouped by book and entry
 
-        :param search_results: A list of db SongBookEntry objects
+        :param search_results: A tuple containing (songbook entry, book name, song title, song id)
         :return: None
         """
-        def get_songbook_key(songbook_entry):
+        def get_songbook_key(result):
             """Get the key to sort by"""
-            return (get_natural_key(songbook_entry.songbook.name), get_natural_key(songbook_entry.entry))
+            return (get_natural_key(result[1]), get_natural_key(result[0]), get_natural_key(result[2]))
 
         log.debug('display results Book')
         self.list_view.clear()
         search_results.sort(key=get_songbook_key)
-        for songbook_entry in search_results:
+        for result in search_results:
-            # Do not display temporary songs
+            song_detail = '%s #%s: %s' % (result[1], result[0], result[2])
-            if songbook_entry.song.temporary:
-                continue
-            song_detail = '%s #%s: %s' % (songbook_entry.songbook.name, songbook_entry.entry, songbook_entry.song.title)
             song_name = QtWidgets.QListWidgetItem(song_detail)
-            song_name.setData(QtCore.Qt.UserRole, songbook_entry.song.id)
+            song_name.setData(QtCore.Qt.UserRole, result[3])
             self.list_view.addItem(song_name)
 
     def display_results_topic(self, search_results):

tests/functional/openlp_plugins/songs/test_mediaitem.py

@@ -23,6 +23,7 @@
 This module contains tests for the lib submodule of the Songs plugin.
 """
 from unittest import TestCase
+from unittest.mock import call
 
 from PyQt5 import QtCore
 
@@ -151,29 +152,7 @@ class TestMediaItem(TestCase, TestMixin):
         # GIVEN: Search results grouped by book and entry, plus a mocked QtListWidgetItem
         with patch('openlp.core.lib.QtWidgets.QListWidgetItem') as MockedQListWidgetItem, \
                 patch('openlp.core.lib.QtCore.Qt.UserRole') as MockedUserRole:
-            mock_search_results = []
+            mock_search_results = [('1', 'My Book', 'My Song', 1)]
-            mock_songbook_entry = MagicMock()
-            mock_songbook_entry_temp = MagicMock()
-            mock_songbook = MagicMock()
-            mock_song = MagicMock()
-            mock_song_temp = MagicMock()
-            mock_songbook_entry.entry = '1'
-            mock_songbook_entry_temp.entry = '2'
-            mock_songbook.name = 'My Book'
-            mock_song.id = 1
-            mock_song.title = 'My Song'
-            mock_song.sort_key = 'My Song'
-            mock_song.temporary = False
-            mock_song_temp.id = 2
-            mock_song_temp.title = 'My Temporary'
-            mock_song_temp.sort_key = 'My Temporary'
-            mock_song_temp.temporary = True
-            mock_songbook_entry.song = mock_song
-            mock_songbook_entry.songbook = mock_songbook
-            mock_songbook_entry_temp.song = mock_song_temp
-            mock_songbook_entry_temp.songbook = mock_songbook
-            mock_search_results.append(mock_songbook_entry)
-            mock_search_results.append(mock_songbook_entry_temp)
             mock_qlist_widget = MagicMock()
             MockedQListWidgetItem.return_value = mock_qlist_widget
 
@@ -183,9 +162,35 @@ class TestMediaItem(TestCase, TestMixin):
             # THEN: The current list view is cleared, the widget is created, and the relevant attributes set
             self.media_item.list_view.clear.assert_called_with()
             MockedQListWidgetItem.assert_called_once_with('My Book #1: My Song')
-            mock_qlist_widget.setData.assert_called_once_with(MockedUserRole, mock_songbook_entry.song.id)
+            mock_qlist_widget.setData.assert_called_once_with(MockedUserRole, 1)
             self.media_item.list_view.addItem.assert_called_once_with(mock_qlist_widget)
 
+    def songbook_natural_sorting_test(self):
+        """
+        Test that songbooks are sorted naturally
+        """
+        # GIVEN: Search results grouped by book and entry, plus a mocked QtListWidgetItem
+        with patch('openlp.core.lib.QtWidgets.QListWidgetItem') as MockedQListWidgetItem:
+            mock_search_results = [('2', 'Thy Book', 'Thy Song', 50),
+                                   ('2', 'My Book', 'Your Song', 7),
+                                   ('10', 'My Book', 'Our Song', 12),
+                                   ('1', 'My Book', 'My Song', 1),
+                                   ('2', 'Thy Book', 'A Song', 8)]
+            mock_qlist_widget = MagicMock()
+            MockedQListWidgetItem.return_value = mock_qlist_widget
+
+            # WHEN: I display song search results grouped by book
+            self.media_item.display_results_book(mock_search_results)
+
+            # THEN: The songbooks are inserted in the right (natural) order,
+            #       grouped first by book, then by number, then by song title
+            calls = [call('My Book #1: My Song'), call().setData(QtCore.Qt.UserRole, 1),
+                     call('My Book #2: Your Song'), call().setData(QtCore.Qt.UserRole, 7),
+                     call('My Book #10: Our Song'), call().setData(QtCore.Qt.UserRole, 12),
+                     call('Thy Book #2: A Song'), call().setData(QtCore.Qt.UserRole, 8),
+                     call('Thy Book #2: Thy Song'), call().setData(QtCore.Qt.UserRole, 50)]
+            MockedQListWidgetItem.assert_has_calls(calls)
+
     def display_results_topic_test(self):
         """
         Test displaying song search results grouped by topic with basic song

tests/functional/openlp_plugins/songs/test_openlpimporter.py

@@ -73,4 +73,3 @@ class TestOpenLPImport(TestCase):
                 self.assertIsNone(importer.do_import(), 'do_import should return None when import_source is not a list')
                 self.assertEqual(mocked_import_wizard.progress_bar.setMaximum.called, False,
                                  'setMaximum on import_wizard.progress_bar should not have been called')
-