Merge lp:~meths/openlp/trivialfixes into lp:openlp

[Jon Tibble]

[Jon Tibble (meths)]

Fixes logging issue found testing Impress presentations.

Problem: sys.exc_info() is an array and passing it as a whole doesn't work.

Solution chosen is to eliminate the log.error() sys.exc_info() combination completely in favour of log.exception()

[Tim Bentley (trb143)]

Note sure about the move to log.exceptional.
The sys.exec provides the only clue as to what has gone wrong and we could be looseing information.
Reading the documentation exceptional should only be used "Call this method only from an exception handler"

review: Needs Information

[Jon Tibble (meths)]

Okay I've fudged a comparison by forcing the plugin to try and load powerpoint and powerpoint viewer when neither are installed. Top section is the information given in a log exception, bottom line is information from the log error:

2009-09-18 12:43:14,010 PresentationPlugin ERROR Failed to setup plugin for Powerpoint
Traceback (most recent call last):
  File "D:\Projects\OpenLP\openlp\trunk\openlp\plugins\presentations\presentationplugin.py", line 97, in check_pre_conditions
    powerpoint = PowerpointController()
NameError: global name 'PowerpointController' is not defined

2009-09-18 12:43:14,010 PresentationPlugin ERROR Reason : (<type 'exceptions.NameError'>, NameError("global name 'PowerpointController' is not defined",), <traceback object at 0x0387A670>)

review: Needs Resubmitting

[Jon Tibble (meths)]

Initial information:

Fixes logging issue found testing Impress presentations.
Problem: sys.exc_info() is an array and passing it as a whole doesn't work.
Solution chosen is to eliminate the log.error() sys.exc_info() combination completely in favour of log.exception()

Addressing TRB's issues:

1- Less information provided:

Okay I've fudged a comparison by forcing the plugin to try and load powerpoint and powerpoint viewer when neither are installed. Top section is the information given in a log exception, bottom line is information from the log error:

2009-09-18 12:43:14,010 PresentationPlugin ERROR Failed to setup plugin for Powerpoint
Traceback (most recent call last):
  File "D:\Projects\OpenLP\openlp\trunk\openlp\plugins\presentations\presentationplugin.py", line 97, in check_pre_conditions
    powerpoint = PowerpointController()
NameError: global name 'PowerpointController' is not defined

2009-09-18 12:43:14,010 PresentationPlugin ERROR Reason : (<type 'exceptions.NameError'>, NameError("global name 'PowerpointController' is not defined",), <traceback object at 0x0387A670>)

2 - Should only be called from an exception
The diff should confirm the only ones I've changed occur in except: branches when we have a problem.

review: Needs Resubmitting

[Raoul Snyman (raoul-snyman)]

Looking at this, we should *definitely* be using log.exception() - when OpenLP is running as a GUI app (instead of directly from the command line) we won't see any exceptions.

review: Approve

[Jon Tibble (meths)]

review: Approve

[Tim Bentley (trb143)]

review: Approve

[lp:~meths/openlp/trivialfixes]

549. By Tim Bentley on 2009-09-18

Correction from == to is

550. By Jon Tibble on 2009-09-18

Merge log.exception() conversion

=== modified file 'openlp/core/lib/rendermanager.py'
--- openlp/core/lib/rendermanager.py	2009-09-08 19:58:05 +0000
+++ openlp/core/lib/rendermanager.py	2009-09-17 17:05:42 +0000
@@ -234,7 +234,7 @@
         realh = preview.height()
         # and move it to the centre of the preview space
         newImage = QtGui.QImage(w, h, QtGui.QImage.Format_ARGB32_Premultiplied)
-        newImage.fill(QtCore.Qt.transparent)
+        newImage.fill(QtCore.Qt.black)
         painter = QtGui.QPainter(newImage)
         painter.drawImage((w-realw) / 2, (h-realh) / 2, preview)
         return newImage

=== modified file 'openlp/core/ui/servicemanager.py'
--- openlp/core/ui/servicemanager.py	2009-09-12 17:24:16 +0000
+++ openlp/core/ui/servicemanager.py	2009-09-17 23:10:25 +0000
@@ -437,7 +437,7 @@
                     #if not present do not worry
                     pass
             except:
