Merge lp:~googol-deactivatedaccount/openlp/bug-804747 into lp:openlp

[Andreas Preikschat]

- Fixed bug #804747 (Bible disappear after upgrade)
- Fixed bug Bug #824129 (Bibles plugin does not always delete corrupted bibles from falied imports)
- Removed not needed or redundant variables.
- Removed not needed code.
- When upgrading bibles move the files to the temp directory.
- clean ups

Hello,

1) Fixed bug #804747 (Bible disappear after upgrade)
The fix is in line 303. We did not pass the file name, that means that we used the version name. If you do not pass the file name the file name is crated from the version name (and since this contained Japanese characters) they were removed (so the file name was ".sqlite")
Furthermore, I changed the way we clean the file name and move the function to utils.
2) Fixed bug Bug #824129 (Bibles plugin does not always delete corrupted bibles from falied imports)
We did not close the session. If you do not close the session, the file cannot be deleted on windows.
3) Removed not needed or redundant variables.
4) Removed not needed code.
I removed the code which allowed you to change the version name in some *rare* (!) circumstances. I do not see why the upgrade wizard should take care of any of this.
5) When upgrading bibles move the files to the temp directory.
6) clean ups

[Tim Bentley (trb143)]

Creation of temp dir name shouls be common to easy for one to be changed and rest missed

review: Needs Fixing

[Andreas Preikschat (googol-deactivatedaccount)]

Note: Especially testing on Windows is requested.

[Tim Bentley (trb143)]

review: Approve

[Raoul Snyman (raoul-snyman)]

At a glance looks fine to me.

review: Approve

[Andreas Preikschat (googol-deactivatedaccount)]

Fixed a traceback on windows and fixed bug #824129 (there might still be cases where corrupted bibles were not removed, but this fixes this for windows).

[Tim Bentley (trb143)]

review: Approve

[Raoul Snyman (raoul-snyman)]

review: Approve

=== modified file 'openlp/core/ui/mainwindow.py'
--- openlp/core/ui/mainwindow.py	2011-08-16 21:19:57 +0000
+++ openlp/core/ui/mainwindow.py	2011-08-17 10:17:27 +0000
@@ -28,6 +28,7 @@
 import logging
 import os
 import sys
+import shutil
 from tempfile import gettempdir
 
 from PyQt4 import QtCore, QtGui
@@ -726,11 +727,7 @@
                 plugin.firstTime()
         Receiver.send_message(u'openlp_process_events')
         temp_dir = os.path.join(unicode(gettempdir()), u'openlp')
-        if not os.path.exists(temp_dir):
-            return
-        for filename in os.listdir(temp_dir):
-            delete_file(os.path.join(temp_dir, filename))
-        os.removedirs(temp_dir)
+        shutil.rmtree(temp_dir, True)
 
     def onFirstTimeWizardClicked(self):
         """
 
=== modified file 'openlp/core/utils/__init__.py'
--- openlp/core/utils/__init__.py	2011-07-17 15:09:30 +0000
+++ openlp/core/utils/__init__.py	2011-08-17 10:17:27 +0000
@@ -386,6 +386,17 @@
     else:
         return os.path.split(path)
 
+def clean_filename(filename):
+    """
+    Removes invalid characters from the given ``filename``.
+
+    ``filename``
+        The "dirty" file name to clean.
+    """
+    if not isinstance(filename, unicode):
+        filename = unicode(filename, u'utf-8')
+    return re.sub(r'[/\\?*|<>\[\]":<>+%]+', u'_', filename).strip(u'_')
+
 def delete_file(file_path_name):
     """
     Deletes a file from the system.
@@ -492,4 +503,4 @@
 __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'file_is_unicode', u'get_uno_command',
-    u'get_uno_instance', u'delete_file']
+    u'get_uno_instance', u'delete_file', u'clean_filename']
 
=== modified file 'openlp/plugins/bibles/forms/bibleupgradeform.py'
--- openlp/plugins/bibles/forms/bibleupgradeform.py	2011-07-14 18:42:38 +0000
+++ openlp/plugins/bibles/forms/bibleupgradeform.py	2011-08-17 10:17:27 +0000
@@ -29,17 +29,17 @@
 import logging
 import os
 import shutil
+from tempfile import gettempdir
 
 from PyQt4 import QtCore, QtGui
 
 from openlp.core.lib import Receiver, SettingsManager, translate, \
     check_directory_exists
-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, delete_file
 from openlp.plugins.bibles.lib.db import BibleDB, BibleMeta, OldBibleDB, \
-    BiblesResourcesDB, clean_filename
+    BiblesResourcesDB
 from openlp.plugins.bibles.lib.http import BSExtract, BGExtract, CWExtract
 
 log = logging.getLogger(__name__)
@@ -70,6 +70,7 @@
         self.suffix = u'.sqlite'
         self.settingsSection = u'bibles'
         self.path = AppLocation.get_section_data_path(self.settingsSection)
+        self.temp_dir = os.path.join(gettempdir(), u'openlp')
         self.files = self.manager.old_bible_databases
         self.success = {}
         self.newbibles = {}
@@ -91,20 +92,6 @@
         log.debug(u'Stopping import')
         self.stop_import_flag = True
 
