Natural sort for authors, topics, songbooks, themes in Edit Song comboboxen & Song Maintenance form, as mentioned previously by Raoul - Matthew 21:28–29! :)

Also some minor fixes that I picked up after merge 2637 for natural sorting:
x Author, Topics, Theme search were performing two sorts (one database sort, then a natural sort in code). Removed the first database sort.
x Changed all sorting to use in-place sort() rather than copy sorted(), as apparently there's a speed bonus (http://stacko...

bzr-revno: 2651

openlp/plugins/songs/forms/editsongform.py

@@ -34,6 +34,7 @@ from PyQt5 import QtCore, QtWidgets
 from openlp.core.common import Registry, RegistryProperties, AppLocation, UiStrings, check_directory_exists, translate
 from openlp.core.lib import FileDialog, PluginStatus, MediaType, create_separated_list
 from openlp.core.lib.ui import set_case_insensitive_completer, critical_error_message_box, find_and_set_in_combo_box
+from openlp.core.common.languagemanager import get_natural_key
 from openlp.plugins.songs.lib import VerseType, clean_song
 from openlp.plugins.songs.lib.db import Book, Song, Author, AuthorType, Topic, MediaFile, SongBookEntry
 from openlp.plugins.songs.lib.ui import SongStrings
@@ -110,7 +111,12 @@ class EditSongForm(QtWidgets.QDialog, Ui_EditSongDialog, RegistryProperties):
         """
         Generically load a set of objects into a cache and a combobox.
         """
-        objects = self.manager.get_all_objects(cls, order_by_ref=cls.name)
+        def get_key(obj):
+            """Get the key to sort by"""
+            return get_natural_key(obj.name)
+
+        objects = self.manager.get_all_objects(cls)
+        objects.sort(key=get_key)
         combo.clear()
         combo.addItem('')
         for obj in objects:
@@ -343,7 +349,12 @@ class EditSongForm(QtWidgets.QDialog, Ui_EditSongDialog, RegistryProperties):
         """
         Load the authors from the database into the combobox.
         """
-        authors = self.manager.get_all_objects(Author, order_by_ref=Author.display_name)
+        def get_author_key(author):
+            """Get the key to sort by"""
+            return get_natural_key(author.display_name)
+
+        authors = self.manager.get_all_objects(Author)
+        authors.sort(key=get_author_key)
         self.authors_combo_box.clear()
         self.authors_combo_box.addItem('')
         self.authors = []
@@ -378,9 +389,14 @@ class EditSongForm(QtWidgets.QDialog, Ui_EditSongDialog, RegistryProperties):
         """
         Load the themes into a combobox.
         """
+        def get_theme_key(theme):
+            """Get the key to sort by"""
+            return get_natural_key(theme)
+
         self.theme_combo_box.clear()
         self.theme_combo_box.addItem('')
         self.themes = theme_list
+        self.themes.sort(key=get_theme_key)
         self.theme_combo_box.addItems(theme_list)
         set_case_insensitive_completer(self.themes, self.theme_combo_box)
 

openlp/plugins/songs/forms/songexportform.py

@@ -203,6 +203,10 @@ class SongExportForm(OpenLPWizard):
         """
         Set default form values for the song export wizard.
         """
+        def get_song_key(song):
+            """Get the key to sort by"""
+            return song.sort_key
+
         self.restart()
         self.finish_button.setVisible(False)
         self.cancel_button.setVisible(True)
@@ -213,7 +217,7 @@ class SongExportForm(OpenLPWizard):
         # Load the list of songs.
         self.application.set_busy_cursor()
         songs = self.plugin.manager.get_all_objects(Song)
-        songs.sort(key=lambda song: song.sort_key)
+        songs.sort(key=get_song_key)
         for song in songs:
             # No need to export temporary songs.
             if song.temporary:

openlp/plugins/songs/forms/songmaintenanceform.py

@@ -27,6 +27,7 @@ from sqlalchemy.sql import and_
 
 from openlp.core.common import Registry, RegistryProperties, UiStrings, translate
 from openlp.core.lib.ui import critical_error_message_box
