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. _______________________________________________ Mailing list: https://launchpad.net/~openlp-core Post to : openlp-core@lists.launchpad.net Unsubscribe : https://launchpad.net/~openlp-core More help : https://help.launchpad.net/ListHelp