Merge lp:~m2j/openlp/bug-687638 into lp:openlp

[Meinert Jordan]

Bug #687638: Use ICU collator by default to overcome issue in Windows locale.strcoll call.

I've splitted a previous merge request. This one contains only ICU integration:

Bug #687638

locale.strcoll does not work correct on Windows with utf-8 [http://sgehrig.wordpress.com/2008/12/08/update-on-strcoll-utf-8-issue/].
The performance ICU is similar to locale.strcoll on Linux (12% faster for a English word list, 10% slower on a German one)

ICU is said to give better results and makes less cross platform troubles. I've implemented it to be ued if available. Fallback is the POSIX locale.strcoll. It seems to be only necessary on Windows, so I suggest to include it only in Windows builds.

[Meinert Jordan (m2j)]

Some more informations:
ICU binaries for Windows have a size of arround 9MB as they contain a lot of data.
I haven't figured out any other way to collide international strings on Windows with comparable performance.
Some information about colliding and Qt can be found on this blog post:
http://labs.qt.nokia.com/2011/06/14/string-collation-with-locales/

[Raoul Snyman (raoul-snyman)]

This looks OK to me.

review: Approve

[Andreas Preikschat (googol-deactivatedaccount)]

Maybe somebody with windows should test this?

[Meinert Jordan (m2j)]

While looking for other i18n things (and figuring out, that Qt just introduced nice methods like QLocale::createSeparatedList()), I stumbled over the fact, that the performance of Qts implementation of collation might be fine as well. Sorting 1500 strings with Python takes about 20ms so that increasing the time by a factor of 5 could be still acceptable.

I will try to find some time to benchmark the Qt algorithm on Windows to see, if this is a alternative to adding so much data to the Windows package.

[Tim Bentley (trb143)]

review: Approve

[Andreas Preikschat (googol-deactivatedaccount)]

A question, did you compare the performance? I am not sure, but isn't the function you specify called dozens times?

review: Needs Information

[Meinert Jordan (m2j)]

I haven't checked it yet, as I have no Windows machine with Python arround ATM.
The only position where we are sorting big lists is in the songs media manager (after editing/importing/deleting a song). Therefore we should have some realtime performance for up to 2000 strings.

[Andreas Preikschat (googol-deactivatedaccount)]

> The only position where we are sorting big lists is in the songs media manager
> (after editing/importing/deleting a song).

Do not forget the "Search as you type" feature!

[Raoul Snyman (raoul-snyman)]

Meinert, I'd just like us to do a little more playing around with this before we merge it. In particular I'm thinking of testing out the "Search as you type" speed, and documenting on the wiki how to include ICU in the Windows build.

review: Needs Information

[Jonathan Corwin (j-corwin)]

Where was this left out of interest? Who is waiting on who?

[matysek (mzibricky)]

I think this merge can be closed since there is a new pending request to fix related bug.

[Meinert Jordan (m2j)]

I suggested the use of Qt comparision as well, but as it was reported to have a poor performance, I delayed this until I had time to test the performance under Windows.

For long time I did not take the time, to setup Windows and make a test. So please test the performance on Windows for big databases (~2000 entries), before you relay on this solution.

[Andreas Preikschat (googol-deactivatedaccount)]

Meinert: any change you pick up with this?

Unmerged revisions

1161. By Meinert Jordan on 2012-01-18

purge scripture reference code from this branch

=== modified file 'openlp/core/ui/exceptionform.py'
--- openlp/core/ui/exceptionform.py	2011-12-27 10:33:55 +0000
+++ openlp/core/ui/exceptionform.py	2012-01-18 18:04:32 +0000
@@ -81,6 +81,11 @@
     WEBKIT_VERSION = QtWebKit.qWebKitVersion()
 except AttributeError:
     WEBKIT_VERSION = u'-'
+try:
+    import icu
+    ICU_VERSION = u'OK'
+except ImportError:
+    ICU_VERSION = u'-'
 
 
 from openlp.core.lib import translate, SettingsManager
@@ -125,7 +130,8 @@
             u'PyEnchant: %s\n' % ENCHANT_VERSION + \
             u'PySQLite: %s\n' % SQLITE_VERSION + \
             u'Mako: %s\n' % MAKO_VERSION + \
-            u'pyUNO bridge: %s\n' % UNO_VERSION
+            u'pyUNO bridge: %s\n' % UNO_VERSION + \
+            u'PyICU: %s\n' % ICU_VERSION
         if platform.system() == u'Linux':
             if os.environ.get(u'KDE_FULL_SESSION') == u'true':
                 system = system + u'Desktop: KDE SC\n'

=== modified file 'openlp/core/ui/thememanager.py'
--- openlp/core/ui/thememanager.py	2012-01-04 17:19:49 +0000
+++ openlp/core/ui/thememanager.py	2012-01-18 18:04:32 +0000
@@ -29,7 +29,6 @@
 import zipfile
 import shutil
 import logging
-import locale
 import re
 
 from xml.etree.ElementTree import ElementTree, XML
@@ -44,7 +43,8 @@
     context_menu_action, context_menu_separator
 from openlp.core.theme import Theme
 from openlp.core.ui import FileRenameForm, ThemeForm
-from openlp.core.utils import AppLocation, delete_file, get_filesystem_encoding
+from openlp.core.utils import AppLocation, delete_file, \
+    get_filesystem_encoding, get_local_collator
 
 log = logging.getLogger(__name__)
 
@@ -459,7 +459,7 @@
         # Sort the themes by its name considering language specific characters.
         # lower() is needed for windows!
         files.sort(key=lambda filename: unicode(filename).lower(),
-           cmp=locale.strcoll)
+           cmp=get_local_collator)
         # now process the file list of png files
         for name in files:
             # check to see file is in theme root directory