-    def onCheckBoxIndexChanged(self, index):
-        """
-        Show/Hide warnings if CheckBox state has changed
-        """
-        for number, filename in enumerate(self.files):
-            if not self.checkBox[number].checkState() == QtCore.Qt.Checked:
-                self.verticalWidget[number].hide()
-                self.formWidget[number].hide()
-            else:
-                version_name = unicode(self.versionNameEdit[number].text())
-                if self.manager.exists(version_name):
-                    self.verticalWidget[number].show()
-                    self.formWidget[number].show()
-
     def reject(self):
         """
         Stop the wizard on cancel button, close button or ESC key.
@@ -113,8 +100,6 @@
         self.stop_import_flag = True
         if not self.currentPage() == self.progressPage:
             self.done(QtGui.QDialog.Rejected)
-        else:
-            self.postWizard()
 
     def onCurrentIdChanged(self, pageId):
         """
@@ -124,7 +109,7 @@
             self.preWizard()
             self.performWizard()
             self.postWizard()
-        elif self.page(pageId) == self.selectPage and self.maxBibles == 0:
+        elif self.page(pageId) == self.selectPage and not self.files:
             self.next()
 
     def onBackupBrowseButtonClicked(self):
@@ -243,78 +228,13 @@
         Add the content to the scrollArea.
         """
         self.checkBox = {}
-        self.versionNameEdit = {}
-        self.versionNameLabel = {}
-        self.versionInfoLabel = {}
-        self.versionInfoPixmap = {}
-        self.verticalWidget = {}
-        self.horizontalLayout = {}
-        self.formWidget = {}
-        self.formLayoutAttention = {}
         for number, filename in enumerate(self.files):
             bible = OldBibleDB(self.mediaItem, path=self.path, file=filename[0])
             self.checkBox[number] = QtGui.QCheckBox(self.scrollAreaContents)
-            checkBoxName = u'checkBox[%d]' % number
-            self.checkBox[number].setObjectName(checkBoxName)
+            self.checkBox[number].setObjectName(u'checkBox[%d]' % number)
             self.checkBox[number].setText(bible.get_name())
             self.checkBox[number].setCheckState(QtCore.Qt.Checked)
             self.formLayout.addWidget(self.checkBox[number])
-            self.verticalWidget[number] = QtGui.QWidget(self.scrollAreaContents)
-            verticalWidgetName = u'verticalWidget[%d]' % number
-            self.verticalWidget[number].setObjectName(verticalWidgetName)
-            self.horizontalLayout[number] = QtGui.QHBoxLayout(
-                self.verticalWidget[number])
-            self.horizontalLayout[number].setContentsMargins(25, 0, 0, 0)
-            horizontalLayoutName = u'horizontalLayout[%d]' % number
-            self.horizontalLayout[number].setObjectName(horizontalLayoutName)
-            self.versionInfoPixmap[number] = QtGui.QLabel(
-                self.verticalWidget[number])
-            versionInfoPixmapName = u'versionInfoPixmap[%d]' % number
-            self.versionInfoPixmap[number].setObjectName(versionInfoPixmapName)
-            self.versionInfoPixmap[number].setPixmap(QtGui.QPixmap(
-                u':/bibles/bibles_upgrade_alert.png'))
-            self.versionInfoPixmap[number].setAlignment(QtCore.Qt.AlignRight)
-            self.horizontalLayout[number].addWidget(
-                self.versionInfoPixmap[number])
-            self.versionInfoLabel[number] = QtGui.QLabel(
-                self.verticalWidget[number])
-            versionInfoLabelName = u'versionInfoLabel[%d]' % number
-            self.versionInfoLabel[number].setObjectName(versionInfoLabelName)
-            sizePolicy = QtGui.QSizePolicy(QtGui.QSizePolicy.Expanding,
-                QtGui.QSizePolicy.Preferred)
-            sizePolicy.setHorizontalStretch(0)
-            sizePolicy.setVerticalStretch(0)
-            sizePolicy.setHeightForWidth(
-                self.versionInfoLabel[number].sizePolicy().hasHeightForWidth())
-            self.versionInfoLabel[number].setSizePolicy(sizePolicy)
-            self.horizontalLayout[number].addWidget(
-                self.versionInfoLabel[number])
-            self.formLayout.addWidget(self.verticalWidget[number])
-            self.formWidget[number] = QtGui.QWidget(self.scrollAreaContents)
-            formWidgetName = u'formWidget[%d]' % number
-            self.formWidget[number].setObjectName(formWidgetName)
-            self.formLayoutAttention[number] = QtGui.QFormLayout(
-                self.formWidget[number])
-            self.formLayoutAttention[number].setContentsMargins(25, 0, 0, 5)
-            formLayoutAttentionName = u'formLayoutAttention[%d]' % number
-            self.formLayoutAttention[number].setObjectName(
-                formLayoutAttentionName)
-            self.versionNameLabel[number] = QtGui.QLabel(
-                self.formWidget[number])
-            self.versionNameLabel[number].setObjectName(u'VersionNameLabel')
-            self.formLayoutAttention[number].setWidget(0,
-                QtGui.QFormLayout.LabelRole, self.versionNameLabel[number])
-            self.versionNameEdit[number] = QtGui.QLineEdit(
-                self.formWidget[number])
-            self.versionNameEdit[number].setObjectName(u'VersionNameEdit')
-            self.formLayoutAttention[number].setWidget(0,
-                QtGui.QFormLayout.FieldRole, self.versionNameEdit[number])
-            self.versionNameEdit[number].setText(bible.get_name())
-            self.formLayout.addWidget(self.formWidget[number])
-            # Set up the Signal for the checkbox.
-            QtCore.QObject.connect(self.checkBox[number],
-                QtCore.SIGNAL(u'stateChanged(int)'),
-                self.onCheckBoxIndexChanged)
         self.spacerItem = QtGui.QSpacerItem(20, 5, QtGui.QSizePolicy.Minimum,
             QtGui.QSizePolicy.Expanding)
         self.formLayout.addItem(self.spacerItem)
@@ -327,23 +247,6 @@
         for number, filename in enumerate(self.files):
             self.formLayout.removeWidget(self.checkBox[number])
             self.checkBox[number].setParent(None)
-            self.horizontalLayout[number].removeWidget(
-                self.versionInfoPixmap[number])
-            self.versionInfoPixmap[number].setParent(None)
-            self.horizontalLayout[number].removeWidget(
-                self.versionInfoLabel[number])
-            self.versionInfoLabel[number].setParent(None)
-            self.formLayout.removeWidget(self.verticalWidget[number])
-            self.verticalWidget[number].setParent(None)
-            self.formLayoutAttention[number].removeWidget(
-                self.versionNameLabel[number])
-            self.versionNameLabel[number].setParent(None)
-            self.formLayoutAttention[number].removeWidget(
-                self.versionNameEdit[number])
-            self.formLayoutAttention[number].deleteLater()
-            self.versionNameEdit[number].setParent(None)
-            self.formLayout.removeWidget(self.formWidget[number])
-            self.formWidget[number].setParent(None)
         self.formLayout.removeItem(self.spacerItem)
 
     def retranslateUi(self):
@@ -385,12 +288,6 @@
         self.selectPage.setSubTitle(
             translate('BiblesPlugin.UpgradeWizardForm',
             'Please select the Bibles to upgrade'))
-        for number, bible in enumerate(self.files):
-            self.versionNameLabel[number].setText(
-                translate('BiblesPlugin.UpgradeWizardForm', 'Version name:'))
-            self.versionInfoLabel[number].setText(
-                translate('BiblesPlugin.UpgradeWizardForm', 'This '
-                'Bible still exists. Please change the name or uncheck it.'))
         self.progressPage.setTitle(translate('BiblesPlugin.UpgradeWizardForm',
             'Upgrading'))
         self.progressPage.setSubTitle(
@@ -425,58 +322,16 @@
                         return False
             return True
         elif self.currentPage() == self.selectPage:
+            check_directory_exists(self.temp_dir)
             for number, filename in enumerate(self.files):
                 if not self.checkBox[number].checkState() == QtCore.Qt.Checked:
                     continue
-                version_name = unicode(self.versionNameEdit[number].text())
-                if not version_name:
-                    critical_error_message_box(UiStrings().EmptyField,
-                        translate('BiblesPlugin.UpgradeWizardForm',
-                        'You need to specify a version name for your Bible.'))
-                    self.versionNameEdit[number].setFocus()
-                    return False
-                elif self.manager.exists(version_name):
-                    critical_error_message_box(
-                        translate('BiblesPlugin.UpgradeWizardForm',
-                            'Bible Exists'),
-                        translate('BiblesPlugin.UpgradeWizardForm',
-                        'This Bible already exists. Please upgrade '
-                        'a different Bible, delete the existing one or '
-                        'uncheck.'))
-                    self.versionNameEdit[number].setFocus()
-                    return False
-                elif os.path.exists(os.path.join(self.path, clean_filename(
-                    version_name))) and version_name == filename[1]:
-                    newfilename = u'old_database_%s' % filename[0]
-                    if not os.path.exists(os.path.join(self.path,
-                        newfilename)):
-                        os.rename(os.path.join(self.path, filename[0]),
-                            os.path.join(self.path, newfilename))
-                        self.files[number] = [newfilename, filename[1]]
-                        continue
-                    else:
-                        critical_error_message_box(
-                            translate('BiblesPlugin.UpgradeWizardForm',
-                            'Bible Exists'),
-                            translate('BiblesPlugin.UpgradeWizardForm',
-                            'This Bible already exists. Please upgrade '
-                            'a different Bible, delete the existing one or '
-                            'uncheck.'))
-                        self.verticalWidget[number].show()
-                        self.formWidget[number].show()
-                        self.versionNameEdit[number].setFocus()
-                        return False
-                elif os.path.exists(os.path.join(self.path,
-                    clean_filename(version_name))):
-                    critical_error_message_box(
-                        translate('BiblesPlugin.UpgradeWizardForm',
-                        'Bible Exists'),
-                        translate('BiblesPlugin.UpgradeWizardForm',
-                        'This Bible already exists. Please upgrade '
-                        'a different Bible, delete the existing one or '
-                        'uncheck.'))
-                    self.versionNameEdit[number].setFocus()
-                    return False
+                # Move bibles to temp dir.
+                if not os.path.exists(os.path.join(self.temp_dir, filename[0])):
+                    shutil.move(
+                        os.path.join(self.path, filename[0]), self.temp_dir)
+                else:
+                    delete_file(os.path.join(self.path, filename[0]))
             return True
         if self.currentPage() == self.progressPage:
             return True
@@ -495,16 +350,8 @@
         self.files = self.manager.old_bible_databases
         self.addScrollArea()
         self.retranslateUi()
-        self.maxBibles = len(self.files)
         for number, filename in enumerate(self.files):
             self.checkBox[number].setCheckState(QtCore.Qt.Checked)
-            oldname = filename[1]
-            if self.manager.exists(oldname):
-                self.verticalWidget[number].show()
-                self.formWidget[number].show()
-            else:
-                self.verticalWidget[number].hide()
-                self.formWidget[number].hide()
         self.progressBar.show()
         self.restart()
         self.finishButton.setVisible(False)
@@ -516,9 +363,8 @@
         Prepare the UI for the upgrade.
         """
         OpenLPWizard.preWizard(self)
