openlp-core team mailing list archive

[Samuel Mehrbrodt <s.mehrbrodt@xxxxxxxxx>]

Samuel Mehrbrodt has proposed merging lp:~sam92/openlp/bug-1552563-2.4 into lp:openlp/2.4.

Requested reviews:
  OpenLP Core (openlp-core)
Related bugs:
  Bug #1552563 in OpenLP: "Performance regression: Search by songbook very slow"
  https://bugs.launchpad.net/openlp/+bug/1552563

For more details, see:
https://code.launchpad.net/~sam92/openlp/bug-1552563-2.4/+merge/293429

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.
-- 
Your team OpenLP Core is requested to review the proposed merge of lp:~sam92/openlp/bug-1552563-2.4 into lp:openlp/2.4.

=== modified file 'openlp/plugins/songs/lib/mediaitem.py'
--- openlp/plugins/songs/lib/mediaitem.py	2016-01-15 20:37:53 +0000
+++ openlp/plugins/songs/lib/mediaitem.py	2016-04-29 17:55:06 +0000
@@ -21,7 +21,6 @@
 ###############################################################################
 
 import logging
-import re
 import os
 import shutil
 
@@ -32,6 +31,7 @@
 from openlp.core.lib import MediaManagerItem, ItemCapabilities, PluginStatus, ServiceItemContext, \
     check_item_selected, create_separated_list
 from openlp.core.lib.ui import create_widget_action
+from openlp.core.utils import get_natural_key
 from openlp.plugins.songs.forms.editsongform import EditSongForm
 from openlp.plugins.songs.forms.songmaintenanceform import SongMaintenanceForm
 from openlp.plugins.songs.forms.songimportform import SongImportForm
@@ -251,23 +251,23 @@
         self.list_view.clear()
 
         search_keywords = search_keywords.rpartition(' ')
-        search_book = search_keywords[0]
-        search_entry = re.sub(r'[^0-9]', '', search_keywords[2])
-
-        songbook_entries = (self.plugin.manager.session.query(SongBookEntry)
-                            .join(Book)
-                            .order_by(Book.name)
-                            .order_by(SongBookEntry.entry))
-        for songbook_entry in songbook_entries:
-            if songbook_entry.song.temporary:
-                continue
-            if search_book.lower() not in songbook_entry.songbook.name.lower():
-                continue
-            if search_entry not in songbook_entry.entry:
-                continue
-            song_detail = '%s #%s: %s' % (songbook_entry.songbook.name, songbook_entry.entry, songbook_entry.song.title)
+        search_book = search_keywords[0] + '%'
+        search_entry = search_keywords[2] + '%'
+        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),
+                                  Song.temporary.is_(False)).all())
+
+        def get_songbook_key(result):
+            """Get the key to sort by"""
+            return (get_natural_key(result[1]), get_natural_key(result[0]), get_natural_key(result[2]))
+
+        search_results.sort(key=get_songbook_key)
+        for result in search_results:
+            song_detail = '%s #%s: %s' % (result[1], result[0], result[2])
             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 on_clear_text_button_click(self):

=== modified file 'tests/functional/openlp_plugins/songs/test_openlpimporter.py'
--- tests/functional/openlp_plugins/songs/test_openlpimporter.py	2016-04-27 18:45:39 +0000
+++ tests/functional/openlp_plugins/songs/test_openlpimporter.py	2016-04-29 17:55:06 +0000
@@ -73,4 +73,3 @@
                 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')
-