Merge lp:~m2j/openlp/bug-1094342 into lp:openlp
- bug-1094342
- Merge into trunk
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Tim Bentley | ||||
Approved revision: | 2234 | ||||
Merged at revision: | 2230 | ||||
Proposed branch: | lp:~m2j/openlp/bug-1094342 | ||||
Merge into: | lp:openlp | ||||
Diff against target: |
515 lines (+97/-103) 15 files modified
openlp/core/ui/exceptionform.py (+9/-0) openlp/core/ui/thememanager.py (+2/-2) openlp/core/utils/__init__.py (+30/-16) openlp/plugins/bibles/forms/bibleimportform.py (+2/-2) openlp/plugins/bibles/lib/mediaitem.py (+3/-3) openlp/plugins/custom/lib/db.py (+3/-4) openlp/plugins/images/lib/mediaitem.py (+6/-6) openlp/plugins/media/lib/mediaitem.py (+3/-3) openlp/plugins/presentations/lib/mediaitem.py (+2/-3) openlp/plugins/songs/forms/songexportform.py (+1/-2) openlp/plugins/songs/lib/__init__.py (+1/-35) openlp/plugins/songs/lib/db.py (+4/-24) openlp/plugins/songs/lib/mediaitem.py (+2/-2) scripts/check_dependencies.py (+1/-0) tests/functional/openlp_core_utils/test_utils.py (+28/-1) |
||||
To merge this branch: | bzr merge lp:~m2j/openlp/bug-1094342 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Tim Bentley | Approve | ||
Andreas Preikschat (community) | Approve | ||
Raoul Snyman | Pending | ||
Review via email: mp+157524@code.launchpad.net |
This proposal supersedes a proposal from 2013-04-06.
Commit message
Description of the change
This branch replaces the "cmp=" argument when sorting a list of strings. This is necessary for Python3 transition.
Additionally it uses ICU for locale aware string sorting. For Windows this is mandatory as Microsoft provides a broken locale.strcoll method. For Python 2 we can't use Pythons locale aware sorting, as a bug breaks the key generation for unicode.
I replaced the natural compare algorithm in openlp/
The code in lines 113 to 114 is commented out as it is not needed in Python 2. The expected performance penalty should be low, but still not necessary.
Andreas Preikschat (googol-deactivatedaccount) wrote : Posted in a previous version of this proposal | # |
Meinert Jordan (m2j) wrote : Posted in a previous version of this proposal | # |
> Works on windows and one linux. I tested the sorting only with py2. On py3 I
> only tested if it starts.
>
> I haven't reviewed the code nor did performance tests yet.
Hi, I've looked to the Song class, and realized, that it makes sence to buffer the whole key generation for song titles. This should improve the overall performance (compared to trunk). So just wait for my next code commit.
Andreas Preikschat (googol-deactivatedaccount) wrote : Posted in a previous version of this proposal | # |
Can you replace this:
105 + key = re.findall(
by
key = REGEX_WHATEVER.
Andreas Preikschat (googol-deactivatedaccount) wrote : Posted in a previous version of this proposal | # |
92 + if ICU_COLLATOR is None:
93 + from languagemanager import LanguageManager
And this is okay.
Andreas Preikschat (googol-deactivatedaccount) wrote : Posted in a previous version of this proposal | # |
I tested your branch and compared it to trunk:
branch: 0:00:00.001660
trunk: 0:00:00.008635
2961 songs
Andreas Preikschat (googol-deactivatedaccount) wrote : Posted in a previous version of this proposal | # |
> I tested your branch and compared it to trunk:
> branch: 0:00:00.001660
> trunk: 0:00:00.008635
>
> 2961 songs
(Note: This does not consider any pre-computing)
Andreas Preikschat (googol-deactivatedaccount) wrote : Posted in a previous version of this proposal | # |
Use list comprehension:
108 + for index, part in enumerate(key):
109 + if part.isdigit():
110 + key[index] = int(part)
111 + else:
112 + key[index] = get_local_key(part)
return [int(part) if part.isdigit() else get_local_key(part) for part in keys]
Hm.. whats with memory usage then? Will "key" be a list with all strings to sort?
Meinert Jordan (m2j) wrote : Posted in a previous version of this proposal | # |
I will compile the regexp in a variable called DIGITS_
I will use list compression. I used a loop before, as there were originally more code in the loop.
About the memory usage: It is approximately same as before. The only difference is, that we are storing keys for the strings instead of the strings themself.
What is key?:
It is a list of string keys and integers. If you compare them, it will compare the elements.
Example:
strings: sort([u'Item 2', u'Item 1b'], key=get_
keys: sort([[u'item ', 2], [u'item ', 1, u' b']])
u'item ' == u'item '
1 < 2
So, key is a item per song, which provides __gt__(), __lt__(), and __eq__() methods for a correct sorting order.
Andreas Preikschat (googol-deactivatedaccount) wrote : Posted in a previous version of this proposal | # |
You should add a test to your proposal ;)
Andreas Preikschat (googol-deactivatedaccount) wrote : Posted in a previous version of this proposal | # |
9 + ICU_VERSION = u'OK'
You should be using icu.VERSION. However, I do not know if that always existed. So maybe we need another try-catch around it.
Andreas Preikschat (googol-deactivatedaccount) wrote : Posted in a previous version of this proposal | # |
But the rest looks good :)
Andreas Preikschat (googol-deactivatedaccount) wrote : Posted in a previous version of this proposal | # |
+
Tim Bentley (trb143) : Posted in a previous version of this proposal | # |
Raoul Snyman (raoul-snyman) wrote : Posted in a previous version of this proposal | # |
93 +def get_local_
Surely that should be "get_locale_key"?
Meinert Jordan (m2j) wrote : Posted in a previous version of this proposal | # |
Thanks the approves and the "locale" hint.
Raoul Snyman (raoul-snyman) wrote : Posted in a previous version of this proposal | # |
=== added file 'tests/
This file needs to be removed.
Andreas Preikschat (googol-deactivatedaccount) : Posted in a previous version of this proposal | # |
Andreas Preikschat (googol-deactivatedaccount) wrote : Posted in a previous version of this proposal | # |
You missed to fix the tests (function renaming).
Andreas Preikschat (googol-deactivatedaccount) : | # |
Tim Bentley (trb143) : | # |
Preview Diff
1 | === modified file 'openlp/core/ui/exceptionform.py' |
2 | --- openlp/core/ui/exceptionform.py 2013-03-14 10:46:19 +0000 |
3 | +++ openlp/core/ui/exceptionform.py 2013-04-06 19:11:20 +0000 |
4 | @@ -70,6 +70,14 @@ |
5 | except ImportError: |
6 | MAKO_VERSION = u'-' |
7 | try: |
8 | + import icu |
9 | + try: |
10 | + ICU_VERSION = icu.VERSION |
11 | + except AttributeError: |
12 | + ICU_VERSION = u'OK' |
13 | +except ImportError: |
14 | + ICU_VERSION = u'-' |
15 | +try: |
16 | import uno |
17 | arg = uno.createUnoStruct(u'com.sun.star.beans.PropertyValue') |
18 | arg.Name = u'nodepath' |
19 | @@ -143,6 +151,7 @@ |
20 | u'PyEnchant: %s\n' % ENCHANT_VERSION + \ |
21 | u'PySQLite: %s\n' % SQLITE_VERSION + \ |
22 | u'Mako: %s\n' % MAKO_VERSION + \ |
23 | + u'pyICU: %s\n' % ICU_VERSION + \ |
24 | u'pyUNO bridge: %s\n' % UNO_VERSION + \ |
25 | u'VLC: %s\n' % VLC_VERSION |
26 | if platform.system() == u'Linux': |
27 | |
28 | === modified file 'openlp/core/ui/thememanager.py' |
29 | --- openlp/core/ui/thememanager.py 2013-03-19 19:43:22 +0000 |
30 | +++ openlp/core/ui/thememanager.py 2013-04-06 19:11:20 +0000 |
31 | @@ -44,7 +44,7 @@ |
32 | from openlp.core.lib.ui import critical_error_message_box, create_widget_action |
33 | from openlp.core.theme import Theme |
34 | from openlp.core.ui import FileRenameForm, ThemeForm |
35 | -from openlp.core.utils import AppLocation, delete_file, locale_compare, get_filesystem_encoding |
36 | +from openlp.core.utils import AppLocation, delete_file, get_locale_key, get_filesystem_encoding |
37 | |
38 | log = logging.getLogger(__name__) |
39 | |
40 | @@ -418,7 +418,7 @@ |
41 | self.theme_list_widget.clear() |
42 | files = AppLocation.get_files(self.settings_section, u'.png') |
43 | # Sort the themes by its name considering language specific |
44 | - files.sort(key=lambda file_name: unicode(file_name), cmp=locale_compare) |
45 | + files.sort(key=lambda file_name: get_locale_key(unicode(file_name))) |
46 | # now process the file list of png files |
47 | for name in files: |
48 | # check to see file is in theme root directory |
49 | |
50 | === modified file 'openlp/core/utils/__init__.py' |
51 | --- openlp/core/utils/__init__.py 2013-03-04 10:55:02 +0000 |
52 | +++ openlp/core/utils/__init__.py 2013-04-06 19:11:20 +0000 |
53 | @@ -38,6 +38,7 @@ |
54 | from subprocess import Popen, PIPE |
55 | import sys |
56 | import urllib2 |
57 | +import icu |
58 | |
59 | from PyQt4 import QtGui, QtCore |
60 | |
61 | @@ -56,10 +57,12 @@ |
62 | log = logging.getLogger(__name__) |
63 | APPLICATION_VERSION = {} |
64 | IMAGES_FILTER = None |
65 | +ICU_COLLATOR = None |
66 | UNO_CONNECTION_TYPE = u'pipe' |
67 | #UNO_CONNECTION_TYPE = u'socket' |
68 | CONTROL_CHARS = re.compile(r'[\x00-\x1F\x7F-\x9F]', re.UNICODE) |
69 | INVALID_FILE_CHARS = re.compile(r'[\\/:\*\?"<>\|\+\[\]%]', re.UNICODE) |
70 | +DIGITS_OR_NONDIGITS = re.compile(r'\d+|\D+', re.UNICODE) |
71 | |
72 | |
73 | class VersionThread(QtCore.QThread): |
74 | @@ -379,21 +382,32 @@ |
75 | return re.sub('\%[a-zA-Z]', match_formatting, text) |
76 | |
77 | |
78 | -def locale_compare(string1, string2): |
79 | - """ |
80 | - Compares two strings according to the current locale settings. |
81 | - |
82 | - As any other compare function, returns a negative, or a positive value, |
83 | - or 0, depending on whether string1 collates before or after string2 or |
84 | - is equal to it. Comparison is case insensitive. |
85 | - """ |
86 | - # Function locale.strcoll() from standard Python library does not work properly on Windows. |
87 | - return locale.strcoll(string1.lower(), string2.lower()) |
88 | - |
89 | - |
90 | -# For performance reasons provide direct reference to compare function without wrapping it in another function making |
91 | -# the string lowercase. This is needed for sorting songs. |
92 | -locale_direct_compare = locale.strcoll |
93 | +def get_locale_key(string): |
94 | + """ |
95 | + Creates a key for case insensitive, locale aware string sorting. |
96 | + """ |
97 | + string = string.lower() |
98 | + # For Python 3 on platforms other than Windows ICU is not necessary. In those cases locale.strxfrm(str) can be used. |
99 | + global ICU_COLLATOR |
100 | + if ICU_COLLATOR is None: |
101 | + from languagemanager import LanguageManager |
102 | + locale = LanguageManager.get_language() |
103 | + icu_locale = icu.Locale(locale) |
104 | + ICU_COLLATOR = icu.Collator.createInstance(icu_locale) |
105 | + return ICU_COLLATOR.getSortKey(string) |
106 | + |
107 | + |
108 | +def get_natural_key(string): |
109 | + """ |
110 | + Generate a key for locale aware natural string sorting. |
111 | + Returns a list of string compare keys and integers. |
112 | + """ |
113 | + key = DIGITS_OR_NONDIGITS.findall(string) |
114 | + key = [int(part) if part.isdigit() else get_locale_key(part) for part in key] |
115 | + # Python 3 does not support comparision of different types anymore. So make sure, that we do not compare str and int. |
116 | + #if string[0].isdigit(): |
117 | + # return [''] + key |
118 | + return key |
119 | |
120 | |
121 | from applocation import AppLocation |
122 | @@ -403,4 +417,4 @@ |
123 | |
124 | __all__ = [u'AppLocation', u'ActionList', u'LanguageManager', u'get_application_version', u'check_latest_version', |
125 | u'add_actions', u'get_filesystem_encoding', u'get_web_page', u'get_uno_command', u'get_uno_instance', |
126 | - u'delete_file', u'clean_filename', u'format_time', u'locale_compare', u'locale_direct_compare'] |
127 | + u'delete_file', u'clean_filename', u'format_time', u'get_locale_key', u'get_natural_key'] |
128 | |
129 | === modified file 'openlp/plugins/bibles/forms/bibleimportform.py' |
130 | --- openlp/plugins/bibles/forms/bibleimportform.py 2013-03-19 19:43:22 +0000 |
131 | +++ openlp/plugins/bibles/forms/bibleimportform.py 2013-04-06 19:11:20 +0000 |
132 | @@ -38,7 +38,7 @@ |
133 | from openlp.core.lib.db import delete_database |
134 | from openlp.core.lib.ui import critical_error_message_box |
135 | from openlp.core.ui.wizard import OpenLPWizard, WizardStrings |
136 | -from openlp.core.utils import AppLocation, locale_compare |
137 | +from openlp.core.utils import AppLocation, get_locale_key |
138 | from openlp.plugins.bibles.lib.manager import BibleFormat |
139 | from openlp.plugins.bibles.lib.db import BiblesResourcesDB, clean_filename |
140 | |
141 | @@ -455,7 +455,7 @@ |
142 | """ |
143 | self.webTranslationComboBox.clear() |
144 | bibles = self.web_bible_list[index].keys() |
145 | - bibles.sort(cmp=locale_compare) |
146 | + bibles.sort(key=get_locale_key) |
147 | self.webTranslationComboBox.addItems(bibles) |
148 | |
149 | def onOsisBrowseButtonClicked(self): |
150 | |
151 | === modified file 'openlp/plugins/bibles/lib/mediaitem.py' |
152 | --- openlp/plugins/bibles/lib/mediaitem.py 2013-03-28 20:08:07 +0000 |
153 | +++ openlp/plugins/bibles/lib/mediaitem.py 2013-04-06 19:11:20 +0000 |
154 | @@ -36,7 +36,7 @@ |
155 | from openlp.core.lib.searchedit import SearchEdit |
156 | from openlp.core.lib.ui import set_case_insensitive_completer, create_horizontal_adjusting_combo_box, \ |
157 | critical_error_message_box, find_and_set_in_combo_box, build_icon |
158 | -from openlp.core.utils import locale_compare |
159 | +from openlp.core.utils import get_locale_key |
160 | from openlp.plugins.bibles.forms import BibleImportForm, EditBibleForm |
161 | from openlp.plugins.bibles.lib import LayoutStyle, DisplayStyle, VerseReferenceList, get_reference_separator, \ |
162 | LanguageSelection, BibleStrings |
163 | @@ -325,7 +325,7 @@ |
164 | # Get all bibles and sort the list. |
165 | bibles = self.plugin.manager.get_bibles().keys() |
166 | bibles = filter(None, bibles) |
167 | - bibles.sort(cmp=locale_compare) |
168 | + bibles.sort(key=get_locale_key) |
169 | # Load the bibles into the combo boxes. |
170 | self.quickVersionComboBox.addItems(bibles) |
171 | self.quickSecondComboBox.addItems(bibles) |
172 | @@ -461,7 +461,7 @@ |
173 | for book in book_data: |
174 | data = BiblesResourcesDB.get_book_by_id(book.book_reference_id) |
175 | books.append(data[u'name'] + u' ') |
176 | - books.sort(cmp=locale_compare) |
177 | + books.sort(key=get_locale_key) |
178 | set_case_insensitive_completer(books, self.quickSearchEdit) |
179 | |
180 | def on_import_click(self): |
181 | |
182 | === modified file 'openlp/plugins/custom/lib/db.py' |
183 | --- openlp/plugins/custom/lib/db.py 2013-01-05 22:17:30 +0000 |
184 | +++ openlp/plugins/custom/lib/db.py 2013-04-06 19:11:20 +0000 |
185 | @@ -35,7 +35,7 @@ |
186 | from sqlalchemy.orm import mapper |
187 | |
188 | from openlp.core.lib.db import BaseModel, init_db |
189 | -from openlp.core.utils import locale_compare |
190 | +from openlp.core.utils import get_locale_key |
191 | |
192 | class CustomSlide(BaseModel): |
193 | """ |
194 | @@ -44,11 +44,10 @@ |
195 | # By default sort the customs by its title considering language specific |
196 | # characters. |
197 | def __lt__(self, other): |
198 | - r = locale_compare(self.title, other.title) |
199 | - return True if r < 0 else False |
200 | + return get_locale_key(self.title) < get_locale_key(other.title) |
201 | |
202 | def __eq__(self, other): |
203 | - return 0 == locale_compare(self.title, other.title) |
204 | + return get_locale_key(self.title) == get_locale_key(other.title) |
205 | |
206 | |
207 | def init_schema(url): |
208 | |
209 | === modified file 'openlp/plugins/images/lib/mediaitem.py' |
210 | --- openlp/plugins/images/lib/mediaitem.py 2013-03-23 07:07:06 +0000 |
211 | +++ openlp/plugins/images/lib/mediaitem.py 2013-04-06 19:11:20 +0000 |
212 | @@ -36,7 +36,7 @@ |
213 | StringContent, TreeWidgetWithDnD, UiStrings, build_icon, check_directory_exists, check_item_selected, \ |
214 | create_thumb, translate, validate_thumb |
215 | from openlp.core.lib.ui import create_widget_action, critical_error_message_box |
216 | -from openlp.core.utils import AppLocation, delete_file, locale_compare, get_images_filter |
217 | +from openlp.core.utils import AppLocation, delete_file, get_locale_key, get_images_filter |
218 | from openlp.plugins.images.forms import AddGroupForm, ChooseGroupForm |
219 | from openlp.plugins.images.lib.db import ImageFilenames, ImageGroups |
220 | |
221 | @@ -255,7 +255,7 @@ |
222 | The ID of the group that will be added recursively |
223 | """ |
224 | image_groups = self.manager.get_all_objects(ImageGroups, ImageGroups.parent_id == parent_group_id) |
225 | - image_groups.sort(cmp=locale_compare, key=lambda group_object: group_object.group_name) |
226 | + image_groups.sort(key=lambda group_object: get_locale_key(group_object.group_name)) |
227 | folder_icon = build_icon(u':/images/image_group.png') |
228 | for image_group in image_groups: |
229 | group = QtGui.QTreeWidgetItem() |
230 | @@ -286,7 +286,7 @@ |
231 | combobox.clear() |
232 | combobox.top_level_group_added = False |
233 | image_groups = self.manager.get_all_objects(ImageGroups, ImageGroups.parent_id == parent_group_id) |
234 | - image_groups.sort(cmp=locale_compare, key=lambda group_object: group_object.group_name) |
235 | + image_groups.sort(key=lambda group_object: get_locale_key(group_object.group_name)) |
236 | for image_group in image_groups: |
237 | combobox.addItem(prefix + image_group.group_name, image_group.id) |
238 | self.fill_groups_combobox(combobox, image_group.id, prefix + ' ') |
239 | @@ -338,7 +338,7 @@ |
240 | self.expand_group(open_group.id) |
241 | # Sort the images by its filename considering language specific |
242 | # characters. |
243 | - images.sort(cmp=locale_compare, key=lambda image_object: os.path.split(unicode(image_object.filename))[1]) |
244 | + images.sort(key=lambda image_object: get_locale_key(os.path.split(unicode(image_object.filename))[1])) |
245 | for imageFile in images: |
246 | log.debug(u'Loading image: %s', imageFile.filename) |
247 | filename = os.path.split(imageFile.filename)[1] |
248 | @@ -525,9 +525,9 @@ |
249 | group_items.append(item) |
250 | if isinstance(item.data(0, QtCore.Qt.UserRole), ImageFilenames): |
251 | image_items.append(item) |
252 | - group_items.sort(cmp=locale_compare, key=lambda item: item.text(0)) |
253 | + group_items.sort(key=lambda item: get_locale_key(item.text(0))) |
254 | target_group.addChildren(group_items) |
255 | - image_items.sort(cmp=locale_compare, key=lambda item: item.text(0)) |
256 | + image_items.sort(key=lambda item: get_locale_key(item.text(0))) |
257 | target_group.addChildren(image_items) |
258 | |
259 | def generate_slide_data(self, service_item, item=None, xmlVersion=False, |
260 | |
261 | === modified file 'openlp/plugins/media/lib/mediaitem.py' |
262 | --- openlp/plugins/media/lib/mediaitem.py 2013-03-23 06:46:41 +0000 |
263 | +++ openlp/plugins/media/lib/mediaitem.py 2013-04-06 19:11:20 +0000 |
264 | @@ -37,7 +37,7 @@ |
265 | from openlp.core.lib.ui import critical_error_message_box, create_horizontal_adjusting_combo_box |
266 | from openlp.core.ui import DisplayController, Display, DisplayControllerType |
267 | from openlp.core.ui.media import get_media_players, set_media_players |
268 | -from openlp.core.utils import AppLocation, locale_compare |
269 | +from openlp.core.utils import AppLocation, get_locale_key |
270 | |
271 | log = logging.getLogger(__name__) |
272 | |
273 | @@ -261,7 +261,7 @@ |
274 | def load_list(self, media, target_group=None): |
275 | # Sort the media by its filename considering language specific |
276 | # characters. |
277 | - media.sort(cmp=locale_compare, key=lambda filename: os.path.split(unicode(filename))[1]) |
278 | + media.sort(key=lambda filename: get_locale_key(os.path.split(unicode(filename))[1])) |
279 | for track in media: |
280 | track_info = QtCore.QFileInfo(track) |
281 | if not os.path.exists(track): |
282 | @@ -287,7 +287,7 @@ |
283 | |
284 | def getList(self, type=MediaType.Audio): |
285 | media = Settings().value(self.settings_section + u'/media files') |
286 | - media.sort(cmp=locale_compare, key=lambda filename: os.path.split(unicode(filename))[1]) |
287 | + media.sort(key=lambda filename: get_locale_key(os.path.split(unicode(filename))[1])) |
288 | ext = [] |
289 | if type == MediaType.Audio: |
290 | ext = self.media_controller.audio_extensions_list |
291 | |
292 | === modified file 'openlp/plugins/presentations/lib/mediaitem.py' |
293 | --- openlp/plugins/presentations/lib/mediaitem.py 2013-03-23 06:46:41 +0000 |
294 | +++ openlp/plugins/presentations/lib/mediaitem.py 2013-04-06 19:11:20 +0000 |
295 | @@ -35,7 +35,7 @@ |
296 | from openlp.core.lib import MediaManagerItem, Registry, ItemCapabilities, ServiceItemContext, Settings, UiStrings, \ |
297 | build_icon, check_item_selected, create_thumb, translate, validate_thumb |
298 | from openlp.core.lib.ui import critical_error_message_box, create_horizontal_adjusting_combo_box |
299 | -from openlp.core.utils import locale_compare |
300 | +from openlp.core.utils import get_locale_key |
301 | from openlp.plugins.presentations.lib import MessageListener |
302 | |
303 | log = logging.getLogger(__name__) |
304 | @@ -153,8 +153,7 @@ |
305 | if not initialLoad: |
306 | self.main_window.display_progress_bar(len(files)) |
307 | # Sort the presentations by its filename considering language specific characters. |
308 | - files.sort(cmp=locale_compare, |
309 | - key=lambda filename: os.path.split(unicode(filename))[1]) |
310 | + files.sort(key=lambda filename: get_locale_key(os.path.split(unicode(filename))[1])) |
311 | for file in files: |
312 | if not initialLoad: |
313 | self.main_window.increment_progress_bar() |
314 | |
315 | === modified file 'openlp/plugins/songs/forms/songexportform.py' |
316 | --- openlp/plugins/songs/forms/songexportform.py 2013-03-14 22:22:18 +0000 |
317 | +++ openlp/plugins/songs/forms/songexportform.py 2013-04-06 19:11:20 +0000 |
318 | @@ -37,7 +37,6 @@ |
319 | from openlp.core.lib import Registry, UiStrings, create_separated_list, build_icon, translate |
320 | from openlp.core.lib.ui import critical_error_message_box |
321 | from openlp.core.ui.wizard import OpenLPWizard, WizardStrings |
322 | -from openlp.plugins.songs.lib import natcmp |
323 | from openlp.plugins.songs.lib.db import Song |
324 | from openlp.plugins.songs.lib.openlyricsexport import OpenLyricsExport |
325 | |
326 | @@ -222,7 +221,7 @@ |
327 | # Load the list of songs. |
328 | self.application.set_busy_cursor() |
329 | songs = self.plugin.manager.get_all_objects(Song) |
330 | - songs.sort(cmp=natcmp, key=lambda song: song.sort_key) |
331 | + songs.sort(key=lambda song: song.sort_key) |
332 | for song in songs: |
333 | # No need to export temporary songs. |
334 | if song.temporary: |
335 | |
336 | === modified file 'openlp/plugins/songs/lib/__init__.py' |
337 | --- openlp/plugins/songs/lib/__init__.py 2013-03-11 21:10:29 +0000 |
338 | +++ openlp/plugins/songs/lib/__init__.py 2013-04-06 19:11:20 +0000 |
339 | @@ -34,7 +34,7 @@ |
340 | from PyQt4 import QtGui |
341 | |
342 | from openlp.core.lib import translate |
343 | -from openlp.core.utils import CONTROL_CHARS, locale_direct_compare |
344 | +from openlp.core.utils import CONTROL_CHARS |
345 | from db import Author |
346 | from ui import SongStrings |
347 | |
348 | @@ -592,37 +592,3 @@ |
349 | text = u''.join(out) |
350 | return text, default_encoding |
351 | |
352 | - |
353 | -def natcmp(a, b): |
354 | - """ |
355 | - Natural string comparison which mimics the behaviour of Python's internal cmp function. |
356 | - """ |
357 | - if len(a) <= len(b): |
358 | - for i, key in enumerate(a): |
359 | - if isinstance(key, int) and isinstance(b[i], int): |
360 | - result = cmp(key, b[i]) |
361 | - elif isinstance(key, int) and not isinstance(b[i], int): |
362 | - result = locale_direct_compare(str(key), b[i]) |
363 | - elif not isinstance(key, int) and isinstance(b[i], int): |
364 | - result = locale_direct_compare(key, str(b[i])) |
365 | - else: |
366 | - result = locale_direct_compare(key, b[i]) |
367 | - if result != 0: |
368 | - return result |
369 | - if len(a) == len(b): |
370 | - return 0 |
371 | - else: |
372 | - return -1 |
373 | - else: |
374 | - for i, key in enumerate(b): |
375 | - if isinstance(a[i], int) and isinstance(key, int): |
376 | - result = cmp(a[i], key) |
377 | - elif isinstance(a[i], int) and not isinstance(key, int): |
378 | - result = locale_direct_compare(str(a[i]), key) |
379 | - elif not isinstance(a[i], int) and isinstance(key, int): |
380 | - result = locale_direct_compare(a[i], str(key)) |
381 | - else: |
382 | - result = locale_direct_compare(a[i], key) |
383 | - if result != 0: |
384 | - return result |
385 | - return 1 |
386 | |
387 | === modified file 'openlp/plugins/songs/lib/db.py' |
388 | --- openlp/plugins/songs/lib/db.py 2013-01-18 23:31:02 +0000 |
389 | +++ openlp/plugins/songs/lib/db.py 2013-04-06 19:11:20 +0000 |
390 | @@ -38,6 +38,7 @@ |
391 | from sqlalchemy.sql.expression import func |
392 | |
393 | from openlp.core.lib.db import BaseModel, init_db |
394 | +from openlp.core.utils import get_natural_key |
395 | |
396 | |
397 | class Author(BaseModel): |
398 | @@ -69,36 +70,15 @@ |
399 | def __init__(self): |
400 | self.sort_key = () |
401 | |
402 | - def _try_int(self, s): |
403 | - """ |
404 | - Convert to integer if possible. |
405 | - """ |
406 | - try: |
407 | - return int(s) |
408 | - except: |
409 | - return s.lower() |
410 | - |
411 | - def _natsort_key(self, s): |
412 | - """ |
413 | - Used internally to get a tuple by which s is sorted. |
414 | - """ |
415 | - return map(self._try_int, re.findall(r'(\d+|\D+)', s)) |
416 | - |
417 | - # This decorator tells sqlalchemy to call this method everytime |
418 | - # any data on this object is updated. |
419 | - |
420 | @reconstructor |
421 | def init_on_load(self): |
422 | """ |
423 | - Precompute a tuple to be used for sorting. |
424 | + Precompute a natural sorting, locale aware sorting key. |
425 | |
426 | Song sorting is performance sensitive operation. |
427 | - To get maximum speed lets precompute the string |
428 | - used for comparison. |
429 | + To get maximum speed lets precompute the sorting key. |
430 | """ |
431 | - # Avoid the overhead of converting string to lowercase and to QString |
432 | - # with every call to sort(). |
433 | - self.sort_key = self._natsort_key(self.title) |
434 | + self.sort_key = get_natural_key(self.title) |
435 | |
436 | |
437 | class Topic(BaseModel): |
438 | |
439 | === modified file 'openlp/plugins/songs/lib/mediaitem.py' |
440 | --- openlp/plugins/songs/lib/mediaitem.py 2013-03-23 06:46:41 +0000 |
441 | +++ openlp/plugins/songs/lib/mediaitem.py 2013-04-06 19:11:20 +0000 |
442 | @@ -43,7 +43,7 @@ |
443 | from openlp.plugins.songs.forms.songmaintenanceform import SongMaintenanceForm |
444 | from openlp.plugins.songs.forms.songimportform import SongImportForm |
445 | from openlp.plugins.songs.forms.songexportform import SongExportForm |
446 | -from openlp.plugins.songs.lib import VerseType, clean_string, natcmp |
447 | +from openlp.plugins.songs.lib import VerseType, clean_string |
448 | from openlp.plugins.songs.lib.db import Author, Song, Book, MediaFile |
449 | from openlp.plugins.songs.lib.ui import SongStrings |
450 | from openlp.plugins.songs.lib.xml import OpenLyrics, SongXML |
451 | @@ -225,7 +225,7 @@ |
452 | log.debug(u'display results Song') |
453 | self.save_auto_select_id() |
454 | self.list_view.clear() |
455 | - searchresults.sort(cmp=natcmp, key=lambda song: song.sort_key) |
456 | + searchresults.sort(key=lambda song: song.sort_key) |
457 | for song in searchresults: |
458 | # Do not display temporary songs |
459 | if song.temporary: |
460 | |
461 | === modified file 'scripts/check_dependencies.py' |
462 | --- scripts/check_dependencies.py 2013-03-14 10:51:49 +0000 |
463 | +++ scripts/check_dependencies.py 2013-04-06 19:11:20 +0000 |
464 | @@ -83,6 +83,7 @@ |
465 | 'mako', |
466 | 'migrate', |
467 | 'uno', |
468 | + 'icu', |
469 | ] |
470 | |
471 | |
472 | |
473 | === modified file 'tests/functional/openlp_core_utils/test_utils.py' |
474 | --- tests/functional/openlp_core_utils/test_utils.py 2012-12-07 21:38:02 +0000 |
475 | +++ tests/functional/openlp_core_utils/test_utils.py 2013-04-06 19:11:20 +0000 |
476 | @@ -5,7 +5,7 @@ |
477 | |
478 | from mock import patch |
479 | |
480 | -from openlp.core.utils import get_filesystem_encoding, _get_frozen_path |
481 | +from openlp.core.utils import get_filesystem_encoding, _get_frozen_path, get_locale_key, get_natural_key |
482 | |
483 | class TestUtils(TestCase): |
484 | """ |
485 | @@ -56,3 +56,30 @@ |
486 | # THEN: The frozen parameter is returned |
487 | assert _get_frozen_path(u'frozen', u'not frozen') == u'frozen', u'Should return "frozen"' |
488 | |
489 | + def get_locale_key_test(self): |
490 | + """ |
491 | + Test the get_locale_key(string) function |
492 | + """ |
493 | + with patch(u'openlp.core.utils.languagemanager.LanguageManager.get_language') as mocked_get_language: |
494 | + # GIVEN: The language is German |
495 | + # 0x00C3 (A with diaresis) should be sorted as "A". 0x00DF (sharp s) should be sorted as "ss". |
496 | + mocked_get_language.return_value = u'de' |
497 | + unsorted_list = [u'Auszug', u'Aushang', u'\u00C4u\u00DFerung'] |
498 | + # WHEN: We sort the list and use get_locale_key() to generate the sorting keys |
499 | + # THEN: We get a properly sorted list |
500 | + test_passes = sorted(unsorted_list, key=get_locale_key) == [u'Aushang', u'\u00C4u\u00DFerung', u'Auszug'] |
501 | + assert test_passes, u'Strings should be sorted properly' |
502 | + |
503 | + def get_natural_key_test(self): |
504 | + """ |
505 | + Test the get_natural_key(string) function |
506 | + """ |
507 | + with patch(u'openlp.core.utils.languagemanager.LanguageManager.get_language') as mocked_get_language: |
508 | + # GIVEN: The language is English (a language, which sorts digits before letters) |
509 | + mocked_get_language.return_value = u'en' |
510 | + unsorted_list = [u'item 10a', u'item 3b', u'1st item'] |
511 | + # WHEN: We sort the list and use get_natural_key() to generate the sorting keys |
512 | + # THEN: We get a properly sorted list |
513 | + test_passes = sorted(unsorted_list, key=get_natural_key) == [u'1st item', u'item 3b', u'item 10a'] |
514 | + assert test_passes, u'Numbers should be sorted naturally' |
515 | + |
Works on windows and one linux. I tested the sorting only with py2. On py3 I only tested if it starts.
I haven't reviewed the code nor did performance tests yet.