openlp-core team mailing list archive

[jerryb <gerald.britton@xxxxxxxxx>]

jerryb has proposed merging lp:~gerald-britton/openlp/newbugs into lp:openlp.

Requested reviews:
  Andreas Preikschat (googol-hush)
  Tim Bentley (trb143)
  Jonathan Corwin (j-corwin)

For more details, see:
https://code.launchpad.net/~gerald-britton/openlp/newbugs/+merge/62532

This patch adds some robustness to the OpenOffice importer module and the Songs of Fellowship importer, which uses it.  Primarily, it tries to avoid giving tracebacks.  First off, it detects failed OpenOffice connections and reports on them.  Then, it catches openoffice errors during import.  Finally, it checks the result of the call to songimport.finish() and checks the result to give a error message in the import dialog, if unsuccessful.  

With this patch, you should be able to point the SoF importer at completely garbage files and get an error message in the dialog instead of a traceback.  Of course, properly formatted files will go through.

26 May - I've implemented suggestions received since the original proposal.

26 May - Added specific error messages.  Note: It seems to be hard to achieve consensus on error message texts.  Please suggest alternates if you don't like mine.


-- 
https://code.launchpad.net/~gerald-britton/openlp/newbugs/+merge/62532
Your team OpenLP Core is subscribed to branch lp:openlp.

=== modified file 'openlp/plugins/songs/lib/oooimport.py'
--- openlp/plugins/songs/lib/oooimport.py	2011-05-26 17:11:22 +0000
+++ openlp/plugins/songs/lib/oooimport.py	2011-05-26 18:27:26 +0000
@@ -30,6 +30,7 @@
 from PyQt4 import QtCore
 
 from openlp.core.utils import get_uno_command, get_uno_instance
+from openlp.core.lib import translate
 from songimport import SongImport
 
 log = logging.getLogger(__name__)
@@ -41,6 +42,7 @@
     PAGE_BOTH = 6
 else:
     import uno
+    from com.sun.star.connection import NoConnectException
     from com.sun.star.style.BreakType import PAGE_BEFORE, PAGE_AFTER, PAGE_BOTH
 
 class OooImport(SongImport):
@@ -57,7 +59,17 @@
         self.process_started = False
 
     def do_import(self):
-        self.start_ooo()
+        if not isinstance(self.import_source, list):
+            return
+        try:
+            self.start_ooo()
+        except NoConnectException as exc:
+            self.log_error(
+                self.import_source[0],
+                translate('SongsPlugin.SongImport',
+                    'Unable to open OpenOffice.org or LibreOffice'))
+            log.error(exc)
+            return
         self.import_wizard.progressBar.setMaximum(len(self.import_source))
         for filename in self.import_source:
             if self.stop_import_flag:
@@ -68,6 +80,14 @@
                 if self.document:
                     self.process_ooo_document()
                     self.close_ooo_file()
+                else:
+                    self.log_error(self.filepath,
+                                   translate('SongsPlugin.SongImport',
+                                        'Unable to open file'))
+            else:
+                self.log_error(self.filepath,
+                               translate('SongsPlugin.SongImport',
+                                    'File not found'))
         self.close_ooo()
 
     def process_ooo_document(self):
@@ -99,13 +119,16 @@
             while uno_instance is None and loop < 5:
                 try:
                     uno_instance = get_uno_instance(resolver)
-                except:
+                except NoConnectException:
                     log.exception("Failed to resolve uno connection")
                     self.start_ooo_process()
                     loop += 1
-            manager = uno_instance.ServiceManager
-            self.desktop = manager.createInstanceWithContext(
-                "com.sun.star.frame.Desktop", uno_instance)
+                else:
+                    manager = uno_instance.ServiceManager
+                    self.desktop = manager.createInstanceWithContext(
+                        "com.sun.star.frame.Desktop", uno_instance)
+                    return
+                raise
 
     def start_ooo_process(self):
         try:
@@ -145,8 +168,8 @@
             else:
                 self.import_wizard.incrementProgressBar(
                     u'Processing file ' + filepath, 0)
-        except:
-            log.exception("open_ooo_file failed")
+        except AttributeError:
+            log.exception("open_ooo_file failed: %s", url)
         return
 
     def close_ooo_file(self):

=== modified file 'openlp/plugins/songs/lib/sofimport.py'
--- openlp/plugins/songs/lib/sofimport.py	2011-05-26 17:11:22 +0000
+++ openlp/plugins/songs/lib/sofimport.py	2011-05-26 18:27:26 +0000
@@ -31,10 +31,14 @@
 # http://www.oooforum.org/forum/viewtopic.phtml?t=14409
 # http://wiki.services.openoffice.org/wiki/Python
 
+import logging
 import os
 import re
 
 from oooimport import OooImport
+from com.sun.star.uno import RuntimeException
+
+log = logging.getLogger(__name__)
 
 if os.name == u'nt':
     BOLD = 150.0
@@ -86,16 +90,18 @@
         """
         self.blanklines = 0
         self.new_song()
-        paragraphs = self.document.getText().createEnumeration()
-        while paragraphs.hasMoreElements():
-            if self.stop_import_flag:
-                return
-            paragraph = paragraphs.nextElement()
-            if paragraph.supportsService("com.sun.star.text.Paragraph"):
-                self.process_paragraph(paragraph)
-        if self.song:
-            self.finish()
-            self.song = False
+        try:
+            paragraphs = self.document.getText().createEnumeration()
+            while paragraphs.hasMoreElements():
+                if self.stop_import_flag:
+                    return
+                paragraph = paragraphs.nextElement()
+                if paragraph.supportsService("com.sun.star.text.Paragraph"):
+                    self.process_paragraph(paragraph)
+        except RuntimeException as exc:
+            log.exception(u'Error processing file: %s', exc)
+        if not self.finish():
+            self.log_error(self.filepath)
 
     def process_paragraph(self, paragraph):
         """