+from openlp.core.common.languagemanager import get_natural_key
 from openlp.plugins.songs.forms.authorsform import AuthorsForm
 from openlp.plugins.songs.forms.topicsform import TopicsForm
 from openlp.plugins.songs.forms.songbookform import SongBookForm
@@ -120,8 +121,13 @@ class SongMaintenanceForm(QtWidgets.QDialog, Ui_SongMaintenanceDialog, RegistryP
         """
         Reloads the Authors list.
         """
+        def get_author_key(author):
+            """Get the key to sort by"""
+            return get_natural_key(author.display_name)
+
         self.authors_list_widget.clear()
-        authors = self.manager.get_all_objects(Author, order_by_ref=Author.display_name)
+        authors = self.manager.get_all_objects(Author)
+        authors.sort(key=get_author_key)
         for author in authors:
             if author.display_name:
                 author_name = QtWidgets.QListWidgetItem(author.display_name)
@@ -134,8 +140,13 @@ class SongMaintenanceForm(QtWidgets.QDialog, Ui_SongMaintenanceDialog, RegistryP
         """
         Reloads the Topics list.
         """
+        def get_topic_key(topic):
+            """Get the key to sort by"""
+            return get_natural_key(topic.name)
+
         self.topics_list_widget.clear()
-        topics = self.manager.get_all_objects(Topic, order_by_ref=Topic.name)
+        topics = self.manager.get_all_objects(Topic)
+        topics.sort(key=get_topic_key)
         for topic in topics:
             topic_name = QtWidgets.QListWidgetItem(topic.name)
             topic_name.setData(QtCore.Qt.UserRole, topic.id)
@@ -145,8 +156,13 @@ class SongMaintenanceForm(QtWidgets.QDialog, Ui_SongMaintenanceDialog, RegistryP
         """
         Reloads the Books list.
         """
+        def get_book_key(book):
+            """Get the key to sort by"""
+            return get_natural_key(book.name)
+
         self.song_books_list_widget.clear()
-        books = self.manager.get_all_objects(Book, order_by_ref=Book.name)
+        books = self.manager.get_all_objects(Book)
+        books.sort(key=get_book_key)
         for book in books:
             book_name = QtWidgets.QListWidgetItem('%s (%s)' % (book.name, book.publisher))
             book_name.setData(QtCore.Qt.UserRole, book.id)

openlp/plugins/songs/lib/mediaitem.py

@@ -194,13 +194,13 @@ class SongMediaItem(MediaManagerItem):
             log.debug('Authors Search')
             search_string = '%' + search_keywords + '%'
             search_results = self.plugin.manager.get_all_objects(
-                Author, Author.display_name.like(search_string), Author.display_name.asc())
+                Author, Author.display_name.like(search_string))
             self.display_results_author(search_results)
         elif search_type == SongSearch.Topics:
             log.debug('Topics Search')
             search_string = '%' + search_keywords + '%'
             search_results = self.plugin.manager.get_all_objects(
-                Topic, Topic.name.like(search_string), Topic.name.asc())
+                Topic, Topic.name.like(search_string))
             self.display_results_topic(search_results)
         elif search_type == SongSearch.Books:
             log.debug('Songbook Search')
@@ -215,7 +215,7 @@ class SongMediaItem(MediaManagerItem):
             log.debug('Theme Search')
             search_string = '%' + search_keywords + '%'
             search_results = self.plugin.manager.get_all_objects(
-                Song, Song.theme_name.like(search_string), Song.theme_name.asc())
+                Song, Song.theme_name.like(search_string))
             self.display_results_themes(search_results)
         elif search_type == SongSearch.Copyright:
             log.debug('Copyright Search')
