openlp-core team mailing list archive
-
openlp-core team
-
Mailing list archive
-
Message #08911
[Merge] lp:~gerald-britton/openlp/bugs into lp:openlp
jerryb has proposed merging lp:~gerald-britton/openlp/bugs into lp:openlp.
Requested reviews:
OpenLP Core (openlp-core)
For more details, see:
https://code.launchpad.net/~gerald-britton/openlp/bugs/+merge/61158
** RFC only. Not ready for merging (and no rush to do so) ***
This patch implements an idea I discussed briefly with RS in IRC to make song imports more robust. In this patch, I show those ideas in songshowplusimport.py. I have sprinkled the code with comments and explanations to indicate what is being done and why. Among other things, these comments would be pulled out before the final commit. Also, some debugging code would be pulled out and the code would be cleaned up to meet project standards.
The basic goal is to handle improperly formatted files without causing a traceback. The main approach is to wrap crucial sections in try/except constructs and catch typical errors. I've tested it with bad (even empty) files to verify that the patch does indeed catch these things.
The patch introduces a custom Exception class which I've called OpenLPException for now (open to suggestion for better ideas). Ideally this definition would live near the top of the package hierarchy. The intention is that this class not be used directly but subclassed as needed. I show that in this patch by defining and using the subclass OLPImportError. Probably that subclass should live in the __init__.py file for the lib subdirectory so that it is available for other import modules.
I've also added some O(1) lookup tables that can reduce some processing steps.
If the general concepts are found useful, we can think about applying some things to other importers as well. The general Exception is an idea that might have a wider applicability.
*** RFC ONLY ***
--
https://code.launchpad.net/~gerald-britton/openlp/bugs/+merge/61158
Your team OpenLP Core is requested to review the proposed merge of lp:~gerald-britton/openlp/bugs into lp:openlp.
=== added directory 'documentation'
=== added directory 'documentation/manual'
=== added directory 'documentation/manual/source'
=== added file 'documentation/manual/source/index.rst.OTHER'
--- documentation/manual/source/index.rst.OTHER 1970-01-01 00:00:00 +0000
+++ documentation/manual/source/index.rst.OTHER 2011-05-16 19:03:38 +0000
@@ -0,0 +1,41 @@
+.. OpenLP documentation master file, created by
+ sphinx-quickstart on Thu Sep 30 21:24:54 2010.
+ You can adapt this file completely to your liking, but it should at least
+ contain the root `toctree` directive.
+
+Welcome to the OpenLP 2.0 User Manual
+=====================================
+
+Getting Started With OpenLP
+---------------------------
+
+.. toctree::
+ :maxdepth: 2
+
+ introduction
+ installation
+
+Reference Manual
+----------------
+
+.. toctree::
+ :maxdepth: 2
+
+ alert
+ bibles
+ configure
+ dualmonitors
+ wizard
+ glossary
+ mediamanager
+ songs
+ themes
+
+Questions and Troubleshooting
+-----------------------------
+
+.. toctree::
+ :maxdepth: 2
+
+ faq
+ troubleshooting
=== added file 'documentation/manual/source/installation.rst'
--- documentation/manual/source/installation.rst 1970-01-01 00:00:00 +0000
+++ documentation/manual/source/installation.rst 2011-05-16 19:03:38 +0000
@@ -0,0 +1,66 @@
+Installing OpenLP
+=================
+
+Microsoft Windows
+-----------------
+
+Installing OpenLP is identical over all supported versions of Windows. After
+downloading the file from the `download page <http://www.openlp.org/en/download.html>`_
+open the file by double clicking when the download is complete. After opening
+the downloaded file agree to open the unverified file if you are prompted.
+
+Next select your language and click :guilabel:`OK`
+
+.. image:: pics/selectlanguage.png
+
+After reading the welcome message click :guilabel:`Next` to continue the
+installation.
+
+.. image:: pics/welcome.png
+
+Agree to the license agreement. Click :guilabel:`Next` to continue.
+
+.. image:: pics/license.png
+
+Select the install location for OpenLP. Choosing the default location is
+generally the best choice. Click :guilabel:`Next` to continue.
+
+.. image:: pics/installlocation.png
+
+Select a start menu folder for OpenLP to be visible in. The default location
+here is generally the best choice. Click :guilabel:`Next` to continue.
+
+.. image:: pics/startmenufolder.png
+
+Select if you want to create a desktop or Quick Launch icon. Click :guilabel:`Next`
+to continue.
+
+.. image:: pics/additionaltask.png
+
+Review your previous choices. If you need to make any changes click the
+:guilabel:`Back button` to get to the previous choices, or click :guilabel:`Install`
+
+.. image:: pics/readytoinstall.png
+
+The progress bar will update you on how far along the installation has gone.
+
+.. image:: pics/progress.png
+
+When the install is complete you will have the option to launch OpenLP. After
+making your selection click :guilabel:`Finish`
+
+Mac OS X
+--------
+
+Installing OpenLP on OS X is very simple. After downloading the correct file
+from the OpenLP `download page <http://www.openlp.org/en/download.html>`_
+double click on the .dmg file. Drag the OpenLP icon over to the Applications
+folder and you will be ready to use OpenLP. OpenLP will be available in your
+Applications folder.
+
+.. image:: pics/osxinstall.png
+
+Linux
+-----
+
+Installation process on Linux distributions vary by distribution. See the OpenLP `download page <http://www.openlp.org/en/download.html>`_ for distribution specific instructions.
=== added directory 'documentation/manual/source/pics'
=== added file 'documentation/manual/source/pics/additionaltask.png'
Binary files documentation/manual/source/pics/additionaltask.png 1970-01-01 00:00:00 +0000 and documentation/manual/source/pics/additionaltask.png 2011-05-16 19:03:38 +0000 differ
=== added file 'documentation/manual/source/pics/installcomplete.png'
Binary files documentation/manual/source/pics/installcomplete.png 1970-01-01 00:00:00 +0000 and documentation/manual/source/pics/installcomplete.png 2011-05-16 19:03:38 +0000 differ
=== added file 'documentation/manual/source/pics/installlocation.png'
Binary files documentation/manual/source/pics/installlocation.png 1970-01-01 00:00:00 +0000 and documentation/manual/source/pics/installlocation.png 2011-05-16 19:03:38 +0000 differ
=== added file 'documentation/manual/source/pics/license.png'
Binary files documentation/manual/source/pics/license.png 1970-01-01 00:00:00 +0000 and documentation/manual/source/pics/license.png 2011-05-16 19:03:38 +0000 differ
=== added file 'documentation/manual/source/pics/osxinstall.png'
Binary files documentation/manual/source/pics/osxinstall.png 1970-01-01 00:00:00 +0000 and documentation/manual/source/pics/osxinstall.png 2011-05-16 19:03:38 +0000 differ
=== added file 'documentation/manual/source/pics/progress.png'
Binary files documentation/manual/source/pics/progress.png 1970-01-01 00:00:00 +0000 and documentation/manual/source/pics/progress.png 2011-05-16 19:03:38 +0000 differ
=== added file 'documentation/manual/source/pics/readytoinstall.png'
Binary files documentation/manual/source/pics/readytoinstall.png 1970-01-01 00:00:00 +0000 and documentation/manual/source/pics/readytoinstall.png 2011-05-16 19:03:38 +0000 differ
=== added file 'documentation/manual/source/pics/selectlanguage.png'
Binary files documentation/manual/source/pics/selectlanguage.png 1970-01-01 00:00:00 +0000 and documentation/manual/source/pics/selectlanguage.png 2011-05-16 19:03:38 +0000 differ
=== added file 'documentation/manual/source/pics/startmenufolder.png'
Binary files documentation/manual/source/pics/startmenufolder.png 1970-01-01 00:00:00 +0000 and documentation/manual/source/pics/startmenufolder.png 2011-05-16 19:03:38 +0000 differ
=== added file 'documentation/manual/source/pics/welcome.png'
Binary files documentation/manual/source/pics/welcome.png 1970-01-01 00:00:00 +0000 and documentation/manual/source/pics/welcome.png 2011-05-16 19:03:38 +0000 differ
=== modified file 'openlp/core/ui/generaltab.py'
--- openlp/core/ui/generaltab.py 2011-05-15 10:38:11 +0000
+++ openlp/core/ui/generaltab.py 2011-05-16 19:03:38 +0000
@@ -342,6 +342,7 @@
"""
Receiver.send_message(u'slidecontroller_live_spin_delay',
self.timeoutSpinBox.value())
+<<<<<<< TREE
# Do not continue on start up.
if not postUpdate:
return
@@ -352,6 +353,14 @@
self.customYValueEdit.value(),
self.customWidthValueEdit.value(),
self.customHeightValueEdit.value())
+=======
+ # Reset screens after initial definition
+ self.screens.override[u'size'] = QtCore.QRect(
+ self.customXValueEdit.value(),
+ self.customYValueEdit.value(),
+ self.customWidthValueEdit.value(),
+ self.customHeightValueEdit.value())
+>>>>>>> MERGE-SOURCE
if self.overrideCheckBox.isChecked():
self.screens.set_override_display()
else:
@@ -377,4 +386,8 @@
"""
Called when the width, height, x position or y position has changed.
"""
+<<<<<<< TREE
self.display_changed = True
+=======
+ self.overrideChanged = True
+>>>>>>> MERGE-SOURCE
=== modified file 'openlp/core/ui/servicemanager.py'
--- openlp/core/ui/servicemanager.py 2011-05-13 13:58:17 +0000
+++ openlp/core/ui/servicemanager.py 2011-05-16 19:03:38 +0000
@@ -678,6 +678,23 @@
if serviceItem[u'service_item'].is_text():
self.themeMenu.menuAction().setVisible(True)
action = self.menu.exec_(self.serviceManagerList.mapToGlobal(point))
+<<<<<<< TREE
+=======
+ if action == self.editAction:
+ self.remoteEdit()
+ elif action == self.maintainAction:
+ self.onServiceItemEditForm()
+ elif action == self.deleteAction:
+ self.onDeleteFromService()
+ elif action == self.notesAction:
+ self.onServiceItemNoteForm()
+ elif action == self.timeAction:
+ self.onStartTimeForm()
+ elif action == self.previewAction:
+ self.makePreview()
+ elif action == self.liveAction:
+ self.makeLive()
+>>>>>>> MERGE-SOURCE
def onServiceItemNoteForm(self):
item = self.findServiceItem()[0]
=== modified file 'openlp/plugins/remotes/html/openlp.js'
=== modified file 'openlp/plugins/songs/forms/editverseform.py'
--- openlp/plugins/songs/forms/editverseform.py 2011-03-24 19:04:02 +0000
+++ openlp/plugins/songs/forms/editverseform.py 2011-05-16 19:03:38 +0000
@@ -172,6 +172,9 @@
return text
def accept(self):
+ QtCore.pyqtRemoveInputHook()
+ import pdb
+ pdb.set_trace()
if self.hasSingleVerse:
value = unicode(self.getVerse()[0])
else:
=== modified file 'openlp/plugins/songs/lib/mediaitem.py'
=== modified file 'openlp/plugins/songs/lib/songshowplusimport.py'
--- openlp/plugins/songs/lib/songshowplusimport.py 2011-05-10 14:08:11 +0000
+++ openlp/plugins/songs/lib/songshowplusimport.py 2011-05-16 19:03:38 +0000
@@ -30,11 +30,13 @@
import os
import logging
import struct
+from functools import partial
from openlp.core.ui.wizard import WizardStrings
from openlp.plugins.songs.lib import VerseType
from openlp.plugins.songs.lib.songimport import SongImport
+# Supported block keys
TITLE = 1
AUTHOR = 2
COPYRIGHT = 3
@@ -44,12 +46,116 @@
TOPIC = 29
COMMENTS = 30
VERSE_ORDER = 31
+BRIDGE = 24
SONG_BOOK = 35
SONG_NUMBER = 36
CUSTOM_VERSE = 37
+<<<<<<< TREE
BRIDGE = 24
+=======
+#
+# Set of all supported block keys
+# Use this set for O(1) lookups of keys we understand
+BLOCKKEYS = set((
+ TITLE,
+ AUTHOR,
+ COPYRIGHT,
+ CCLI_NO,
+ VERSE,
+ CHORUS,
+ TOPIC,
+ COMMENTS,
+ VERSE_ORDER,
+ BRIDGE,
+ SONG_BOOK,
+ SONG_NUMBER,
+ CUSTOM_VERSE,
+ ))
+#
+# Logger
+>>>>>>> MERGE-SOURCE
log = logging.getLogger(__name__)
+#
+# Helper function for encoding as unicode
+# This abstracts the encoding function and curries the encoding type
+WINDOWS_1252 = u'cp1252'
+encode = partial(unicode, encoding=WINDOWS_1252)
+
+
+def start_trace():
+ """Helper function to enable pdb tracing"""
+ from PyQt4 import QtCore, QtGui #, QtOpenGL
+ QtCore.pyqtRemoveInputHook()
+ import pdb
+ pdb.set_trace()
+
+
+'''
+Define a general exception. This should probably go into a top-level file
+in the package hierarchy that is then available to all modules. This
+general exception should not be used directly but rather subclassed as
+needed. This exception definition includes a call to logging.error for
+convenience. Whether to include the logging call here or not is a
+higher-pay-grade decision.
+'''
+
+class OpenLPException(Exception):
+ """My Exception class"""
+
+ def __init__(self, *args, **kwargs):
+ Exception.__init__(self, *args)
+ self.kwargs = kwargs
+ self.exc = getattr(self, u'exc', u'OpenLPException')
+ log.error(self, extra=kwargs)
+
+ def __str__(self):
+ values = ((self.exc,) + self.args)
+ if len(self.args) < 1:
+ return u'[%s]' % values
+ elif len(self.args) < 2:
+ return u'[%s %s]' % values
+ else: # len(self.args) >= 2:
+ return (u'[%s %s] %s:' +
+ (u' %s,' * (len(self.args) - 2))[:-1]) % values
+
+'''
+Subclass the general exception for use here. Again, this definition should
+probably go into a higher-level module or perhaps the __init__.py for the
+lib sub-directory.
+'''
+
+class OLPImportError(OpenLPException):
+ """ OLPImportError exception class"""
+
+ def __init__(self, *args, **kwargs):
+ self.exc = u'OLPImportError'
+ OpenLPException.__init__(self, *args, **kwargs)
+
+'''
+This function will be used to perform the two operations -- read data from
+the source file and unpack them as specified -- used in the main processing
+loop. The two calls are wrapped in try/except to catch the typical errors.
+Upon catching such an error, an OPLImportError exception is raised.
+'''
+
+def read_unpack(handle, fmt, size):
+ """Helper function combining file.read and struct.unpack"""
+ try:
+ return struct.unpack(fmt, handle.read(size))
+ except IOError as exc:
+ # File I/O error
+ log.error(u'Error reading "%s": %s', handle.name, exc)
+ raise OLPImportError( 1, u'Error reading file', handle.name)
+ # -------------- ----- ---------------------- -----------
+ # Our Exception errno message extra info
+ #
+ except struct.error as exc:
+ # unpack error
+ log.error(u'Error unpacking data from "%s" at position %d: %s',
+ handle.name, handle.tell(), exc)
+ raise OLPImportError(2, u'Incorrect file format', handle.name)
+
class SongShowPlusImport(SongImport):
"""
@@ -98,17 +204,29 @@
"""
Receive a single file or a list of files to import.
"""
- if not isinstance(self.import_source, list):
+<<<<<<< TREE
+ if not isinstance(self.import_source, list):
+=======
+ # Start up the trace
+ #start_trace()
+ if not isinstance(self.import_source, list):
+>>>>>>> MERGE-SOURCE
return
self.import_wizard.progressBar.setMaximum(len(self.import_source))
+<<<<<<< TREE
for file in self.import_source:
+=======
+ # Process each file in list
+ for file_ in self.import_source:
+>>>>>>> MERGE-SOURCE
self.sspVerseOrderList = []
otherCount = 0
otherList = {}
- file_name = os.path.split(file)[1]
+ file_name = os.path.split(file_)[1]
self.import_wizard.incrementProgressBar(
WizardStrings.ImportingType % file_name, 0)
+<<<<<<< TREE
songData = open(file, 'rb')
while True:
@@ -177,10 +295,167 @@
log.debug("Unrecognised blockKey: %s, data: %s"
% (blockKey, data))
songData.seek(nextBlockStarts)
+=======
+
+ '''
+ Each file is processed, but wrapped in try/except to catch
+ errors for reporting and to prevent tracebacks.
+ '''
+
+ # Wrap file processing in try/except to enable meaningful error
+ # messages.
+ try:
+ songData = open(file_, 'rb')
+ self.__process_file(songData)
+ except IOError as exc:
+ # Open failed
+ log.error(exc)
+ self.log_error(file_, reason=exc[1])
+ except OLPImportError as exc:
+ # An error occured during processing
+
+ '''
+ The next line is a call to log the error, but is commented
+ for now since the logging occurs in the exception class.
+ If that is deemed sub-optimal, the following call to
+ log.error should be uncommented.
+ '''
+
+ #log.error(exc) #Logged in the exception constructor
+ self.log_error(file_, reason=exc[1])
+ else:
+ songData.close()
+>>>>>>> MERGE-SOURCE
self.verse_order_list = self.sspVerseOrderList
- songData.close()
if not self.finish():
- self.log_error(file)
+ self.log_error(file_)
+
+ '''
+ Detail processing for each file has been moved to a (pseudo-) private
+ function.
+ '''
+
+ def __process_file(self, songData):
+ """ Process the song data file"""
+
+ '''
+ We want to use the read_unpack function defined above, but first
+ we'll define a partial function to curry the file handle. This
+ also allows for visually-streamlined code.
+ '''
+
+ # Set up a partial function to read and unpack data using read_unpack
+ get_data = partial(read_unpack, songData)
+ while True:
+ blockKey, = get_data("I", 4)
+ # The file ends with 4 NUL's
+ if blockKey == 0:
+ break
+ nextBlockStarts, = get_data("I", 4)
+ nextBlockStarts += songData.tell()
+ # If we don't know how to handle this key, skip it
+
+ '''
+ Here, we use the set of block keys that we recognize to do an
+ O(1) check to see if we can process this block.
+ '''
+
+ if blockKey not in BLOCKKEYS:
+ log.debug("Unrecognised blockKey: %d, offset: %d, data: %s",
+ blockKey, songData.tell() - 8, data)
+ '''
+ Since we don't recognize this key, we'll skip to the next block.
+ '''
+
+ songData.seek(nextBlockStarts)
+ continue
+ if blockKey in (VERSE, CHORUS, BRIDGE):
+ null, verseNo, = get_data("BB", 2)
+ elif blockKey == CUSTOM_VERSE:
+ null, verseNameLength, = get_data("BB", 2)
+ verseName = songData.read(verseNameLength)
+ lengthDescriptorSize, = get_data("B", 1)
+ # Detect if/how long the length descriptor is
+ if lengthDescriptorSize == 12:
+ lengthDescriptor, = get_data("I", 4)
+ elif lengthDescriptorSize == 2:
+ lengthDescriptor = 1
+ elif lengthDescriptorSize == 9:
+ lengthDescriptor = 0
+ else:
+ lengthDescriptor, = get_data("B", 1)
+ # Read the data block according to the computed length
+ data = songData.read(lengthDescriptor)
+ if blockKey == TITLE:
+ self.title = encode(data)
+ elif blockKey == AUTHOR:
+ authors = data.split(" / ")
+ for author in authors:
+ if author.find(",") != -1:
+ authorParts = author.split(", ")
+ author = authorParts[1] + " " + authorParts[0]
+ self.parse_author(encode(author))
+ elif blockKey == COPYRIGHT:
+ self.add_copyright(encode(data))
+ elif blockKey == CCLI_NO:
+ self.ccli_number = int(data)
+ elif blockKey == VERSE:
+ self.add_verse(encode(data), "V%s" % verseNo)
+ elif blockKey == CHORUS:
+ self.add_verse(encode(data), "C%s" % verseNo)
+ elif blockKey == BRIDGE:
+ self.add_verse(encode(data), "B%s" % verseNo)
+ elif blockKey == TOPIC:
+ self.topics.append(encode(data))
+ elif blockKey == COMMENTS:
+ self.comments = encode(data)
+ elif blockKey == VERSE_ORDER:
+ verseTag = self.toOpenLPVerseTag(data, True)
+ if verseTag:
+ self.sspVerseOrderList.append(encode(verseTag))
+ elif blockKey == SONG_BOOK:
+ self.song_book_name = encode(data)
+ elif blockKey == SONG_NUMBER:
+ self.song_number = ord(data)
+ elif blockKey == CUSTOM_VERSE:
+ verseTag = self.toOpenLPVerseTag(verseName)
+ self.add_verse(encode(data), verseTag)
+ else:
+ # We should never reach this code since we are only
+ # considering keys in the set of valid block keys
+ raise ValueError("Shouldn't get here, blockKey: %s" % blockKey)
+ songData.seek(nextBlockStarts)
+ # Optionally, reset the file pointer here, in case we don't trust
+ # the block structure
+ songData.seek(nextBlockStarts)
+
+ '''
+ At this point, we have three different calls to
+ songData.seek(nextBlockStarts). It might be simpler to put the
+ seek() at the top of the loop and initialize nextBlockStarts to 0.
+ '''
+
+ return
+
+ '''
+ Here, we define a dictionary to use in the function that follows.
+ The dictionary allows O(1) lookups and is built when the class is
+ defined, thus reducing calls to do the lookups every time as in the
+ original code.
+
+ Note: The definition is made outside the function to force its
+ evaluation at class definition time. If it is moved inside the
+ function definition, then the dictionary definition will occur
+ every time the function is called, negating the advantage.
+ '''
+
+ VERSE_MAP = {
+ u'verse': VerseType.Tags[VerseType.Verse],
+ u'chorus': VerseType.Tags[VerseType.Chorus],
+ u'bridge': VerseType.Tags[VerseType.Bridge],
+ u'pre-chorus': VerseType.Tags[VerseType.PreChorus],
+ }
+
def toOpenLPVerseTag(self, verseName, ignoreUnique=False):
if verseName.find(" ") != -1:
@@ -191,6 +466,11 @@
verseType = verseName
verseNumber = "1"
verseType = verseType.lower()
+
+ '''
+ Original code, using if/elif... to check the verse type
+
+
if verseType == "verse":
verseTag = VerseType.Tags[VerseType.Verse]
elif verseType == "chorus":
@@ -199,8 +479,15 @@
verseTag = VerseType.Tags[VerseType.Bridge]
elif verseType == "pre-chorus":
verseTag = VerseType.Tags[VerseType.PreChorus]
+
+
+ Simplified code, using the dictionary built above
+ '''
+
+ if verseType in self.VERSE_MAP:
+ verseTag = self.VERSE_MAP[verseType]
else:
- if not self.otherList.has_key(verseName):
+ if verseName not in self.otherList:
if ignoreUnique:
return None
self.otherCount = self.otherCount + 1
=== modified file 'openlp/plugins/songs/songsplugin.py'
Follow ups