-        self.progressLabel.setText(translate(
-            'BiblesPlugin.UpgradeWizardForm',
-            'Starting upgrade...'))
+        self.progressLabel.setText(
+            translate('BiblesPlugin.UpgradeWizardForm', 'Starting upgrade...'))
         Receiver.send_message(u'openlp_process_events')
 
     def performWizard(self):
@@ -527,48 +373,42 @@
         """
         self.include_webbible = False
         proxy_server = None
-        if self.maxBibles == 0:
+        if not self.files:
             self.progressLabel.setText(
                 translate('BiblesPlugin.UpgradeWizardForm', 'There are no '
                 'Bibles that need to be upgraded.'))
             self.progressBar.hide()
             return
-        self.maxBibles = 0
+        max_bibles = 0
         for number, file in enumerate(self.files):
             if self.checkBox[number].checkState() == QtCore.Qt.Checked:
-                self.maxBibles += 1
-        number = 0
-        for biblenumber, filename in enumerate(self.files):
+                max_bibles += 1
+        oldBible = None
+        for number, filename in enumerate(self.files):
+            # Close the previous bible's connection.
+            if oldBible is not None:
+                oldBible.close_connection()
+                # Set to None to make obvious that we have already closed the
+                # database.
+                oldBible = None
             if self.stop_import_flag:
-                bible_failed = True
+                self.success[number] = False
                 break
-            bible_failed = False
-            self.success[biblenumber] = False
-            if not self.checkBox[biblenumber].checkState() == QtCore.Qt.Checked:
+            if not self.checkBox[number].checkState() == QtCore.Qt.Checked:
+                self.success[number] = False
                 continue
             self.progressBar.reset()
-            oldbible = OldBibleDB(self.mediaItem, path=self.path,
+            oldBible = OldBibleDB(self.mediaItem, path=self.temp_dir,
                 file=filename[0])
             name = filename[1]
-            if name is None:
-                delete_file(os.path.join(self.path, filename[0]))
-                self.incrementProgressBar(unicode(translate(
-                    'BiblesPlugin.UpgradeWizardForm',
-                    'Upgrading Bible %s of %s: "%s"\nFailed')) %
-                    (number + 1, self.maxBibles, name),
-                    self.progressBar.maximum() - self.progressBar.value())
-                number += 1
-                continue
             self.progressLabel.setText(unicode(translate(
                 'BiblesPlugin.UpgradeWizardForm',
                 'Upgrading Bible %s of %s: "%s"\nUpgrading ...')) %
-                (number + 1, self.maxBibles, name))
-            if os.path.exists(os.path.join(self.path, filename[0])):
-                name = unicode(self.versionNameEdit[biblenumber].text())
+                (number + 1, max_bibles, name))
             self.newbibles[number] = BibleDB(self.mediaItem, path=self.path,
-                name=name)
+                name=name, file=filename[0])
             self.newbibles[number].register(self.plugin.upgrade_wizard)
-            metadata = oldbible.get_metadata()
+            metadata = oldBible.get_metadata()
             webbible = False
             meta_data = {}
             for meta in metadata:
@@ -595,7 +435,7 @@
                         u'name: "%s" failed' % (
                         meta_data[u'download source'],
                         meta_data[u'download name']))
-                    delete_database(self.path, clean_filename(name))
+                    self.newbibles[number].session.close()
                     del self.newbibles[number]
                     critical_error_message_box(
                         translate('BiblesPlugin.UpgradeWizardForm',
@@ -606,9 +446,9 @@
                     self.incrementProgressBar(unicode(translate(
                         'BiblesPlugin.UpgradeWizardForm',
                         'Upgrading Bible %s of %s: "%s"\nFailed')) %
-                        (number + 1, self.maxBibles, name),
+                        (number + 1, max_bibles, name),
                         self.progressBar.maximum() - self.progressBar.value())
-                    number += 1
+                    self.success[number] = False
                     continue
                 bible = BiblesResourcesDB.get_webbible(
                     meta_data[u'download name'],
@@ -621,25 +461,25 @@
                     language_id = self.newbibles[number].get_language(name)
                 if not language_id:
                     log.warn(u'Upgrading from "%s" failed' % filename[0])
-                    delete_database(self.path, clean_filename(name))
+                    self.newbibles[number].session.close()
                     del self.newbibles[number]
                     self.incrementProgressBar(unicode(translate(
                         'BiblesPlugin.UpgradeWizardForm',
                         'Upgrading Bible %s of %s: "%s"\nFailed')) %
-                        (number + 1, self.maxBibles, name),
+                        (number + 1, max_bibles, name),
                         self.progressBar.maximum() - self.progressBar.value())
-                    number += 1
+                    self.success[number] = False
                     continue
                 self.progressBar.setMaximum(len(books))
                 for book in books:
                     if self.stop_import_flag:
-                        bible_failed = True
+                        self.success[number] = False
                         break
                     self.incrementProgressBar(unicode(translate(
                         'BiblesPlugin.UpgradeWizardForm',
                         'Upgrading Bible %s of %s: "%s"\n'
                         'Upgrading %s ...')) %
-                        (number + 1, self.maxBibles, name, book))
+                        (number + 1, max_bibles, name, book))
                     book_ref_id = self.newbibles[number].\
                         get_book_ref_id_by_name(book, len(books), language_id)
                     if not book_ref_id:
@@ -647,24 +487,24 @@
                             u'name: "%s" aborted by user' % (
                             meta_data[u'download source'],
                             meta_data[u'download name']))
-                        delete_database(self.path, clean_filename(name))
+                        self.newbibles[number].session.close()
                         del self.newbibles[number]
-                        bible_failed = True
+                        self.success[number] = False
                         break
                     book_details = BiblesResourcesDB.get_book_by_id(book_ref_id)
                     db_book = self.newbibles[number].create_book(book,
                         book_ref_id, book_details[u'testament_id'])
-                    # Try to import still downloaded verses
-                    oldbook = oldbible.get_book(book)
+                    # Try to import already downloaded verses.
+                    oldbook = oldBible.get_book(book)
                     if oldbook:
-                        verses = oldbible.get_verses(oldbook[u'id'])
+                        verses = oldBible.get_verses(oldbook[u'id'])
                         if not verses:
                             log.warn(u'No verses found to import for book '
                                 u'"%s"', book)
                             continue
                         for verse in verses:
                             if self.stop_import_flag:
-                                bible_failed = True
+                                self.success[number] = False
                                 break
                             self.newbibles[number].create_verse(db_book.id,
                                 int(verse[u'chapter']),
@@ -678,40 +518,40 @@
                     language_id = self.newbibles[number].get_language(name)
                 if not language_id:
                     log.warn(u'Upgrading books from "%s" failed' % name)
-                    delete_database(self.path, clean_filename(name))
+                    self.newbibles[number].session.close()
                     del self.newbibles[number]
                     self.incrementProgressBar(unicode(translate(
                         'BiblesPlugin.UpgradeWizardForm',
                         'Upgrading Bible %s of %s: "%s"\nFailed')) %
-                        (number + 1, self.maxBibles, name),
+                        (number + 1, max_bibles, name),
                         self.progressBar.maximum() - self.progressBar.value())
-                    number += 1
+                    self.success[number] = False
                     continue
-                books = oldbible.get_books()
+                books = oldBible.get_books()
                 self.progressBar.setMaximum(len(books))
                 for book in books:
                     if self.stop_import_flag:
-                        bible_failed = True
+                        self.success[number] = False
                         break
                     self.incrementProgressBar(unicode(translate(
                         'BiblesPlugin.UpgradeWizardForm',
                         'Upgrading Bible %s of %s: "%s"\n'
                         'Upgrading %s ...')) %
-                        (number + 1, self.maxBibles, name, book[u'name']))
+                        (number + 1, max_bibles, name, book[u'name']))
                     book_ref_id = self.newbibles[number].\
                         get_book_ref_id_by_name(book[u'name'], len(books),
                         language_id)
                     if not book_ref_id:
                         log.warn(u'Upgrading books from %s " '\
                             'failed - aborted by user' % name)
-                        delete_database(self.path, clean_filename(name))
+                        self.newbibles[number].session.close()
                         del self.newbibles[number]
-                        bible_failed = True
+                        self.success[number] = False
                         break
                     book_details = BiblesResourcesDB.get_book_by_id(book_ref_id)
                     db_book = self.newbibles[number].create_book(book[u'name'],
                         book_ref_id, book_details[u'testament_id'])
-                    verses = oldbible.get_verses(book[u'id'])
+                    verses = oldBible.get_verses(book[u'id'])
                     if not verses:
                         log.warn(u'No verses found to import for book '
                             u'"%s"', book[u'name'])
@@ -719,31 +559,32 @@
                         continue
                     for verse in verses:
                         if self.stop_import_flag:
-                            bible_failed = True
+                            self.success[number] = False
                             break
                         self.newbibles[number].create_verse(db_book.id,
                             int(verse[u'chapter']),
                             int(verse[u'verse']), unicode(verse[u'text']))
                         Receiver.send_message(u'openlp_process_events')
                     self.newbibles[number].session.commit()
-            if not bible_failed:
+            if self.success.has_key(number) and not self.success[number]:
+                self.incrementProgressBar(unicode(translate(
+                    'BiblesPlugin.UpgradeWizardForm',
+                    'Upgrading Bible %s of %s: "%s"\nFailed')) %
+                    (number + 1, max_bibles, name),
+                    self.progressBar.maximum() - self.progressBar.value())
+            else:
+                self.success[number] = True
                 self.newbibles[number].create_meta(u'Version', name)
-                oldbible.close_connection()
-                delete_file(os.path.join(self.path, filename[0]))
                 self.incrementProgressBar(unicode(translate(
                     'BiblesPlugin.UpgradeWizardForm',
                     'Upgrading Bible %s of %s: "%s"\n'
                     'Complete')) %
-                    (number + 1, self.maxBibles, name))
-                self.success[biblenumber] = True
-            else:
-                self.incrementProgressBar(unicode(translate(
-                    'BiblesPlugin.UpgradeWizardForm',
-                    'Upgrading Bible %s of %s: "%s"\nFailed')) %
-                    (number + 1, self.maxBibles, name),
-                    self.progressBar.maximum() - self.progressBar.value())
-                delete_database(self.path, clean_filename(name))
-            number += 1
+                    (number + 1, max_bibles, name))
+            if self.newbibles.has_key(number):
+                self.newbibles[number].session.close()
+        # Close the last bible's connection if possible.
+        if oldBible is not None:
+            oldBible.close_connection()
 
     def postWizard(self):
         """