-                log.error(u'Problem processing oos load %s', sys.exc_info()[0])
+                log.exception(u'Problem loading a service file')
                 pass
         self.serviceName = name[len(name) - 1]
         self.parent.OosChanged(True, self.serviceName)

=== modified file 'openlp/plugins/bibles/lib/manager.py'
--- openlp/plugins/bibles/lib/manager.py	2009-09-08 19:58:05 +0000
+++ openlp/plugins/bibles/lib/manager.py	2009-09-17 23:10:25 +0000
@@ -192,7 +192,8 @@
                 nbible.save_meta(u'proxypass', proxypass)
             return True
         else:
-            log.debug(u'register_http_file_bible %s not created already exists', biblename)
+            log.debug(u'register_http_file_bible %s not created already exists',
+                biblename)
             return False
 
     def register_csv_file_bible(self, biblename, booksfile, versefile):
@@ -201,34 +202,46 @@
         If the database exists it is deleted and the database is reloaded
         from scratch.
         """
-        log.debug(u'register_CSV_file_bible %s,%s,%s', biblename, booksfile, versefile)
+        log.debug(u'register_CSV_file_bible %s,%s,%s',
+            biblename, booksfile, versefile)
         if self._is_new_bible(biblename):
-            nbible = BibleDBImpl(self.biblePath, biblename, self.config) # Create new Bible
-            nbible.create_tables() # Create Database
-            self.bible_db_cache[biblename] = nbible # cache the database for use later
-            bcsv = BibleCSVImpl(nbible) # create the loader and pass in the database
+            # Create new Bible
+            nbible = BibleDBImpl(self.biblePath, biblename, self.config)
+            # Create database
+            nbible.create_tables()
+            # Cache the database for use later
+            self.bible_db_cache[biblename] = nbible
+            # Create the loader and pass in the database
+            bcsv = BibleCSVImpl(nbible)
             bcsv.load_data(booksfile, versefile, self.dialogobject)
             return True
         else:
-            log.debug(u'register_csv_file_bible %s not created already exists', biblename)
+            log.debug(u'register_csv_file_bible %s not created already exists',
+                biblename)
             return False
 
     def register_osis_file_bible(self, biblename, osisfile):
         """
-        Method to load a bible from a osis xml file extracted from Sword bible viewer.
-        If the database exists it is deleted and the database is reloaded
-        from scratch.
+        Method to load a bible from a osis xml file extracted from Sword bible
+        viewer.  If the database exists it is deleted and the database is
+        reloaded from scratch.
         """
         log.debug(u'register_OSIS_file_bible %s , %s', biblename, osisfile)
         if self._is_new_bible(biblename):
-            nbible = BibleDBImpl(self.biblePath, biblename, self.config) # Create new Bible
-            nbible.create_tables() # Create Database
-            self.bible_db_cache[biblename] = nbible # cache the database for use later
-            bcsv = BibleOSISImpl(self.biblePath, nbible) # create the loader and pass in the database
+            # Create new Bible
+            nbible = BibleDBImpl(self.biblePath, biblename, self.config)
+            # Create Database
+            nbible.create_tables()
+            # Cache the database for use later
+            self.bible_db_cache[biblename] = nbible
+            # Create the loader and pass in the database
+            bcsv = BibleOSISImpl(self.biblePath, nbible)
             bcsv.load_data(osisfile, self.dialogobject)
             return True
         else:
-            log.debug(u'register_OSIS_file_bible %s , %s not created already exists', biblename, osisfile)
+            log.debug(
+                u'register_OSIS_file_bible %s , %s not created already exists',
+                biblename, osisfile)
             return False
 
     def get_bibles(self, mode=BibleMode.Full):
@@ -271,7 +284,8 @@
         book and chapterMaxBibleBookVerses
         """
         log.debug(u'get_book_verse_count %s,%s,%s', bible, book,  chapter)
