openlp-core team mailing list archive

[Tim Bentley <tim.bentley@xxxxxxxxx>]

Review: Needs Fixing

One minor nit pick

Diff comments:

> === modified file 'openlp/core/lib/serviceitem.py'
> --- openlp/core/lib/serviceitem.py	2014-09-23 21:45:42 +0000
> +++ openlp/core/lib/serviceitem.py	2014-12-05 22:44:42 +0000
> @@ -36,6 +36,7 @@
>  import logging
>  import os
>  import uuid
> +import ntpath
>  
>  from PyQt4 import QtGui
>  
> @@ -423,8 +424,12 @@
>          if 'background_audio' in header:
>              self.background_audio = []
>              for filename in header['background_audio']:
> -                # Give them real file paths
> -                self.background_audio.append(os.path.join(path, filename))
> +                # Give them real file paths.
> +                filepath = filename
> +                if path:
> +                    # Windows can handle both forward and backward slashes, so we use ntpath to get the basename
> +                    filepath = os.path.join(path, ntpath.basename(filename))
> +                self.background_audio.append(filepath)
>          self.theme_overwritten = header.get('theme_overwritten', False)
>          if self.service_item_type == ServiceItemType.Text:
>              for slide in service_item['serviceitem']['data']:
> 
> === modified file 'openlp/core/ui/slidecontroller.py'
> --- openlp/core/ui/slidecontroller.py	2014-11-13 11:21:54 +0000
> +++ openlp/core/ui/slidecontroller.py	2014-12-05 22:44:42 +0000
> @@ -141,6 +141,7 @@
>          self.slide_list = {}
>          self.slide_count = 0
>          self.slide_image = None
> +        self.controller_width = 0
>          # Layout for holding panel
>          self.panel_layout = QtGui.QVBoxLayout(self.panel)
>          self.panel_layout.setSpacing(0)
> @@ -331,9 +332,6 @@
>          self.slide_layout.setMargin(0)
>          self.slide_layout.setObjectName('SlideLayout')
>          self.preview_display = Display(self)
> -        self.preview_display.setGeometry(QtCore.QRect(0, 0, 300, 300))
> -        self.preview_display.screen = {'size': self.preview_display.geometry()}
> -        self.preview_display.setup()
>          self.slide_layout.insertWidget(0, self.preview_display)
>          self.preview_display.hide()
>          # Actual preview screen
> @@ -382,13 +380,11 @@
>              Registry().register_function('slidecontroller_live_spin_delay', self.receive_spin_delay)
>              self.toolbar.set_widget_visible(LOOP_LIST, False)
>              self.toolbar.set_widget_visible(WIDE_MENU, False)
> +            self.set_live_hot_keys(self)
> +            self.__add_actions_to_widget(self.controller)
>          else:
>              self.preview_widget.doubleClicked.connect(self.on_preview_add_to_service)
>              self.toolbar.set_widget_visible(['editSong'], False)
> -        if self.is_live:
> -            self.set_live_hot_keys(self)
> -            self.__add_actions_to_widget(self.controller)
> -        else:
>              self.controller.addActions([self.next_item, self.previous_item])
>          Registry().register_function('slidecontroller_%s_stop_loop' % self.type_prefix, self.on_stop_loop)
>          Registry().register_function('slidecontroller_%s_change' % self.type_prefix, self.on_slide_change)
> @@ -599,7 +595,10 @@
>              self.slide_preview.setFixedSize(QtCore.QSize(max_width, max_width / self.ratio))
>              self.preview_display.setFixedSize(QtCore.QSize(max_width, max_width / self.ratio))
>              self.preview_display.screen = {'size': self.preview_display.geometry()}
> -        self.on_controller_size_changed(self.controller.width())
> +        # Only update controller layout if width has actually changed
> +        if self.controller_width != self.controller.width():
> +            self.controller_width = self.controller.width()
> +            self.on_controller_size_changed(self.controller_width)
>  
>      def on_controller_size_changed(self, width):
>          """
> @@ -618,6 +617,9 @@
>              elif width < used_space - HIDE_MENU_THRESHOLD and not self.hide_menu.isVisible():
>                  self.set_blank_menu(False)
>                  self.toolbar.set_widget_visible(NARROW_MENU)
> +            # Fallback to the standard blank toolbar if the hide_menu is not visible.
> +            elif not self.hide_menu.isVisible():
> +                self.set_blank_menu()
>  
>      def set_blank_menu(self, visible=True):
>          """
> 
> === modified file 'openlp/plugins/presentations/lib/pdfcontroller.py'
> --- openlp/plugins/presentations/lib/pdfcontroller.py	2014-10-14 09:05:35 +0000
> +++ openlp/plugins/presentations/lib/pdfcontroller.py	2014-12-05 22:44:42 +0000
> @@ -31,6 +31,7 @@
>  import logging
>  from tempfile import NamedTemporaryFile
>  import re
> +import shutil