@@ -752,10 +593,14 @@
         successful_import = 0
         failed_import = 0
         for number, filename in enumerate(self.files):
-            if number in self.success and self.success[number] == True:
+            if self.success.has_key(number) and self.success[number]:
                 successful_import += 1
             elif self.checkBox[number].checkState() == QtCore.Qt.Checked:
                 failed_import += 1
+                # Delete upgraded (but not complete, corrupted, ...) bible.
+                delete_file(os.path.join(self.path, filename[0]))
+                # Copy not upgraded bible back.
+                shutil.move(os.path.join(self.temp_dir, filename[0]), self.path)
         if failed_import > 0:
             failed_import_text = unicode(translate(
                 'BiblesPlugin.UpgradeWizardForm',
@@ -776,7 +621,8 @@
                     'Bible(s): %s successful%s')) % (successful_import,
                     failed_import_text))
         else:
-            self.progressLabel.setText(
-                    translate('BiblesPlugin.UpgradeWizardForm', 'Upgrade '
-                    'failed.'))
+            self.progressLabel.setText(translate(
+                'BiblesPlugin.UpgradeWizardForm', 'Upgrade failed.'))
+        # Remove temp directory.
+        shutil.rmtree(self.temp_dir, True)
         OpenLPWizard.postWizard(self)
 