-        return self.bible_db_cache[bible].get_max_bible_book_verses(book, chapter)
+        return self.bible_db_cache[bible].get_max_bible_book_verses(
+            book, chapter)
 
     def get_verse_from_text(self, bible, versetext):
         """
@@ -285,7 +299,8 @@
         """
         Saves the bibles meta data
         """
-        log.debug(u'save_meta data %s,%s, %s,%s', bible,  version, copyright, permissions)
+        log.debug(u'save_meta data %s,%s, %s,%s',
+            bible,  version, copyright, permissions)
         self.bible_db_cache[bible].save_meta(u'Version', version)
         self.bible_db_cache[bible].save_meta(u'Copyright', copyright)
         self.bible_db_cache[bible].save_meta(u'Permissions', permissions)
@@ -297,7 +312,8 @@
         log.debug(u'get_meta %s,%s', bible,  key)
         return self.bible_db_cache[bible].get_meta(key)
 
-    def get_verse_text(self, bible, bookname, schapter, echapter, sverse, everse=0):
+    def get_verse_text(self, bible, bookname, schapter, echapter, sverse,
+        everse=0):
         """
         Returns a list of verses for a given Book, Chapter and ranges of verses.
         If the end verse(everse) is less then the start verse(sverse)
@@ -311,8 +327,10 @@
         text  = []
         self.media.setQuickMsg1(u'')
         self.media.setQuickMsg2(u'')
-        log.debug(u'get_verse_text %s,%s,%s,%s,%s,%s',  bible, bookname,  schapter, echapter, sverse, everse)
-        # check to see if book/chapter exists fow HTTP bibles and load cache if necessary
+        log.debug(u'get_verse_text %s,%s,%s,%s,%s,%s',
+            bible, bookname, schapter, echapter, sverse, everse)
+        # check to see if book/chapter exists fow HTTP bibles and load cache
+        # if necessary
         if self.bible_http_cache[bible] is not None:
             book= self.bible_db_cache[bible].get_bible_book(bookname)
             if book == None:
@@ -320,49 +338,61 @@
                 log.debug(u'get_verse_text : new book')
                 for chapter in range(schapter, echapter + 1):
                     self.media.setQuickMsg2(u'%s: %s'% (bookname, chapter))
-                    search_results = self.bible_http_cache [bible].get_bible_chapter(bible, 0, bookname, chapter)
+                    search_results = \
+                        self.bible_http_cache[bible].get_bible_chapter(
+                            bible, 0, bookname, chapter)
                     if search_results.has_verselist() :
-                        ## We have found a book of the bible lets check to see if it was there.
-                        ## By reusing the returned book name we get a correct book.
-                        ## For example it is possible to request ac and get Acts back.
+                        ## We have found a book of the bible lets check to see
+                        ## if it was there.  By reusing the returned book name
+                        ## we get a correct book.  For example it is possible
+                        ## to request ac and get Acts back.
                         bookname = search_results.get_book()
                         # check to see if book/chapter exists
-                        book= self.bible_db_cache[bible].get_bible_book(bookname)
+                        book = self.bible_db_cache[bible].get_bible_book(
+                            bookname)
                         if book == None:
                             ## Then create book, chapter and text
-                            book = self.bible_db_cache[bible].create_book(bookname, \
-                                                                          self.book_abbreviations[bookname],  \
-                                                                          self.book_testaments[bookname])
-                            log.debug(u'New http book %s , %s, %s',  book, book.id, book.name)
-                            self.bible_db_cache[bible].create_chapter(book.id, \
-                                                                      search_results.get_chapter(),\
-                                                                      search_results.get_verselist())
+                            book = self.bible_db_cache[bible].create_book(
+                                bookname, self.book_abbreviations[bookname],
+                                self.book_testaments[bookname])
+                            log.debug(u'New http book %s , %s, %s',
+                                book, book.id, book.name)
+                            self.bible_db_cache[bible].create_chapter(
+                                book.id, search_results.get_chapter(),
+                                search_results.get_verselist())
                         else:
                             ## Book exists check chapter and texts only.
-                            v = self.bible_db_cache[bible].get_bible_chapter(book.id, chapter)
+                            v = self.bible_db_cache[bible].get_bible_chapter(
+                                book.id, chapter)
                             if v == None:
