openlp-core team mailing list archive
-
openlp-core team
-
Mailing list archive
-
Message #29395
[Merge] lp:~minkus/openlp/naturalsortfix into lp:openlp
Chris Hill has proposed merging lp:~minkus/openlp/naturalsortfix into lp:openlp.
Commit message:
Natural sort in Edit Song, Song Maintenance form; performance improvements in sorting when performing various searches, extended unit tests
Requested reviews:
OpenLP Core (openlp-core)
For more details, see:
https://code.launchpad.net/~minkus/openlp/naturalsortfix/+merge/292788
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://stackoverflow.com/questions/22442378/what-is-the-difference-between-sortedlist-vs-list-sort-python)
x Added tests for temporary song removal in search functions (improves test coverage)
x Extracted lambdas from sorts to speed up per Raoul's request, converted to defs for PEP8 compliance
--------------------------------
lp:~minkus/openlp/naturalsortfix (revision 2651)
[SUCCESS] https://ci.openlp.io/job/Branch-01-Pull/1497/
[SUCCESS] https://ci.openlp.io/job/Branch-02-Functional-Tests/1408/
[SUCCESS] https://ci.openlp.io/job/Branch-03-Interface-Tests/1346/
[SUCCESS] https://ci.openlp.io/job/Branch-04a-Windows_Functional_Tests/1143/
[SUCCESS] https://ci.openlp.io/job/Branch-04b-Windows_Interface_Tests/734/
[SUCCESS] https://ci.openlp.io/job/Branch-05a-Code_Analysis/801/
[SUCCESS] https://ci.openlp.io/job/Branch-05b-Test_Coverage/669/
--
Your team OpenLP Core is requested to review the proposed merge of lp:~minkus/openlp/naturalsortfix into lp:openlp.
=== modified file 'openlp/plugins/songs/forms/editsongform.py'
--- openlp/plugins/songs/forms/editsongform.py 2016-01-30 20:41:22 +0000
+++ openlp/plugins/songs/forms/editsongform.py 2016-04-25 11:04:46 +0000
@@ -34,6 +34,7 @@
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 @@
"""
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 @@
"""
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 @@
"""
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)
=== modified file 'openlp/plugins/songs/forms/songexportform.py'
--- openlp/plugins/songs/forms/songexportform.py 2016-04-22 18:25:57 +0000
+++ openlp/plugins/songs/forms/songexportform.py 2016-04-25 11:04:46 +0000
@@ -203,6 +203,10 @@
"""
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 @@
# 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:
=== modified file 'openlp/plugins/songs/forms/songmaintenanceform.py'
--- openlp/plugins/songs/forms/songmaintenanceform.py 2016-01-09 16:26:14 +0000
+++ openlp/plugins/songs/forms/songmaintenanceform.py 2016-04-25 11:04:46 +0000
@@ -27,6 +27,7 @@
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 @@
"""
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 @@
"""
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 @@
"""
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)
=== modified file 'openlp/plugins/songs/lib/mediaitem.py'
--- openlp/plugins/songs/lib/mediaitem.py 2016-04-03 19:44:09 +0000
+++ openlp/plugins/songs/lib/mediaitem.py 2016-04-25 11:04:46 +0000
@@ -194,13 +194,13 @@
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 @@
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 @@
: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 @@
: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)
- for song in songs:
+ author.songs.sort(key=get_song_key)
+ for song in author.songs:
# Do not display temporary songs
if song.temporary:
continue
@@ -304,11 +316,15 @@
: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:
- (get_natural_key(songbook_entry.songbook.name), get_natural_key(songbook_entry.entry)))
+ 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)
@@ -323,12 +339,20 @@
: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)
- for song in songs:
+ topic.songs.sort(key=get_song_key)
+ for song in topic.songs:
# Do not display temporary songs
if song.temporary:
continue
@@ -344,10 +368,13 @@
: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),
- song.sort_key))
+ search_results.sort(key=get_theme_key)
for song in search_results:
# Do not display temporary songs
if song.temporary:
@@ -364,11 +391,14 @@
: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),
- song.sort_key))
- for song in songs:
+ search_results.sort(key=get_cclinumber_key)
+ for song in search_results:
# Do not display temporary songs
if song.temporary:
continue
=== modified file 'tests/functional/openlp_plugins/songs/test_mediaitem.py'
--- tests/functional/openlp_plugins/songs/test_mediaitem.py 2016-04-03 10:57:39 +0000
+++ tests/functional/openlp_plugins/songs/test_mediaitem.py 2016-04-25 11:04:46 +0000
@@ -53,6 +53,7 @@
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 @@
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 @@
# 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)')
- mock_qlist_widget.setData.assert_called_with(MockedUserRole, mock_song.id)
- self.media_item.list_view.addItem.assert_called_with(mock_qlist_widget)
+ MockedQListWidgetItem.assert_called_once_with('My Song (My Author)')
+ mock_qlist_widget.setData.assert_called_once_with(MockedUserRole, mock_song.id)
+ 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 @@
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 @@
# 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)')
- mock_qlist_widget.setData.assert_called_with(MockedUserRole, mock_song.id)
- self.media_item.list_view.addItem.assert_called_with(mock_qlist_widget)
+ MockedQListWidgetItem.assert_called_once_with('My Author (My Song)')
+ mock_qlist_widget.setData.assert_called_once_with(MockedUserRole, mock_song.id)
+ self.media_item.list_view.addItem.assert_called_once_with(mock_qlist_widget)
def display_results_book_test(self):
"""
@@ -136,17 +153,27 @@
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 @@
# 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')
- mock_qlist_widget.setData.assert_called_with(MockedUserRole, mock_songbook_entry.song.id)
- self.media_item.list_view.addItem.assert_called_with(mock_qlist_widget)
+ MockedQListWidgetItem.assert_called_once_with('My Book #1: My Song')
+ mock_qlist_widget.setData.assert_called_once_with(MockedUserRole, mock_songbook_entry.song.id)
+ self.media_item.list_view.addItem.assert_called_once_with(mock_qlist_widget)
def display_results_topic_test(self):
"""
@@ -169,13 +196,19 @@
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 @@
# 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)')
- mock_qlist_widget.setData.assert_called_with(MockedUserRole, mock_song.id)
- self.media_item.list_view.addItem.assert_called_with(mock_qlist_widget)
+ MockedQListWidgetItem.assert_called_once_with('My Topic (My Song)')
+ mock_qlist_widget.setData.assert_called_once_with(MockedUserRole, mock_song.id)
+ self.media_item.list_view.addItem.assert_called_once_with(mock_qlist_widget)
def display_results_themes_test(self):
"""
@@ -198,12 +231,19 @@
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 @@
# 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)')
- mock_qlist_widget.setData.assert_called_with(MockedUserRole, mock_song.id)
- self.media_item.list_view.addItem.assert_called_with(mock_qlist_widget)
+ MockedQListWidgetItem.assert_called_once_with('My Theme (My Song)')
+ mock_qlist_widget.setData.assert_called_once_with(MockedUserRole, mock_song.id)
+ self.media_item.list_view.addItem.assert_called_once_with(mock_qlist_widget)
def display_results_cclinumber_test(self):
"""
@@ -225,12 +265,19 @@
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 @@
# 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)')
- mock_qlist_widget.setData.assert_called_with(MockedUserRole, mock_song.id)
- self.media_item.list_view.addItem.assert_called_with(mock_qlist_widget)
+ MockedQListWidgetItem.assert_called_once_with('12345 (My Song)')
+ mock_qlist_widget.setData.assert_called_once_with(MockedUserRole, mock_song.id)
+ self.media_item.list_view.addItem.assert_called_once_with(mock_qlist_widget)
def build_song_footer_one_author_test(self):
"""
Follow ups