=== modified file 'openlp/core/utils/__init__.py'
--- openlp/core/utils/__init__.py	2011-12-27 10:33:55 +0000
+++ openlp/core/utils/__init__.py	2012-01-18 18:04:32 +0000
@@ -33,6 +33,7 @@
 import sys
 import time
 import urllib2
+import locale
 from datetime import datetime
 from subprocess import Popen, PIPE
 
@@ -45,6 +46,17 @@
     except ImportError:
         XDG_BASE_AVAILABLE = False
 
+LOCALE_COLLATOR = locale.strcoll
+try:
+    import icu
+    try:
+        icu_locale = icu.Locale(locale.getlocale()[0])
+        LOCALE_COLLATOR = icu.Collator.createInstance(icu_locale).compare
+    except icu.InvalidArgsError:
+        pass
+except ImportError:
+    pass
+
 import openlp
 from openlp.core.lib import Receiver, translate, check_directory_exists
 
@@ -482,10 +494,16 @@
         return resolver.resolve(u'uno:socket,host=localhost,port=2002;' \
             + u'urp;StarOffice.ComponentContext')
 
+def get_local_collator(string1, string2):
+    """
+    Returns a collator for locale aware string sorting.
+    """
+    return LOCALE_COLLATOR(string1, string2)
+
 from languagemanager import LanguageManager
 from actions import ActionList
 
 __all__ = [u'AppLocation', u'get_application_version', u'check_latest_version',
     u'add_actions', u'get_filesystem_encoding', u'LanguageManager',
     u'ActionList', u'get_web_page', u'get_uno_command', u'get_uno_instance',
-    u'delete_file', u'clean_filename']
+    u'get_local_collator', u'delete_file', u'clean_filename']

=== modified file 'openlp/plugins/bibles/forms/bibleimportform.py'
--- openlp/plugins/bibles/forms/bibleimportform.py	2011-12-27 10:33:55 +0000
+++ openlp/plugins/bibles/forms/bibleimportform.py	2012-01-18 18:04:32 +0000
@@ -30,7 +30,6 @@
 import logging
 import os
 import os.path
-import locale
 
 from PyQt4 import QtCore, QtGui
 
@@ -38,7 +37,7 @@
 from openlp.core.lib.db import delete_database
 from openlp.core.lib.ui import UiStrings, critical_error_message_box
 from openlp.core.ui.wizard import OpenLPWizard, WizardStrings
-from openlp.core.utils import AppLocation
+from openlp.core.utils import AppLocation, get_local_collator
 from openlp.plugins.bibles.lib.manager import BibleFormat
 from openlp.plugins.bibles.lib.db import BiblesResourcesDB, clean_filename
 
