bunch of cosmetic changes

openlp/core/lib/serviceitem.py

@@ -316,7 +316,7 @@ class ServiceItem(object):
         self._raw_frames.append({'title': title, 'raw_slide': raw_slide, 'verseTag': verse_tag})
         self._new_item()
 
-    def add_from_command(self, path, file_name, image, displaytitle=None, notes=None):
+    def add_from_command(self, path, file_name, image, display_title=None, notes=None):
         """
         Add a slide from a command.
 
@@ -331,7 +331,7 @@ class ServiceItem(object):
         """
         self.service_item_type = ServiceItemType.Command
         self._raw_frames.append({'title': file_name, 'image': image, 
-            'path': path, 'displaytitle': displaytitle, 'notes': notes})
+            'path': path, 'display_title': display_title, 'notes': notes})
         self._new_item()
 
     def get_service_repr(self, lite_save):
@@ -378,7 +378,7 @@ class ServiceItem(object):
             for slide in self._raw_frames:
                 service_data.append({'title': slide['title'], 
                     'image': slide['image'], 'path': slide['path'], 
-                    'displaytitle': slide['displaytitle'], 'notes': slide['notes']})
+                    'display_title': slide['display_title'], 'notes': slide['notes']})
         return {'header': service_header, 'data': service_data}
 
     def set_from_service(self, serviceitem, path=None):