=== modified file 'openlp/plugins/bibles/forms/languageform.py'
--- openlp/plugins/bibles/forms/languageform.py	2011-05-26 19:13:11 +0000
+++ openlp/plugins/bibles/forms/languageform.py	2011-08-17 10:17:27 +0000
@@ -44,8 +44,8 @@
     Class to manage a dialog which ask the user for a language.
     """
     log.info(u'LanguageForm loaded')
-    
-    def __init__(self, parent = None):
+
+    def __init__(self, parent=None):
         """
         Constructor
         """
@@ -57,12 +57,11 @@
         if bible_name:
             self.bibleLabel.setText(unicode(bible_name))
         items = BiblesResourcesDB.get_languages()
-        for item in items:
-            self.languageComboBox.addItem(item[u'name'])
+        self.languageComboBox.addItems([item[u'name'] for item in items])
         return QDialog.exec_(self)
-    
+
     def accept(self):
-        if self.languageComboBox.currentText() == u'':
+        if not self.languageComboBox.currentText():
             critical_error_message_box(
                 message=translate('BiblesPlugin.LanguageForm',
                 'You need to choose a language.'))
 
=== modified file 'openlp/plugins/bibles/lib/db.py'
--- openlp/plugins/bibles/lib/db.py	2011-07-07 18:03:12 +0000
+++ openlp/plugins/bibles/lib/db.py	2011-08-17 10:17:27 +0000
@@ -39,7 +39,7 @@
 from openlp.core.lib import Receiver, translate
 from openlp.core.lib.db import BaseModel, init_db, Manager
 from openlp.core.lib.ui import critical_error_message_box
-from openlp.core.utils import AppLocation
+from openlp.core.utils import AppLocation, clean_filename
 
 log = logging.getLogger(__name__)
 
@@ -63,19 +63,6 @@
     """
     pass
 
