openlp-core team mailing list archive

[Samuel Mehrbrodt <s.mehrbrodt@xxxxxxxxx>]

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

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/+merge/293424

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 into lp:openlp.

=== modified file 'openlp/plugins/songs/lib/mediaitem.py'
--- openlp/plugins/songs/lib/mediaitem.py	2016-04-25 11:01:32 +0000
+++ openlp/plugins/songs/lib/mediaitem.py	2016-04-29 17:21:44 +0000
@@ -21,7 +21,6 @@
 ###############################################################################
 
 import logging
-import re
 import os
 import shutil
 
@@ -207,9 +206,11 @@
             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 @@
         """
         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:
-            # Do not display temporary songs
-            if songbook_entry.song.temporary:
-                continue
-            song_detail = '%s #%s: %s' % (songbook_entry.songbook.name, songbook_entry.entry, songbook_entry.song.title)
+        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 display_results_topic(self, search_results):

=== modified file 'tests/functional/openlp_plugins/songs/test_mediaitem.py'
--- tests/functional/openlp_plugins/songs/test_mediaitem.py	2016-04-17 21:22:30 +0000
+++ tests/functional/openlp_plugins/songs/test_mediaitem.py	2016-04-29 17:21:44 +0000
@@ -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 @@
         # 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_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_search_results = [('1', 'My Book', 'My Song', 1)]
             mock_qlist_widget = MagicMock()
             MockedQListWidgetItem.return_value = mock_qlist_widget
 
@@ -183,9 +162,35 @@
             # 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