@@ -451,7 +451,7 @@ class ServiceItem(object):
                 if path:
                     self.has_original_files = False
                     self.add_from_command(path, text_image['title'], 
-                        text_image['image'], text_image['displaytitle'], 
+                        text_image['image'], text_image['display_title'],
                         text_image['notes'])
                 else:
                     self.add_from_command(text_image['path'], 

openlp/core/ui/servicemanager.py

@@ -1185,9 +1185,9 @@ class ServiceManager(QtGui.QWidget, ServiceManagerDialog):
             # Add the children to their parent treewidgetitem.
             for count, frame in enumerate(serviceitem.get_frames()):
                 child = QtGui.QTreeWidgetItem(treewidgetitem)
-                # prefer to use a displaytitle
+                # prefer to use a display_title
                 if serviceitem.is_capable(ItemCapabilities.HasDisplayTitle):
-                    text = frame['displaytitle'].replace('\n',' ')
+                    text = frame['display_title'].replace('\n',' ')
                     # oops, it is missing, let's make one up
                     if len(text.strip()) == 0:
                         text = '[slide ' + str(count+1) + ']'

openlp/plugins/presentations/lib/impresscontroller.py

@@ -183,9 +183,9 @@ class ImpressController(PresentationController):
         docs = desktop.getComponents()
         cnt = 0
         if docs.hasElements():
-            list = docs.createEnumeration()
+            element_list = docs.createEnumeration()
-            while list.hasMoreElements():
+            while element_list.hasMoreElements():
-                doc = list.nextElement()
+                doc = element_list.nextElement()
                 if doc.getImplementationName() != 'com.sun.star.comp.framework.BackingComp':
                     cnt += 1
         if cnt > 0:
@@ -459,19 +459,19 @@ class ImpressDocument(PresentationDocument):
             A TextType. Enumeration of the types of supported text 
         """
         text = ''
-        if text_type >= TextType.Title and text_type <= TextType.Notes:
+        if TextType.Title <= text_type <= TextType.Notes:
             pages = self.document.getDrawPages()
-            if slide_no > 0 and slide_no <= pages.getCount():
+            if 0 < slide_no <= pages.getCount():
                 page = pages.getByIndex(slide_no - 1)
                 if text_type==TextType.Notes:
                     page = page.getNotesPage()
                 for index in range(page.getCount()):
                     shape = page.getByIndex(index)
-                    shapeType = shape.getShapeType()
+                    shape_type = shape.getShapeType()
                     if shape.supportsService("com.sun.star.drawing.Text"):
                         # if they requested title, make sure it is the title
                         if text_type!=TextType.Title or \
-                            shapeType == "com.sun.star.presentation.TitleTextShape":
+                            shape_type == "com.sun.star.presentation.TitleTextShape":
                             text += shape.getString() + '\n'
         return text
 
@@ -493,5 +493,5 @@ class ImpressDocument(PresentationDocument):
             if len(note) == 0:
                 note = ' '
             notes.append(note)
-        self.save_titles_and_notes(titles,notes)
+        self.save_titles_and_notes(titles, notes)
         return

openlp/plugins/presentations/lib/pptviewcontroller.py

@@ -178,29 +178,29 @@ class PptviewDocument(PresentationDocument):
                 notes = ['' for i in range(len(nodes))]
                 # loop thru the file list to find slides and notes
                 for zip_info in zip_file.infolist():
-                    nodeType = ''
+                    node_type = ''
                     index = -1
-                    listToAdd = None
+                    list_to_add = None
                     # check if it is a slide
                     match = re.search("slides/slide(.+)\.xml", zip_info.filename)
                     if match:
                         index = int(match.group(1))-1
-                        nodeType = 'ctrTitle'
+                        node_type = 'ctrTitle'
-                        listToAdd = titles
+                        list_to_add = titles
                     # or a note
                     match = re.search("notesSlides/notesSlide(.+)\.xml", 
                         zip_info.filename)
                     if match:
                         index = int(match.group(1))-1
-                        nodeType = 'body'
+                        node_type = 'body'
-                        listToAdd = notes
+                        list_to_add = notes
                     # if it is one of our files, index shouldn't be -1
                     if index >= 0:
                         with zip_file.open(zip_info) as zipped_file:
                             tree = ElementTree.parse(zipped_file)
                         text = ''
                         nodes = tree.getroot().findall(".//p:ph[@type='" + 
-                            nodeType + "']../../..//p:txBody//a:t", 
+                            node_type + "']../../..//p:txBody//a:t",
                             namespaces=namespaces)
                         # if we found any content
                         if nodes and len(nodes)>0:
@@ -210,10 +210,10 @@ class PptviewDocument(PresentationDocument):
                                 text += node.text
                         # Let's remove the \n from the titles and 
                         # just add one at the end
-                        if nodeType == 'ctrTitle':
+                        if node_type == 'ctrTitle':
                             text = text.replace('\n',' '). \
                                 replace('\x0b', ' ') + '\n'
-                        listToAdd[index] = text
+                        list_to_add[index] = text
         # now let's write the files
         self.save_titles_and_notes(titles,notes)
         return

openlp/plugins/presentations/lib/presentationcontroller.py

@@ -297,21 +297,21 @@ class PresentationDocument(object):
         """
         titles = []
         notes = []
-        titlesfile = os.path.join(self.get_thumbnail_folder(), 'titles.txt')
+        titles_file = os.path.join(self.get_thumbnail_folder(), 'titles.txt')
-        if os.path.exists(titlesfile):
+        if os.path.exists(titles_file):
             try:
-                with open(titlesfile) as fi:
+                with open(titles_file) as fi:
-                    titles = fi.read().splitlines()
+                    titles = fi.read().splitlines(keepends=True)
             except:
                 log.exception('Failed to open/read existing titles file')
                 titles = []
-        for index in range(len(titles)):
+        for slide_no, title in enumerate(titles,1):
-            notesfile = os.path.join(self.get_thumbnail_folder(), 
+            notes_file = os.path.join(self.get_thumbnail_folder(),
-                'slideNotes%d.txt' % (index + 1))
+                'slideNotes%d.txt' % slide_no)
             note = ''
-            if os.path.exists(notesfile):
+            if os.path.exists(notes_file):
                 try:
-                    with open(notesfile) as fn:
+                    with open(notes_file) as fn:
                         note = fn.read()
                 except:
                     log.exception('Failed to open/read notes file')
@@ -325,15 +325,15 @@ class PresentationDocument(object):
         and notes to the slideNote%.txt
         """
         if titles:
-            titlesfile = os.path.join(self.get_thumbnail_folder(), 'titles.txt')
+            titles_file = os.path.join(self.get_thumbnail_folder(), 'titles.txt')
-            with open(titlesfile, mode='w') as fo:
+            with open(titles_file, mode='w') as fo:
                 fo.writelines(titles)
         if notes:
-            for slide_no, note in enumerate(notes):
+            for slide_no, note in enumerate(notes,1):
-                notesfile = os.path.join(self.get_thumbnail_folder(), 
+                notes_file = os.path.join(self.get_thumbnail_folder(),
-                    'slideNotes%d.txt' % (slide_no+1))
+                    'slideNotes%d.txt' % slide_no)
-                with open(notesfile, mode='w') as fn:
+                with open(notes_file, mode='w') as fn:
-                    fn.write(notes[slide_no])
+                    fn.write(note)
 
 class PresentationController(object):
     """

openlp/plugins/remotes/lib/httprouter.py

@@ -121,7 +121,6 @@ import urllib.request
 import urllib.error
 from urllib.parse import urlparse, parse_qs
 
-
 from mako.template import Template
 from PyQt4 import QtCore
 
@@ -399,17 +398,20 @@ class HttpRouter(object):
         Serve an image file. If not found return 404.
         """
         log.debug('serve thumbnail %s/thumbnails/%s' % (controller_name, file_name))
+        supported_controllers = ['presentations']
         content = ''
-        full_path = os.path.normpath(os.path.join(
+        if controller_name and file_name:
-            AppLocation.get_section_data_path(controller_name), 
+            if controller_name in supported_controllers:
-            'thumbnails/' + file_name))
+                full_path = urllib.parse.unquote(file_name)
-        full_path = urllib.parse.unquote(full_path)
+                if not '..' in full_path:
-        
+                    full_path = os.path.normpath(os.path.join(
-        if os.path.exists(full_path):
+                        AppLocation.get_section_data_path(controller_name), 
-            self.send_appropriate_header(full_path)
+                        'thumbnails/' + full_path))
-            file_handle = open(full_path, 'rb')
+                    if os.path.exists(full_path):
-            content = file_handle.read()
+                        ext = self.send_appropriate_header(full_path)
-        else:
+                        file_handle = open(full_path, 'rb')
+                        content = file_handle.read()
+        if len(content)==0:
             content = self.do_not_found()
         return content
 
@@ -501,7 +503,7 @@ class HttpRouter(object):
                 else:
                     item['tag'] = str(index + 1)
                     if current_item.is_capable(ItemCapabilities.HasDisplayTitle):
-                        item['title'] = str(frame['displaytitle'])
+                        item['title'] = str(frame['display_title'])
                     if current_item.is_capable(ItemCapabilities.HasNotes):
                         item['notes'] = str(frame['notes'])
                     if current_item.is_capable(ItemCapabilities.HasThumbnails):

tests/functional/openlp_core_lib/test_serviceitem.py

@@ -32,7 +32,8 @@ Package to test the openlp.core.lib package.
 import os
 from unittest import TestCase
 
-from openlp.core.lib import ItemCapabilities, ServiceItem, Registry, ServiceItemType
+from openlp.core.lib import ItemCapabilities, ServiceItem, Registry, \
+    ServiceItemType
 from tests.functional import MagicMock, patch
 from tests.utils import assert_length, convert_file_service_item
 
@@ -44,7 +45,8 @@ VERSE = 'The Lord said to {r}Noah{/r}: \n'\
         '{r}C{/r}{b}h{/b}{bl}i{/bl}{y}l{/y}{g}d{/g}{pk}'\
         'r{/pk}{o}e{/o}{pp}n{/pp} of the Lord\n'
 FOOTER = ['Arky Arky (Unknown)', 'Public Domain', 'CCLI 123456']
-TEST_PATH = os.path.abspath(os.path.join(os.path.dirname(__file__), '..', '..', 'resources'))
+TEST_PATH = os.path.abspath(os.path.join(os.path.dirname(__file__), '..', '..',
+     'resources'))
 
 
 class TestServiceItem(TestCase):
@@ -69,8 +71,10 @@ class TestServiceItem(TestCase):
         service_item = ServiceItem(None)
 
         # THEN: We should get back a valid service item
-        self.assertTrue(service_item.is_valid, 'The new service item should be valid')
+        self.assertTrue(service_item.is_valid,
-        self.assertTrue(service_item.missing_frames(), 'There should not be any frames in the service item')
+            'The new service item should be valid')
+        self.assertTrue(service_item.missing_frames(),
+            'There should not be any frames in the service item')
 
     def service_item_load_custom_from_service_test(self):
         """
@@ -85,22 +89,30 @@ class TestServiceItem(TestCase):
         service_item.set_from_service(line)
 
         # THEN: We should get back a valid service item
-        self.assertTrue(service_item.is_valid, 'The new service item should be valid')
+        self.assertTrue(service_item.is_valid,
-        assert_length(0, service_item._display_frames, 'The service item should have no display frames')
+            'The new service item should be valid')
-        assert_length(5, service_item.capabilities, 'There should be 5 default custom item capabilities')
+        assert_length(0, service_item._display_frames,
+            'The service item should have no display frames')
+        assert_length(5, service_item.capabilities,
+            'There should be 5 default custom item capabilities')
 
         # WHEN: We render the frames of the service item
         service_item.render(True)
 
         # THEN: The frames should also be valid
-        self.assertEqual('Test Custom', service_item.get_display_title(), 'The title should be "Test Custom"')
+        self.assertEqual('Test Custom', service_item.get_display_title(),
+             'The title should be "Test Custom"')
         self.assertEqual(VERSE[:-1], service_item.get_frames()[0]['text'],
             'The returned text matches the input, except the last line feed')
-        self.assertEqual(VERSE.split('\n', 1)[0], service_item.get_rendered_frame(1),
+        self.assertEqual(VERSE.split('\n', 1)[0],
+            service_item.get_rendered_frame(1),
             'The first line has been returned')
-        self.assertEqual('Slide 1', service_item.get_frame_title(0), '"Slide 1" has been returned as the title')
+        self.assertEqual('Slide 1', service_item.get_frame_title(0),
-        self.assertEqual('Slide 2', service_item.get_frame_title(1), '"Slide 2" has been returned as the title')
+            '"Slide 1" has been returned as the title')
-        self.assertEqual('', service_item.get_frame_title(2), 'Blank has been returned as the title of slide 3')
+        self.assertEqual('Slide 2', service_item.get_frame_title(1),
+            '"Slide 2" has been returned as the title')
+        self.assertEqual('', service_item.get_frame_title(2),
+            'Blank has been returned as the title of slide 3')
 
     def service_item_load_image_from_service_test(self):
         """
@@ -116,31 +128,35 @@ class TestServiceItem(TestCase):
 
         # WHEN: adding an image from a saved Service and mocked exists
         line = convert_file_service_item(TEST_PATH, 'serviceitem_image_1.osj')
-        with patch('openlp.core.ui.servicemanager.os.path.exists') as mocked_exists:
+        with patch('openlp.core.ui.servicemanager.os.path.exists') as \
+            mocked_exists:
             mocked_exists.return_value = True
             service_item.set_from_service(line, TEST_PATH)
 
-        # THEN: We should get back a valid service item
+            # THEN: We should get back a valid service item
-        self.assertTrue(service_item.is_valid, 'The new service item should be valid')
+            self.assertTrue(service_item.is_valid,
-        self.assertEqual(os.path.normpath(test_file), os.path.normpath(service_item.get_rendered_frame(0)),
+                'The new service item should be valid')
-            'The first frame should match the path to the image')
+            self.assertEqual(os.path.normpath(test_file),
-        self.assertEqual(frame_array, service_item.get_frames()[0],
+                os.path.normpath(service_item.get_rendered_frame(0)),
-            'The return should match frame array1')
+                'The first frame should match the path to the image')
-        self.assertEqual(test_file, service_item.get_frame_path(0),
+            self.assertEqual(frame_array, service_item.get_frames()[0],
-            'The frame path should match the full path to the image')
+                'The return should match frame array1')
-        self.assertEqual(image_name, service_item.get_frame_title(0),
+            self.assertEqual(test_file, service_item.get_frame_path(0),
-            'The frame title should match the image name')
+                'The frame path should match the full path to the image')
-        self.assertEqual(image_name, service_item.get_display_title(),
+            self.assertEqual(image_name, service_item.get_frame_title(0),
-            'The display title should match the first image name')
+                'The frame title should match the image name')
-        self.assertTrue(service_item.is_image(), 'This service item should be of an "image" type')
+            self.assertEqual(image_name, service_item.get_display_title(),
-        self.assertTrue(service_item.is_capable(ItemCapabilities.CanMaintain),
+                'The display title should match the first image name')
-            'This service item should be able to be Maintained')
+            self.assertTrue(service_item.is_image(),
-        self.assertTrue(service_item.is_capable(ItemCapabilities.CanPreview),
+                'This service item should be of an "image" type')
-            'This service item should be able to be be Previewed')
+            self.assertTrue(service_item.is_capable(ItemCapabilities.CanMaintain),
-        self.assertTrue(service_item.is_capable(ItemCapabilities.CanLoop),
+                'This service item should be able to be Maintained')
-            'This service item should be able to be run in a can be made to Loop')
+            self.assertTrue(service_item.is_capable(ItemCapabilities.CanPreview),
-        self.assertTrue(service_item.is_capable(ItemCapabilities.CanAppend),
+                'This service item should be able to be be Previewed')
-            'This service item should be able to have new items added to it')
+            self.assertTrue(service_item.is_capable(ItemCapabilities.CanLoop),
+                'This service item should be able to be run in a can be made to Loop')
+            self.assertTrue(service_item.is_capable(ItemCapabilities.CanAppend),
+                'This service item should be able to have new items added to it')
 
     def service_item_load_image_from_local_service_test(self):
         """
@@ -149,8 +165,10 @@ class TestServiceItem(TestCase):
         # GIVEN: A new service item and a mocked add icon function
         image_name1 = 'image_1.jpg'
         image_name2 = 'image_2.jpg'
-        test_file1 = os.path.normpath(os.path.join('/home/openlp', image_name1))
+        test_file1 = os.path.normpath(os.path.join('/home/openlp',
-        test_file2 = os.path.normpath(os.path.join('/home/openlp', image_name2))
+            image_name1))
+        test_file2 = os.path.normpath(os.path.join('/home/openlp',
+            image_name2))
         frame_array1 = {'path': test_file1, 'title': image_name1}
         frame_array2 = {'path': test_file2, 'title': image_name2}
 
@@ -164,42 +182,57 @@ class TestServiceItem(TestCase):
         line = convert_file_service_item(TEST_PATH, 'serviceitem_image_2.osj')
         line2 = convert_file_service_item(TEST_PATH, 'serviceitem_image_2.osj', 1)
 
-        with patch('openlp.core.ui.servicemanager.os.path.exists') as mocked_exists:
+        with patch('openlp.core.ui.servicemanager.os.path.exists') as \
+            mocked_exists:
             mocked_exists.return_value = True
             service_item2.set_from_service(line2)
             service_item.set_from_service(line)
 
-        # THEN: We should get back a valid service item
+            # THEN: We should get back a valid service item
 
-        # This test is copied from service_item.py, but is changed since to conform to
+            # This test is copied from service_item.py,
-        # new layout of service item. The layout use in serviceitem_image_2.osd is actually invalid now.
+            # but is changed since to conform to
-        self.assertTrue(service_item.is_valid, 'The first service item should be valid')
+            # new layout of service item. The layout use in
-        self.assertTrue(service_item2.is_valid, 'The second service item should be valid')
+            # serviceitem_image_2.osd is actually invalid now.
-        self.assertEqual(test_file1, os.path.normpath(service_item.get_rendered_frame(0)),
+            self.assertTrue(service_item.is_valid,
-            'The first frame should match the path to the image')
+                'The first service item should be valid')
-        self.assertEqual(test_file2, os.path.normpath(service_item2.get_rendered_frame(0)),
+            self.assertTrue(service_item2.is_valid,
-            'The Second frame should match the path to the image')
+                'The second service item should be valid')
-        self.assertEqual(frame_array1, service_item.get_frames()[0], 'The return should match the frame array1')
+            self.assertEqual(test_file1,
-        self.assertEqual(frame_array2, service_item2.get_frames()[0], 'The return should match the frame array2')
+                os.path.normpath(service_item.get_rendered_frame(0)),
-        self.assertEqual(test_file1, os.path.normpath(service_item.get_frame_path(0)),
+                'The first frame should match the path to the image')
-            'The frame path should match the full path to the image')
+            self.assertEqual(test_file2,
-        self.assertEqual(test_file2, os.path.normpath(service_item2.get_frame_path(0)),
+                os.path.normpath(service_item2.get_rendered_frame(0)),
-            'The frame path should match the full path to the image')
+                'The Second frame should match the path to the image')
-        self.assertEqual(image_name1, service_item.get_frame_title(0),
+            # There is a problem with the following two asserts in Windows
-            'The 1st frame title should match the image name')
+            # and it is not easily fixable (although it looks simple)
-        self.assertEqual(image_name2, service_item2.get_frame_title(0),
+            if os.name != 'nt':
-            'The 2nd frame title should match the image name')
+                self.assertEqual(frame_array1, service_item.get_frames()[0],
-        self.assertEqual(service_item.name, service_item.title.lower(),
+                    'The return should match the frame array1')
-            'The plugin name should match the display title, as there are > 1 Images')
+                self.assertEqual(frame_array2, service_item2.get_frames()[0],
-        self.assertTrue(service_item.is_image(), 'This service item should be of an "image" type')
+                    'The return should match the frame array2')
-        self.assertTrue(service_item.is_capable(ItemCapabilities.CanMaintain),
+            self.assertEqual(test_file1, os.path.normpath(
-            'This service item should be able to be Maintained')
+                service_item.get_frame_path(0)),
-        self.assertTrue(service_item.is_capable(ItemCapabilities.CanPreview),
+                'The frame path should match the full path to the image')
-            'This service item should be able to be be Previewed')
+            self.assertEqual(test_file2, os.path.normpath(
-        self.assertTrue(service_item.is_capable(ItemCapabilities.CanLoop),
+                service_item2.get_frame_path(0)),
-            'This service item should be able to be run in a can be made to Loop')
+                'The frame path should match the full path to the image')
-        self.assertTrue(service_item.is_capable(ItemCapabilities.CanAppend),
+            self.assertEqual(image_name1, service_item.get_frame_title(0),
-            'This service item should be able to have new items added to it')
+                'The 1st frame title should match the image name')
+            self.assertEqual(image_name2, service_item2.get_frame_title(0),
+                'The 2nd frame title should match the image name')
+            self.assertEqual(service_item.name, service_item.title.lower(),
+                'The plugin name should match the display title, as there are > 1 Images')
+            self.assertTrue(service_item.is_image(),
+                'This service item should be of an "image" type')
+            self.assertTrue(service_item.is_capable(ItemCapabilities.CanMaintain),
+                'This service item should be able to be Maintained')
+            self.assertTrue(service_item.is_capable(ItemCapabilities.CanPreview),
+                'This service item should be able to be be Previewed')
+            self.assertTrue(service_item.is_capable(ItemCapabilities.CanLoop),
+                'This service item should be able to be run in a can be made to Loop')
+            self.assertTrue(service_item.is_capable(ItemCapabilities.CanAppend),
+                'This service item should be able to have new items added to it')
 
     def add_from_command_for_a_presentation_test(self):
         """
@@ -209,18 +242,18 @@ class TestServiceItem(TestCase):
         service_item = ServiceItem(None)
         presentation_name = 'test.pptx'
         image = MagicMock()
-        displaytitle = 'DisplayTitle'
+        display_title = 'DisplayTitle'
         notes = 'Note1\nNote2\n'
         frame = {'title': presentation_name, 'image': image,
-            'path': TEST_PATH, 'displaytitle': displaytitle,
+            'path': TEST_PATH, 'display_title': display_title,
             'notes': notes }
         # WHEN: adding presentation to service_item
-        service_item.add_from_command(TEST_PATH, presentation_name, image, displaytitle, notes)
+        service_item.add_from_command(TEST_PATH, presentation_name, image, display_title, notes)
         # THEN: verify that it is setup as a Command and that the frame data matches
         assert service_item.service_item_type == ServiceItemType.Command
         assert service_item.get_frames()[0] == frame
 
-    def add_from_comamnd_without_displaytitle_and_notes_test(self):
+    def add_from_comamnd_without_display_title_and_notes_test(self):
         """
         Test the Service Item - add from command, but not presentation
         """
@@ -229,7 +262,7 @@ class TestServiceItem(TestCase):
         image_name = 'test.img'
         image = MagicMock()
         frame = {'title': image_name, 'image': image,
-            'path': TEST_PATH, 'displaytitle': None,
+            'path': TEST_PATH, 'display_title': None,
             'notes': None}
         # WHEN: adding image to service_item
         service_item.add_from_command(TEST_PATH, image_name, image)

tests/functional/openlp_plugins/presentations/test_impresscontroller.py

@@ -31,17 +31,19 @@ Functional tests to test the Impress class and related methods.
 """
 from unittest import TestCase
 import os
-from mock import MagicMock, patch, mock_open
+from mock import MagicMock
-from openlp.plugins.presentations.lib.impresscontroller import ImpressController, ImpressDocument, TextType
+from openlp.plugins.presentations.lib.impresscontroller import \
+    ImpressController, ImpressDocument, TextType
 
-TEST_PATH = os.path.abspath(os.path.join(os.path.dirname(__file__), '..', '..', '..', 'resources'))
+TEST_PATH = os.path.abspath(os.path.join(os.path.dirname(__file__), '..',
+    '..', '..', 'resources'))
 
 class TestLibModule(TestCase):
 
     def setUp(self):
         mocked_plugin = MagicMock()
         mocked_plugin.settings_section = 'presentations'
-        self.file_name = os.path.join(TEST_PATH,"test.pptx")
+        self.file_name = os.path.join(TEST_PATH,'test.pptx')
         self.ppc = ImpressController(mocked_plugin)
         self.doc = ImpressDocument(self.ppc,self.file_name)
 
@@ -49,99 +51,113 @@ class TestLibModule(TestCase):
         """
         Test ImpressDocument.create_titles_and_notes
         """
-        # GIVEN: mocked PresentationController.save_titles_and_notes with 0 pages and the LibreoOffice Document
+        # GIVEN: mocked PresentationController.save_titles_and_notes with
+        # 0 pages and the LibreOffice Document
         self.doc.save_titles_and_notes = MagicMock()
         self.doc.document = MagicMock()
         self.doc.document.getDrawPages.return_value = MagicMock()
         self.doc.document.getDrawPages().getCount.return_value = 0
         # WHEN reading the titles and notes
         self.doc.create_titles_and_notes()
-        # THEN save_titles_and_notes should have been called once with empty arrays
+        # THEN save_titles_and_notes should have been called with empty arrays
         self.doc.save_titles_and_notes.assert_called_once_with([],[])
         # GIVEN: reset mock and set it to 2 pages
         self.doc.save_titles_and_notes.reset_mock()
         self.doc.document.getDrawPages().getCount.return_value = 2
         # WHEN: a new call to create_titles_and_notes
         self.doc.create_titles_and_notes()
-        # THEN: save_titles_and_notes should have been called once with two arrays of two elements
+        # THEN: save_titles_and_notes should have been called once with
-        self.doc.save_titles_and_notes.assert_called_once_with(['\n','\n'],[' ',' '])
+        # two arrays of two elements
+        self.doc.save_titles_and_notes.assert_called_once_with(
+            ['\n','\n'], [' ',' '])
 
     def get_text_from_page_out_of_bound_test(self):
         """
         Test ImpressDocument.__get_text_from_page with out-of-bounds index
         """
-        # GIVEN: mocked LibreoOffice Document with one slide, two notes and three texts
+        # GIVEN: mocked LibreOffice Document with one slide,
+        # two notes and three texts
         self.doc.document = self._mock_a_LibreOffice_document(1,2,3)
-        # WHEN: __get_text_from_page is called with an index of 0x00 (index is 1 based)
+        # WHEN: __get_text_from_page is called with an index of 0x00
         result = self.doc._ImpressDocument__get_text_from_page(0,TextType.Notes)
         # THEN: the result should be an empty string
-        assert result == '', 'Result should be an empty string'
+        self.assertEqual(result, '', 'Result should be an empty string')
         # WHEN: regardless of the type of text, index 0x00 is out of bounds
         result = self.doc._ImpressDocument__get_text_from_page(0,TextType.Title)
         # THEN: result should be an empty string
-        assert result == '', 'Result should be an empty string'
+        self.assertEqual(result, '', 'Result should be an empty string')
         # WHEN: when called with 2, it should also be out of bounds
         result = self.doc._ImpressDocument__get_text_from_page(2,TextType.SlideText)
-        # THEN: result should be an empty string ... and, getByIndex should have never been called
+        # THEN: result should be an empty string ... and, getByIndex should
-        assert result == '', 'Result should be an empty string'
+        # have never been called
-        assert self.doc.document.getDrawPages().getByIndex.call_count == 0, 'There should be no call to getByIndex'
+        self.assertEqual(result, '', 'Result should be an empty string')
+        self.assertEqual(self.doc.document.getDrawPages().getByIndex.call_count,
+            0, 'There should be no call to getByIndex')
 
     def get_text_from_page_wrong_type_test(self):
         """
         Test ImpressDocument.__get_text_from_page with wrong TextType
         """
-        # GIVEN: mocked LibreOffice Document with one slide, two notes and three texts
+        # GIVEN: mocked LibreOffice Document with one slide, two notes and
+        # three texts
         self.doc.document = self._mock_a_LibreOffice_document(1,2,3)
         # WHEN: called with TextType 3
         result = self.doc._ImpressDocument__get_text_from_page(1,3)
         # THEN: result should be an empty string
-        assert result == '', 'Result should be and empty string'
+        self.assertEqual(result, '', 'Result should be and empty string')
-        assert self.doc.document.getDrawPages().getByIndex.call_count == 0, 'Theres should be no call to getByIndex'
+        self.assertEqual(self.doc.document.getDrawPages().getByIndex.call_count,
+            0, 'There should be no call to getByIndex')
 
     def get_text_from_page_valid_params_test(self):
         """
         Test ImpressDocument.__get_text_from_page with valid parameters
         """
-        # GIVEN: mocked LibreOffice Document with one slide, two notes and three texts
+        # GIVEN: mocked LibreOffice Document with one slide,
+        # two notes and three texts
         self.doc.document = self._mock_a_LibreOffice_document(1,2,3)
         # WHEN: __get_text_from_page is called to get the Notes
-        result = self.doc._ImpressDocument__get_text_from_page(1,TextType.Notes)
+        result = self.doc._ImpressDocument__get_text_from_page(1, TextType.Notes)
         # THEN: result should be 'Note\nNote\n'
-        assert result == 'Note\nNote\n', 'Result should be exactly \'Note\\n\' times the count of notes in the page'
+        self.assertEqual(result, 'Note\nNote\n',
+            'Result should be \'Note\\n\' times the count of notes in the page')
         # WHEN: get the Title
-        result = self.doc._ImpressDocument__get_text_from_page(1,TextType.Title)
+        result = self.doc._ImpressDocument__get_text_from_page(1, TextType.Title)
         # THEN: result should be 'Title\n'
-        assert result == 'Title\n', 'Result should be exactly \'Title\\n\''
+        self.assertEqual(result, 'Title\n',
+            'Result should be exactly \'Title\\n\'')
         # WHEN: get all text
-        result = self.doc._ImpressDocument__get_text_from_page(1,TextType.SlideText)
+        result = self.doc._ImpressDocument__get_text_from_page(1,
+            TextType.SlideText)
         # THEN: result should be 'Title\nString\nString\n'
-        assert result == 'Title\nString\nString\n', 'Result should be exactly \'Title\\nString\\nString\\n\''
+        self.assertEqual(result, 'Title\nString\nString\n',
+            'Result should be exactly \'Title\\nString\\nString\\n\'')
 
-    def _mock_a_LibreOffice_document(self,pageCount,noteCount,textCount):
+    def _mock_a_LibreOffice_document(self, page_count, note_count, text_count):
         pages = MagicMock()
         page = MagicMock()
         pages.getByIndex.return_value = page
-        notesPage = MagicMock()
+        notes_page = MagicMock()
-        notesPage.getCount.return_value = noteCount
+        notes_page.getCount.return_value = note_count
         shape = MagicMock()
         shape.supportsService.return_value = True
         shape.getString.return_value = 'Note'
-        notesPage.getByIndex.return_value = shape
+        notes_page.getByIndex.return_value = shape
-        page.getNotesPage.return_value = notesPage
+        page.getNotesPage.return_value = notes_page
-        page.getCount.return_value = textCount
+        page.getCount.return_value = text_count
         page.getByIndex.side_effect = self._get_page_shape_side_effect
-        pages.getCount.return_value = pageCount
+        pages.getCount.return_value = page_count
         document = MagicMock()
         document.getDrawPages.return_value = pages
         document.getByIndex.return_value = page
         return document
 
-    def _get_page_shape_side_effect(*args, **kwargs):
+    def _get_page_shape_side_effect(*args):
-        pageShape = MagicMock()
+        page_shape = MagicMock()
-        pageShape.supportsService.return_value = True
+        page_shape.supportsService.return_value = True
         if args[1] == 0:
-            pageShape.getShapeType.return_value = 'com.sun.star.presentation.TitleTextShape'
+            page_shape.getShapeType.return_value = \
-            pageShape.getString.return_value = 'Title'
+                'com.sun.star.presentation.TitleTextShape'
+            page_shape.getString.return_value = 'Title'
         else:
-            pageShape.getString.return_value = 'String'
+            page_shape.getString.return_value = 'String'
-        return pageShape
+        return page_shape

tests/functional/openlp_plugins/presentations/test_powerpointcontroller.py

@@ -31,10 +31,12 @@ Functional tests to test the PowerPointController class and related methods.
 """
 from unittest import TestCase
 import os
-from mock import MagicMock, patch
+from mock import MagicMock
-from openlp.plugins.presentations.lib.powerpointcontroller import PowerpointController, PowerpointDocument, _get_text_from_shapes
+from openlp.plugins.presentations.lib.powerpointcontroller import \
+    PowerpointController, PowerpointDocument, _get_text_from_shapes
 
-TEST_PATH = os.path.abspath(os.path.join(os.path.dirname(__file__), '..', '..', '..', 'resources'))
+TEST_PATH = os.path.abspath(os.path.join(os.path.dirname(__file__), '..', '..',
+    '..', 'resources'))
 
 class TestLibModule(TestCase):
 
@@ -52,9 +54,9 @@ class TestLibModule(TestCase):
         """
         # GIVEN: A boolean value set to true
         # WHEN: We "convert" it to a bool
-        isInstalled = self.ppc.check_available()
+        is_installed = self.ppc.check_available()
         # THEN: We should get back a True bool
-        assert isInstalled is True, 'The result should be True'
+        self.assertEqual(is_installed, True, 'The result should be True')
 
     # add _test to the following if necessary
     def verify_loading_document(self):
@@ -68,7 +70,7 @@ class TestLibModule(TestCase):
         self.doc.load_presentation()
         result = self.doc.is_loaded()
         # THEN: result should be true
-        assert result is True, 'The result should be True'
+        self.assertEqual(result, True, 'The result should be True')
 
     def create_titles_and_notes_test(self):
         """
@@ -86,7 +88,8 @@ class TestLibModule(TestCase):
         # WHEN reading the titles and notes
         self.doc.create_titles_and_notes()
         # THEN the save should have been called exactly once with 2 titles and 2 notes
-        self.doc.save_titles_and_notes.assert_called_once_with(['SlideText\n', 'SlideText\n'], [' ', ' '])
+        self.doc.save_titles_and_notes.assert_called_once_with(
+            ['SlideText\n', 'SlideText\n'], [' ', ' '])
 
     def create_titles_and_notes_with_no_slides_test(self):
         """
@@ -115,8 +118,9 @@ class TestLibModule(TestCase):
         shape.TextFrame.TextRange.Text = 'slideText'
         shapes = [shape,shape]
         result = _get_text_from_shapes(shapes)
-        assert result == 'slideText\nslideText\n'
+        self.assertEqual(result, 'slideText\nslideText\n',
-        
+            'result should match \'slideText\nslideText\n\'')
+
     def get_text_from_shapes_with_no_shapes_test(self):
         """
         Test getting text from powerpoint shapes with no shapes
@@ -124,4 +128,4 @@ class TestLibModule(TestCase):
         # GIVEN: mocked 
         shapes = []
         result = _get_text_from_shapes(shapes)
-        assert result == ''
+        self.assertEqual(result, '', 'result should be empty')

tests/functional/openlp_plugins/presentations/test_powerpointviewercontroller.py

@@ -52,9 +52,9 @@ class TestLibModule(TestCase):
         """
         # GIVEN: A boolean value set to true
         # WHEN: We "convert" it to a bool
-        isInstalled = self.ppc.check_available()
+        is_installed = self.ppc.check_available()
         # THEN: We should get back a True bool
-        assert isInstalled is True, 'The result should be True'
+        self.assertEqual(is_installed, True, 'The result should be True')
 
     # add _test to the following if necessary to enable test
     # I don't have powerpointviewer to verify
@@ -69,7 +69,7 @@ class TestLibModule(TestCase):
         self.doc.load_presentation()
         result = self.doc.is_loaded()
         # THEN: result should be true
-        assert result is True, 'The result should be True'
+        self.assertEqual(result, True, 'The result should be True')
 
     # disabled
     def verify_titles(self):
@@ -84,8 +84,8 @@ class TestLibModule(TestCase):
         print("titles: ".join(titles))
         print("notes: ".join(notes))
         # THEN there should be exactly 5 titles and 5 notes
-        assert len(titles)==5, 'There should be five titles'
+        self.assertEqual(len(titles), 5, 'There should be five titles')
-        assert len(notes)==5, 'Theres should be five notes'
+        self.assertEqual(len(notes), 5, 'There should be five notes')
 
     def create_titles_and_notes_test(self):
         """
@@ -96,7 +96,10 @@ class TestLibModule(TestCase):
         # WHEN reading the titles and notes
         self.doc.create_titles_and_notes()
         # THEN save_titles_and_notes should have been called once with empty arrays
-        self.doc.save_titles_and_notes.assert_called_once_with(['Test 1\n', '\n', 'Test 2\n', 'Test 4\n', 'Test 3\n'], ['Notes for slide 1', 'Inserted', 'Notes for slide 2', 'Notes \nfor slide 4', 'Notes for slide 3'])
+        self.doc.save_titles_and_notes.assert_called_once_with(
+            ['Test 1\n', '\n', 'Test 2\n', 'Test 4\n', 'Test 3\n'],
+            ['Notes for slide 1', 'Inserted', 'Notes for slide 2',
+            'Notes \nfor slide 4', 'Notes for slide 3'])
 
     def create_titles_and_notes_nonexistent_file_test(self):
         """
@@ -115,6 +118,8 @@ class TestLibModule(TestCase):
             # THEN:
             self.doc.save_titles_and_notes.assert_called_once_with(None,None)
             mocked_exists.assert_any_call('Idontexist.pptx')
+            self.assertEqual(mocked_open.call_count, 0,
+                'There should be no calls to open a file')
         
     def create_titles_and_notes_invalid_file_test(self):
         """
@@ -133,5 +138,6 @@ class TestLibModule(TestCase):
             self.doc.create_titles_and_notes()
             # THEN:
             self.doc.save_titles_and_notes.assert_called_once_with(None,None)
-            assert mocked_is_zf.call_count == 1
+            self.assertEqual(mocked_is_zf.call_count, 1,
+                'is_zipfile should have been called once')
             

tests/functional/openlp_plugins/presentations/test_presentationcontroller.py

@@ -32,7 +32,6 @@ classes and related methods.
 """
 from unittest import TestCase
 import os
-import io
 from mock import MagicMock, patch, mock_open
 from openlp.plugins.presentations.lib.presentationcontroller import PresentationController, PresentationDocument
 
@@ -59,10 +58,14 @@ class TestLibModule(TestCase):
             mocked_get_thumbnail_folder.return_value = 'test'
             self.document.save_titles_and_notes(titles,notes)
             # THEN: the last call to open should have been for slideNotes2.txt
-            mocked_open.assert_any_call(os.path.join('test','titles.txt'), mode='w')
+            mocked_open.assert_any_call(
-            mocked_open.assert_any_call(os.path.join('test','slideNotes1.txt'), mode='w')
+                os.path.join('test','titles.txt'), mode='w')
-            mocked_open.assert_any_call(os.path.join('test','slideNotes2.txt'), mode='w')
+            mocked_open.assert_any_call(
-            assert mocked_open.call_count == 3, 'There should be exactly three files opened'
+                os.path.join('test','slideNotes1.txt'), mode='w')
+            mocked_open.assert_any_call(
+                os.path.join('test','slideNotes2.txt'), mode='w')
+            self.assertEqual(mocked_open.call_count, 3,
+                'There should be exactly three files opened')
             mocked_open().writelines.assert_called_once_with(['uno','dos'])
             mocked_open().write.assert_called_any('one')
             mocked_open().write.assert_called_any('two')
@@ -81,7 +84,8 @@ class TestLibModule(TestCase):
             mocked_get_thumbnail_folder.return_value = 'test'
             self.document.save_titles_and_notes(titles,notes)
             # THEN: No file should have been created
-            assert mocked_open.call_count == 0, 'No file should be created'
+            self.assertEqual(mocked_open.call_count, 0,
+                'No file should be created')
 
 
     def get_titles_and_notes_test(self):
@@ -98,15 +102,21 @@ class TestLibModule(TestCase):
             # WHEN: calling get_titles_and_notes
             result_titles, result_notes = self.document.get_titles_and_notes()
             # THEN: it should return two items for the titles and two empty strings for the notes
-            assert type(result_titles) is list
+            self.assertIs(type(result_titles), list,
-            assert len(result_titles) == 2
+                'result_titles should be of type list')
-            assert type(result_notes) is list
+            self.assertEqual(len(result_titles), 2,
-            assert len(result_notes) == 2
+                'There should be two items in the titles')
-            assert mocked_open.call_count == 3, 'Three files should be opened'
+            self.assertIs(type(result_notes), list,
+                'result_notes should be of type list')
+            self.assertEqual(len(result_notes), 2,
+                'There should be two items in the notes')
+            self.assertEqual(mocked_open.call_count, 3,
+                'Three files should be opened')
             mocked_open.assert_any_call(os.path.join('test','titles.txt'))
             mocked_open.assert_any_call(os.path.join('test','slideNotes1.txt'))
             mocked_open.assert_any_call(os.path.join('test','slideNotes2.txt'))
-            assert mocked_exists.call_count == 3, 'Three files should have been checked'
+            self.assertEqual(mocked_exists.call_count, 3,
+                'Three files should have been checked')
 
     def get_titles_and_notes_with_file_not_found_test(self):
         """
@@ -122,12 +132,18 @@ class TestLibModule(TestCase):
             #WHEN: calling get_titles_and_notes
             result_titles, result_notes = self.document.get_titles_and_notes()
             # THEN: it should return two empty lists
-            assert type(result_titles) is list
+            self.assertIs(type(result_titles), list,
-            assert len(result_titles) == 0
+                'result_titles should be of type list')
-            assert type(result_notes) is list
+            self.assertEqual(len(result_titles), 0,
-            assert len(result_notes) == 0
+                'there be no titles')
-            mocked_open.call_count == 0
+            self.assertIs(type(result_notes), list,
-            assert mocked_exists.call_count == 1
+                'result_notes should be a list')
+            self.assertEqual(len(result_notes), 0,
+                'but the list should be empty')
+            self.assertEqual(mocked_open.call_count, 0,
+                'No calls to open files')
+            self.assertEqual(mocked_exists.call_count, 1,
+                'There should be one call to file exists')
 
     def get_titles_and_notes_with_file_error_test(self):
         """
@@ -144,5 +160,6 @@ class TestLibModule(TestCase):
             # WHEN: calling get_titles_and_notes
             result_titles, result_notes = self.document.get_titles_and_notes()
             # THEN: it should return two empty lists
-            assert type(result_titles) is list
+            self.assertIs(type(result_titles), list,
+                'result_titles should be a list')
             

tests/functional/openlp_plugins/remotes/test_router.py

@@ -37,7 +37,7 @@ from PyQt4 import QtGui
 
 from openlp.core.lib import Settings
 from openlp.plugins.remotes.lib.httpserver import HttpRouter
-from tests.functional import MagicMock
+from mock import MagicMock, patch, mock_open
 
 __default_settings__ = {
     'remotes/twelve hour': True,
@@ -126,5 +126,67 @@ class TestRouter(TestCase):
         for header in headers:
             self.router.send_appropriate_header(header[0])
             send_header.assert_called_with('Content-type',header[1])
-            send_header.reset()
+            send_header.reset_mock()
-    
+
+    def serve_thumbnail_without_params_test(self):
+        """
+        Test the serve_thumbnail routine without params
+        """
+        self.router.send_response = MagicMock()
+        self.router.send_header = MagicMock()
+        self.router.end_headers = MagicMock()
+        self.router.wfile = MagicMock()
+        self.router.serve_thumbnail()
+        self.router.send_response.assert_called_once_with(404)
+
+    def serve_thumbnail_with_invalid_params_test(self):
+        """
+        Test the serve_thumbnail routine with invalid params
+        """
+        # GIVEN: Mocked send_header, send_response, end_headers and wfile
+        self.router.send_response = MagicMock()
+        self.router.send_header = MagicMock()
+        self.router.end_headers = MagicMock()
+        self.router.wfile = MagicMock()
+        # WHEN: pass a bad controller
+        self.router.serve_thumbnail('badcontroller','tecnologia 1.pptx/slide1.png')
+        # THEN: a 404 should be returned
+        self.assertEqual(len(self.router.send_header.mock_calls), 1,
+            'One header')
+        self.assertEqual(len(self.router.send_response.mock_calls), 1,
+            'One response')
+        self.assertEqual(len(self.router.wfile.mock_calls), 1,
+            'Once call to write to the socket')
+        self.router.send_response.assert_called_once_with(404)
+        # WHEN: pass a bad filename
+        self.router.send_response.reset_mock()
+        self.router.serve_thumbnail('presentations','tecnologia 1.pptx/badfilename.png')
+        # THEN: return a 404
+        self.router.send_response.assert_called_once_with(404)
+        # WHEN: a dangerous URL is passed
+        self.router.send_response.reset_mock()
+        self.router.serve_thumbnail('presentations','../tecnologia 1.pptx/slide1.png')
+        # THEN: return a 404
+        self.router.send_response.assert_called_once_with(404)
+
+    def serve_thumbnail_with_valid_params_test(self):
+        """
+        Test the serve_thumbnail routine with valid params
+        """
+        # GIVEN: Mocked send_header, send_response, end_headers and wfile
+        self.router.send_response = MagicMock()
+        self.router.send_header = MagicMock()
+        self.router.end_headers = MagicMock()
+        self.router.wfile = MagicMock()
+        with patch('openlp.core.lib.os.path.exists') as mocked_exists, \
+            patch('builtins.open', mock_open(read_data='123')), \
+            patch('openlp.plugins.remotes.lib.httprouter.AppLocation') as mocked_location:
+            mocked_exists.return_value = True
+            mocked_location.get_section_data_path.return_value = ''
+            # WHEN: pass good controller and filename
+            result = self.router.serve_thumbnail('presentations','another%20test/slide1.png')
+            # THEN: a file should be returned
+            self.assertEqual(len(self.router.send_header.mock_calls), 1,
+                'One header')
+            self.assertEqual(result, '123', 'The content should match \'123\'')
+            mocked_exists.assert_called_with(os.path.normpath('thumbnails/another test/slide1.png'))