-def clean_filename(filename):
-    """
-    Clean up the version name of the Bible and convert it into a valid
-    file name.
-
-    ``filename``
-        The "dirty" file name or version name.
-    """
-    if not isinstance(filename, unicode):
-        filename = unicode(filename, u'utf-8')
-    filename = re.sub(r'[^\w]+', u'_', filename).strip(u'_')
-    return filename + u'.sqlite'
-
 def init_schema(url):
     """
     Setup a bible database connection and initialise the database schema.
@@ -158,7 +145,7 @@
             self.name = kwargs[u'name']
             if not isinstance(self.name, unicode):
                 self.name = unicode(self.name, u'utf-8')
-            self.file = clean_filename(self.name)
+            self.file = clean_filename(self.name) + u'.sqlite'
         if u'file' in kwargs:
             self.file = kwargs[u'file']
         Manager.__init__(self, u'bibles', init_schema, self.file)
@@ -210,7 +197,7 @@
             The book_reference_id from bibles_resources.sqlite of the book.
 
         ``testament``
-            *Defaults to 1.* The testament_reference_id from 
+            *Defaults to 1.* The testament_reference_id from
             bibles_resources.sqlite of the testament this book belongs to.
         """
         log.debug(u'BibleDB.create_book("%s", "%s")', name, bk_ref_id)
@@ -329,7 +316,7 @@
         return self.get_object_filtered(Book, Book.book_reference_id.like(id))
 
     def get_book_ref_id_by_name(self, book, maxbooks, language_id=None):
