openlp-core team mailing list archive
-
openlp-core team
-
Mailing list archive
-
Message #29277
[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/292101
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)
--------------------------------
lp:~minkus/openlp/naturalsortfix (revision 2646)
[SUCCESS] https://ci.openlp.io/job/Branch-01-Pull/1454/
[SUCCESS] https://ci.openlp.io/job/Branch-02-Functional-Tests/1371/
[SUCCESS] https://ci.openlp.io/job/Branch-03-Interface-Tests/1309/
[SUCCESS] https://ci.openlp.io/job/Branch-04a-Windows_Functional_Tests/1113/
[SUCCESS] https://ci.openlp.io/job/Branch-04b-Windows_Interface_Tests/704/
[SUCCESS] https://ci.openlp.io/job/Branch-05a-Code_Analysis/771/
[SUCCESS] https://ci.openlp.io/job/Branch-05b-Test_Coverage/639/
--
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-17 21:48:18 +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,8 @@
"""
Generically load a set of objects into a cache and a combobox.
"""
- objects = self.manager.get_all_objects(cls, order_by_ref=cls.name)
+ objects = self.manager.get_all_objects(cls)
+ objects.sort(key=lambda object: get_natural_key(object.name))
combo.clear()
combo.addItem('')
for obj in objects:
@@ -343,7 +345,8 @@
"""
Load the authors from the database into the combobox.
"""
- authors = self.manager.get_all_objects(Author, order_by_ref=Author.display_name)
+ authors = self.manager.get_all_objects(Author)
+ authors.sort(key=lambda author: get_natural_key(author.display_name))
self.authors_combo_box.clear()
self.authors_combo_box.addItem('')
self.authors = []
@@ -381,6 +384,7 @@
self.theme_combo_box.clear()
self.theme_combo_box.addItem('')
self.themes = theme_list
+ self.themes.sort(key=lambda theme: get_natural_key(theme))
self.theme_combo_box.addItems(theme_list)
set_case_insensitive_completer(self.themes, self.theme_combo_box)
=== 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-17 21:48:18 +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
@@ -121,7 +122,8 @@
Reloads the Authors list.
"""
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=lambda author: get_natural_key(author.display_name))
for author in authors:
if author.display_name:
author_name = QtWidgets.QListWidgetItem(author.display_name)
@@ -135,7 +137,8 @@
Reloads the Topics list.
"""
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=lambda topic: get_natural_key(topic.name))
for topic in topics:
topic_name = QtWidgets.QListWidgetItem(topic.name)
topic_name.setData(QtCore.Qt.UserRole, topic.id)
@@ -146,7 +149,8 @@
Reloads the Books list.
"""
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=lambda book: get_natural_key(book.name))
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-17 21:48:18 +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')
@@ -285,10 +285,10 @@
"""
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=lambda author: get_natural_key(author.display_name))
for author in search_results:
- songs = sorted(author.songs, key=lambda song: song.sort_key)
- for song in songs:
+ author.songs.sort(key=lambda song: song.sort_key)
+ for song in author.songs:
# Do not display temporary songs
if song.temporary:
continue
@@ -306,9 +306,10 @@
"""
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=lambda songbook_entry:
+ (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)
@@ -325,10 +326,10 @@
"""
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=lambda topic: get_natural_key(topic.name))
for topic in search_results:
- songs = sorted(topic.songs, key=lambda song: song.sort_key)
- for song in songs:
+ topic.songs.sort(key=lambda song: song.sort_key)
+ for song in topic.songs:
# Do not display temporary songs
if song.temporary:
continue
@@ -346,8 +347,8 @@
"""
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=lambda song: (get_natural_key(song.theme_name),
+ song.sort_key))
for song in search_results:
# Do not display temporary songs
if song.temporary:
@@ -366,9 +367,9 @@
"""
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=lambda song: (get_natural_key(song.ccli_number),
+ song.sort_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-17 21:48:18 +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