-                                self.media.setQuickMsg2(u'%s: %s'%(bookname, chapter))
-                                self.bible_db_cache[bible].create_chapter(book.id, \
-                                                                          chapter, \
-                                                                          search_results.get_verselist())
+                                self.media.setQuickMsg2(u'%s: %s'% (
+                                    bookname, chapter))
+                                self.bible_db_cache[bible].create_chapter(
+                                    book.id, chapter,
+                                    search_results.get_verselist())
             else:
                 log.debug(u'get_verse_text : old book')
                 for chapter in range(schapter, echapter + 1):
-                    v = self.bible_db_cache[bible].get_bible_chapter(book.id, chapter)
+                    v = self.bible_db_cache[bible].get_bible_chapter(
+                        book.id, chapter)
                     if v == None:
                         try:
                             self.media.setQuickMsg1(u'Downloading')
-                            self.media.setQuickMsg2(u'%s: %s'% (bookname, chapter))
-                            search_results = self.bible_http_cache [bible].get_bible_chapter(bible, book.id, bookname, chapter)
+                            self.media.setQuickMsg2(u'%s: %s'% \
+                                (bookname, chapter))
+                            search_results = \
+                                self.bible_http_cache[bible].get_bible_chapter(
+                                    bible, book.id, bookname, chapter)
                             if search_results.has_verselist():
-                                self.bible_db_cache[bible].create_chapter(book.id, \
-                                                                          search_results.get_chapter(),\
-                                                                          search_results.get_verselist())
-                        except :
-                            log.error(u'Errow thrown %s', sys.exc_info()[1])
+                                self.bible_db_cache[bible].create_chapter(
+                                    book.id, search_results.get_chapter(),
+                                    search_results.get_verselist())
+                        except:
+                            log.exception(u'Problem getting scripture online')
         #Now get verses from database
         if schapter == echapter:
-            text = self.bible_db_cache[bible].get_bible_text(bookname, schapter, sverse, everse)
+            text = self.bible_db_cache[bible].get_bible_text(bookname,
+                schapter, sverse, everse)
         else:
             for i in range (schapter, echapter + 1):
                 if i == schapter:
@@ -375,7 +405,8 @@
                     start = 1
                     end = self.get_book_verse_count(bible, bookname, i)
 
-                txt = self.bible_db_cache[bible].get_bible_text(bookname, i, start, end)
+                txt = self.bible_db_cache[bible].get_bible_text(
+                    bookname, i, start, end)
                 text.extend(txt)
         return text
 

=== modified file 'openlp/plugins/presentations/lib/impresscontroller.py'
--- openlp/plugins/presentations/lib/impresscontroller.py	2009-09-11 19:29:57 +0000
+++ openlp/plugins/presentations/lib/impresscontroller.py	2009-09-17 23:10:25 +0000
@@ -23,8 +23,8 @@
 # http://www.oooforum.org/forum/viewtopic.phtml?t=5252
 # http://wiki.services.openoffice.org/wiki/Documentation/DevGuide/Working_with_Presentations
 # http://mail.python.org/pipermail/python-win32/2008-January/006676.html
-#http://www.linuxjournal.com/content/starting-stopping-and-connecting-openoffice-python
-#http://nxsy.org/comparing-documents-with-openoffice-and-python
+# http://www.linuxjournal.com/content/starting-stopping-and-connecting-openoffice-python
+# http://nxsy.org/comparing-documents-with-openoffice-and-python
 
 import logging
 import os ,  subprocess
@@ -58,7 +58,8 @@
         """
         log.debug(u'start Openoffice')
         # -headless
-        cmd = u'openoffice.org -nologo -norestore -minimized -invisible ' + u'"' + u'-accept=socket,host=localhost,port=2002;urp;'+ u'"'
+        cmd = u'openoffice.org -nologo -norestore -minimized -invisible ' + \
+            u'"' + u'-accept=socket,host=localhost,port=2002;urp;'+ u'"'
         self.process = QtCore.QProcess()
         self.process.startDetached(cmd)
         self.process.waitForStarted()
@@ -74,8 +75,9 @@
         """
         Called when a presentation is added to the SlideController.
         It builds the environment, starts communcations with the background