-        log.debug(u'BibleDB.get_book_ref_id_by_name:("%s", "%s")', book, 
+        log.debug(u'BibleDB.get_book_ref_id_by_name:("%s", "%s")', book,
             language_id)
         if BiblesResourcesDB.get_book(book, True):
             book_temp = BiblesResourcesDB.get_book(book, True)
@@ -471,7 +458,7 @@
 
     def get_language(self, bible_name=None):
         """
-        If no language is given it calls a dialog window where the user could 
+        If no language is given it calls a dialog window where the user could
         select the bible language.
         Return the language id of a bible.
 
@@ -521,9 +508,9 @@
     some resources which are used in the Bibles plugin.
     A wrapper class around a small SQLite database which contains the download
     resources, a biblelist from the different download resources, the books,
-    chapter counts and verse counts for the web download Bibles, a language 
-    reference, the testament reference and some alternative book names. This 
-    class contains a singleton "cursor" so that only one connection to the 
+    chapter counts and verse counts for the web download Bibles, a language
+    reference, the testament reference and some alternative book names. This
+    class contains a singleton "cursor" so that only one connection to the
     SQLite database is ever used.
     """
     cursor = None
@@ -582,7 +569,7 @@
 
         ``name``
             The name or abbreviation of the book.
-        
+
         ``lower``
             True if the comparsion should be only lowercase
         """
@@ -592,7 +579,7 @@
         if lower:
             books = BiblesResourcesDB.run_sql(u'SELECT id, testament_id, name, '
                     u'abbreviation, chapters FROM book_reference WHERE '
-                    u'LOWER(name) = ? OR LOWER(abbreviation) = ?', 
+                    u'LOWER(name) = ? OR LOWER(abbreviation) = ?',
                     (name.lower(), name.lower()))
         else:
             books = BiblesResourcesDB.run_sql(u'SELECT id, testament_id, name, '
@@ -621,7 +608,7 @@
         if not isinstance(id, int):
             id = int(id)
         books = BiblesResourcesDB.run_sql(u'SELECT id, testament_id, name, '
-                u'abbreviation, chapters FROM book_reference WHERE id = ?', 
+                u'abbreviation, chapters FROM book_reference WHERE id = ?',
                 (id, ))
         if books:
             return {
@@ -645,12 +632,12 @@
         ``chapter``
             The chapter number.
         """
-        log.debug(u'BiblesResourcesDB.get_chapter("%s", "%s")', book_id, 
+        log.debug(u'BiblesResourcesDB.get_chapter("%s", "%s")', book_id,
             chapter)
         if not isinstance(chapter, int):
             chapter = int(chapter)
         chapters = BiblesResourcesDB.run_sql(u'SELECT id, book_reference_id, '
-            u'chapter, verse_count FROM chapters WHERE book_reference_id = ?', 
+            u'chapter, verse_count FROM chapters WHERE book_reference_id = ?',
             (book_id,))
         if chapters:
             return {
@@ -687,7 +674,7 @@
         ``chapter``
             The number of the chapter.
         """
-        log.debug(u'BiblesResourcesDB.get_verse_count("%s", "%s")', book_id,  
+        log.debug(u'BiblesResourcesDB.get_verse_count("%s", "%s")', book_id,
             chapter)
         details = BiblesResourcesDB.get_chapter(book_id, chapter)
         if details:
@@ -715,7 +702,7 @@
             }
         else:
             return None
-    
+
     @staticmethod
     def get_webbibles(source):
         """
@@ -737,7 +724,7 @@
                 u'id': bible[0],
                 u'name': bible[1],
                 u'abbreviation': bible[2],
-                u'language_id': bible[3], 
+                u'language_id': bible[3],
                 u'download_source_id': bible[4]
                 }
                 for bible in bibles
@@ -752,11 +739,11 @@
 
         ``abbreviation``
             The abbreviation of the webbible.
-            
+
         ``source``
             The source of the webbible.
         """
-        log.debug(u'BiblesResourcesDB.get_webbibles("%s", "%s")', abbreviation, 
+        log.debug(u'BiblesResourcesDB.get_webbibles("%s", "%s")', abbreviation,
             source)
         if not isinstance(abbreviation, unicode):
             abbreviation = unicode(abbreviation)
@@ -765,14 +752,14 @@
         source = BiblesResourcesDB.get_download_source(source)
         bible = BiblesResourcesDB.run_sql(u'SELECT id, name, abbreviation, '
             u'language_id, download_source_id FROM webbibles WHERE '
-            u'download_source_id = ? AND abbreviation = ?', (source[u'id'], 
+            u'download_source_id = ? AND abbreviation = ?', (source[u'id'],
             abbreviation))
         if bible:
             return {
                 u'id': bible[0][0],
                 u'name': bible[0][1],
                 u'abbreviation': bible[0][2],
-                u'language_id': bible[0][3], 
+                u'language_id': bible[0][3],
                 u'download_source_id': bible[0][4]
                 }
         else:
@@ -785,11 +772,11 @@
 
         ``name``
             The name to search the id.
-            
+
         ``language_id``
             The language_id for which language should be searched
         """
-        log.debug(u'BiblesResourcesDB.get_alternative_book_name("%s", "%s")', 
+        log.debug(u'BiblesResourcesDB.get_alternative_book_name("%s", "%s")',
             name, language_id)
         if language_id:
             books = BiblesResourcesDB.run_sql(u'SELECT book_reference_id, name '
@@ -806,7 +793,7 @@
     @staticmethod
     def get_language(name):
         """
-        Return a dict containing the language id, name and code by name or 
+        Return a dict containing the language id, name and code by name or
         abbreviation.
 
         ``name``
@@ -865,7 +852,7 @@
 
 class AlternativeBookNamesDB(QtCore.QObject, Manager):
     """
-    This class represents a database-bound alternative book names system. 
+    This class represents a database-bound alternative book names system.
     """
     cursor = None
     conn = None
@@ -874,7 +861,7 @@
     def get_cursor():
         """
         Return the cursor object. Instantiate one if it doesn't exist yet.
-        If necessary loads up the database and creates the tables if the 
+        If necessary loads up the database and creates the tables if the
         database doesn't exist.
         """
         if AlternativeBookNamesDB.cursor is None:
@@ -904,7 +891,7 @@
 
         ``parameters``
             Any variable parameters to add to the query
-        
+
         ``commit``
             If a commit statement is necessary this should be True.
         """
@@ -921,11 +908,11 @@
 
         ``name``
             The name to search the id.
-            
+
         ``language_id``
             The language_id for which language should be searched
         """
-        log.debug(u'AlternativeBookNamesDB.get_book_reference_id("%s", "%s")', 
+        log.debug(u'AlternativeBookNamesDB.get_book_reference_id("%s", "%s")',
             name, language_id)
         if language_id:
             books = AlternativeBookNamesDB.run_sql(u'SELECT book_reference_id, '
@@ -962,11 +949,11 @@
 
 class OldBibleDB(QtCore.QObject, Manager):
     """
-    This class conects to the old bible databases to reimport them to the new 
+    This class conects to the old bible databases to reimport them to the new
     database scheme.
     """
     cursor = None
-    
+
     def __init__(self, parent, **kwargs):
         """
         The constructor loads up the database and creates and initialises the
 
=== modified file 'openlp/plugins/bibles/lib/manager.py'
--- openlp/plugins/bibles/lib/manager.py	2011-07-07 18:03:12 +0000
+++ openlp/plugins/bibles/lib/manager.py	2011-08-17 10:17:27 +0000
@@ -151,9 +151,10 @@
             name = bible.get_name()
             # Remove corrupted files.
             if name is None:
+                bible.session.close()
                 delete_file(os.path.join(self.path, filename))
                 continue
-            # Find old database versions
+            # Find old database versions.
             if bible.is_old_database():
                 self.old_bible_databases.append([filename, name])
                 bible.session.close()
@@ -220,7 +221,7 @@
         return [
             {
                 u'name': book.name,
-                u'book_reference_id': book.book_reference_id, 
+                u'book_reference_id': book.book_reference_id,
                 u'chapters': self.db_cache[bible].get_chapter_count(book)
             }
             for book in self.db_cache[bible].get_books()
@@ -229,10 +230,10 @@
     def get_chapter_count(self, bible, book):
         """
         Returns the number of Chapters for a given book.
-        
+
         ``bible``
             Unicode. The Bible to get the list of books from.
-        
+
         ``book``
             The book object to get the chapter count for.
         """
@@ -295,7 +296,7 @@
                     if db_book:
                         book_id = db_book.book_reference_id
                         log.debug(u'Book name corrected to "%s"', db_book.name)
-                        new_reflist.append((book_id, item[1], item[2], 
+                        new_reflist.append((book_id, item[1], item[2],
                             item[3]))
                     else:
                         log.debug(u'OpenLP failed to find book %s', item[0])
 
=== modified file 'openlp/plugins/bibles/lib/mediaitem.py'
--- openlp/plugins/bibles/lib/mediaitem.py	2011-08-13 10:49:53 +0000
+++ openlp/plugins/bibles/lib/mediaitem.py	2011-08-17 10:17:27 +0000
@@ -613,7 +613,7 @@
         if restore:
             old_text = unicode(combo.currentText())
         combo.clear()
-        combo.addItems([unicode(i) for i in range(range_from, range_to + 1)])
+        combo.addItems(map(unicode, range(range_from, range_to + 1)))
         if restore and combo.findText(old_text) != -1:
             combo.setCurrentIndex(combo.findText(old_text))
 
 
=== modified file 'openlp/plugins/songs/lib/openlyricsexport.py'
--- openlp/plugins/songs/lib/openlyricsexport.py	2011-06-19 21:11:29 +0000
+++ openlp/plugins/songs/lib/openlyricsexport.py	2011-08-17 10:17:27 +0000
@@ -35,6 +35,7 @@
 from lxml import etree
 
 from openlp.core.lib import check_directory_exists, Receiver, translate
+from openlp.core.utils import clean_filename
 from openlp.plugins.songs.lib import OpenLyrics
 
 log = logging.getLogger(__name__)
@@ -72,8 +73,7 @@
             tree = etree.ElementTree(etree.fromstring(xml))
             filename = u'%s (%s)' % (song.title,
                 u', '.join([author.display_name for author in song.authors]))
-            filename = re.sub(
-                r'[/\\?*|<>\[\]":<>+%]+', u'_', filename).strip(u'_')
+            filename = clean_filename(filename)
             # Ensure the filename isn't too long for some filesystems
             filename = u'%s.xml' % filename[0:250 - len(self.save_path)]
             # Pass a file object, because lxml does not cope with some special