@@ -522,7 +521,7 @@
         """
         self.webTranslationComboBox.clear()
         bibles = self.web_bible_list[index].keys()
-        bibles.sort(cmp=locale.strcoll)
+        bibles.sort(cmp=get_local_collator)
         self.webTranslationComboBox.addItems(bibles)
 
     def onOsisBrowseButtonClicked(self):

=== modified file 'openlp/plugins/bibles/lib/mediaitem.py'
--- openlp/plugins/bibles/lib/mediaitem.py	2011-12-27 10:33:55 +0000
+++ openlp/plugins/bibles/lib/mediaitem.py	2012-01-18 18:04:32 +0000
@@ -26,7 +26,6 @@
 ###############################################################################
 
 import logging
-import locale
 
 from PyQt4 import QtCore, QtGui
 
@@ -36,6 +35,7 @@
 from openlp.core.lib.ui import UiStrings, add_widget_completer, \
     media_item_combo_box, critical_error_message_box, \
     find_and_set_in_combo_box, build_icon
+from openlp.core.utils import get_local_collator
 from openlp.plugins.bibles.forms import BibleImportForm
 from openlp.plugins.bibles.lib import LayoutStyle, DisplayStyle, \
     VerseReferenceList, get_reference_match
@@ -373,7 +373,7 @@
         self.advancedSecondComboBox.addItem(u'')
         # Get all bibles and sort the list.
         bibles = self.plugin.manager.get_bibles().keys()
-        bibles.sort(cmp=locale.strcoll)
+        bibles.sort(cmp=get_local_collator)
         # Load the bibles into the combo boxes.
         for bible in bibles:
             if bible:
@@ -481,7 +481,7 @@
                                 book_data_temp.append(book)
                     book_data = book_data_temp
                 books = [book.name + u' ' for book in book_data]
-                books.sort(cmp=locale.strcoll)
+                books.sort(cmp=get_local_collator)
         add_widget_completer(books, self.quickSearchEdit)
 
     def onQuickVersionComboBox(self):

=== modified file 'openlp/plugins/custom/lib/mediaitem.py'
--- openlp/plugins/custom/lib/mediaitem.py	2011-12-30 21:40:13 +0000
+++ openlp/plugins/custom/lib/mediaitem.py	2012-01-18 18:04:32 +0000
@@ -26,7 +26,6 @@
 ###############################################################################
 
 import logging
-import locale
 
 from PyQt4 import QtCore, QtGui
 from sqlalchemy.sql import or_, func
@@ -34,6 +33,7 @@
 from openlp.core.lib import MediaManagerItem, Receiver, ItemCapabilities, \
     check_item_selected, translate
 from openlp.core.lib.ui import UiStrings
+from oprnlp.core.utils import get_local_collator
 from openlp.plugins.custom.forms import EditCustomForm
 from openlp.plugins.custom.lib import CustomXMLParser
 from openlp.plugins.custom.lib.db import CustomSlide
@@ -109,7 +109,7 @@
         # Sort the customs by its title considering language specific
         # characters. lower() is needed for windows!
         custom_slides.sort(
-            cmp=locale.strcoll, key=lambda custom: custom.title.lower())
+            cmp=get_local_collator, key=lambda custom: custom.title.lower())
         for custom_slide in custom_slides:
             custom_name = QtGui.QListWidgetItem(custom_slide.title)
             custom_name.setData(

=== modified file 'openlp/plugins/images/lib/mediaitem.py'
--- openlp/plugins/images/lib/mediaitem.py	2011-12-27 10:33:55 +0000
+++ openlp/plugins/images/lib/mediaitem.py	2012-01-18 18:04:32 +0000
@@ -27,7 +27,6 @@
 
 import logging
 import os
-import locale
 
 from PyQt4 import QtCore, QtGui
 
@@ -35,7 +34,8 @@
     SettingsManager, translate, check_item_selected, check_directory_exists, \
     Receiver, create_thumb, validate_thumb
 from openlp.core.lib.ui import UiStrings, critical_error_message_box
-from openlp.core.utils import AppLocation, delete_file, get_images_filter
+from openlp.core.utils import AppLocation, delete_file, get_images_filter, \
+    get_local_collator
 
 log = logging.getLogger(__name__)
 
@@ -120,7 +120,7 @@
             self.plugin.formparent.displayProgressBar(len(images))
         # Sort the themes by its filename considering language specific
         # characters. lower() is needed for windows!
-        images.sort(cmp=locale.strcoll,
+        images.sort(cmp=get_local_collator,
             key=lambda filename: os.path.split(unicode(filename))[1].lower())
         for imageFile in images:
             if not initialLoad:

=== modified file 'openlp/plugins/media/lib/mediaitem.py'
--- openlp/plugins/media/lib/mediaitem.py	2011-12-27 10:33:55 +0000
+++ openlp/plugins/media/lib/mediaitem.py	2012-01-18 18:04:32 +0000
@@ -27,7 +27,6 @@
 
 import logging
 import os
-import locale
 
 from PyQt4 import QtCore, QtGui
 
@@ -37,6 +36,7 @@
 from openlp.core.lib.ui import UiStrings, critical_error_message_box, \
     media_item_combo_box
 from openlp.core.ui import Controller, Display
+from openlp.core.utils import get_local_collator
 
 log = logging.getLogger(__name__)
 
@@ -278,7 +278,7 @@
     def loadList(self, media):
         # Sort the themes by its filename considering language specific
         # characters. lower() is needed for windows!
-        media.sort(cmp=locale.strcoll,
+        media.sort(cmp=get_local_collator,
             key=lambda filename: os.path.split(unicode(filename))[1].lower())
         for track in media:
             track_info = QtCore.QFileInfo(track)
@@ -298,7 +298,7 @@
 
     def getList(self, type=MediaType.Audio):
         media = SettingsManager.load_list(self.settingsSection, u'media')
-        media.sort(cmp=locale.strcoll,
+        media.sort(cmp=get_local_collator,
             key=lambda filename: os.path.split(unicode(filename))[1].lower())
         ext = []
         if type == MediaType.Audio:

=== modified file 'openlp/plugins/presentations/lib/mediaitem.py'
--- openlp/plugins/presentations/lib/mediaitem.py	2011-12-27 10:33:55 +0000
+++ openlp/plugins/presentations/lib/mediaitem.py	2012-01-18 18:04:32 +0000
@@ -27,7 +27,6 @@
 
 import logging
 import os
-import locale
 
 from PyQt4 import QtCore, QtGui
 
@@ -36,6 +35,7 @@
     validate_thumb
 from openlp.core.lib.ui import UiStrings, critical_error_message_box, \
     media_item_combo_box
+from openlp.core.utils import get_local_collator
 from openlp.plugins.presentations.lib import MessageListener
 
 log = logging.getLogger(__name__)
@@ -168,7 +168,7 @@
             self.plugin.formparent.displayProgressBar(len(files))
         # Sort the themes by its filename considering language specific
         # characters. lower() is needed for windows!
-        files.sort(cmp=locale.strcoll,
+        files.sort(cmp=get_local_collator,
             key=lambda filename: os.path.split(unicode(filename))[1].lower())
         for file in files:
             if not initialLoad:

=== modified file 'openlp/plugins/songs/forms/songexportform.py'
--- openlp/plugins/songs/forms/songexportform.py	2011-12-27 10:33:55 +0000
+++ openlp/plugins/songs/forms/songexportform.py	2012-01-18 18:04:32 +0000
@@ -28,7 +28,6 @@
 The :mod:`songexportform` module provides the wizard for exporting songs to the
 OpenLyrics format.
 """