-        OpenOffice task started earlier.  If OpenOffice is not present is ts started.
-        Once the environment is available the presentation is loaded and started.
+        OpenOffice task started earlier.  If OpenOffice is not present is is
+        started.  Once the environment is available the presentation is loaded
+        and started.
 
         ``presentation``
         The file name of the presentatios to the run.
@@ -84,7 +86,8 @@
         ctx = None
         loop = 0
         context = uno.getComponentContext()
-        resolver = context.ServiceManager.createInstanceWithContext(u'com.sun.star.bridge.UnoUrlResolver', context)
+        resolver = context.ServiceManager.createInstanceWithContext(
+            u'com.sun.star.bridge.UnoUrlResolver', context)
         while ctx == None and loop < 3:
             try:
                 ctx = resolver.resolve(u'uno:socket,host=localhost,port=2002;urp;StarOffice.ComponentContext')
@@ -93,16 +96,19 @@
                 loop += 1
         try:
             smgr = ctx.ServiceManager
-            desktop = smgr.createInstanceWithContext( "com.sun.star.frame.Desktop", ctx )
+            desktop = smgr.createInstanceWithContext(
+                "com.sun.star.frame.Desktop", ctx)
             url = uno.systemPathToFileUrl(presentation)
             properties = []
             properties = tuple(properties)
-            self.document = desktop.loadComponentFromURL(url, "_blank", 0, properties)
+            self.document = desktop.loadComponentFromURL(
+                url, "_blank", 0, properties)
             self.presentation = self.document.getPresentation()
             self.presentation.start()
-            self.xSlideShowController =  desktop.getCurrentComponent().Presentation.getController()
+            self.xSlideShowController = \
+                desktop.getCurrentComponent().Presentation.getController()
         except:
-            log.error(u'Failed reason %s' % sys.exc_info())
+            log.exception(u'Failed to load presentation')
 
     def closePresentation(self):
         """

=== modified file 'openlp/plugins/presentations/presentationplugin.py'
--- openlp/plugins/presentations/presentationplugin.py	2009-09-13 07:39:48 +0000
+++ openlp/plugins/presentations/presentationplugin.py	2009-09-17 23:10:25 +0000
@@ -29,8 +29,8 @@
 from PyQt4 import QtCore, QtGui
 
 from openlp.core.lib import Plugin,  MediaManagerItem
-from openlp.plugins.presentations.lib import PresentationMediaItem, PresentationTab, \
-    ImpressController
+from openlp.plugins.presentations.lib import PresentationMediaItem, \
+    PresentationTab, ImpressController
 try:
     from openlp.plugins.presentations.lib import PowerpointController
 except:
@@ -64,7 +64,8 @@
         """
         Create the Media Manager List
         """
-        self.media_item = PresentationMediaItem(self, self.icon, u'Presentations', self.controllers)
+        self.media_item = PresentationMediaItem(
+            self, self.icon, u'Presentations', self.controllers)
         return self.media_item
 
     def registerControllers(self, handle, controller):
@@ -77,32 +78,35 @@
         """
         log.debug('check_pre_conditions')
         #Lets see if Impress is required (Default is Not wanted)
-        if int(self.config.get_config(u'Impress', QtCore.Qt.Unchecked)) == QtCore.Qt.Checked:
+        if int(self.config.get_config(
+            u'Impress', QtCore.Qt.Unchecked)) == QtCore.Qt.Checked:
             try:
                 #Check to see if we have uno installed
                 import uno
                 openoffice = ImpressController()
                 self.registerControllers(u'Impress', openoffice)
             except:
-                log.error(u'Reason : %s', sys.exc_info())
+                log.exception(u'Failed to set up plugin for Impress')
         #Lets see if Powerpoint is required (Default is Not wanted)
-        if int(self.config.get_config(u'Powerpoint', QtCore.Qt.Unchecked)) == QtCore.Qt.Checked:
+        if int(self.config.get_config(
+            u'Powerpoint', QtCore.Qt.Unchecked)) == QtCore.Qt.Checked:
             try:
                 #Check to see if we are Win32
                 from win32com.client import Dispatch
                 powerpoint = PowerpointController()
                 self.registerControllers(u'Powerpoint', powerpoint)
             except:
-                log.error(u'Reason : %s', sys.exc_info())
+                log.exception(u'Failed to set up plugin for Powerpoint')
         #Lets see if Powerpoint Viewer is required (Default is Not wanted)
-        if int(self.config.get_config(u'Powerpoint Viewer', QtCore.Qt.Unchecked)) == QtCore.Qt.Checked:
+        if int(self.config.get_config(
+            u'Powerpoint Viewer', QtCore.Qt.Unchecked)) == QtCore.Qt.Checked:
             try:
                 #Check to see if we are Win32
                 from win32com.client import Dispatch
                 powerpoint = PowerpointController()
                 self.registerControllers(u'Powerpoint Viewer', powerpoint)
             except:
-                log.error(u'Reason : %s', sys.exc_info())
+                log.exception(u'Failed to set up plugin for Powerpoint Viewer')
         #If we have no available controllers disable plugin
         if len(self.controllers) > 0:
             return True

=== modified file 'openlp/plugins/songs/lib/manager.py'
--- openlp/plugins/songs/lib/manager.py	2009-09-08 19:58:05 +0000
+++ openlp/plugins/songs/lib/manager.py	2009-09-18 11:55:26 +0000
@@ -51,7 +51,8 @@
         self.db_url = u''
         db_type = self.config.get_config(u'db type', u'sqlite')
         if db_type == u'sqlite':
-            self.db_url = u'sqlite:///%s/songs.sqlite' % self.config.get_data_path()
+            self.db_url = u'sqlite:///%s/songs.sqlite' % \
+                self.config.get_data_path()
         else:
             self.db_url = db_type + 'u://' + \
                 self.config.get_config(u'db username') + u':' + \
@@ -88,7 +89,8 @@
         """
         Searches the song authors for keywords.
         """
-        return self.session.query(Author).filter(Author.display_name.like(u'%' + keywords + u'%')).order_by(Author.display_name.asc()).all()
+        return self.session.query(Author).filter(Author.display_name.like(
+            u'%' + keywords + u'%')).order_by(Author.display_name.asc()).all()
 
     def get_song(self, id=None):
         """
@@ -109,7 +111,7 @@
             return True
         except:
             self.session.rollback()
-            log.error(u'Errow thrown %s', sys.exc_info()[1])
+            log.exception(u'Could not save song to song database')
             return False
 
     def delete_song(self, songid):
@@ -120,8 +122,7 @@
             return True
         except:
             self.session.rollback()
-            log.error(u'Errow thrown %s', sys.exc_info()[1])
-            print u'Errow thrown ', sys.exc_info()[1]
+            log.exception(u'Could not delete song from song database')
             return False
 
     def get_authors(self):
@@ -146,7 +147,7 @@
             return True
         except:
             self.session.rollback()
-            log.error(u'Errow thrown %s', sys.exc_info()[1])
+            log.exception(u'Could not save author to song database')
             return False
 
     def delete_author(self, authorid):
@@ -160,7 +161,7 @@
             return True
         except:
             self.session.rollback()
-            log.error(u'Errow thrown %s', sys.exc_info()[1])
+            log.exception(u'Could not delete author from song database')
             return False
 
     def get_topics(self):
@@ -185,7 +186,7 @@
             return True
         except:
             self.session.rollback()
-            log.error(u'Errow thrown %s', sys.exc_info()[1])
+            log.exception(u'Could not save topic to song database')
             return False
 
     def delete_topic(self, topicid):
@@ -199,7 +200,7 @@
             return True
         except:
             self.session.rollback()
-            log.error(u'Errow thrown %s', sys.exc_info()[1])
+            log.exception(u'Could not delete topic from song database')
             return False
 
     def get_books(self):
@@ -224,7 +225,7 @@
             return True
         except Exception, e:
             self.session.rollback()
-            log.error(u'Errow thrown %s', e.args[0])
+            log.exception(u'Could not save book to song database')
             return False
 
     def delete_book(self, bookid):
@@ -238,5 +239,6 @@
             return True
         except:
             self.session.rollback()
-            log.error(u'Errow thrown %s', sys.exc_info()[1])
+            log.exception(u'Could not delete book from song database')
             return False
+