@@ -258,10 +258,14 @@ class SongMediaItem(MediaManagerItem):
         :param search_results: A list of db Song objects
         :return: None
         """
+        def get_song_key(song):
+            """Get the key to sort by"""
+            return song.sort_key
+
         log.debug('display results Song')
         self.save_auto_select_id()
         self.list_view.clear()
-        search_results.sort(key=lambda song: song.sort_key)
+        search_results.sort(key=get_song_key)
         for song in search_results:
             # Do not display temporary songs
             if song.temporary:
@@ -283,12 +287,20 @@ class SongMediaItem(MediaManagerItem):
         :param search_results: A list of db Author objects
         :return: None
         """
+        def get_author_key(author):
+            """Get the key to sort by"""
+            return get_natural_key(author.display_name)
+
+        def get_song_key(song):
+            """Get the key to sort by"""
+            return song.sort_key
+
         log.debug('display results Author')
         self.list_view.clear()
-        search_results = sorted(search_results, key=lambda author: get_natural_key(author.display_name))
+        search_results.sort(key=get_author_key)
         for author in search_results:
-            songs = sorted(author.songs, key=lambda song: song.sort_key)
+            author.songs.sort(key=get_song_key)
-            for song in songs:
+            for song in author.songs:
                 # Do not display temporary songs
                 if song.temporary:
                     continue
@@ -304,11 +316,15 @@ class SongMediaItem(MediaManagerItem):
         :param search_results: A list of db SongBookEntry objects
         :return: None
         """
+        def get_songbook_key(songbook_entry):
+            """Get the key to sort by"""
+            return (get_natural_key(songbook_entry.songbook.name), get_natural_key(songbook_entry.entry))
+
         log.debug('display results Book')
         self.list_view.clear()
-        search_results = sorted(search_results, key=lambda songbook_entry:
+        search_results.sort(key=get_songbook_key)
-                                (get_natural_key(songbook_entry.songbook.name), get_natural_key(songbook_entry.entry)))
         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)
@@ -323,12 +339,20 @@ class SongMediaItem(MediaManagerItem):
         :param search_results: A list of db Topic objects
         :return: None
         """
+        def get_topic_key(topic):
+            """Get the key to sort by"""
+            return get_natural_key(topic.name)
+
+        def get_song_key(song):
+            """Get the key to sort by"""
+            return song.sort_key
+
         log.debug('display results Topic')
         self.list_view.clear()
-        search_results = sorted(search_results, key=lambda topic: get_natural_key(topic.name))
+        search_results.sort(key=get_topic_key)
         for topic in search_results:
-            songs = sorted(topic.songs, key=lambda song: song.sort_key)
+            topic.songs.sort(key=get_song_key)
-            for song in songs:
+            for song in topic.songs:
                 # Do not display temporary songs
                 if song.temporary:
                     continue
@@ -344,10 +368,13 @@ class SongMediaItem(MediaManagerItem):
         :param search_results: A list of db Song objects
         :return: None
         """
+        def get_theme_key(song):
+            """Get the key to sort by"""
+            return (get_natural_key(song.theme_name), song.sort_key)
+
         log.debug('display results Themes')
         self.list_view.clear()
-        search_results = sorted(search_results, key=lambda song: (get_natural_key(song.theme_name),
+        search_results.sort(key=get_theme_key)
-                                song.sort_key))
         for song in search_results:
             # Do not display temporary songs
             if song.temporary:
@@ -364,11 +391,14 @@ class SongMediaItem(MediaManagerItem):
         :param search_results: A list of db Song objects
         :return: None
         """
+        def get_cclinumber_key(song):
+            """Get the key to sort by"""
+            return (get_natural_key(song.ccli_number), song.sort_key)
+
         log.debug('display results CCLI number')
         self.list_view.clear()
-        songs = sorted(search_results, key=lambda song: (get_natural_key(song.ccli_number),
+        search_results.sort(key=get_cclinumber_key)
-                       song.sort_key))
+        for song in search_results:
-        for song in songs:
             # Do not display temporary songs
             if song.temporary:
                 continue

tests/functional/openlp_plugins/songs/test_mediaitem.py