-import locale
 import logging
 
 from PyQt4 import QtCore, QtGui
@@ -36,6 +35,7 @@
 from openlp.core.lib import build_icon, Receiver, SettingsManager, translate
 from openlp.core.lib.ui import UiStrings, critical_error_message_box
 from openlp.core.ui.wizard import OpenLPWizard, WizardStrings
+from openlp.core.utils import get_local_collator
 from openlp.plugins.songs.lib.db import Song
 from openlp.plugins.songs.lib.openlyricsexport import OpenLyricsExport
 
@@ -250,7 +250,7 @@
         # Load the list of songs.
         Receiver.send_message(u'cursor_busy')
         songs = self.plugin.manager.get_all_objects(Song)
-        songs.sort(cmp=locale.strcoll, key=lambda song: song.title.lower())
+        songs.sort(cmp=get_local_collator, key=lambda song: song.title.lower())
         for song in songs:
             # No need to export temporary songs.
             if song.temporary:

=== modified file 'openlp/plugins/songs/lib/mediaitem.py'
--- openlp/plugins/songs/lib/mediaitem.py	2011-12-30 21:40:13 +0000
+++ openlp/plugins/songs/lib/mediaitem.py	2012-01-18 18:04:32 +0000
@@ -26,7 +26,6 @@
 ###############################################################################
 
 import logging
-import locale
 import re
 import os
 import shutil
@@ -38,7 +37,7 @@
     translate, check_item_selected, PluginStatus
 from openlp.core.lib.ui import UiStrings, context_menu_action, \
     context_menu_separator
-from openlp.core.utils import AppLocation
+from openlp.core.utils import AppLocation, get_local_collator
 from openlp.plugins.songs.forms import EditSongForm, SongMaintenanceForm, \
     SongImportForm, SongExportForm
 from openlp.plugins.songs.lib import OpenLyrics, SongXML, VerseType, \
@@ -240,7 +239,7 @@
         # Sort the songs by its title considering language specific characters.
         # lower() is needed for windows!
         searchresults.sort(
-            cmp=locale.strcoll, key=lambda song: song.title.lower())
+            cmp=get_local_collator, key=lambda song: song.title.lower())
         for song in searchresults:
             # Do not display temporary songs
             if song.temporary:

=== modified file 'scripts/check_dependencies.py'
--- scripts/check_dependencies.py	2011-10-26 07:40:12 +0000
+++ scripts/check_dependencies.py	2012-01-18 18:04:32 +0000
@@ -74,6 +74,7 @@
     'mako',
     'migrate',
     'uno',
+    'icu',
 ]