openlp-core team mailing list archive

[jerryb <gerald.britton@xxxxxxxxx>]

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

Requested reviews:
  Jonathan Corwin (j-corwin)
  Tim Bentley (trb143)
  Raoul Snyman (raoul-snyman)
  jerryb (gerald-britton)
  Andreas Preikschat (googol-hush)

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

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.

28 May - Uglified code to conform to weird standards :)

30 May - only import uno exceptions if not on Windows

Note: this also sets the specific uno exception names equal to Exception when running Windows.  Otherwise, the references to these exceptions will fail.  Better would be to find the equivalent exceptions under Windows and use those.  However, I do not have access to a Windows system to test this.  If this patch works out, perhaps a Windows-based developer can find the correct exception names

31 May - corrected logic raising an exception if attempts to connect with uno fail.

-- 
https://code.launchpad.net/~gerald-britton/openlp/newbugs/+merge/63035
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-31 20:18:22 +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__)
@@ -39,8 +40,10 @@
     PAGE_BEFORE = 4
     PAGE_AFTER = 5
     PAGE_BOTH = 6
+    NoConnectException = Exception
 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 +60,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 +81,13 @@
                 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-31 20:18:22 +0000
@@ -31,21 +31,27 @@
 # 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
 
+
+log = logging.getLogger(__name__)
+
 if os.name == u'nt':
     BOLD = 150.0
     ITALIC = 2
     from oooimport import PAGE_BEFORE, PAGE_AFTER, PAGE_BOTH
+    RuntimeException = Exception
 else:
     try:
         from com.sun.star.awt.FontWeight import BOLD
         from com.sun.star.awt.FontSlant import ITALIC
         from com.sun.star.style.BreakType import PAGE_BEFORE, PAGE_AFTER, \
             PAGE_BOTH
+        from com.sun.star.uno import RuntimeException
     except ImportError:
         pass
 
@@ -86,16 +92,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):
         """