@@ -53,6 +53,7 @@ class TestMediaItem(TestCase, TestMixin):
             self.media_item.list_view.save_auto_select_id = MagicMock()
             self.media_item.list_view.clear = MagicMock()
             self.media_item.list_view.addItem = MagicMock()
+            self.media_item.list_view.setCurrentItem = MagicMock()
             self.media_item.auto_select_id = -1
             self.media_item.display_songbook = False
             self.media_item.display_copyright_symbol = False
@@ -79,13 +80,22 @@ class TestMediaItem(TestCase, TestMixin):
             mock_song.title = 'My Song'
             mock_song.sort_key = 'My Song'
             mock_song.authors = []
+            mock_song_temp = MagicMock()
+            mock_song_temp.id = 2
+            mock_song_temp.title = 'My Temporary'
+            mock_song_temp.sort_key = 'My Temporary'
+            mock_song_temp.authors = []
             mock_author = MagicMock()
             mock_author.display_name = 'My Author'
             mock_song.authors.append(mock_author)
+            mock_song_temp.authors.append(mock_author)
             mock_song.temporary = False
+            mock_song_temp.temporary = True
             mock_search_results.append(mock_song)
+            mock_search_results.append(mock_song_temp)
             mock_qlist_widget = MagicMock()
             MockedQListWidgetItem.return_value = mock_qlist_widget
+            self.media_item.auto_select_id = 1
 
             # WHEN: I display song search results
             self.media_item.display_results_song(mock_search_results)
@@ -93,9 +103,10 @@ 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()
             self.media_item.save_auto_select_id.assert_called_with()
-            MockedQListWidgetItem.assert_called_with('My Song (My Author)')
+            MockedQListWidgetItem.assert_called_once_with('My Song (My Author)')
-            mock_qlist_widget.setData.assert_called_with(MockedUserRole, mock_song.id)
+            mock_qlist_widget.setData.assert_called_once_with(MockedUserRole, mock_song.id)
-            self.media_item.list_view.addItem.assert_called_with(mock_qlist_widget)
+            self.media_item.list_view.addItem.assert_called_once_with(mock_qlist_widget)
+            self.media_item.list_view.setCurrentItem.assert_called_with(mock_qlist_widget)
 
     def display_results_author_test(self):
         """
@@ -107,13 +118,19 @@ class TestMediaItem(TestCase, TestMixin):
             mock_search_results = []
             mock_author = MagicMock()
             mock_song = MagicMock()
+            mock_song_temp = MagicMock()
             mock_author.display_name = 'My Author'
             mock_author.songs = []
             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_author.songs.append(mock_song)
+            mock_author.songs.append(mock_song_temp)
             mock_search_results.append(mock_author)
             mock_qlist_widget = MagicMock()
             MockedQListWidgetItem.return_value = mock_qlist_widget
@@ -123,9 +140,9 @@ 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_with('My Author (My Song)')
+            MockedQListWidgetItem.assert_called_once_with('My Author (My Song)')
-            mock_qlist_widget.setData.assert_called_with(MockedUserRole, mock_song.id)
+            mock_qlist_widget.setData.assert_called_once_with(MockedUserRole, mock_song.id)
-            self.media_item.list_view.addItem.assert_called_with(mock_qlist_widget)
+            self.media_item.list_view.addItem.assert_called_once_with(mock_qlist_widget)
 
     def display_results_book_test(self):
         """
@@ -136,17 +153,27 @@ class TestMediaItem(TestCase, TestMixin):
                 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_qlist_widget = MagicMock()
             MockedQListWidgetItem.return_value = mock_qlist_widget
 
