openlp-core team mailing list archive

[Raoul Snyman <raoul@xxxxxxxxxxx>]

Review: Needs Information

I have one question, but otherwise all good.

Diff comments:

> === modified file 'openlp/core/ui/media/vendor/mediainfoWrapper.py'
> --- openlp/core/ui/media/vendor/mediainfoWrapper.py	2016-12-31 11:01:36 +0000
> +++ openlp/core/ui/media/vendor/mediainfoWrapper.py	2017-11-16 17:32:21 +0000
> @@ -25,10 +25,10 @@
>  """
>  import json
>  import os
> -from subprocess import Popen
> +import re
> +from subprocess import Popen, check_output

Is Popen still used? I only see you using check_output.

>  from tempfile import mkstemp
>  
> -import six
>  from bs4 import BeautifulSoup, NavigableString
>  
>  ENV_DICT = os.environ
> @@ -100,20 +100,10 @@
>  
>      @staticmethod
>      def parse(filename, environment=ENV_DICT):
> -        command = ["mediainfo", "-f", "--Output=XML", filename]
> -        fileno_out, fname_out = mkstemp(suffix=".xml", prefix="media-")
> -        fileno_err, fname_err = mkstemp(suffix=".err", prefix="media-")
> -        fp_out = os.fdopen(fileno_out, 'r+b')
> -        fp_err = os.fdopen(fileno_err, 'r+b')
> -        p = Popen(command, stdout=fp_out, stderr=fp_err, env=environment)
> -        p.wait()
> -        fp_out.seek(0)
> -
> -        xml_dom = MediaInfoWrapper.parse_xml_data_into_dom(fp_out.read())
> -        fp_out.close()
> -        fp_err.close()
> -        os.unlink(fname_out)
> -        os.unlink(fname_err)
> +        xml = check_output(['mediainfo', '-f', '--Output=XML', '--Inform=OLDXML', filename])
> +        if not xml.startswith(b'<?xml'):
> +            xml = check_output(['mediainfo', '-f', '--Output=XML', filename])

Haha! I see what you did there, clever!

> +        xml_dom = MediaInfoWrapper.parse_xml_data_into_dom(xml)
>          return MediaInfoWrapper(xml_dom)
>  
>      def _populate_tracks(self):
> 
> === modified file 'tests/functional/openlp_plugins/presentations/test_mediaitem.py'
> --- tests/functional/openlp_plugins/presentations/test_mediaitem.py	2017-10-07 07:05:07 +0000
> +++ tests/functional/openlp_plugins/presentations/test_mediaitem.py	2017-11-16 17:32:21 +0000
> @@ -133,3 +133,27 @@
>  
>          # THEN: doc.presentation_deleted should have been called since the presentation file did not exists.
>          mocked_doc.assert_has_calls([call.get_thumbnail_path(1, True), call.presentation_deleted()], True)
> +
> +    @patch('openlp.plugins.presentations.lib.mediaitem.MediaManagerItem._setup')
> +    @patch('openlp.plugins.presentations.lib.mediaitem.PresentationMediaItem.setup_item')
> +    @patch('openlp.plugins.presentations.lib.mediaitem.Settings')
> +    def test_search(self, mocked_settings, *unreferenced_mocks):

*unreferenced_mocks - another clever. I need to remember this.

> +        """
> +        Test that the search method finds the correct results
> +        """
> +        # GIVEN: A mocked Settings class which returns a list of Path objects,
> +        #        and an instance of the PresentationMediaItem
> +        path_1 = Path('some_dir', 'Impress_file_1')
> +        path_2 = Path('some_other_dir', 'impress_file_2')
> +        path_3 = Path('another_dir', 'ppt_file')
> +        mocked_returned_settings = MagicMock()
> +        mocked_returned_settings.value.return_value = [path_1, path_2, path_3]
> +        mocked_settings.return_value = mocked_returned_settings
> +        media_item = PresentationMediaItem(None, MagicMock(), None)
> +        media_item.settings_section = ''
> +
> +        # WHEN: Calling search
> +        results = media_item.search('IMPRE', False)
> +
> +        # THEN: The first two results should have been returned
> +        assert results == [[str(path_1), 'Impress_file_1'], [str(path_2), 'impress_file_2']]


-- 
https://code.launchpad.net/~phill-ridout/openlp/fixes-mkIII/+merge/333834
Your team OpenLP Core is subscribed to branch lp:openlp.