Phil has used "from shutil import which" can you use this please for consistancy

>  from subprocess import check_output, CalledProcessError, STDOUT
>  
>  from openlp.core.utils import AppLocation
> @@ -144,17 +145,10 @@
>              else:
>                  DEVNULL = open(os.devnull, 'wb')
>                  # First try to find mupdf
> -                try:
> -                    self.mudrawbin = check_output(['which', 'mudraw'],
> -                                                  stderr=DEVNULL).decode(encoding='UTF-8').rstrip('\n')
> -                except CalledProcessError:
> -                    self.mudrawbin = ''
> +                self.mudrawbin = shutil.which('mudraw')
>                  # if mupdf isn't installed, fallback to ghostscript
>                  if not self.mudrawbin:
> -                    try:
> -                        self.gsbin = check_output(['which', 'gs'], stderr=DEVNULL).decode(encoding='UTF-8').rstrip('\n')
> -                    except CalledProcessError:
> -                        self.gsbin = ''
> +                    self.gsbin = shutil.which('gs')
>                  # Last option: check if mudraw is placed in OpenLP base folder
>                  if not self.mudrawbin and not self.gsbin:
>                      application_path = AppLocation.get_directory(AppLocation.AppDir)
> 
> === modified file 'openlp/plugins/songs/lib/mediaitem.py'
> --- openlp/plugins/songs/lib/mediaitem.py	2014-11-12 22:18:57 +0000
> +++ openlp/plugins/songs/lib/mediaitem.py	2014-12-05 22:44:42 +0000
> @@ -395,6 +395,18 @@
>              new_song = self.open_lyrics.xml_to_song(song_xml)
>              new_song.title = '%s <%s>' % \
>                               (new_song.title, translate('SongsPlugin.MediaItem', 'copy', 'For song cloning'))
> +            # Copy audio files from the old to the new song
> +            if len(old_song.media_files) > 0:
> +                save_path = os.path.join(AppLocation.get_section_data_path(self.plugin.name), 'audio', str(new_song.id))
> +                check_directory_exists(save_path)
> +                for media_file in old_song.media_files:
> +                    new_media_file_name = os.path.join(save_path, os.path.basename(media_file.file_name))
> +                    shutil.copyfile(media_file.file_name, new_media_file_name)
> +                    new_media_file = MediaFile()
> +                    new_media_file.file_name = new_media_file_name
> +                    new_media_file.type = media_file.type
> +                    new_media_file.weight = media_file.weight
> +                    new_song.media_files.append(new_media_file)
>              self.plugin.manager.save_object(new_song)
>          self.on_song_list_load()
>  
> 
> === modified file 'tests/functional/openlp_core_lib/test_serviceitem.py'
> --- tests/functional/openlp_core_lib/test_serviceitem.py	2014-09-23 21:45:42 +0000
> +++ tests/functional/openlp_core_lib/test_serviceitem.py	2014-12-05 22:44:42 +0000
> @@ -271,3 +271,36 @@
>          self.assertEqual(service_item.start_time, 654.375, 'Start time should be 654.375')
>          self.assertEqual(service_item.end_time, 672.069, 'End time should be 672.069')
>          self.assertEqual(service_item.media_length, 17.694, 'Media length should be 17.694')
> +
> +    def service_item_load_song_and_audio_from_service_test(self):
> +        """
> +        Test the Service Item - adding a song slide from a saved service
> +        """
> +        # GIVEN: A new service item and a mocked add icon function
> +        service_item = ServiceItem(None)
> +        service_item.add_icon = MagicMock()
> +
> +        # WHEN: We add a custom from a saved service
> +        line = convert_file_service_item(TEST_PATH, 'service/serviceitem-song-linked-audio.osj')
> +        service_item.set_from_service(line, '/test/')
> +
> +        # THEN: We should get back a valid service item
> +        self.assertTrue(service_item.is_valid, 'The new service item should be valid')
> +        assert_length(0, service_item._display_frames, 'The service item should have no display frames')
> +        assert_length(7, service_item.capabilities, 'There should be 7 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('Amazing Grace', service_item.get_display_title(), 'The title should be "Amazing Grace"')
> +        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),
> +                         'The first line has been returned')
> +        self.assertEqual('Amazing Grace! how sweet the s', service_item.get_frame_title(0),
> +                         '"Amazing Grace! how sweet the s" has been returned as the title')
> +        self.assertEqual('’Twas grace that taught my hea', service_item.get_frame_title(1),
> +                         '"’Twas grace that taught my hea" has been returned as the title')
> +        self.assertEqual('/test/amazing_grace.mp3', service_item.background_audio[0],
> +                         '"/test/amazing_grace.mp3" should be in the background_audio list')
> 
> === added file 'tests/resources/service/serviceitem-song-linked-audio.osj'
> --- tests/resources/service/serviceitem-song-linked-audio.osj	1970-01-01 00:00:00 +0000
> +++ tests/resources/service/serviceitem-song-linked-audio.osj	2014-12-05 22:44:42 +0000
> @@ -0,0 +1,1 @@
> +[{"serviceitem": {"header": {"will_auto_start": false, "title": "Amazing Grace", "audit": ["Amazing Grace", ["John Newton"], "", ""], "processor": null, "theme_overwritten": false, "start_time": 0, "auto_play_slides_loop": false, "plugin": "songs", "auto_play_slides_once": false, "from_plugin": false, "media_length": 0, "xml_version": "<?xml version='1.0' encoding='UTF-8'?>\n<song xmlns=\"http://openlyrics.info/namespace/2009/song\"; version=\"0.8\" createdIn=\"OpenLP 2.1.0\" modifiedIn=\"OpenLP 2.1.0\" modifiedDate=\"2014-12-03T21:31:35\"><properties><titles><title>Amazing Grace</title></titles><authors><author>John Newton</author></authors></properties><lyrics><verse name=\"v1\"><lines>Amazing Grace! how sweet the sound<br/>That saved a wretch like me;<br/>I once was lost, but now am found,<br/>Was blind, but now I see.</lines></verse><verse name=\"v2\"><lines>\u2019Twas grace that taught my heart to fear,<br/>And grace my fears relieved;<br/>How precious did that grace appear,<br/>The hour I first believed!</lines></verse><verse name=\"v3\"><lines>Through many dangers, toils and snares<br/>I have already come;<br/>\u2019Tis grace that brought me safe thus far,<br/>And grace will lead me home.</lines></verse><verse name=\"v4\"><lines>The Lord has promised good to me,<br/>His word my hope secures;<br/>He will my shield and portion be<br/>As long as life endures.</lines></verse><verse name=\"v5\"><lines>Yes, when this heart and flesh shall fail,<br/>And mortal life shall cease,<br/>I shall possess within the veil<br/>A life of joy and peace.</lines></verse><verse name=\"v6\"><lines>When we\u2019ve been there a thousand years,<br/>Bright shining as the sun,<br/>We\u2019ve no less days to sing God\u2019s praise<br/>Than when we first begun.</lines></verse></lyrics></song>", "timed_slide_interval": 0, "data": {"title": "amazing grace@", "authors": "John Newton"}, "type": 1, "background_audio": ["/home/tgc/.local/share/openlp/songs/audio/7/amazing_grace.mp3"], "theme": null, "footer": ["Amazing Grace", "Written by: John Newton"], "name": "songs", "capabilities": [2, 1, 5, 8, 9, 13, 15], "end_time": 0, "notes": "", "search": "", "icon": ":/plugins/plugin_songs.png"}, "data": [{"title": "Amazing Grace! how sweet the s", "verseTag": "V1", "raw_slide": "Amazing Grace! how sweet the sound\nThat saved a wretch like me;\nI once was lost, but now am found,\nWas blind, but now I see."}, {"title": "\u2019Twas grace that taught my hea", "verseTag": "V2", "raw_slide": "\u2019Twas grace that taught my heart to fear,\nAnd grace my fears relieved;\nHow precious did that grace appear,\nThe hour I first believed!"}, {"title": "Through many dangers, toils an", "verseTag": "V3", "raw_slide": "Through many dangers, toils and snares\nI have already come;\n\u2019Tis grace that brought me safe thus far,\nAnd grace will lead me home."}, {"title": "The Lord has promised good to ", "verseTag": "V4", "raw_slide": "The Lord has promised good to me,\nHis word my hope secures;\nHe will my shield and portion be\nAs long as life endures."}, {"title": "Yes, when this heart and flesh", "verseTag": "V5", "raw_slide": "Yes, when this heart and flesh shall fail,\nAnd mortal life shall cease,\nI shall possess within the veil\nA life of joy and peace."}, {"title": "When we\u2019ve been there a thousa", "verseTag": "V6", "raw_slide": "When we\u2019ve been there a thousand years,\nBright shining as the sun,\nWe\u2019ve no less days to sing God\u2019s praise\nThan when we first begun."}]}}]
> 


-- 
https://code.launchpad.net/~tomasgroth/openlp/bugfixes9/+merge/243879
Your team OpenLP Core is subscribed to branch lp:openlp.