@@ -155,9 +182,9 @@ 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_with('My Book #1: My Song')
+            MockedQListWidgetItem.assert_called_once_with('My Book #1: My Song')
-            mock_qlist_widget.setData.assert_called_with(MockedUserRole, mock_songbook_entry.song.id)
+            mock_qlist_widget.setData.assert_called_once_with(MockedUserRole, mock_songbook_entry.song.id)
-            self.media_item.list_view.addItem.assert_called_with(mock_qlist_widget)
+            self.media_item.list_view.addItem.assert_called_once_with(mock_qlist_widget)
 
     def display_results_topic_test(self):
         """
@@ -169,13 +196,19 @@ class TestMediaItem(TestCase, TestMixin):
             mock_search_results = []
             mock_topic = MagicMock()
             mock_song = MagicMock()
+            mock_song_temp = MagicMock()
             mock_topic.name = 'My Topic'
             mock_topic.songs = []
             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_topic.songs.append(mock_song)
+            mock_topic.songs.append(mock_song_temp)
             mock_search_results.append(mock_topic)
             mock_qlist_widget = MagicMock()
             MockedQListWidgetItem.return_value = mock_qlist_widget
@@ -185,9 +218,9 @@ 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_with('My Topic (My Song)')
+            MockedQListWidgetItem.assert_called_once_with('My Topic (My Song)')
-            mock_qlist_widget.setData.assert_called_with(MockedUserRole, mock_song.id)
+            mock_qlist_widget.setData.assert_called_once_with(MockedUserRole, mock_song.id)
-            self.media_item.list_view.addItem.assert_called_with(mock_qlist_widget)
+            self.media_item.list_view.addItem.assert_called_once_with(mock_qlist_widget)
 
     def display_results_themes_test(self):
         """
@@ -198,12 +231,19 @@ class TestMediaItem(TestCase, TestMixin):
                 patch('openlp.core.lib.QtCore.Qt.UserRole') as MockedUserRole:
             mock_search_results = []
             mock_song = MagicMock()
+            mock_song_temp = MagicMock()
             mock_song.id = 1
             mock_song.title = 'My Song'
             mock_song.sort_key = 'My Song'
             mock_song.theme_name = 'My Theme'
             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.theme_name = 'My Theme'
+            mock_song_temp.temporary = True
             mock_search_results.append(mock_song)
+            mock_search_results.append(mock_song_temp)
             mock_qlist_widget = MagicMock()
             MockedQListWidgetItem.return_value = mock_qlist_widget
 
@@ -212,9 +252,9 @@ 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_with('My Theme (My Song)')
+            MockedQListWidgetItem.assert_called_once_with('My Theme (My Song)')
-            mock_qlist_widget.setData.assert_called_with(MockedUserRole, mock_song.id)
+            mock_qlist_widget.setData.assert_called_once_with(MockedUserRole, mock_song.id)
-            self.media_item.list_view.addItem.assert_called_with(mock_qlist_widget)
+            self.media_item.list_view.addItem.assert_called_once_with(mock_qlist_widget)
 
     def display_results_cclinumber_test(self):
         """
@@ -225,12 +265,19 @@ class TestMediaItem(TestCase, TestMixin):
                 patch('openlp.core.lib.QtCore.Qt.UserRole') as MockedUserRole:
             mock_search_results = []
             mock_song = MagicMock()
+            mock_song_temp = MagicMock()
             mock_song.id = 1
             mock_song.title = 'My Song'
             mock_song.sort_key = 'My Song'
             mock_song.ccli_number = '12345'
             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.ccli_number = '12346'
+            mock_song_temp.temporary = True
             mock_search_results.append(mock_song)
+            mock_search_results.append(mock_song_temp)
             mock_qlist_widget = MagicMock()
             MockedQListWidgetItem.return_value = mock_qlist_widget
 
@@ -239,9 +286,9 @@ 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_with('12345 (My Song)')
+            MockedQListWidgetItem.assert_called_once_with('12345 (My Song)')
-            mock_qlist_widget.setData.assert_called_with(MockedUserRole, mock_song.id)
+            mock_qlist_widget.setData.assert_called_once_with(MockedUserRole, mock_song.id)
-            self.media_item.list_view.addItem.assert_called_with(mock_qlist_widget)
+            self.media_item.list_view.addItem.assert_called_once_with(mock_qlist_widget)
 
     def build_song_footer_one_author_test(self):
         """