From 9cb2b2e3c2774a609a31a31efc503d5b628998f7 Mon Sep 17 00:00:00 2001 From: Philip Ridout Date: Tue, 5 Sep 2017 21:48:55 +0100 Subject: [PATCH 01/12] Pathlib changes --- openlp/core/__init__.py | 42 +++-- openlp/core/common/httputils.py | 46 +++--- openlp/core/lib/shutil.py | 97 +++++++++++ openlp/core/ui/firsttimeform.py | 6 +- openlp/plugins/remotes/deploy.py | 2 +- .../openlp_core_common/test_httputils.py | 3 +- .../functional/openlp_core_lib/test_shutil.py | 151 ++++++++++++++++++ 7 files changed, 294 insertions(+), 53 deletions(-) create mode 100755 openlp/core/lib/shutil.py create mode 100755 tests/functional/openlp_core_lib/test_shutil.py diff --git a/openlp/core/__init__.py b/openlp/core/__init__.py index 88b3dbfb7..0fcea2d1a 100644 --- a/openlp/core/__init__.py +++ b/openlp/core/__init__.py @@ -29,8 +29,6 @@ logging and a plugin framework are contained within the openlp.core module. import argparse import logging -import os -import shutil import sys import time from datetime import datetime @@ -43,6 +41,7 @@ from openlp.core.common import Registry, OpenLPMixin, AppLocation, LanguageManag from openlp.core.common.path import Path from openlp.core.common.versionchecker import VersionThread, get_application_version from openlp.core.lib import ScreenList +from openlp.core.lib.shutil import copytree from openlp.core.resources import qInitResources from openlp.core.ui import SplashScreen from openlp.core.ui.exceptionform import ExceptionForm @@ -181,25 +180,20 @@ class OpenLP(OpenLPMixin, QtWidgets.QApplication): """ Check if the data folder path exists. """ - data_folder_path = str(AppLocation.get_data_path()) - if not os.path.exists(data_folder_path): - log.critical('Database was not found in: ' + data_folder_path) - status = QtWidgets.QMessageBox.critical(None, translate('OpenLP', 'Data Directory Error'), - translate('OpenLP', 'OpenLP data folder was not found in:\n\n{path}' - '\n\nThe location of the data folder was ' - 'previously changed from the OpenLP\'s ' - 'default location. If the data was stored on ' - 'removable device, that device needs to be ' - 'made available.\n\nYou may reset the data ' - 'location back to the default location, ' - 'or you can try to make the current location ' - 'available.\n\nDo you want to reset to the ' - 'default data location? If not, OpenLP will be ' - 'closed so you can try to fix the the problem.') - .format(path=data_folder_path), - QtWidgets.QMessageBox.StandardButtons(QtWidgets.QMessageBox.Yes | - QtWidgets.QMessageBox.No), - QtWidgets.QMessageBox.No) + data_folder_path = AppLocation.get_data_path() + if not data_folder_path.exists(): + log.critical('Database was not found in: %s', data_folder_path) + status = QtWidgets.QMessageBox.critical( + None, translate('OpenLP', 'Data Directory Error'), + translate('OpenLP', 'OpenLP data folder was not found in:\n\n{path}\n\nThe location of the data folder ' + 'was previously changed from the OpenLP\'s default location. If the data was ' + 'stored on removable device, that device needs to be made available.\n\nYou may ' + 'reset the data location back to the default location, or you can try to make the ' + 'current location available.\n\nDo you want to reset to the default data location? ' + 'If not, OpenLP will be closed so you can try to fix the the problem.') + .format(path=data_folder_path), + QtWidgets.QMessageBox.StandardButtons(QtWidgets.QMessageBox.Yes | QtWidgets.QMessageBox.No), + QtWidgets.QMessageBox.No) if status == QtWidgets.QMessageBox.No: # If answer was "No", return "True", it will shutdown OpenLP in def main log.info('User requested termination') @@ -253,11 +247,11 @@ class OpenLP(OpenLPMixin, QtWidgets.QApplication): 'a backup of the old data folder?'), defaultButton=QtWidgets.QMessageBox.Yes) == QtWidgets.QMessageBox.Yes: # Create copy of data folder - data_folder_path = str(AppLocation.get_data_path()) + data_folder_path = AppLocation.get_data_path() timestamp = time.strftime("%Y%m%d-%H%M%S") - data_folder_backup_path = data_folder_path + '-' + timestamp + data_folder_backup_path = data_folder_path.with_name(data_folder_path.name + '-' + timestamp) try: - shutil.copytree(data_folder_path, data_folder_backup_path) + copytree(data_folder_path, data_folder_backup_path) except OSError: QtWidgets.QMessageBox.warning(None, translate('OpenLP', 'Backup'), translate('OpenLP', 'Backup of the data folder failed!')) diff --git a/openlp/core/common/httputils.py b/openlp/core/common/httputils.py index b0c9c1b2f..9f95f4924 100644 --- a/openlp/core/common/httputils.py +++ b/openlp/core/common/httputils.py @@ -211,38 +211,32 @@ def url_get_file(callback, url, f_path, sha256=None): :param callback: the class which needs to be updated :param url: URL to download - :param f_path: Destination file + :param openlp.core.common.path.Path f_path: Destination file :param sha256: The check sum value to be checked against the download value """ block_count = 0 block_size = 4096 retries = 0 log.debug("url_get_file: " + url) + if sha256: + hasher = hashlib.sha256() while True: try: - filename = open(f_path, "wb") - url_file = urllib.request.urlopen(url, timeout=CONNECTION_TIMEOUT) - if sha256: - hasher = hashlib.sha256() - # Download until finished or canceled. - while not callback.was_cancelled: - data = url_file.read(block_size) - if not data: - break - filename.write(data) - if sha256: - hasher.update(data) - block_count += 1 - callback._download_progress(block_count, block_size) - filename.close() - if sha256 and hasher.hexdigest() != sha256: - log.error('sha256 sums did not match for file: {file}'.format(file=f_path)) - os.remove(f_path) - return False - except (urllib.error.URLError, socket.timeout) as err: + with f_path.open('wb') as file: + url_file = urllib.request.urlopen(url, timeout=CONNECTION_TIMEOUT) + # Download until finished or canceled. + while not callback.was_cancelled: + data = url_file.read(block_size) + if not data: + break + file.write(data) + if sha256: + hasher.update(data) + block_count += 1 + callback._download_progress(block_count, block_size) + except (urllib.error.URLError, socket.timeout): trace_error_handler(log) - filename.close() - os.remove(f_path) + f_path.unlink() if retries > CONNECTION_RETRIES: return False else: @@ -251,8 +245,12 @@ def url_get_file(callback, url, f_path, sha256=None): continue break # Delete file if cancelled, it may be a partial file. + if sha256 and hasher.hexdigest() != sha256: + log.error('sha256 sums did not match for file: {file}'.format(file=f_path)) + f_path.unlink() + return False if callback.was_cancelled: - os.remove(f_path) + f_path.unlink() return True diff --git a/openlp/core/lib/shutil.py b/openlp/core/lib/shutil.py new file mode 100755 index 000000000..d2f6ae34d --- /dev/null +++ b/openlp/core/lib/shutil.py @@ -0,0 +1,97 @@ +# -*- coding: utf-8 -*- +# vim: autoindent shiftwidth=4 expandtab textwidth=120 tabstop=4 softtabstop=4 + +############################################################################### +# OpenLP - Open Source Lyrics Projection # +# --------------------------------------------------------------------------- # +# Copyright (c) 2008-2017 OpenLP Developers # +# --------------------------------------------------------------------------- # +# This program is free software; you can redistribute it and/or modify it # +# under the terms of the GNU General Public License as published by the Free # +# Software Foundation; version 2 of the License. # +# # +# This program is distributed in the hope that it will be useful, but WITHOUT # +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or # +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for # +# more details. # +# # +# You should have received a copy of the GNU General Public License along # +# with this program; if not, write to the Free Software Foundation, Inc., 59 # +# Temple Place, Suite 330, Boston, MA 02111-1307 USA # +############################################################################### +""" Patch the shutil methods we use so they accept and return Path objects""" +import shutil + +from openlp.core.common.path import path_to_str, str_to_path +from openlp.core.lib import replace_params + + +def copy(*args, **kwargs): + """ + Wraps :func:`shutil.copy` so that we can accept Path objects. + + :param src openlp.core.common.path.Path: Takes a Path object which is then converted to a str object + :param dst openlp.core.common.path.Path: Takes a Path object which is then converted to a str object + :return: Converts the str object received from :func:`shutil.copy` to a Path or NoneType object + :rtype: openlp.core.common.path.Path | None + + See the following link for more information on the other parameters: + https://docs.python.org/3/library/shutil.html#shutil.copy + """ + + args, kwargs = replace_params(args, kwargs, ((0, 'src', path_to_str), (1, 'dst', path_to_str))) + + return str_to_path(shutil.copy(*args, **kwargs)) + + +def copyfile(*args, **kwargs): + """ + Wraps :func:`shutil.copyfile` so that we can accept Path objects. + + :param openlp.core.common.path.Path src: Takes a Path object which is then converted to a str object + :param openlp.core.common.path.Path dst: Takes a Path object which is then converted to a str object + :return: Converts the str object received from :func:`shutil.copyfile` to a Path or NoneType object + :rtype: openlp.core.common.path.Path | None + + See the following link for more information on the other parameters: + https://docs.python.org/3/library/shutil.html#shutil.copyfile + """ + + args, kwargs = replace_params(args, kwargs, ((0, 'src', path_to_str), (1, 'dst', path_to_str))) + + return str_to_path(shutil.copyfile(*args, **kwargs)) + + +def copytree(*args, **kwargs): + """ + Wraps :func:shutil.copytree` so that we can accept Path objects. + + :param openlp.core.common.path.Path src : Takes a Path object which is then converted to a str object + :param openlp.core.common.path.Path dst: Takes a Path object which is then converted to a str object + :return: Converts the str object received from :func:`shutil.copytree` to a Path or NoneType object + :rtype: openlp.core.common.path.Path | None + + See the following link for more information on the other parameters: + https://docs.python.org/3/library/shutil.html#shutil.copytree + """ + + args, kwargs = replace_params(args, kwargs, ((0, 'src', path_to_str), (1, 'dst', path_to_str))) + + return str_to_path(shutil.copytree(*args, **kwargs)) + + +def rmtree(*args, **kwargs): + """ + Wraps :func:shutil.rmtree` so that we can accept Path objects. + + :param openlp.core.common.path.Path path: Takes a Path object which is then converted to a str object + :return: Passes the return from :func:`shutil.rmtree` back + :rtype: None + + See the following link for more information on the other parameters: + https://docs.python.org/3/library/shutil.html#shutil.rmtree + """ + + args, kwargs = replace_params(args, kwargs, ((0, 'path', path_to_str),)) + + return shutil.rmtree(*args, **kwargs) diff --git a/openlp/core/ui/firsttimeform.py b/openlp/core/ui/firsttimeform.py index 1ae923467..133d7d65b 100644 --- a/openlp/core/ui/firsttimeform.py +++ b/openlp/core/ui/firsttimeform.py @@ -563,7 +563,7 @@ class FirstTimeForm(QtWidgets.QWizard, UiFirstTimeWizard, RegistryProperties): filename, sha256 = item.data(QtCore.Qt.UserRole) self._increment_progress_bar(self.downloading.format(name=filename), 0) self.previous_size = 0 - destination = os.path.join(songs_destination, str(filename)) + destination = Path(songs_destination, str(filename)) if not url_get_file(self, '{path}{name}'.format(path=self.songs_url, name=filename), destination, sha256): missed_files.append('Song: {name}'.format(name=filename)) @@ -576,7 +576,7 @@ class FirstTimeForm(QtWidgets.QWizard, UiFirstTimeWizard, RegistryProperties): self._increment_progress_bar(self.downloading.format(name=bible), 0) self.previous_size = 0 if not url_get_file(self, '{path}{name}'.format(path=self.bibles_url, name=bible), - os.path.join(bibles_destination, bible), + Path(bibles_destination, bible), sha256): missed_files.append('Bible: {name}'.format(name=bible)) bibles_iterator += 1 @@ -588,7 +588,7 @@ class FirstTimeForm(QtWidgets.QWizard, UiFirstTimeWizard, RegistryProperties): self._increment_progress_bar(self.downloading.format(name=theme), 0) self.previous_size = 0 if not url_get_file(self, '{path}{name}'.format(path=self.themes_url, name=theme), - os.path.join(themes_destination, theme), + Path(themes_destination, theme), sha256): missed_files.append('Theme: {name}'.format(name=theme)) if missed_files: diff --git a/openlp/plugins/remotes/deploy.py b/openlp/plugins/remotes/deploy.py index d971499f0..8706bc011 100644 --- a/openlp/plugins/remotes/deploy.py +++ b/openlp/plugins/remotes/deploy.py @@ -64,6 +64,6 @@ def download_and_check(callback=None): file_size = get_url_file_size('https://get.openlp.org/webclient/site.zip') callback.setRange(0, file_size) if url_get_file(callback, '{host}{name}'.format(host='https://get.openlp.org/webclient/', name='site.zip'), - os.path.join(str(AppLocation.get_section_data_path('remotes')), 'site.zip'), + AppLocation.get_section_data_path('remotes') / 'site.zip', sha256=sha256): deploy_zipfile(str(AppLocation.get_section_data_path('remotes')), 'site.zip') diff --git a/tests/functional/openlp_core_common/test_httputils.py b/tests/functional/openlp_core_common/test_httputils.py index 26ac297dd..03dde026c 100644 --- a/tests/functional/openlp_core_common/test_httputils.py +++ b/tests/functional/openlp_core_common/test_httputils.py @@ -29,6 +29,7 @@ from unittest import TestCase from unittest.mock import MagicMock, patch from openlp.core.common.httputils import get_user_agent, get_web_page, get_url_file_size, url_get_file, ping +from openlp.core.common.path import Path from tests.helpers.testmixin import TestMixin @@ -267,7 +268,7 @@ class TestHttpUtils(TestCase, TestMixin): mocked_urlopen.side_effect = socket.timeout() # WHEN: Attempt to retrieve a file - url_get_file(MagicMock(), url='http://localhost/test', f_path=self.tempfile) + url_get_file(MagicMock(), url='http://localhost/test', f_path=Path(self.tempfile)) # THEN: socket.timeout should have been caught # NOTE: Test is if $tmpdir/tempfile is still there, then test fails since ftw deletes bad downloaded files diff --git a/tests/functional/openlp_core_lib/test_shutil.py b/tests/functional/openlp_core_lib/test_shutil.py new file mode 100755 index 000000000..f502e403b --- /dev/null +++ b/tests/functional/openlp_core_lib/test_shutil.py @@ -0,0 +1,151 @@ +import os +from unittest import TestCase +from unittest.mock import ANY, MagicMock, patch + +from openlp.core.common.path import Path +from openlp.core.lib import shutilpatches + + +class TestShutilPatches(TestCase): + """ + Tests for the :mod:`openlp.core.lib.shutil` module + """ + + def test_pcopy(self): + """ + Test :func:`copy` + """ + # GIVEN: A mocked `shutil.copy` which returns a test path as a string + with patch('openlp.core.lib.shutil.shutil.copy', return_value=os.path.join('destination', 'test', 'path')) \ + as mocked_shutil_copy: + + # WHEN: Calling shutilpatches.copy with the src and dst parameters as Path object types + result = shutilpatches.copy(Path('source', 'test', 'path'), Path('destination', 'test', 'path')) + + # THEN: `shutil.copy` should have been called with the str equivalents of the Path objects. + # `shutilpatches.copy` should return the str type result of calling `shutil.copy` as a Path + # object. + mocked_shutil_copy.assert_called_once_with(os.path.join('source', 'test', 'path'), + os.path.join('destination', 'test', 'path')) + self.assertEqual(result, Path('destination', 'test', 'path')) + + def test_pcopy_follow_optional_params(self): + """ + Test :func:`copy` when follow_symlinks is set to false + """ + # GIVEN: A mocked `shutil.copy` + with patch('openlp.core.lib.shutil.shutil.copy', return_value='') as mocked_shutil_copy: + + # WHEN: Calling shutilpatches.copy with `follow_symlinks` set to False + shutilpatches.copy(Path('source', 'test', 'path'), + Path('destination', 'test', 'path'), + follow_symlinks=False) + + # THEN: `shutil.copy` should have been called with follow_symlinks is set to false + mocked_shutil_copy.assert_called_once_with(ANY, ANY, follow_symlinks=False) + + def test_pcopyfile(self): + """ + Test :func:`copyfile` + """ + # GIVEN: A mocked `shutil.copyfile` which returns a test path as a string + with patch('openlp.core.lib.shutil.shutil.copyfile', return_value=os.path.join('destination', 'test', 'path')) \ + as mocked_shutil_copyfile: + + # WHEN: Calling shutilpatches.copyfile with the src and dst parameters as Path object types + result = shutilpatches.copyfile(Path('source', 'test', 'path'), Path('destination', 'test', 'path')) + + # THEN: `shutil.copyfile` should have been called with the str equivalents of the Path objects. + # `shutilpatches.copyfile` should return the str type result of calling `shutil.copyfile` as a Path + # object. + mocked_shutil_copyfile.assert_called_once_with(os.path.join('source', 'test', 'path'), + os.path.join('destination', 'test', 'path')) + self.assertEqual(result, Path('destination', 'test', 'path')) + + def test_pcopyfile_optional_params(self): + """ + Test :func:`copyfile` when follow_symlinks is set to false + """ + # GIVEN: A mocked `shutil.copyfile` + with patch('openlp.core.lib.shutil.shutil.copyfile', return_value='') as mocked_shutil_copyfile: + + # WHEN: Calling shutilpatches.copyfile with `follow_symlinks` set to False + shutilpatches.copyfile(Path('source', 'test', 'path'), + Path('destination', 'test', 'path'), + follow_symlinks=False) + + # THEN: `shutil.copyfile` should have been called with the optional parameters, with out any of the values + # being modified + mocked_shutil_copyfile.assert_called_once_with(ANY, ANY, follow_symlinks=False) + + def test_pcopytree(self): + """ + Test :func:`copytree` + """ + # GIVEN: A mocked `shutil.copytree` which returns a test path as a string + with patch('openlp.core.lib.shutil.shutil.copytree', return_value=os.path.join('destination', 'test', 'path')) \ + as mocked_shutil_copytree: + + # WHEN: Calling shutilpatches.copytree with the src and dst parameters as Path object types + result = shutilpatches.copytree(Path('source', 'test', 'path'), Path('destination', 'test', 'path')) + + # THEN: `shutil.copytree` should have been called with the str equivalents of the Path objects. + # `shutilpatches.copytree` should return the str type result of calling `shutil.copytree` as a Path + # object. + mocked_shutil_copytree.assert_called_once_with(os.path.join('source', 'test', 'path'), + os.path.join('destination', 'test', 'path')) + self.assertEqual(result, Path('destination', 'test', 'path')) + + def test_pcopytree_optional_params(self): + """ + Test :func:`copytree` when optional parameters are passed + """ + # GIVEN: A mocked `shutil.copytree` + with patch('openlp.core.lib.shutil.shutil.copytree', return_value='') as mocked_shutil_copytree: + mocked_ignore = MagicMock() + mocked_copy_function = MagicMock() + + # WHEN: Calling shutilpatches.copytree with the optional parameters set + shutilpatches.copytree(Path('source', 'test', 'path'), + Path('destination', 'test', 'path'), + symlinks=True, + ignore=mocked_ignore, + copy_function=mocked_copy_function, + ignore_dangling_symlinks=True) + + # THEN: `shutil.copytree` should have been called with the optional parameters, with out any of the values + # being modified + mocked_shutil_copytree.assert_called_once_with(ANY, ANY, + symlinks=True, + ignore=mocked_ignore, + copy_function=mocked_copy_function, + ignore_dangling_symlinks=True) + + def test_prmtree(self): + """ + Test :func:`rmtree` + """ + # GIVEN: A mocked `shutil.rmtree` + with patch('openlp.core.lib.shutil.shutil.rmtree', return_value=None) as mocked_rmtree: + + # WHEN: Calling shutilpatches.rmtree with the path parameter as Path object type + result = shutilpatches.rmtree(Path('test', 'path')) + + # THEN: `shutil.rmtree` should have been called with the str equivalents of the Path object. + mocked_rmtree.assert_called_once_with(os.path.join('test', 'path')) + self.assertIsNone(result) + + def test_prmtree_optional_params(self): + """ + Test :func:`rmtree` when optional parameters are passed + """ + # GIVEN: A mocked `shutil.rmtree` + with patch('openlp.core.lib.shutil.shutil.rmtree', return_value='') as mocked_shutil_rmtree: + mocked_on_error = MagicMock() + + # WHEN: Calling shutilpatches.rmtree with `ignore_errors` set to True and `onerror` set to a mocked object + shutilpatches.rmtree(Path('test', 'path'), ignore_errors=True, onerror=mocked_on_error) + + # THEN: `shutil.rmtree` should have been called with the optional parameters, with out any of the values + # being modified + mocked_shutil_rmtree.assert_called_once_with(ANY, ignore_errors=True, onerror=mocked_on_error) From 292861907ef944e14bf620c52997b118cf9c056f Mon Sep 17 00:00:00 2001 From: Philip Ridout Date: Wed, 6 Sep 2017 21:18:08 +0100 Subject: [PATCH 02/12] minor edits --- openlp/core/common/applocation.py | 1 - openlp/core/common/httputils.py | 4 ++-- openlp/core/common/languagemanager.py | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/openlp/core/common/applocation.py b/openlp/core/common/applocation.py index 87ef7e6c1..02a872303 100644 --- a/openlp/core/common/applocation.py +++ b/openlp/core/common/applocation.py @@ -29,7 +29,6 @@ import sys from openlp.core.common import Settings, is_win, is_macosx from openlp.core.common.path import Path - if not is_win() and not is_macosx(): try: from xdg import BaseDirectory diff --git a/openlp/core/common/httputils.py b/openlp/core/common/httputils.py index 9f95f4924..90e128063 100644 --- a/openlp/core/common/httputils.py +++ b/openlp/core/common/httputils.py @@ -218,10 +218,10 @@ def url_get_file(callback, url, f_path, sha256=None): block_size = 4096 retries = 0 log.debug("url_get_file: " + url) - if sha256: - hasher = hashlib.sha256() while True: try: + if sha256: + hasher = hashlib.sha256() with f_path.open('wb') as file: url_file = urllib.request.urlopen(url, timeout=CONNECTION_TIMEOUT) # Download until finished or canceled. diff --git a/openlp/core/common/languagemanager.py b/openlp/core/common/languagemanager.py index 35b195031..40e4930fb 100644 --- a/openlp/core/common/languagemanager.py +++ b/openlp/core/common/languagemanager.py @@ -141,7 +141,7 @@ class LanguageManager(object): if reg_ex.exactMatch(qmf): name = '{regex}'.format(regex=reg_ex.cap(1)) LanguageManager.__qm_list__[ - '{count:>2i} {name}'.format(count=counter + 1, name=LanguageManager.language_name(qmf))] = name + '{count:>2d} {name}'.format(count=counter + 1, name=LanguageManager.language_name(qmf))] = name @staticmethod def get_qm_list(): From 24358337e7c3e25d378828e302fc61da1e92fbae Mon Sep 17 00:00:00 2001 From: Philip Ridout Date: Wed, 6 Sep 2017 22:36:31 +0100 Subject: [PATCH 03/12] Pathlib changes --- openlp/core/common/registry.py | 5 +- openlp/core/common/uistrings.py | 3 -- openlp/core/ui/servicemanager.py | 37 ++++++------- openlp/plugins/songs/reporting.py | 88 ++++++++++++++----------------- 4 files changed, 59 insertions(+), 74 deletions(-) diff --git a/openlp/core/common/registry.py b/openlp/core/common/registry.py index 95f1ce721..33afe6f21 100644 --- a/openlp/core/common/registry.py +++ b/openlp/core/common/registry.py @@ -138,12 +138,9 @@ class Registry(object): if result: results.append(result) except TypeError: - # Who has called me can help in debugging - trace_error_handler(log) log.exception('Exception for function {function}'.format(function=function)) else: - trace_error_handler(log) - log.error("Event {event} called but not registered".format(event=event)) + log.exception('Event {event} called but not registered'.format(event=event)) return results def get_flag(self, key): diff --git a/openlp/core/common/uistrings.py b/openlp/core/common/uistrings.py index 02937351d..9dd24a866 100644 --- a/openlp/core/common/uistrings.py +++ b/openlp/core/common/uistrings.py @@ -88,9 +88,6 @@ class UiStrings(object): self.Error = translate('OpenLP.Ui', 'Error') self.Export = translate('OpenLP.Ui', 'Export') self.File = translate('OpenLP.Ui', 'File') - self.FileNotFound = translate('OpenLP.Ui', 'File Not Found') - self.FileNotFoundMessage = translate('OpenLP.Ui', - 'File {name} not found.\nPlease try selecting it individually.') self.FontSizePtUnit = translate('OpenLP.Ui', 'pt', 'Abbreviated font pointsize unit') self.Help = translate('OpenLP.Ui', 'Help') self.Hours = translate('OpenLP.Ui', 'h', 'The abbreviated unit for hours') diff --git a/openlp/core/ui/servicemanager.py b/openlp/core/ui/servicemanager.py index 5051e43c1..eb279f267 100644 --- a/openlp/core/ui/servicemanager.py +++ b/openlp/core/ui/servicemanager.py @@ -366,16 +366,17 @@ class ServiceManager(OpenLPMixin, RegistryMixin, QtWidgets.QWidget, Ui_ServiceMa """ return self._modified - def set_file_name(self, file_name): + def set_file_name(self, file_path): """ Setter for service file. - :param file_name: The service file name + :param openlp.core.common.path.Path file_path: The service file name + :rtype: None """ - self._file_name = str(file_name) + self._file_name = path_to_str(file_path) self.main_window.set_service_modified(self.is_modified(), self.short_file_name()) - Settings().setValue('servicemanager/last file', Path(file_name)) - self._save_lite = self._file_name.endswith('.oszl') + Settings().setValue('servicemanager/last file', file_path) + self._save_lite = file_path.suffix() == '.oszl' def file_name(self): """ @@ -474,7 +475,7 @@ class ServiceManager(OpenLPMixin, RegistryMixin, QtWidgets.QWidget, Ui_ServiceMa """ self.service_manager_list.clear() self.service_items = [] - self.set_file_name('') + self.set_file_name(None) self.service_id += 1 self.set_modified(False) Settings().setValue('servicemanager/last file', None) @@ -695,27 +696,23 @@ class ServiceManager(OpenLPMixin, RegistryMixin, QtWidgets.QWidget, Ui_ServiceMa default_file_name = format_time(default_pattern, local_time) else: default_file_name = '' - directory = path_to_str(Settings().value(self.main_window.service_manager_settings_section + '/last directory')) - path = os.path.join(directory, default_file_name) + directory_path = Settings().value(self.main_window.service_manager_settings_section + '/last directory') + file_path = directory_path / default_file_name # SaveAs from osz to oszl is not valid as the files will be deleted on exit which is not sensible or usable in # the long term. if self._file_name.endswith('oszl') or self.service_has_all_original_files: - file_name, filter_used = QtWidgets.QFileDialog.getSaveFileName( - self.main_window, UiStrings().SaveService, path, + file_path, filter_used = FileDialog.getSaveFileName( + self.main_window, UiStrings().SaveService, file_path, translate('OpenLP.ServiceManager', 'OpenLP Service Files (*.osz);; OpenLP Service Files - lite (*.oszl)')) else: - file_name, filter_used = QtWidgets.QFileDialog.getSaveFileName( - self.main_window, UiStrings().SaveService, path, + file_path, filter_used = FileDialog.getSaveFileName( + self.main_window, UiStrings().SaveService, file_path, translate('OpenLP.ServiceManager', 'OpenLP Service Files (*.osz);;')) - if not file_name: + if not file_path: return False - if os.path.splitext(file_name)[1] == '': - file_name += '.osz' - else: - ext = os.path.splitext(file_name)[1] - file_name.replace(ext, '.osz') - self.set_file_name(file_name) + file_path.with_suffix('.osz') + self.set_file_name(file_path) self.decide_save_method() def decide_save_method(self, field=None): @@ -772,7 +769,7 @@ class ServiceManager(OpenLPMixin, RegistryMixin, QtWidgets.QWidget, Ui_ServiceMa return file_to.close() self.new_file() - self.set_file_name(file_name) + self.set_file_name(str_to_path(file_name)) self.main_window.display_progress_bar(len(items)) self.process_service_items(items) delete_file(Path(p_file)) diff --git a/openlp/plugins/songs/reporting.py b/openlp/plugins/songs/reporting.py index fc1a5f3f5..b50cd0a0c 100644 --- a/openlp/plugins/songs/reporting.py +++ b/openlp/plugins/songs/reporting.py @@ -25,10 +25,10 @@ The :mod:`db` module provides the ability to provide a csv file of all songs import csv import logging -from PyQt5 import QtWidgets - from openlp.core.common import Registry, translate +from openlp.core.common.path import Path from openlp.core.lib.ui import critical_error_message_box +from openlp.core.ui.lib.filedialog import FileDialog from openlp.plugins.songs.lib.db import Song @@ -42,58 +42,55 @@ def report_song_list(): """ main_window = Registry().get('main_window') plugin = Registry().get('songs').plugin - report_file_name, filter_used = QtWidgets.QFileDialog.getSaveFileName( + report_file_path, filter_used = FileDialog.getSaveFileName( main_window, translate('SongPlugin.ReportSongList', 'Save File'), - translate('SongPlugin.ReportSongList', 'song_extract.csv'), + Path(translate('SongPlugin.ReportSongList', 'song_extract.csv')), translate('SongPlugin.ReportSongList', 'CSV format (*.csv)')) - if not report_file_name: + if not report_file_path: main_window.error_message( translate('SongPlugin.ReportSongList', 'Output Path Not Selected'), - translate('SongPlugin.ReportSongList', 'You have not set a valid output location for your ' - 'report. \nPlease select an existing path ' - 'on your computer.') + translate('SongPlugin.ReportSongList', 'You have not set a valid output location for your report. \n' + 'Please select an existing path on your computer.') ) return - if not report_file_name.endswith('csv'): - report_file_name += '.csv' - file_handle = None + report_file_path.with_suffix('.csv') Registry().get('application').set_busy_cursor() try: - file_handle = open(report_file_name, 'wt') - fieldnames = ('Title', 'Alternative Title', 'Copyright', 'Author(s)', 'Song Book', 'Topic') - writer = csv.DictWriter(file_handle, fieldnames=fieldnames, quoting=csv.QUOTE_ALL) - headers = dict((n, n) for n in fieldnames) - writer.writerow(headers) - song_list = plugin.manager.get_all_objects(Song) - for song in song_list: - author_list = [] - for author_song in song.authors_songs: - author_list.append(author_song.author.display_name) - author_string = ' | '.join(author_list) - book_list = [] - for book_song in song.songbook_entries: - if hasattr(book_song, 'entry') and book_song.entry: - book_list.append('{name} #{entry}'.format(name=book_song.songbook.name, entry=book_song.entry)) - book_string = ' | '.join(book_list) - topic_list = [] - for topic_song in song.topics: - if hasattr(topic_song, 'name'): - topic_list.append(topic_song.name) - topic_string = ' | '.join(topic_list) - writer.writerow({'Title': song.title, - 'Alternative Title': song.alternate_title, - 'Copyright': song.copyright, - 'Author(s)': author_string, - 'Song Book': book_string, - 'Topic': topic_string}) - Registry().get('application').set_normal_cursor() - main_window.information_message( - translate('SongPlugin.ReportSongList', 'Report Creation'), - translate('SongPlugin.ReportSongList', - 'Report \n{name} \nhas been successfully created. ').format(name=report_file_name) - ) + with report_file_path.open('wt') as file_handle: + fieldnames = ('Title', 'Alternative Title', 'Copyright', 'Author(s)', 'Song Book', 'Topic') + writer = csv.DictWriter(file_handle, fieldnames=fieldnames, quoting=csv.QUOTE_ALL) + headers = dict((n, n) for n in fieldnames) + writer.writerow(headers) + song_list = plugin.manager.get_all_objects(Song) + for song in song_list: + author_list = [] + for author_song in song.authors_songs: + author_list.append(author_song.author.display_name) + author_string = ' | '.join(author_list) + book_list = [] + for book_song in song.songbook_entries: + if hasattr(book_song, 'entry') and book_song.entry: + book_list.append('{name} #{entry}'.format(name=book_song.songbook.name, entry=book_song.entry)) + book_string = ' | '.join(book_list) + topic_list = [] + for topic_song in song.topics: + if hasattr(topic_song, 'name'): + topic_list.append(topic_song.name) + topic_string = ' | '.join(topic_list) + writer.writerow({'Title': song.title, + 'Alternative Title': song.alternate_title, + 'Copyright': song.copyright, + 'Author(s)': author_string, + 'Song Book': book_string, + 'Topic': topic_string}) + Registry().get('application').set_normal_cursor() + main_window.information_message( + translate('SongPlugin.ReportSongList', 'Report Creation'), + translate('SongPlugin.ReportSongList', + 'Report \n{name} \nhas been successfully created. ').format(name=report_file_path) + ) except OSError as ose: Registry().get('application').set_normal_cursor() log.exception('Failed to write out song usage records') @@ -101,6 +98,3 @@ def report_song_list(): translate('SongPlugin.ReportSongList', 'An error occurred while extracting: {error}' ).format(error=ose.strerror)) - finally: - if file_handle: - file_handle.close() From f0e7381f5c107830f4d5a4164ca53206edb9436a Mon Sep 17 00:00:00 2001 From: Philip Ridout Date: Thu, 7 Sep 2017 22:52:39 +0100 Subject: [PATCH 04/12] Pathlib changes in presentation plugin --- openlp/core/ui/lib/wizard.py | 2 +- openlp/core/ui/mainwindow.py | 19 +++---- .../presentations/lib/impresscontroller.py | 15 +++--- openlp/plugins/presentations/lib/mediaitem.py | 14 +++--- .../presentations/lib/pdfcontroller.py | 34 ++++++------- .../presentations/lib/powerpointcontroller.py | 2 +- .../presentations/lib/pptviewcontroller.py | 12 ++--- .../lib/presentationcontroller.py | 50 +++++++++++-------- openlp/plugins/songs/reporting.py | 2 +- .../openlp_core_ui/test_exceptionform.py | 2 +- 10 files changed, 82 insertions(+), 70 deletions(-) diff --git a/openlp/core/ui/lib/wizard.py b/openlp/core/ui/lib/wizard.py index 8f3093fef..677949b33 100644 --- a/openlp/core/ui/lib/wizard.py +++ b/openlp/core/ui/lib/wizard.py @@ -310,7 +310,7 @@ class OpenLPWizard(QtWidgets.QWizard, RegistryProperties): """ folder_path = FileDialog.getExistingDirectory( self, title, Settings().value(self.plugin.settings_section + '/' + setting_name), - QtWidgets.QFileDialog.ShowDirsOnly) + FileDialog.ShowDirsOnly) if folder_path: editbox.setText(str(folder_path)) Settings().setValue(self.plugin.settings_section + '/' + setting_name, folder_path) diff --git a/openlp/core/ui/mainwindow.py b/openlp/core/ui/mainwindow.py index 413e97a17..88799b060 100644 --- a/openlp/core/ui/mainwindow.py +++ b/openlp/core/ui/mainwindow.py @@ -42,6 +42,7 @@ from openlp.core.common.actions import ActionList, CategoryOrder from openlp.core.common.path import Path, path_to_str, str_to_path from openlp.core.common.versionchecker import get_application_version from openlp.core.lib import Renderer, PluginManager, ImageManager, PluginStatus, ScreenList, build_icon +from openlp.core.lib.shutil import copyfile from openlp.core.lib.ui import create_action from openlp.core.ui import AboutForm, SettingsForm, ServiceManager, ThemeManager, LiveController, PluginForm, \ ShortcutListForm, FormattingTagForm, PreviewController @@ -848,12 +849,12 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): QtWidgets.QMessageBox.No) if answer == QtWidgets.QMessageBox.No: return - import_file_name, filter_used = QtWidgets.QFileDialog.getOpenFileName( + import_file_path, filter_used = FileDialog.getOpenFileName( self, translate('OpenLP.MainWindow', 'Import settings'), - '', + None, translate('OpenLP.MainWindow', 'OpenLP Settings (*.conf)')) - if not import_file_name: + if import_file_path is None: return setting_sections = [] # Add main sections. @@ -871,12 +872,12 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): # Add plugin sections. setting_sections.extend([plugin.name for plugin in self.plugin_manager.plugins]) # Copy the settings file to the tmp dir, because we do not want to change the original one. - temp_directory = os.path.join(str(gettempdir()), 'openlp') - check_directory_exists(Path(temp_directory)) - temp_config = os.path.join(temp_directory, os.path.basename(import_file_name)) - shutil.copyfile(import_file_name, temp_config) + temp_dir_path = Path(gettempdir(), 'openlp') + check_directory_exists(temp_dir_path) + temp_config_path = temp_dir_path / import_file_path.name + copyfile(import_file_path, temp_config_path) settings = Settings() - import_settings = Settings(temp_config, Settings.IniFormat) + import_settings = Settings(str(temp_config_path), Settings.IniFormat) log.info('hook upgrade_plugin_settings') self.plugin_manager.hook_upgrade_plugin_settings(import_settings) @@ -920,7 +921,7 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): settings.setValue('{key}'.format(key=section_key), value) now = datetime.now() settings.beginGroup(self.header_section) - settings.setValue('file_imported', import_file_name) + settings.setValue('file_imported', import_file_path) settings.setValue('file_date_imported', now.strftime("%Y-%m-%d %H:%M")) settings.endGroup() settings.sync() diff --git a/openlp/plugins/presentations/lib/impresscontroller.py b/openlp/plugins/presentations/lib/impresscontroller.py index 25c470f48..7699946a0 100644 --- a/openlp/plugins/presentations/lib/impresscontroller.py +++ b/openlp/plugins/presentations/lib/impresscontroller.py @@ -255,10 +255,10 @@ class ImpressDocument(PresentationDocument): if self.check_thumbnails(): return if is_win(): - thumb_dir_url = 'file:///' + self.get_temp_folder().replace('\\', '/') \ + thumb_dir_url = 'file:///' + str(self.get_temp_folder()).replace('\\', '/') \ .replace(':', '|').replace(' ', '%20') else: - thumb_dir_url = uno.systemPathToFileUrl(self.get_temp_folder()) + thumb_dir_url = uno.systemPathToFileUrl(str(self.get_temp_folder())) properties = [] properties.append(self.create_property('FilterName', 'impress_png_Export')) properties = tuple(properties) @@ -266,17 +266,18 @@ class ImpressDocument(PresentationDocument): pages = doc.getDrawPages() if not pages: return - if not os.path.isdir(self.get_temp_folder()): - os.makedirs(self.get_temp_folder()) + temp_folder_path = self.get_temp_folder() + if not temp_folder_path.isdir(): + temp_folder_path.mkdir() for index in range(pages.getCount()): page = pages.getByIndex(index) doc.getCurrentController().setCurrentPage(page) url_path = '{path}/{name}.png'.format(path=thumb_dir_url, name=str(index + 1)) - path = os.path.join(self.get_temp_folder(), str(index + 1) + '.png') + path = temp_folder_path / '{number).png'.format(number=index + 1) try: doc.storeToURL(url_path, properties) - self.convert_thumbnail(path, index + 1) - delete_file(Path(path)) + self.convert_thumbnail(str(path), index + 1) + delete_file(path) except ErrorCodeIOException as exception: log.exception('ERROR! ErrorCodeIOException {error:d}'.format(error=exception.ErrCode)) except: diff --git a/openlp/plugins/presentations/lib/mediaitem.py b/openlp/plugins/presentations/lib/mediaitem.py index 99c937eb0..275279e15 100644 --- a/openlp/plugins/presentations/lib/mediaitem.py +++ b/openlp/plugins/presentations/lib/mediaitem.py @@ -187,7 +187,7 @@ class PresentationMediaItem(MediaManagerItem): if controller_name: controller = self.controllers[controller_name] doc = controller.add_document(file) - thumb = os.path.join(doc.get_thumbnail_folder(), 'icon.png') + thumb = str(doc.get_thumbnail_folder() / 'icon.png') preview = doc.get_thumbnail_path(1, True) if not preview and not initial_load: doc.load_presentation() @@ -304,17 +304,17 @@ class PresentationMediaItem(MediaManagerItem): controller = self.controllers[processor] service_item.processor = None doc = controller.add_document(filename) - if doc.get_thumbnail_path(1, True) is None or not os.path.isfile( - os.path.join(doc.get_temp_folder(), 'mainslide001.png')): + if doc.get_thumbnail_path(1, True) is None or \ + not (doc.get_temp_folder() / 'mainslide001.png').is_file(): doc.load_presentation() i = 1 - image = os.path.join(doc.get_temp_folder(), 'mainslide{number:0>3d}.png'.format(number=i)) - thumbnail = os.path.join(doc.get_thumbnail_folder(), 'slide%d.png' % i) + image = str(doc.get_temp_folder() / 'mainslide{number:0>3d}.png'.format(number=i)) + thumbnail = str(doc.get_thumbnail_folder() / 'slide{number:d}.png'.format(number=i)) while os.path.isfile(image): service_item.add_from_image(image, name, thumbnail=thumbnail) i += 1 - image = os.path.join(doc.get_temp_folder(), 'mainslide{number:0>3d}.png'.format(number=i)) - thumbnail = os.path.join(doc.get_thumbnail_folder(), 'slide{number:d}.png'.format(number=i)) + image = str(doc.get_temp_folder() / 'mainslide{number:0>3d}.png'.format(number=i)) + thumbnail = str(doc.get_thumbnail_folder() / 'slide{number:d}.png'.format(number=i)) service_item.add_capability(ItemCapabilities.HasThumbnails) doc.close_presentation() return True diff --git a/openlp/plugins/presentations/lib/pdfcontroller.py b/openlp/plugins/presentations/lib/pdfcontroller.py index f4a091551..f80ad94ba 100644 --- a/openlp/plugins/presentations/lib/pdfcontroller.py +++ b/openlp/plugins/presentations/lib/pdfcontroller.py @@ -240,46 +240,46 @@ class PdfDocument(PresentationDocument): :return: True is loading succeeded, otherwise False. """ log.debug('load_presentation pdf') + temp_dir_path = self.get_temp_folder() # Check if the images has already been created, and if yes load them - if os.path.isfile(os.path.join(self.get_temp_folder(), 'mainslide001.png')): - created_files = sorted(os.listdir(self.get_temp_folder())) - for fn in created_files: - if os.path.isfile(os.path.join(self.get_temp_folder(), fn)): - self.image_files.append(os.path.join(self.get_temp_folder(), fn)) + if (temp_dir_path / 'mainslide001.png').is_file(): + created_files = sorted(temp_dir_path.glob('*')) + for image_path in created_files: + if image_path.is_file(): + self.image_files.append(str(image_path)) self.num_pages = len(self.image_files) return True size = ScreenList().current['size'] # Generate images from PDF that will fit the frame. runlog = '' try: - if not os.path.isdir(self.get_temp_folder()): - os.makedirs(self.get_temp_folder()) + if not temp_dir_path.is_dir(): + temp_dir_path.mkdir(parents=True) # The %03d in the file name is handled by each binary if self.controller.mudrawbin: log.debug('loading presentation using mudraw') runlog = check_output([self.controller.mudrawbin, '-w', str(size.width()), '-h', str(size.height()), - '-o', os.path.join(self.get_temp_folder(), 'mainslide%03d.png'), self.file_path], + '-o', str(temp_dir_path / 'mainslide%03d.png'), self.file_path], startupinfo=self.startupinfo) elif self.controller.mutoolbin: log.debug('loading presentation using mutool') runlog = check_output([self.controller.mutoolbin, 'draw', '-w', str(size.width()), '-h', str(size.height()), - '-o', os.path.join(self.get_temp_folder(), 'mainslide%03d.png'), self.file_path], + '-o', str(temp_dir_path / 'mainslide%03d.png'), self.file_path], startupinfo=self.startupinfo) elif self.controller.gsbin: log.debug('loading presentation using gs') resolution = self.gs_get_resolution(size) runlog = check_output([self.controller.gsbin, '-dSAFER', '-dNOPAUSE', '-dBATCH', '-sDEVICE=png16m', '-r' + str(resolution), '-dTextAlphaBits=4', '-dGraphicsAlphaBits=4', - '-sOutputFile=' + os.path.join(self.get_temp_folder(), 'mainslide%03d.png'), + '-sOutputFile=' + str(temp_dir_path / 'mainslide%03d.png'), self.file_path], startupinfo=self.startupinfo) - created_files = sorted(os.listdir(self.get_temp_folder())) - for fn in created_files: - if os.path.isfile(os.path.join(self.get_temp_folder(), fn)): - self.image_files.append(os.path.join(self.get_temp_folder(), fn)) - except Exception as e: - log.debug(e) - log.debug(runlog) + created_files = sorted(temp_dir_path.glob('*')) + for image_path in created_files: + if image_path.is_file(): + self.image_files.append(str(image_path)) + except Exception: + log.exception(runlog) return False self.num_pages = len(self.image_files) # Create thumbnails diff --git a/openlp/plugins/presentations/lib/powerpointcontroller.py b/openlp/plugins/presentations/lib/powerpointcontroller.py index 3bd726027..1014db851 100644 --- a/openlp/plugins/presentations/lib/powerpointcontroller.py +++ b/openlp/plugins/presentations/lib/powerpointcontroller.py @@ -177,7 +177,7 @@ class PowerpointDocument(PresentationDocument): if not self.presentation.Slides(num + 1).SlideShowTransition.Hidden: self.index_map[key] = num + 1 self.presentation.Slides(num + 1).Export( - os.path.join(self.get_thumbnail_folder(), 'slide{key:d}.png'.format(key=key)), 'png', 320, 240) + str(self.get_thumbnail_folder() / 'slide{key:d}.png'.format(key=key)), 'png', 320, 240) key += 1 self.slide_count = key - 1 diff --git a/openlp/plugins/presentations/lib/pptviewcontroller.py b/openlp/plugins/presentations/lib/pptviewcontroller.py index 645bef053..c936fe65c 100644 --- a/openlp/plugins/presentations/lib/pptviewcontroller.py +++ b/openlp/plugins/presentations/lib/pptviewcontroller.py @@ -121,17 +121,17 @@ class PptviewDocument(PresentationDocument): the background PptView task started earlier. """ log.debug('LoadPresentation') - temp_folder = self.get_temp_folder() + temp_dir_path = self.get_temp_folder() size = ScreenList().current['size'] rect = RECT(size.x(), size.y(), size.right(), size.bottom()) self.file_path = os.path.normpath(self.file_path) - preview_path = os.path.join(temp_folder, 'slide') + preview_path = temp_dir_path / 'slide' # Ensure that the paths are null terminated byte_file_path = self.file_path.encode('utf-16-le') + b'\0' - preview_path = preview_path.encode('utf-16-le') + b'\0' - if not os.path.isdir(temp_folder): - os.makedirs(temp_folder) - self.ppt_id = self.controller.process.OpenPPT(byte_file_path, None, rect, preview_path) + preview_file_name = str(preview_path).encode('utf-16-le') + b'\0' + if not temp_dir_path: + temp_dir_path.mkdir(parents=True) + self.ppt_id = self.controller.process.OpenPPT(byte_file_path, None, rect, preview_file_name) if self.ppt_id >= 0: self.create_thumbnails() self.stop_presentation() diff --git a/openlp/plugins/presentations/lib/presentationcontroller.py b/openlp/plugins/presentations/lib/presentationcontroller.py index e5f8ccf21..af665bb55 100644 --- a/openlp/plugins/presentations/lib/presentationcontroller.py +++ b/openlp/plugins/presentations/lib/presentationcontroller.py @@ -29,6 +29,7 @@ from PyQt5 import QtCore from openlp.core.common import Registry, AppLocation, Settings, check_directory_exists, md5_hash from openlp.core.common.path import Path from openlp.core.lib import create_thumb, validate_thumb +from openlp.core.lib.shutil import rmtree log = logging.getLogger(__name__) @@ -99,7 +100,7 @@ class PresentationDocument(object): """ self.slide_number = 0 self.file_path = name - check_directory_exists(Path(self.get_thumbnail_folder())) + check_directory_exists(self.get_thumbnail_folder()) def load_presentation(self): """ @@ -116,10 +117,12 @@ class PresentationDocument(object): a file, e.g. thumbnails """ try: - if os.path.exists(self.get_thumbnail_folder()): - shutil.rmtree(self.get_thumbnail_folder()) - if os.path.exists(self.get_temp_folder()): - shutil.rmtree(self.get_temp_folder()) + thumbnail_folder_path = self.get_thumbnail_folder() + temp_folder_path = self.get_temp_folder() + if thumbnail_folder_path.exists(): + rmtree(thumbnail_folder_path) + if temp_folder_path.exists(): + rmtree(temp_folder_path) except OSError: log.exception('Failed to delete presentation controller files') @@ -132,24 +135,30 @@ class PresentationDocument(object): def get_thumbnail_folder(self): """ The location where thumbnail images will be stored + + :return: The path to the thumbnail + :rtype: openlp.core.common.path.Path """ # TODO: If statement can be removed when the upgrade path from 2.0.x to 2.2.x is no longer needed if Settings().value('presentations/thumbnail_scheme') == 'md5': folder = md5_hash(self.file_path.encode('utf-8')) else: folder = self.get_file_name() - return os.path.join(self.controller.thumbnail_folder, folder) + return Path(self.controller.thumbnail_folder, folder) def get_temp_folder(self): """ The location where thumbnail images will be stored + + :return: The path to the temporary file folder + :rtype: openlp.core.common.path.Path """ # TODO: If statement can be removed when the upgrade path from 2.0.x to 2.2.x is no longer needed if Settings().value('presentations/thumbnail_scheme') == 'md5': folder = md5_hash(self.file_path.encode('utf-8')) else: - folder = folder = self.get_file_name() - return os.path.join(self.controller.temp_folder, folder) + folder = self.get_file_name() + return Path(self.controller.temp_folder, folder) def check_thumbnails(self): """ @@ -251,15 +260,17 @@ class PresentationDocument(object): thumb_path = self.get_thumbnail_path(idx, False) create_thumb(file, thumb_path, False, QtCore.QSize(-1, 360)) - def get_thumbnail_path(self, slide_no, check_exists): + def get_thumbnail_path(self, slide_no, check_exists=True): """ Returns an image path containing a preview for the requested slide - :param slide_no: The slide an image is required for, starting at 1 - :param check_exists: + :param int slide_no: The slide an image is required for, starting at 1 + :param bool check_exists: Check if the generated path exists + :return: The path, or None if the :param:`check_exists` is True and the file does not exist + :rtype: openlp.core.common.path.Path, None """ - path = os.path.join(self.get_thumbnail_folder(), self.controller.thumbnail_prefix + str(slide_no) + '.png') - if os.path.isfile(path) or not check_exists: + path = self.get_thumbnail_folder() / (self.controller.thumbnail_prefix + str(slide_no) + '.png') + if path.is_file() or not check_exists: return path else: return None @@ -304,7 +315,7 @@ class PresentationDocument(object): """ titles = [] notes = [] - titles_file = os.path.join(self.get_thumbnail_folder(), 'titles.txt') + titles_file = str(self.get_thumbnail_folder() / 'titles.txt') if os.path.exists(titles_file): try: with open(titles_file, encoding='utf-8') as fi: @@ -313,7 +324,7 @@ class PresentationDocument(object): log.exception('Failed to open/read existing titles file') titles = [] for slide_no, title in enumerate(titles, 1): - notes_file = os.path.join(self.get_thumbnail_folder(), 'slideNotes{number:d}.txt'.format(number=slide_no)) + notes_file = str(self.get_thumbnail_folder() / 'slideNotes{number:d}.txt'.format(number=slide_no)) note = '' if os.path.exists(notes_file): try: @@ -331,14 +342,13 @@ class PresentationDocument(object): and notes to the slideNote%.txt """ if titles: - titles_file = os.path.join(self.get_thumbnail_folder(), 'titles.txt') - with open(titles_file, mode='wt', encoding='utf-8') as fo: + titles_path = self.get_thumbnail_folder() / 'titles.txt' + with titles_path.open(mode='wt', encoding='utf-8') as fo: fo.writelines(titles) if notes: for slide_no, note in enumerate(notes, 1): - notes_file = os.path.join(self.get_thumbnail_folder(), - 'slideNotes{number:d}.txt'.format(number=slide_no)) - with open(notes_file, mode='wt', encoding='utf-8') as fn: + notes_path = self.get_thumbnail_folder() / 'slideNotes{number:d}.txt'.format(number=slide_no) + with notes_path.open(mode='wt', encoding='utf-8') as fn: fn.write(note) diff --git a/openlp/plugins/songs/reporting.py b/openlp/plugins/songs/reporting.py index b50cd0a0c..066a0ea26 100644 --- a/openlp/plugins/songs/reporting.py +++ b/openlp/plugins/songs/reporting.py @@ -48,7 +48,7 @@ def report_song_list(): Path(translate('SongPlugin.ReportSongList', 'song_extract.csv')), translate('SongPlugin.ReportSongList', 'CSV format (*.csv)')) - if not report_file_path: + if report_file_path is None: main_window.error_message( translate('SongPlugin.ReportSongList', 'Output Path Not Selected'), translate('SongPlugin.ReportSongList', 'You have not set a valid output location for your report. \n' diff --git a/tests/functional/openlp_core_ui/test_exceptionform.py b/tests/functional/openlp_core_ui/test_exceptionform.py index 37a040aa6..40eb19ac8 100644 --- a/tests/functional/openlp_core_ui/test_exceptionform.py +++ b/tests/functional/openlp_core_ui/test_exceptionform.py @@ -103,7 +103,7 @@ class TestExceptionForm(TestMixin, TestCase): os.remove(self.tempfile) @patch("openlp.core.ui.exceptionform.Ui_ExceptionDialog") - @patch("openlp.core.ui.exceptionform.QtWidgets.QFileDialog") + @patch("openlp.core.ui.exceptionform.FileDialog") @patch("openlp.core.ui.exceptionform.QtCore.QUrl") @patch("openlp.core.ui.exceptionform.QtCore.QUrlQuery.addQueryItem") @patch("openlp.core.ui.exceptionform.Qt") From 8ed5903ced06462290dac9f959230a1ade566138 Mon Sep 17 00:00:00 2001 From: Philip Ridout Date: Fri, 15 Sep 2017 20:01:09 +0100 Subject: [PATCH 05/12] Moved most of the presentation plugin over to pathlib --- openlp/core/lib/__init__.py | 20 ++- openlp/core/lib/mediamanageritem.py | 2 - openlp/core/lib/shutil.py | 17 ++ .../presentations/lib/impresscontroller.py | 45 +++-- openlp/plugins/presentations/lib/mediaitem.py | 156 +++++++++--------- .../presentations/lib/messagelistener.py | 29 ++-- .../presentations/lib/pdfcontroller.py | 83 +++++----- .../presentations/lib/powerpointcontroller.py | 12 +- .../presentations/lib/pptviewcontroller.py | 35 ++-- .../lib/presentationcontroller.py | 119 ++++++------- .../presentations/lib/presentationtab.py | 9 +- .../test_presentationcontroller.py | 17 -- 12 files changed, 278 insertions(+), 266 deletions(-) diff --git a/openlp/core/lib/__init__.py b/openlp/core/lib/__init__.py index 08b675000..0babbc0d1 100644 --- a/openlp/core/lib/__init__.py +++ b/openlp/core/lib/__init__.py @@ -29,9 +29,10 @@ import os import re import math -from PyQt5 import QtCore, QtGui, Qt, QtWidgets +from PyQt5 import QtCore, QtGui, QtWidgets from openlp.core.common import translate +from openlp.core.common.path import Path log = logging.getLogger(__name__ + '.__init__') @@ -125,10 +126,11 @@ def build_icon(icon): Build a QIcon instance from an existing QIcon, a resource location, or a physical file location. If the icon is a QIcon instance, that icon is simply returned. If not, it builds a QIcon instance from the resource or file name. - :param icon: - The icon to build. This can be a QIcon, a resource string in the form ``:/resource/file.png``, or a file - location like ``/path/to/file.png``. However, the **recommended** way is to specify a resource string. + :param QtGui.QIcon | Path | QtGui.QIcon | str icon: + The icon to build. This can be a QIcon, a resource string in the form ``:/resource/file.png``, or a file path + location like ``Path(/path/to/file.png)``. However, the **recommended** way is to specify a resource string. :return: The build icon. + :rtype: QtGui.QIcon """ if isinstance(icon, QtGui.QIcon): return icon @@ -136,6 +138,8 @@ def build_icon(icon): button_icon = QtGui.QIcon() if isinstance(icon, str): pix_map = QtGui.QPixmap(icon) + elif isinstance(icon, Path): + pix_map = QtGui.QPixmap(str(icon)) elif isinstance(icon, QtGui.QImage): pix_map = QtGui.QPixmap.fromImage(icon) if pix_map: @@ -221,10 +225,12 @@ def validate_thumb(file_path, thumb_path): :param thumb_path: The path to the thumb. :return: True, False if the image has changed since the thumb was created. """ - if not os.path.exists(thumb_path): + file_path = Path(file_path) + thumb_path = Path(thumb_path) + if not thumb_path.exists(): return False - image_date = os.stat(file_path).st_mtime - thumb_date = os.stat(thumb_path).st_mtime + image_date = file_path.stat().st_mtime + thumb_date = thumb_path.stat().st_mtime return image_date <= thumb_date diff --git a/openlp/core/lib/mediamanageritem.py b/openlp/core/lib/mediamanageritem.py index 59a0e4e33..1c7a5b4ef 100644 --- a/openlp/core/lib/mediamanageritem.py +++ b/openlp/core/lib/mediamanageritem.py @@ -359,10 +359,8 @@ class MediaManagerItem(QtWidgets.QWidget, RegistryProperties): :param files: The files to be loaded. :param target_group: The QTreeWidgetItem of the group that will be the parent of the added files """ - names = [] full_list = [] for count in range(self.list_view.count()): - names.append(self.list_view.item(count).text()) full_list.append(self.list_view.item(count).data(QtCore.Qt.UserRole)) duplicates_found = False files_added = False diff --git a/openlp/core/lib/shutil.py b/openlp/core/lib/shutil.py index d2f6ae34d..44dea590a 100755 --- a/openlp/core/lib/shutil.py +++ b/openlp/core/lib/shutil.py @@ -95,3 +95,20 @@ def rmtree(*args, **kwargs): args, kwargs = replace_params(args, kwargs, ((0, 'path', path_to_str),)) return shutil.rmtree(*args, **kwargs) +# TODO: Test and tidy +def which(*args, **kwargs): + """ + Wraps :func:shutil.rmtree` so that we can accept Path objects. + + :param openlp.core.common.path.Path path: Takes a Path object which is then converted to a str object + :return: Passes the return from :func:`shutil.rmtree` back + :rtype: None + + See the following link for more information on the other parameters: + https://docs.python.org/3/library/shutil.html#shutil.rmtree + """ + + file_name = shutil.which(*args, **kwargs) + if file_name: + return str_to_path(file_name) + return None diff --git a/openlp/plugins/presentations/lib/impresscontroller.py b/openlp/plugins/presentations/lib/impresscontroller.py index 7699946a0..472e07801 100644 --- a/openlp/plugins/presentations/lib/impresscontroller.py +++ b/openlp/plugins/presentations/lib/impresscontroller.py @@ -32,11 +32,14 @@ # http://nxsy.org/comparing-documents-with-openoffice-and-python import logging -import os import time -from openlp.core.common import is_win, Registry, delete_file -from openlp.core.common.path import Path +from PyQt5 import QtCore + +from openlp.core.common import Registry, delete_file, get_uno_command, get_uno_instance, is_win +from openlp.core.lib import ScreenList +from openlp.plugins.presentations.lib.presentationcontroller import PresentationController, PresentationDocument, \ + TextType if is_win(): from win32com.client import Dispatch @@ -55,14 +58,6 @@ else: except ImportError: uno_available = False -from PyQt5 import QtCore - -from openlp.core.lib import ScreenList -from openlp.core.common import get_uno_command, get_uno_instance -from openlp.plugins.presentations.lib.presentationcontroller import PresentationController, PresentationDocument, \ - TextType - - log = logging.getLogger(__name__) @@ -203,12 +198,15 @@ class ImpressDocument(PresentationDocument): Class which holds information and controls a single presentation. """ - def __init__(self, controller, presentation): + def __init__(self, controller, document_path): """ Constructor, store information about the file and initialise. + + :param openlp.core.common.path.Path document_path: File path for the document to load + :rtype: None """ log.debug('Init Presentation OpenOffice') - super(ImpressDocument, self).__init__(controller, presentation) + super().__init__(controller, document_path) self.document = None self.presentation = None self.control = None @@ -225,10 +223,10 @@ class ImpressDocument(PresentationDocument): if desktop is None: self.controller.start_process() desktop = self.controller.get_com_desktop() - url = 'file:///' + self.file_path.replace('\\', '/').replace(':', '|').replace(' ', '%20') + url = self.file_path.as_uri() else: desktop = self.controller.get_uno_desktop() - url = uno.systemPathToFileUrl(self.file_path) + url = uno.systemPathToFileUrl(str(self.file_path)) if desktop is None: return False self.desktop = desktop @@ -254,11 +252,11 @@ class ImpressDocument(PresentationDocument): log.debug('create thumbnails OpenOffice') if self.check_thumbnails(): return + temp_folder_path = self.get_temp_folder() if is_win(): - thumb_dir_url = 'file:///' + str(self.get_temp_folder()).replace('\\', '/') \ - .replace(':', '|').replace(' ', '%20') + thumb_dir_url = temp_folder_path.as_uri() else: - thumb_dir_url = uno.systemPathToFileUrl(str(self.get_temp_folder())) + thumb_dir_url = uno.systemPathToFileUrl(str(temp_folder_path)) properties = [] properties.append(self.create_property('FilterName', 'impress_png_Export')) properties = tuple(properties) @@ -266,17 +264,16 @@ class ImpressDocument(PresentationDocument): pages = doc.getDrawPages() if not pages: return - temp_folder_path = self.get_temp_folder() - if not temp_folder_path.isdir(): - temp_folder_path.mkdir() + if not temp_folder_path.is_dir(): + temp_folder_path.mkdir(parents=True) for index in range(pages.getCount()): page = pages.getByIndex(index) doc.getCurrentController().setCurrentPage(page) - url_path = '{path}/{name}.png'.format(path=thumb_dir_url, name=str(index + 1)) - path = temp_folder_path / '{number).png'.format(number=index + 1) + url_path = '{path}/{name:d}.png'.format(path=thumb_dir_url, name=index + 1) + path = temp_folder_path / '{number:d}.png'.format(number=index + 1) try: doc.storeToURL(url_path, properties) - self.convert_thumbnail(str(path), index + 1) + self.convert_thumbnail(path, index + 1) delete_file(path) except ErrorCodeIOException as exception: log.exception('ERROR! ErrorCodeIOException {error:d}'.format(error=exception.ErrCode)) diff --git a/openlp/plugins/presentations/lib/mediaitem.py b/openlp/plugins/presentations/lib/mediaitem.py index 275279e15..aa5bfc0d6 100644 --- a/openlp/plugins/presentations/lib/mediaitem.py +++ b/openlp/plugins/presentations/lib/mediaitem.py @@ -19,15 +19,13 @@ # with this program; if not, write to the Free Software Foundation, Inc., 59 # # Temple Place, Suite 330, Boston, MA 02111-1307 USA # ############################################################################### - import logging -import os from PyQt5 import QtCore, QtGui, QtWidgets from openlp.core.common import Registry, Settings, UiStrings, translate from openlp.core.common.languagemanager import get_locale_key -from openlp.core.common.path import path_to_str +from openlp.core.common.path import Path, path_to_str, str_to_path from openlp.core.lib import MediaManagerItem, ItemCapabilities, ServiceItemContext,\ build_icon, check_item_selected, create_thumb, validate_thumb from openlp.core.lib.ui import critical_error_message_box, create_horizontal_adjusting_combo_box @@ -128,7 +126,7 @@ class PresentationMediaItem(MediaManagerItem): """ self.list_view.setIconSize(QtCore.QSize(88, 50)) file_paths = Settings().value(self.settings_section + '/presentations files') - self.load_list([path_to_str(file) for file in file_paths], initial_load=True) + self.load_list([path_to_str(path) for path in file_paths], initial_load=True) self.populate_display_types() def populate_display_types(self): @@ -152,54 +150,57 @@ class PresentationMediaItem(MediaManagerItem): else: self.presentation_widget.hide() - def load_list(self, files, target_group=None, initial_load=False): + def load_list(self, file_paths, target_group=None, initial_load=False): """ Add presentations into the media manager. This is called both on initial load of the plugin to populate with existing files, and when the user adds new files via the media manager. + + :param list[openlp.core.common.path.Path] file_paths: List of file paths to add to the media manager. """ - current_list = self.get_file_list() - titles = [file_path.name for file_path in current_list] + file_paths = [str_to_path(filename) for filename in file_paths] + current_paths = self.get_file_list() + titles = [file_path.name for file_path in current_paths] self.application.set_busy_cursor() if not initial_load: - self.main_window.display_progress_bar(len(files)) + self.main_window.display_progress_bar(len(file_paths)) # Sort the presentations by its filename considering language specific characters. - files.sort(key=lambda filename: get_locale_key(os.path.split(str(filename))[1])) - for file in files: + file_paths.sort(key=lambda file_path: get_locale_key(file_path.name)) + for file_path in file_paths: if not initial_load: self.main_window.increment_progress_bar() - if current_list.count(file) > 0: + if current_paths.count(file_path) > 0: continue - filename = os.path.split(file)[1] - if not os.path.exists(file): - item_name = QtWidgets.QListWidgetItem(filename) + file_name = file_path.name + if not file_path.exists(): + item_name = QtWidgets.QListWidgetItem(file_name) item_name.setIcon(build_icon(ERROR_IMAGE)) - item_name.setData(QtCore.Qt.UserRole, file) - item_name.setToolTip(file) + item_name.setData(QtCore.Qt.UserRole, path_to_str(file_path)) + item_name.setToolTip(str(file_path)) self.list_view.addItem(item_name) else: - if titles.count(filename) > 0: + if titles.count(file_name) > 0: if not initial_load: critical_error_message_box(translate('PresentationPlugin.MediaItem', 'File Exists'), translate('PresentationPlugin.MediaItem', 'A presentation with that filename already exists.')) continue - controller_name = self.find_controller_by_type(filename) + controller_name = self.find_controller_by_type(file_path) if controller_name: controller = self.controllers[controller_name] - doc = controller.add_document(file) - thumb = str(doc.get_thumbnail_folder() / 'icon.png') - preview = doc.get_thumbnail_path(1, True) - if not preview and not initial_load: + doc = controller.add_document(file_path) + thumbnail_path = doc.get_thumbnail_folder() / 'icon.png' + preview_path = doc.get_thumbnail_path(1, True) + if not preview_path and not initial_load: doc.load_presentation() - preview = doc.get_thumbnail_path(1, True) + preview_path = doc.get_thumbnail_path(1, True) doc.close_presentation() - if not (preview and os.path.exists(preview)): + if not (preview_path and preview_path.exists()): icon = build_icon(':/general/general_delete.png') else: - if validate_thumb(preview, thumb): - icon = build_icon(thumb) + if validate_thumb(preview_path, thumbnail_path): + icon = build_icon(thumbnail_path) else: - icon = create_thumb(preview, thumb) + icon = create_thumb(str(preview_path), str(thumbnail_path)) else: if initial_load: icon = build_icon(':/general/general_delete.png') @@ -208,10 +209,10 @@ class PresentationMediaItem(MediaManagerItem): translate('PresentationPlugin.MediaItem', 'This type of presentation is not supported.')) continue - item_name = QtWidgets.QListWidgetItem(filename) - item_name.setData(QtCore.Qt.UserRole, file) + item_name = QtWidgets.QListWidgetItem(file_name) + item_name.setData(QtCore.Qt.UserRole, path_to_str(file_path)) item_name.setIcon(icon) - item_name.setToolTip(file) + item_name.setToolTip(str(file_path)) self.list_view.addItem(item_name) if not initial_load: self.main_window.finished_progress_bar() @@ -228,8 +229,8 @@ class PresentationMediaItem(MediaManagerItem): self.application.set_busy_cursor() self.main_window.display_progress_bar(len(row_list)) for item in items: - filepath = str(item.data(QtCore.Qt.UserRole)) - self.clean_up_thumbnails(filepath) + file_path = str_to_path(item.data(QtCore.Qt.UserRole)) + self.clean_up_thumbnails(file_path) self.main_window.increment_progress_bar() self.main_window.finished_progress_bar() for row in row_list: @@ -237,30 +238,29 @@ class PresentationMediaItem(MediaManagerItem): Settings().setValue(self.settings_section + '/presentations files', self.get_file_list()) self.application.set_normal_cursor() - def clean_up_thumbnails(self, filepath, clean_for_update=False): + def clean_up_thumbnails(self, file_path, clean_for_update=False): """ Clean up the files created such as thumbnails - :param filepath: File path of the presention to clean up after - :param clean_for_update: Only clean thumbnails if update is needed - :return: None + :param openlp.core.common.path.Path file_path: File path of the presention to clean up after + :param bool clean_for_update: Only clean thumbnails if update is needed + :rtype: None """ for cidx in self.controllers: - root, file_ext = os.path.splitext(filepath) - file_ext = file_ext[1:] + file_ext = file_path.suffix[1:] if file_ext in self.controllers[cidx].supports or file_ext in self.controllers[cidx].also_supports: - doc = self.controllers[cidx].add_document(filepath) + doc = self.controllers[cidx].add_document(file_path) if clean_for_update: thumb_path = doc.get_thumbnail_path(1, True) - if not thumb_path or not os.path.exists(filepath) or os.path.getmtime( - thumb_path) < os.path.getmtime(filepath): + if not thumb_path or not file_path.exists() or \ + thumb_path.stat().st_mtime < file_path.stat().st_mtime: doc.presentation_deleted() else: doc.presentation_deleted() doc.close_presentation() def generate_slide_data(self, service_item, item=None, xml_version=False, remote=False, - context=ServiceItemContext.Service, presentation_file=None): + context=ServiceItemContext.Service, file_path=None): """ Generate the slide data. Needs to be implemented by the plugin. @@ -276,10 +276,9 @@ class PresentationMediaItem(MediaManagerItem): items = self.list_view.selectedItems() if len(items) > 1: return False - filename = presentation_file - if filename is None: - filename = items[0].data(QtCore.Qt.UserRole) - file_type = os.path.splitext(filename.lower())[1][1:] + if file_path is None: + file_path = str_to_path(items[0].data(QtCore.Qt.UserRole)) + file_type = file_path.suffix.lower()[1:] if not self.display_type_combo_box.currentText(): return False service_item.add_capability(ItemCapabilities.CanEditTitle) @@ -292,29 +291,28 @@ class PresentationMediaItem(MediaManagerItem): # force a nonexistent theme service_item.theme = -1 for bitem in items: - filename = presentation_file - if filename is None: - filename = bitem.data(QtCore.Qt.UserRole) - (path, name) = os.path.split(filename) - service_item.title = name - if os.path.exists(filename): - processor = self.find_controller_by_type(filename) + if file_path is None: + file_path = str_to_path(bitem.data(QtCore.Qt.UserRole)) + path, file_name = file_path.parent, file_path.name + service_item.title = file_name + if file_path.exists(): + processor = self.find_controller_by_type(file_path) if not processor: return False controller = self.controllers[processor] service_item.processor = None - doc = controller.add_document(filename) + doc = controller.add_document(file_path) if doc.get_thumbnail_path(1, True) is None or \ - not (doc.get_temp_folder() / 'mainslide001.png').is_file(): + not (doc.get_temp_folder() / 'mainslide001.png').is_file(): doc.load_presentation() i = 1 - image = str(doc.get_temp_folder() / 'mainslide{number:0>3d}.png'.format(number=i)) - thumbnail = str(doc.get_thumbnail_folder() / 'slide{number:d}.png'.format(number=i)) - while os.path.isfile(image): - service_item.add_from_image(image, name, thumbnail=thumbnail) + image_path = doc.get_temp_folder() / 'mainslide{number:0>3d}.png'.format(number=i) + thumbnail_path = doc.get_thumbnail_folder() / 'slide{number:d}.png'.format(number=i) + while image_path.is_file(): + service_item.add_from_image(str(image_path), file_name, thumbnail=str(thumbnail_path)) i += 1 - image = str(doc.get_temp_folder() / 'mainslide{number:0>3d}.png'.format(number=i)) - thumbnail = str(doc.get_thumbnail_folder() / 'slide{number:d}.png'.format(number=i)) + image_path = doc.get_temp_folder() / 'mainslide{number:0>3d}.png'.format(number=i) + thumbnail_path = doc.get_thumbnail_folder() / 'slide{number:d}.png'.format(number=i) service_item.add_capability(ItemCapabilities.HasThumbnails) doc.close_presentation() return True @@ -324,34 +322,34 @@ class PresentationMediaItem(MediaManagerItem): critical_error_message_box(translate('PresentationPlugin.MediaItem', 'Missing Presentation'), translate('PresentationPlugin.MediaItem', 'The presentation {name} no longer exists.' - ).format(name=filename)) + ).format(name=file_path)) return False else: service_item.processor = self.display_type_combo_box.currentText() service_item.add_capability(ItemCapabilities.ProvidesOwnDisplay) for bitem in items: - filename = bitem.data(QtCore.Qt.UserRole) - (path, name) = os.path.split(filename) - service_item.title = name - if os.path.exists(filename): + file_path = str_to_path(bitem.data(QtCore.Qt.UserRole)) + path, file_name = file_path.parent, file_path.name + service_item.title = file_name + if file_path.exists: if self.display_type_combo_box.itemData(self.display_type_combo_box.currentIndex()) == 'automatic': - service_item.processor = self.find_controller_by_type(filename) + service_item.processor = self.find_controller_by_type(file_path) if not service_item.processor: return False controller = self.controllers[service_item.processor] - doc = controller.add_document(filename) + doc = controller.add_document(file_path) if doc.get_thumbnail_path(1, True) is None: doc.load_presentation() i = 1 - img = doc.get_thumbnail_path(i, True) - if img: + thumbnail_path = doc.get_thumbnail_path(i, True) + if thumbnail_path: # Get titles and notes titles, notes = doc.get_titles_and_notes() service_item.add_capability(ItemCapabilities.HasDisplayTitle) if notes.count('') != len(notes): service_item.add_capability(ItemCapabilities.HasNotes) service_item.add_capability(ItemCapabilities.HasThumbnails) - while img: + while thumbnail_path: # Use title and note if available title = '' if titles and len(titles) >= i: @@ -359,9 +357,9 @@ class PresentationMediaItem(MediaManagerItem): note = '' if notes and len(notes) >= i: note = notes[i - 1] - service_item.add_from_command(path, name, img, title, note) + service_item.add_from_command(str(path), file_name, str(thumbnail_path), title, note) i += 1 - img = doc.get_thumbnail_path(i, True) + thumbnail_path = doc.get_thumbnail_path(i, True) doc.close_presentation() return True else: @@ -371,7 +369,7 @@ class PresentationMediaItem(MediaManagerItem): 'Missing Presentation'), translate('PresentationPlugin.MediaItem', 'The presentation {name} is incomplete, ' - 'please reload.').format(name=filename)) + 'please reload.').format(name=file_path)) return False else: # File is no longer present @@ -379,18 +377,20 @@ class PresentationMediaItem(MediaManagerItem): critical_error_message_box(translate('PresentationPlugin.MediaItem', 'Missing Presentation'), translate('PresentationPlugin.MediaItem', 'The presentation {name} no longer exists.' - ).format(name=filename)) + ).format(name=file_path)) return False - def find_controller_by_type(self, filename): + def find_controller_by_type(self, file_path): """ Determine the default application controller to use for the selected file type. This is used if "Automatic" is set as the preferred controller. Find the first (alphabetic) enabled controller which "supports" the extension. If none found, then look for a controller which "also supports" it instead. - :param filename: The file name + :param openlp.core.common.path.Path file_path: The file path + :return: The default application controller for this file type, or None if not supported + :rtype: PresentationController """ - file_type = os.path.splitext(filename)[1][1:] + file_type = file_path.suffix[1:] if not file_type: return None for controller in self.controllers: diff --git a/openlp/plugins/presentations/lib/messagelistener.py b/openlp/plugins/presentations/lib/messagelistener.py index 8e5de3e2d..5ad46d0fe 100644 --- a/openlp/plugins/presentations/lib/messagelistener.py +++ b/openlp/plugins/presentations/lib/messagelistener.py @@ -19,16 +19,15 @@ # with this program; if not, write to the Free Software Foundation, Inc., 59 # # Temple Place, Suite 330, Boston, MA 02111-1307 USA # ############################################################################### - -import logging import copy -import os +import logging from PyQt5 import QtCore from openlp.core.common import Registry, Settings -from openlp.core.ui import HideMode +from openlp.core.common.path import Path from openlp.core.lib import ServiceItemContext +from openlp.core.ui import HideMode from openlp.plugins.presentations.lib.pdfcontroller import PDF_CONTROLLER_FILETYPES log = logging.getLogger(__name__) @@ -325,21 +324,25 @@ class MessageListener(object): is_live = message[1] item = message[0] hide_mode = message[2] - file = item.get_frame_path() + file_path = Path(item.get_frame_path()) self.handler = item.processor # When starting presentation from the servicemanager we convert # PDF/XPS/OXPS-serviceitems into image-serviceitems. When started from the mediamanager # the conversion has already been done at this point. - file_type = os.path.splitext(file.lower())[1][1:] + file_type = file_path.suffix.lower()[1:] if file_type in PDF_CONTROLLER_FILETYPES: - log.debug('Converting from pdf/xps/oxps to images for serviceitem with file {name}'.format(name=file)) + log.debug('Converting from pdf/xps/oxps to images for serviceitem with file {name}'.format(name=file_path)) # Create a copy of the original item, and then clear the original item so it can be filled with images item_cpy = copy.copy(item) item.__init__(None) if is_live: - self.media_item.generate_slide_data(item, item_cpy, False, False, ServiceItemContext.Live, file) + # TODO: To Path object + self.media_item.generate_slide_data(item, item_cpy, False, False, ServiceItemContext.Live, + str(file_path)) else: - self.media_item.generate_slide_data(item, item_cpy, False, False, ServiceItemContext.Preview, file) + # TODO: To Path object + self.media_item.generate_slide_data(item, item_cpy, False, False, ServiceItemContext.Preview, + str(file_path)) # Some of the original serviceitem attributes is needed in the new serviceitem item.footer = item_cpy.footer item.from_service = item_cpy.from_service @@ -352,13 +355,13 @@ class MessageListener(object): self.handler = None else: if self.handler == self.media_item.automatic: - self.handler = self.media_item.find_controller_by_type(file) + self.handler = self.media_item.find_controller_by_type(file_path) if not self.handler: return else: - # the saved handler is not present so need to use one based on file suffix. + # the saved handler is not present so need to use one based on file_path suffix. if not self.controllers[self.handler].available: - self.handler = self.media_item.find_controller_by_type(file) + self.handler = self.media_item.find_controller_by_type(file_path) if not self.handler: return if is_live: @@ -370,7 +373,7 @@ class MessageListener(object): if self.handler is None: self.controller = controller else: - controller.add_handler(self.controllers[self.handler], file, hide_mode, message[3]) + controller.add_handler(self.controllers[self.handler], file_path, hide_mode, message[3]) self.timer.start() def slide(self, message): diff --git a/openlp/plugins/presentations/lib/pdfcontroller.py b/openlp/plugins/presentations/lib/pdfcontroller.py index f80ad94ba..9f4aa1b4f 100644 --- a/openlp/plugins/presentations/lib/pdfcontroller.py +++ b/openlp/plugins/presentations/lib/pdfcontroller.py @@ -23,13 +23,13 @@ import os import logging import re -from shutil import which from subprocess import check_output, CalledProcessError from openlp.core.common import AppLocation, check_binary_exists from openlp.core.common import Settings, is_win from openlp.core.common.path import Path, path_to_str from openlp.core.lib import ScreenList +from openlp.core.lib.shutil import which from openlp.plugins.presentations.lib.presentationcontroller import PresentationController, PresentationDocument if is_win(): @@ -66,11 +66,12 @@ class PdfController(PresentationController): Function that checks whether a binary is either ghostscript or mudraw or neither. Is also used from presentationtab.py - :param program_path:The full path to the binary to check. + :param openlp.core.common.path.Path program_path: The full path to the binary to check. :return: Type of the binary, 'gs' if ghostscript, 'mudraw' if mudraw, None if invalid. + :rtype: str | None """ program_type = None - runlog = check_binary_exists(Path(program_path)) + runlog = check_binary_exists(program_path) # Analyse the output to see it the program is mudraw, ghostscript or neither for line in runlog.splitlines(): decoded_line = line.decode() @@ -107,30 +108,29 @@ class PdfController(PresentationController): :return: True if program to open PDF-files was found, otherwise False. """ log.debug('check_installed Pdf') - self.mudrawbin = '' - self.mutoolbin = '' - self.gsbin = '' + self.mudrawbin = None + self.mutoolbin = None + self.gsbin = None self.also_supports = [] # Use the user defined program if given if Settings().value('presentations/enable_pdf_program'): - pdf_program = path_to_str(Settings().value('presentations/pdf_program')) - program_type = self.process_check_binary(pdf_program) + program_path = Settings().value('presentations/pdf_program') + program_type = self.process_check_binary(program_path) if program_type == 'gs': - self.gsbin = pdf_program + self.gsbin = program_path elif program_type == 'mudraw': - self.mudrawbin = pdf_program + self.mudrawbin = program_path elif program_type == 'mutool': - self.mutoolbin = pdf_program + self.mutoolbin = program_path else: # Fallback to autodetection - application_path = str(AppLocation.get_directory(AppLocation.AppDir)) + application_path = AppLocation.get_directory(AppLocation.AppDir) if is_win(): # for windows we only accept mudraw.exe or mutool.exe in the base folder - application_path = str(AppLocation.get_directory(AppLocation.AppDir)) - if os.path.isfile(os.path.join(application_path, 'mudraw.exe')): - self.mudrawbin = os.path.join(application_path, 'mudraw.exe') - elif os.path.isfile(os.path.join(application_path, 'mutool.exe')): - self.mutoolbin = os.path.join(application_path, 'mutool.exe') + if (application_path / 'mudraw.exe').is_file(): + self.mudrawbin = application_path / 'mudraw.exe' + elif (application_path / 'mutool.exe').is_file(): + self.mutoolbin = application_path / 'mutool.exe' else: DEVNULL = open(os.devnull, 'wb') # First try to find mudraw @@ -143,11 +143,11 @@ class PdfController(PresentationController): self.gsbin = which('gs') # Last option: check if mudraw or mutool is placed in OpenLP base folder if not self.mudrawbin and not self.mutoolbin and not self.gsbin: - application_path = str(AppLocation.get_directory(AppLocation.AppDir)) - if os.path.isfile(os.path.join(application_path, 'mudraw')): - self.mudrawbin = os.path.join(application_path, 'mudraw') - elif os.path.isfile(os.path.join(application_path, 'mutool')): - self.mutoolbin = os.path.join(application_path, 'mutool') + application_path = AppLocation.get_directory(AppLocation.AppDir) + if (application_path / 'mudraw').is_file(): + self.mudrawbin = application_path / 'mudraw' + elif (application_path / 'mutool').is_file(): + self.mutoolbin = application_path / 'mutool' if self.mudrawbin or self.mutoolbin: self.also_supports = ['xps', 'oxps'] return True @@ -172,12 +172,15 @@ class PdfDocument(PresentationDocument): image-serviceitem on the fly and present as such. Therefore some of the 'playback' functions is not implemented. """ - def __init__(self, controller, presentation): + def __init__(self, controller, document_path): """ Constructor, store information about the file and initialise. + + :param openlp.core.common.path.Path document_path: Path to the document to load + :rtype: None """ log.debug('Init Presentation Pdf') - PresentationDocument.__init__(self, controller, presentation) + super().__init__(controller, document_path) self.presentation = None self.blanked = False self.hidden = False @@ -200,13 +203,13 @@ class PdfDocument(PresentationDocument): :return: The resolution dpi to be used. """ # Use a postscript script to get size of the pdf. It is assumed that all pages have same size - gs_resolution_script = str(AppLocation.get_directory( - AppLocation.PluginsDir)) + '/presentations/lib/ghostscript_get_resolution.ps' + gs_resolution_script = AppLocation.get_directory( + AppLocation.PluginsDir) / 'presentations' / 'lib' / 'ghostscript_get_resolution.ps' # Run the script on the pdf to get the size runlog = [] try: - runlog = check_output([self.controller.gsbin, '-dNOPAUSE', '-dNODISPLAY', '-dBATCH', - '-sFile=' + self.file_path, gs_resolution_script], + runlog = check_output([str(self.controller.gsbin), '-dNOPAUSE', '-dNODISPLAY', '-dBATCH', + '-sFile={file_path}'.format(file_path=self.file_path), str(gs_resolution_script)], startupinfo=self.startupinfo) except CalledProcessError as e: log.debug(' '.join(e.cmd)) @@ -246,7 +249,7 @@ class PdfDocument(PresentationDocument): created_files = sorted(temp_dir_path.glob('*')) for image_path in created_files: if image_path.is_file(): - self.image_files.append(str(image_path)) + self.image_files.append(image_path) self.num_pages = len(self.image_files) return True size = ScreenList().current['size'] @@ -258,27 +261,27 @@ class PdfDocument(PresentationDocument): # The %03d in the file name is handled by each binary if self.controller.mudrawbin: log.debug('loading presentation using mudraw') - runlog = check_output([self.controller.mudrawbin, '-w', str(size.width()), '-h', str(size.height()), - '-o', str(temp_dir_path / 'mainslide%03d.png'), self.file_path], + runlog = check_output([str(self.controller.mudrawbin), '-w', str(size.width()), '-h', str(size.height()), + '-o', str(temp_dir_path / 'mainslide%03d.png'), str(self.file_path)], startupinfo=self.startupinfo) elif self.controller.mutoolbin: log.debug('loading presentation using mutool') - runlog = check_output([self.controller.mutoolbin, 'draw', '-w', str(size.width()), '-h', - str(size.height()), - '-o', str(temp_dir_path / 'mainslide%03d.png'), self.file_path], + runlog = check_output([str(self.controller.mutoolbin), 'draw', '-w', str(size.width()), + '-h', str(size.height()), '-o', str(temp_dir_path / 'mainslide%03d.png'), + str(self.file_path)], startupinfo=self.startupinfo) elif self.controller.gsbin: log.debug('loading presentation using gs') resolution = self.gs_get_resolution(size) - runlog = check_output([self.controller.gsbin, '-dSAFER', '-dNOPAUSE', '-dBATCH', '-sDEVICE=png16m', - '-r' + str(resolution), '-dTextAlphaBits=4', '-dGraphicsAlphaBits=4', - '-sOutputFile=' + str(temp_dir_path / 'mainslide%03d.png'), - self.file_path], startupinfo=self.startupinfo) + runlog = check_output([str(self.controller.gsbin), '-dSAFER', '-dNOPAUSE', '-dBATCH', '-sDEVICE=png16m', + '-r{res}'.format(res=resolution), '-dTextAlphaBits=4', '-dGraphicsAlphaBits=4', + '-sOutputFile={output}'.format(output=temp_dir_path / 'mainslide%03d.png'), + str(self.file_path)], startupinfo=self.startupinfo) created_files = sorted(temp_dir_path.glob('*')) for image_path in created_files: if image_path.is_file(): - self.image_files.append(str(image_path)) - except Exception: + self.image_files.append(image_path) + except Exception as e: log.exception(runlog) return False self.num_pages = len(self.image_files) diff --git a/openlp/plugins/presentations/lib/powerpointcontroller.py b/openlp/plugins/presentations/lib/powerpointcontroller.py index 1014db851..fa253ffda 100644 --- a/openlp/plugins/presentations/lib/powerpointcontroller.py +++ b/openlp/plugins/presentations/lib/powerpointcontroller.py @@ -120,15 +120,16 @@ class PowerpointDocument(PresentationDocument): Class which holds information and controls a single presentation. """ - def __init__(self, controller, presentation): + def __init__(self, controller, document_path): """ Constructor, store information about the file and initialise. :param controller: - :param presentation: + :param openlp.core.common.path.Path document_path: Path to the document to load + :rtype: None """ log.debug('Init Presentation Powerpoint') - super(PowerpointDocument, self).__init__(controller, presentation) + super().__init__(controller, document_path) self.presentation = None self.index_map = {} self.slide_count = 0 @@ -145,7 +146,7 @@ class PowerpointDocument(PresentationDocument): try: if not self.controller.process: self.controller.start_process() - self.controller.process.Presentations.Open(os.path.normpath(self.file_path), False, False, False) + self.controller.process.Presentations.Open(str(self.file_path), False, False, False) self.presentation = self.controller.process.Presentations(self.controller.process.Presentations.Count) self.create_thumbnails() self.create_titles_and_notes() @@ -363,9 +364,8 @@ class PowerpointDocument(PresentationDocument): width=size.width(), horizontal=(right - left))) log.debug('window title: {title}'.format(title=window_title)) - filename_root, filename_ext = os.path.splitext(os.path.basename(self.file_path)) if size.y() == top and size.height() == (bottom - top) and size.x() == left and \ - size.width() == (right - left) and filename_root in window_title: + size.width() == (right - left) and self.file_path.stem in window_title: log.debug('Found a match and will save the handle') self.presentation_hwnd = hwnd # Stop powerpoint from flashing in the taskbar diff --git a/openlp/plugins/presentations/lib/pptviewcontroller.py b/openlp/plugins/presentations/lib/pptviewcontroller.py index c936fe65c..547636026 100644 --- a/openlp/plugins/presentations/lib/pptviewcontroller.py +++ b/openlp/plugins/presentations/lib/pptviewcontroller.py @@ -85,9 +85,9 @@ class PptviewController(PresentationController): if self.process: return log.debug('start PPTView') - dll_path = os.path.join(str(AppLocation.get_directory(AppLocation.AppDir)), - 'plugins', 'presentations', 'lib', 'pptviewlib', 'pptviewlib.dll') - self.process = cdll.LoadLibrary(dll_path) + dll_path = AppLocation.get_directory(AppLocation.AppDir) \ + / 'plugins' / 'presentations' / 'lib' / 'pptviewlib' / 'pptviewlib.dll' + self.process = cdll.LoadLibrary(str(dll_path)) if log.isEnabledFor(logging.DEBUG): self.process.SetDebug(1) @@ -104,12 +104,15 @@ class PptviewDocument(PresentationDocument): """ Class which holds information and controls a single presentation. """ - def __init__(self, controller, presentation): + def __init__(self, controller, document_path): """ Constructor, store information about the file and initialise. + + :param openlp.core.common.path.Path document_path: File path to the document to load + :rtype: None """ log.debug('Init Presentation PowerPoint') - super(PptviewDocument, self).__init__(controller, presentation) + super().__init__(controller, document_path) self.presentation = None self.ppt_id = None self.blanked = False @@ -121,17 +124,16 @@ class PptviewDocument(PresentationDocument): the background PptView task started earlier. """ log.debug('LoadPresentation') - temp_dir_path = self.get_temp_folder() + temp_path = self.get_temp_folder() size = ScreenList().current['size'] rect = RECT(size.x(), size.y(), size.right(), size.bottom()) - self.file_path = os.path.normpath(self.file_path) - preview_path = temp_dir_path / 'slide' + preview_path = temp_path / 'slide' # Ensure that the paths are null terminated - byte_file_path = self.file_path.encode('utf-16-le') + b'\0' - preview_file_name = str(preview_path).encode('utf-16-le') + b'\0' - if not temp_dir_path: - temp_dir_path.mkdir(parents=True) - self.ppt_id = self.controller.process.OpenPPT(byte_file_path, None, rect, preview_file_name) + file_path_utf16 = str(self.file_path).encode('utf-16-le') + b'\0' + preview_path_utf16 = str(preview_path).encode('utf-16-le') + b'\0' + if not temp_path.is_dir(): + temp_path.mkdir(parents=True) + self.ppt_id = self.controller.process.OpenPPT(file_path_utf16, None, rect, preview_path_utf16) if self.ppt_id >= 0: self.create_thumbnails() self.stop_presentation() @@ -148,7 +150,7 @@ class PptviewDocument(PresentationDocument): return log.debug('create_thumbnails proceeding') for idx in range(self.get_slide_count()): - path = '{folder}\\slide{index}.bmp'.format(folder=self.get_temp_folder(), index=str(idx + 1)) + path = self.get_temp_folder() / 'slide{index:d}.bmp'.format(index=idx + 1) self.convert_thumbnail(path, idx + 1) def create_titles_and_notes(self): @@ -161,13 +163,12 @@ class PptviewDocument(PresentationDocument): """ titles = None notes = None - filename = os.path.normpath(self.file_path) # let's make sure we have a valid zipped presentation - if os.path.exists(filename) and zipfile.is_zipfile(filename): + if self.file_path.exists() and zipfile.is_zipfile(str(self.file_path)): namespaces = {"p": "http://schemas.openxmlformats.org/presentationml/2006/main", "a": "http://schemas.openxmlformats.org/drawingml/2006/main"} # open the file - with zipfile.ZipFile(filename) as zip_file: + with zipfile.ZipFile(str(self.file_path)) as zip_file: # find the presentation.xml to get the slide count with zip_file.open('ppt/presentation.xml') as pres: tree = ElementTree.parse(pres) diff --git a/openlp/plugins/presentations/lib/presentationcontroller.py b/openlp/plugins/presentations/lib/presentationcontroller.py index af665bb55..13a759a5e 100644 --- a/openlp/plugins/presentations/lib/presentationcontroller.py +++ b/openlp/plugins/presentations/lib/presentationcontroller.py @@ -19,10 +19,7 @@ # with this program; if not, write to the Free Software Foundation, Inc., 59 # # Temple Place, Suite 330, Boston, MA 02111-1307 USA # ############################################################################### - import logging -import os -import shutil from PyQt5 import QtCore @@ -87,19 +84,26 @@ class PresentationDocument(object): Returns a path to an image containing a preview for the requested slide """ - def __init__(self, controller, name): + def __init__(self, controller, document_path): """ Constructor for the PresentationController class + + :param controller: + :param openlp.core.common.path.Path document_path: Path to the document to load. + :rtype: None """ self.controller = controller - self._setup(name) + self._setup(document_path) - def _setup(self, name): + def _setup(self, document_path): """ Run some initial setup. This method is separate from __init__ in order to mock it out in tests. + + :param openlp.core.common.path.Path document_path: Path to the document to load. + :rtype: None """ self.slide_number = 0 - self.file_path = name + self.file_path = document_path check_directory_exists(self.get_thumbnail_folder()) def load_presentation(self): @@ -126,12 +130,6 @@ class PresentationDocument(object): except OSError: log.exception('Failed to delete presentation controller files') - def get_file_name(self): - """ - Return just the filename of the presentation, without the directory - """ - return os.path.split(self.file_path)[1] - def get_thumbnail_folder(self): """ The location where thumbnail images will be stored @@ -141,9 +139,9 @@ class PresentationDocument(object): """ # TODO: If statement can be removed when the upgrade path from 2.0.x to 2.2.x is no longer needed if Settings().value('presentations/thumbnail_scheme') == 'md5': - folder = md5_hash(self.file_path.encode('utf-8')) + folder = md5_hash(bytes(self.file_path)) else: - folder = self.get_file_name() + folder = self.file_path.name return Path(self.controller.thumbnail_folder, folder) def get_temp_folder(self): @@ -155,19 +153,22 @@ class PresentationDocument(object): """ # TODO: If statement can be removed when the upgrade path from 2.0.x to 2.2.x is no longer needed if Settings().value('presentations/thumbnail_scheme') == 'md5': - folder = md5_hash(self.file_path.encode('utf-8')) + folder = md5_hash(bytes(self.file_path)) else: - folder = self.get_file_name() + folder = self.file_path.name return Path(self.controller.temp_folder, folder) def check_thumbnails(self): """ - Returns ``True`` if the thumbnail images exist and are more recent than the powerpoint file. + Check that the last thumbnail image exists and is valid and are more recent than the powerpoint file. + + :return: If the thumbnail is valid + :rtype: bool """ - last_image = self.get_thumbnail_path(self.get_slide_count(), True) - if not (last_image and os.path.isfile(last_image)): + last_image_path = self.get_thumbnail_path(self.get_slide_count(), True) + if not (last_image_path and last_image_path.is_file()): return False - return validate_thumb(self.file_path, last_image) + return validate_thumb(self.file_path, last_image_path) def close_presentation(self): """ @@ -250,24 +251,28 @@ class PresentationDocument(object): """ pass - def convert_thumbnail(self, file, idx): + def convert_thumbnail(self, image_path, index): """ Convert the slide image the application made to a scaled 360px height .png image. + + :param openlp.core.common.path.Path image_path: Path to the image to create a thumb nail of + :param int index: The index of the slide to create the thumbnail for. + :rtype: None """ if self.check_thumbnails(): return - if os.path.isfile(file): - thumb_path = self.get_thumbnail_path(idx, False) - create_thumb(file, thumb_path, False, QtCore.QSize(-1, 360)) + if image_path.is_file(): + thumb_path = self.get_thumbnail_path(index, False) + create_thumb(str(image_path), str(thumb_path), False, QtCore.QSize(-1, 360)) - def get_thumbnail_path(self, slide_no, check_exists=True): + def get_thumbnail_path(self, slide_no, check_exists=False): """ Returns an image path containing a preview for the requested slide :param int slide_no: The slide an image is required for, starting at 1 :param bool check_exists: Check if the generated path exists :return: The path, or None if the :param:`check_exists` is True and the file does not exist - :rtype: openlp.core.common.path.Path, None + :rtype: openlp.core.common.path.Path | None """ path = self.get_thumbnail_folder() / (self.controller.thumbnail_prefix + str(slide_no) + '.png') if path.is_file() or not check_exists: @@ -313,43 +318,38 @@ class PresentationDocument(object): Reads the titles from the titles file and the notes files and returns the content in two lists """ - titles = [] notes = [] - titles_file = str(self.get_thumbnail_folder() / 'titles.txt') - if os.path.exists(titles_file): - try: - with open(titles_file, encoding='utf-8') as fi: - titles = fi.read().splitlines() - except: - log.exception('Failed to open/read existing titles file') - titles = [] + titles_path = self.get_thumbnail_folder() / 'titles.txt' + try: + titles = titles_path.read_text().splitlines() + except: + log.exception('Failed to open/read existing titles file') + titles = [] for slide_no, title in enumerate(titles, 1): - notes_file = str(self.get_thumbnail_folder() / 'slideNotes{number:d}.txt'.format(number=slide_no)) - note = '' - if os.path.exists(notes_file): - try: - with open(notes_file, encoding='utf-8') as fn: - note = fn.read() - except: - log.exception('Failed to open/read notes file') - note = '' + notes_path = self.get_thumbnail_folder() / 'slideNotes{number:d}.txt'.format(number=slide_no) + try: + note = notes_path.read_text() + except: + log.exception('Failed to open/read notes file') + note = '' notes.append(note) return titles, notes def save_titles_and_notes(self, titles, notes): """ - Performs the actual persisting of titles to the titles.txt - and notes to the slideNote%.txt + Performs the actual persisting of titles to the titles.txt and notes to the slideNote%.txt + + :param list[str] titles: The titles to save + :param list[str] notes: The notes to save + :rtype: None """ if titles: titles_path = self.get_thumbnail_folder() / 'titles.txt' - with titles_path.open(mode='wt', encoding='utf-8') as fo: - fo.writelines(titles) + titles_path.write_text('\n'.join(titles)) if notes: for slide_no, note in enumerate(notes, 1): notes_path = self.get_thumbnail_folder() / 'slideNotes{number:d}.txt'.format(number=slide_no) - with notes_path.open(mode='wt', encoding='utf-8') as fn: - fn.write(note) + notes_path.write_text(note) class PresentationController(object): @@ -426,12 +426,11 @@ class PresentationController(object): self.document_class = document_class self.settings_section = self.plugin.settings_section self.available = None - self.temp_folder = os.path.join(str(AppLocation.get_section_data_path(self.settings_section)), name) - self.thumbnail_folder = os.path.join( - str(AppLocation.get_section_data_path(self.settings_section)), 'thumbnails') + self.temp_folder = AppLocation.get_section_data_path(self.settings_section) / name + self.thumbnail_folder = AppLocation.get_section_data_path(self.settings_section) / 'thumbnails' self.thumbnail_prefix = 'slide' - check_directory_exists(Path(self.thumbnail_folder)) - check_directory_exists(Path(self.temp_folder)) + check_directory_exists(self.thumbnail_folder) + check_directory_exists(self.temp_folder) def enabled(self): """ @@ -466,11 +465,15 @@ class PresentationController(object): log.debug('Kill') self.close_presentation() - def add_document(self, name): + def add_document(self, document_path): """ Called when a new presentation document is opened. + + :param openlp.core.common.path.Path document_path: Path to the document to load + :return: The document + :rtype: PresentationDocument """ - document = self.document_class(self, name) + document = self.document_class(self, document_path) self.docs.append(document) return document diff --git a/openlp/plugins/presentations/lib/presentationtab.py b/openlp/plugins/presentations/lib/presentationtab.py index 3e92827c8..ca9ceacbc 100644 --- a/openlp/plugins/presentations/lib/presentationtab.py +++ b/openlp/plugins/presentations/lib/presentationtab.py @@ -38,7 +38,6 @@ class PresentationTab(SettingsTab): """ Constructor """ - self.parent = parent self.controllers = controllers super(PresentationTab, self).__init__(parent, title, visible_title, icon_path) self.activated = False @@ -194,7 +193,7 @@ class PresentationTab(SettingsTab): pdf_program_path = self.program_path_edit.path enable_pdf_program = self.pdf_program_check_box.checkState() # If the given program is blank disable using the program - if not pdf_program_path: + if pdf_program_path is None: enable_pdf_program = 0 if pdf_program_path != Settings().value(self.settings_section + '/pdf_program'): Settings().setValue(self.settings_section + '/pdf_program', pdf_program_path) @@ -220,9 +219,11 @@ class PresentationTab(SettingsTab): def on_program_path_edit_path_changed(self, new_path): """ - Select the mudraw or ghostscript binary that should be used. + Handle the `pathEditChanged` signal from program_path_edit + + :param openlp.core.common.path.Path new_path: File path to the new program + :rtype: None """ - new_path = path_to_str(new_path) if new_path: if not PdfController.process_check_binary(new_path): critical_error_message_box(UiStrings().Error, diff --git a/tests/functional/openlp_plugins/presentations/test_presentationcontroller.py b/tests/functional/openlp_plugins/presentations/test_presentationcontroller.py index 1bbd29522..c5e6d3df3 100644 --- a/tests/functional/openlp_plugins/presentations/test_presentationcontroller.py +++ b/tests/functional/openlp_plugins/presentations/test_presentationcontroller.py @@ -244,20 +244,3 @@ class TestPresentationDocument(TestCase): # THEN: load_presentation should return false self.assertFalse(result, "PresentationDocument.load_presentation should return false.") - - def test_get_file_name(self): - """ - Test the PresentationDocument.get_file_name method. - """ - - # GIVEN: A mocked os.path.split which returns a list, an instance of PresentationDocument and - # arbitary file_path. - self.mock_os.path.split.return_value = ['directory', 'file.ext'] - instance = PresentationDocument(self.mock_controller, 'Name') - instance.file_path = 'filepath' - - # WHEN: Calling get_file_name - result = instance.get_file_name() - - # THEN: get_file_name should return 'file.ext' - self.assertEqual(result, 'file.ext') From 7f98003d5492603c62db5fac1d1c000ebef4d442 Mon Sep 17 00:00:00 2001 From: Philip Ridout Date: Sun, 17 Sep 2017 20:43:15 +0100 Subject: [PATCH 06/12] test fixes --- openlp/core/lib/__init__.py | 11 ++-- openlp/core/ui/thememanager.py | 2 +- openlp/plugins/images/lib/mediaitem.py | 2 +- openlp/plugins/presentations/lib/mediaitem.py | 2 +- .../presentations/lib/pdfcontroller.py | 3 +- .../lib/presentationcontroller.py | 2 +- tests/functional/openlp_core_lib/test_lib.py | 55 ++++++---------- .../presentations/test_impresscontroller.py | 7 +- .../presentations/test_mediaitem.py | 19 +++--- .../presentations/test_pdfcontroller.py | 15 +++-- .../presentations/test_pptviewcontroller.py | 14 ++-- .../test_presentationcontroller.py | 65 +++++++------------ 12 files changed, 83 insertions(+), 114 deletions(-) diff --git a/openlp/core/lib/__init__.py b/openlp/core/lib/__init__.py index 0babbc0d1..1d55df497 100644 --- a/openlp/core/lib/__init__.py +++ b/openlp/core/lib/__init__.py @@ -29,7 +29,7 @@ import os import re import math -from PyQt5 import QtCore, QtGui, QtWidgets +from PyQt5 import QtCore, QtGui, Qt, QtWidgets from openlp.core.common import translate from openlp.core.common.path import Path @@ -221,12 +221,11 @@ def validate_thumb(file_path, thumb_path): Validates whether an file's thumb still exists and if is up to date. **Note**, you must **not** call this function, before checking the existence of the file. - :param file_path: The path to the file. The file **must** exist! - :param thumb_path: The path to the thumb. - :return: True, False if the image has changed since the thumb was created. + :param openlp.core.common.path.Path file_path: The path to the file. The file **must** exist! + :param openlp.core.common.path.Path thumb_path: The path to the thumb. + :return: Has the image changed since the thumb was created? + :rtype: bool """ - file_path = Path(file_path) - thumb_path = Path(thumb_path) if not thumb_path.exists(): return False image_date = file_path.stat().st_mtime diff --git a/openlp/core/ui/thememanager.py b/openlp/core/ui/thememanager.py index 61381a902..15e33cdb2 100644 --- a/openlp/core/ui/thememanager.py +++ b/openlp/core/ui/thememanager.py @@ -483,7 +483,7 @@ class ThemeManager(OpenLPMixin, RegistryMixin, QtWidgets.QWidget, Ui_ThemeManage name = text_name thumb = os.path.join(self.thumb_path, '{name}.png'.format(name=text_name)) item_name = QtWidgets.QListWidgetItem(name) - if validate_thumb(theme, thumb): + if validate_thumb(Path(theme), Path(thumb)): icon = build_icon(thumb) else: icon = create_thumb(theme, thumb) diff --git a/openlp/plugins/images/lib/mediaitem.py b/openlp/plugins/images/lib/mediaitem.py index bcf222eb0..d1ea2003f 100644 --- a/openlp/plugins/images/lib/mediaitem.py +++ b/openlp/plugins/images/lib/mediaitem.py @@ -360,7 +360,7 @@ class ImageMediaItem(MediaManagerItem): if not os.path.exists(image_file.filename): icon = build_icon(':/general/general_delete.png') else: - if validate_thumb(image_file.filename, thumb): + if validate_thumb(Path(image_file.filename), Path(thumb)): icon = build_icon(thumb) else: icon = create_thumb(image_file.filename, thumb) diff --git a/openlp/plugins/presentations/lib/mediaitem.py b/openlp/plugins/presentations/lib/mediaitem.py index aa5bfc0d6..d9a14e0ed 100644 --- a/openlp/plugins/presentations/lib/mediaitem.py +++ b/openlp/plugins/presentations/lib/mediaitem.py @@ -197,7 +197,7 @@ class PresentationMediaItem(MediaManagerItem): if not (preview_path and preview_path.exists()): icon = build_icon(':/general/general_delete.png') else: - if validate_thumb(preview_path, thumbnail_path): + if validate_thumb(Path(preview_path), Path(thumbnail_path)): icon = build_icon(thumbnail_path) else: icon = create_thumb(str(preview_path), str(thumbnail_path)) diff --git a/openlp/plugins/presentations/lib/pdfcontroller.py b/openlp/plugins/presentations/lib/pdfcontroller.py index 9f4aa1b4f..a39cce36c 100644 --- a/openlp/plugins/presentations/lib/pdfcontroller.py +++ b/openlp/plugins/presentations/lib/pdfcontroller.py @@ -261,7 +261,8 @@ class PdfDocument(PresentationDocument): # The %03d in the file name is handled by each binary if self.controller.mudrawbin: log.debug('loading presentation using mudraw') - runlog = check_output([str(self.controller.mudrawbin), '-w', str(size.width()), '-h', str(size.height()), + runlog = check_output([str(self.controller.mudrawbin), '-w', str(size.width()), + '-h', str(size.height()), '-o', str(temp_dir_path / 'mainslide%03d.png'), str(self.file_path)], startupinfo=self.startupinfo) elif self.controller.mutoolbin: diff --git a/openlp/plugins/presentations/lib/presentationcontroller.py b/openlp/plugins/presentations/lib/presentationcontroller.py index 13a759a5e..3225eac24 100644 --- a/openlp/plugins/presentations/lib/presentationcontroller.py +++ b/openlp/plugins/presentations/lib/presentationcontroller.py @@ -168,7 +168,7 @@ class PresentationDocument(object): last_image_path = self.get_thumbnail_path(self.get_slide_count(), True) if not (last_image_path and last_image_path.is_file()): return False - return validate_thumb(self.file_path, last_image_path) + return validate_thumb(Path(self.file_path), Path(last_image_path)) def close_presentation(self): """ diff --git a/tests/functional/openlp_core_lib/test_lib.py b/tests/functional/openlp_core_lib/test_lib.py index 2056665f4..8b46e99c3 100644 --- a/tests/functional/openlp_core_lib/test_lib.py +++ b/tests/functional/openlp_core_lib/test_lib.py @@ -595,61 +595,46 @@ class TestLib(TestCase): Test the validate_thumb() function when the thumbnail does not exist """ # GIVEN: A mocked out os module, with path.exists returning False, and fake paths to a file and a thumb - with patch('openlp.core.lib.os') as mocked_os: - file_path = 'path/to/file' - thumb_path = 'path/to/thumb' - mocked_os.path.exists.return_value = False + with patch.object(Path, 'exists', return_value=False) as mocked_path_exists: + file_path = Path('path', 'to', 'file') + thumb_path = Path('path', 'to', 'thumb') # WHEN: we run the validate_thumb() function result = validate_thumb(file_path, thumb_path) # THEN: we should have called a few functions, and the result should be False - mocked_os.path.exists.assert_called_with(thumb_path) - assert result is False, 'The result should be False' + thumb_path.exists.assert_called_once_with() + self.assertFalse(result, 'The result should be False') def test_validate_thumb_file_exists_and_newer(self): """ Test the validate_thumb() function when the thumbnail exists and has a newer timestamp than the file """ - # GIVEN: A mocked out os module, functions rigged to work for us, and fake paths to a file and a thumb - with patch('openlp.core.lib.os') as mocked_os: - file_path = 'path/to/file' - thumb_path = 'path/to/thumb' - file_mocked_stat = MagicMock() - file_mocked_stat.st_mtime = datetime.now() - thumb_mocked_stat = MagicMock() - thumb_mocked_stat.st_mtime = datetime.now() + timedelta(seconds=10) - mocked_os.path.exists.return_value = True - mocked_os.stat.side_effect = [file_mocked_stat, thumb_mocked_stat] + with patch.object(Path, 'exists'), patch.object(Path, 'stat'): + # GIVEN: Mocked file_path and thumb_path which return different values fo the modified times + file_path = MagicMock(**{'stat.return_value': MagicMock(st_mtime=10)}) + thumb_path = MagicMock(**{'exists.return_value': True, 'stat.return_value': MagicMock(st_mtime=11)}) # WHEN: we run the validate_thumb() function + result = validate_thumb(file_path, thumb_path) - # THEN: we should have called a few functions, and the result should be True - # mocked_os.path.exists.assert_called_with(thumb_path) + # THEN: `validate_thumb` should return True + self.assertTrue(result) def test_validate_thumb_file_exists_and_older(self): """ Test the validate_thumb() function when the thumbnail exists but is older than the file """ - # GIVEN: A mocked out os module, functions rigged to work for us, and fake paths to a file and a thumb - with patch('openlp.core.lib.os') as mocked_os: - file_path = 'path/to/file' - thumb_path = 'path/to/thumb' - file_mocked_stat = MagicMock() - file_mocked_stat.st_mtime = datetime.now() - thumb_mocked_stat = MagicMock() - thumb_mocked_stat.st_mtime = datetime.now() - timedelta(seconds=10) - mocked_os.path.exists.return_value = True - mocked_os.stat.side_effect = lambda fname: file_mocked_stat if fname == file_path else thumb_mocked_stat + # GIVEN: Mocked file_path and thumb_path which return different values fo the modified times + file_path = MagicMock(**{'stat.return_value': MagicMock(st_mtime=10)}) + thumb_path = MagicMock(**{'exists.return_value': True, 'stat.return_value': MagicMock(st_mtime=9)}) - # WHEN: we run the validate_thumb() function - result = validate_thumb(file_path, thumb_path) + # WHEN: we run the validate_thumb() function + result = validate_thumb(file_path, thumb_path) - # THEN: we should have called a few functions, and the result should be False - mocked_os.path.exists.assert_called_with(thumb_path) - mocked_os.stat.assert_any_call(file_path) - mocked_os.stat.assert_any_call(thumb_path) - assert result is False, 'The result should be False' + # THEN: `validate_thumb` should return False + thumb_path.stat.assert_called_once_with() + self.assertFalse(result, 'The result should be False') def test_replace_params_no_params(self): """ diff --git a/tests/functional/openlp_plugins/presentations/test_impresscontroller.py b/tests/functional/openlp_plugins/presentations/test_impresscontroller.py index d383b16e4..a792988e2 100644 --- a/tests/functional/openlp_plugins/presentations/test_impresscontroller.py +++ b/tests/functional/openlp_plugins/presentations/test_impresscontroller.py @@ -24,13 +24,12 @@ Functional tests to test the Impress class and related methods. """ from unittest import TestCase from unittest.mock import MagicMock -import os import shutil from tempfile import mkdtemp from openlp.core.common import Settings -from openlp.plugins.presentations.lib.impresscontroller import \ - ImpressController, ImpressDocument, TextType +from openlp.core.common.path import Path +from openlp.plugins.presentations.lib.impresscontroller import ImpressController, ImpressDocument, TextType from openlp.plugins.presentations.presentationplugin import __default_settings__ from tests.utils.constants import TEST_RESOURCES_PATH @@ -82,7 +81,7 @@ class TestImpressDocument(TestCase): mocked_plugin = MagicMock() mocked_plugin.settings_section = 'presentations' Settings().extend_default_settings(__default_settings__) - self.file_name = os.path.join(TEST_RESOURCES_PATH, 'presentations', 'test.pptx') + self.file_name = Path(TEST_RESOURCES_PATH, 'presentations', 'test.pptx') self.ppc = ImpressController(mocked_plugin) self.doc = ImpressDocument(self.ppc, self.file_name) diff --git a/tests/functional/openlp_plugins/presentations/test_mediaitem.py b/tests/functional/openlp_plugins/presentations/test_mediaitem.py index b5299d785..9ce0a5fdc 100644 --- a/tests/functional/openlp_plugins/presentations/test_mediaitem.py +++ b/tests/functional/openlp_plugins/presentations/test_mediaitem.py @@ -26,6 +26,7 @@ from unittest import TestCase from unittest.mock import patch, MagicMock, call from openlp.core.common import Registry +from openlp.core.common.path import Path from openlp.plugins.presentations.lib.mediaitem import PresentationMediaItem from tests.helpers.testmixin import TestMixin @@ -92,17 +93,18 @@ class TestMediaItem(TestCase, TestMixin): """ # GIVEN: A mocked controller, and mocked os.path.getmtime mocked_controller = MagicMock() - mocked_doc = MagicMock() + mocked_doc = MagicMock(**{'get_thumbnail_path.return_value': Path()}) mocked_controller.add_document.return_value = mocked_doc mocked_controller.supports = ['tmp'] self.media_item.controllers = { 'Mocked': mocked_controller } - presentation_file = 'file.tmp' - with patch('openlp.plugins.presentations.lib.mediaitem.os.path.getmtime') as mocked_getmtime, \ - patch('openlp.plugins.presentations.lib.mediaitem.os.path.exists') as mocked_exists: - mocked_getmtime.side_effect = [100, 200] - mocked_exists.return_value = True + + thmub_path = MagicMock(st_mtime=100) + file_path = MagicMock(st_mtime=400) + with patch.object(Path, 'stat', side_effect=[thmub_path, file_path]), \ + patch.object(Path, 'exists', return_value=True): + presentation_file = Path('file.tmp') # WHEN: calling clean_up_thumbnails self.media_item.clean_up_thumbnails(presentation_file, True) @@ -123,9 +125,8 @@ class TestMediaItem(TestCase, TestMixin): self.media_item.controllers = { 'Mocked': mocked_controller } - presentation_file = 'file.tmp' - with patch('openlp.plugins.presentations.lib.mediaitem.os.path.exists') as mocked_exists: - mocked_exists.return_value = False + presentation_file = Path('file.tmp') + with patch.object(Path, 'exists', return_value=False): # WHEN: calling clean_up_thumbnails self.media_item.clean_up_thumbnails(presentation_file, True) diff --git a/tests/functional/openlp_plugins/presentations/test_pdfcontroller.py b/tests/functional/openlp_plugins/presentations/test_pdfcontroller.py index be4aeeaa4..25a8394f0 100644 --- a/tests/functional/openlp_plugins/presentations/test_pdfcontroller.py +++ b/tests/functional/openlp_plugins/presentations/test_pdfcontroller.py @@ -32,6 +32,7 @@ from PyQt5 import QtCore, QtGui from openlp.plugins.presentations.lib.pdfcontroller import PdfController, PdfDocument from openlp.core.common import Settings +from openlp.core.common.path import Path from openlp.core.lib import ScreenList from tests.utils.constants import TEST_RESOURCES_PATH @@ -66,8 +67,8 @@ class TestPdfController(TestCase, TestMixin): self.desktop.screenGeometry.return_value = SCREEN['size'] self.screens = ScreenList.create(self.desktop) Settings().extend_default_settings(__default_settings__) - self.temp_folder = mkdtemp() - self.thumbnail_folder = mkdtemp() + self.temp_folder = Path(mkdtemp()) + self.thumbnail_folder = Path(mkdtemp()) self.mock_plugin = MagicMock() self.mock_plugin.settings_section = self.temp_folder @@ -77,8 +78,8 @@ class TestPdfController(TestCase, TestMixin): """ del self.screens self.destroy_settings() - shutil.rmtree(self.thumbnail_folder) - shutil.rmtree(self.temp_folder) + shutil.rmtree(str(self.thumbnail_folder)) + shutil.rmtree(str(self.temp_folder)) def test_constructor(self): """ @@ -98,7 +99,7 @@ class TestPdfController(TestCase, TestMixin): Test loading of a Pdf using the PdfController """ # GIVEN: A Pdf-file - test_file = os.path.join(TEST_RESOURCES_PATH, 'presentations', 'pdf_test1.pdf') + test_file = Path(TEST_RESOURCES_PATH, 'presentations', 'pdf_test1.pdf') # WHEN: The Pdf is loaded controller = PdfController(plugin=self.mock_plugin) @@ -118,7 +119,7 @@ class TestPdfController(TestCase, TestMixin): Test loading of a Pdf and check size of generate pictures """ # GIVEN: A Pdf-file - test_file = os.path.join(TEST_RESOURCES_PATH, 'presentations', 'pdf_test1.pdf') + test_file = Path(TEST_RESOURCES_PATH, 'presentations', 'pdf_test1.pdf') # WHEN: The Pdf is loaded controller = PdfController(plugin=self.mock_plugin) @@ -131,7 +132,7 @@ class TestPdfController(TestCase, TestMixin): # THEN: The load should succeed and pictures should be created and have been scales to fit the screen self.assertTrue(loaded, 'The loading of the PDF should succeed.') - image = QtGui.QImage(os.path.join(self.temp_folder, 'pdf_test1.pdf', 'mainslide001.png')) + image = QtGui.QImage(os.path.join(str(self.temp_folder), 'pdf_test1.pdf', 'mainslide001.png')) # Based on the converter used the resolution will differ a bit if controller.gsbin: self.assertEqual(760, image.height(), 'The height should be 760') diff --git a/tests/functional/openlp_plugins/presentations/test_pptviewcontroller.py b/tests/functional/openlp_plugins/presentations/test_pptviewcontroller.py index 3c08d226a..bfa74a7fa 100644 --- a/tests/functional/openlp_plugins/presentations/test_pptviewcontroller.py +++ b/tests/functional/openlp_plugins/presentations/test_pptviewcontroller.py @@ -22,7 +22,6 @@ """ This module contains tests for the pptviewcontroller module of the Presentations plugin. """ -import os import shutil from tempfile import mkdtemp from unittest import TestCase @@ -30,6 +29,7 @@ from unittest.mock import MagicMock, patch from openlp.plugins.presentations.lib.pptviewcontroller import PptviewDocument, PptviewController from openlp.core.common import is_win +from openlp.core.common.path import Path from tests.helpers.testmixin import TestMixin from tests.utils.constants import TEST_RESOURCES_PATH @@ -184,7 +184,7 @@ class TestPptviewDocument(TestCase): """ # GIVEN: mocked PresentationController.save_titles_and_notes and a pptx file doc = PptviewDocument(self.mock_controller, self.mock_presentation) - doc.file_path = os.path.join(TEST_RESOURCES_PATH, 'presentations', 'test.pptx') + doc.file_path = Path(TEST_RESOURCES_PATH, 'presentations', 'test.pptx') doc.save_titles_and_notes = MagicMock() # WHEN reading the titles and notes @@ -201,13 +201,13 @@ class TestPptviewDocument(TestCase): """ # GIVEN: mocked PresentationController.save_titles_and_notes and an nonexistent file with patch('builtins.open') as mocked_open, \ - patch('openlp.plugins.presentations.lib.pptviewcontroller.os.path.exists') as mocked_exists, \ + patch.object(Path, 'exists') as mocked_path_exists, \ patch('openlp.plugins.presentations.lib.presentationcontroller.check_directory_exists') as \ mocked_dir_exists: - mocked_exists.return_value = False + mocked_path_exists.return_value = False mocked_dir_exists.return_value = False doc = PptviewDocument(self.mock_controller, self.mock_presentation) - doc.file_path = 'Idontexist.pptx' + doc.file_path = Path('Idontexist.pptx') doc.save_titles_and_notes = MagicMock() # WHEN: Reading the titles and notes @@ -215,7 +215,7 @@ class TestPptviewDocument(TestCase): # THEN: File existens should have been checked, and not have been opened. doc.save_titles_and_notes.assert_called_once_with(None, None) - mocked_exists.assert_any_call('Idontexist.pptx') + mocked_path_exists.assert_called_with() self.assertEqual(mocked_open.call_count, 0, 'There should be no calls to open a file.') def test_create_titles_and_notes_invalid_file(self): @@ -228,7 +228,7 @@ class TestPptviewDocument(TestCase): mocked_is_zf.return_value = False mocked_open.filesize = 10 doc = PptviewDocument(self.mock_controller, self.mock_presentation) - doc.file_path = os.path.join(TEST_RESOURCES_PATH, 'presentations', 'test.ppt') + doc.file_path = Path(TEST_RESOURCES_PATH, 'presentations', 'test.ppt') doc.save_titles_and_notes = MagicMock() # WHEN: reading the titles and notes diff --git a/tests/functional/openlp_plugins/presentations/test_presentationcontroller.py b/tests/functional/openlp_plugins/presentations/test_presentationcontroller.py index c5e6d3df3..36ceb6f43 100644 --- a/tests/functional/openlp_plugins/presentations/test_presentationcontroller.py +++ b/tests/functional/openlp_plugins/presentations/test_presentationcontroller.py @@ -23,9 +23,8 @@ Functional tests to test the PresentationController and PresentationDocument classes and related methods. """ -import os from unittest import TestCase -from unittest.mock import MagicMock, mock_open, patch +from unittest.mock import MagicMock, call, patch from openlp.core.common.path import Path from openlp.plugins.presentations.lib.presentationcontroller import PresentationController, PresentationDocument @@ -40,7 +39,7 @@ class TestPresentationController(TestCase): def setUp(self): self.get_thumbnail_folder_patcher = \ patch('openlp.plugins.presentations.lib.presentationcontroller.PresentationDocument.get_thumbnail_folder', - return_value=Path()) + return_value=Path()) self.get_thumbnail_folder_patcher.start() mocked_plugin = MagicMock() mocked_plugin.settings_section = 'presentations' @@ -67,23 +66,18 @@ class TestPresentationController(TestCase): Test PresentationDocument.save_titles_and_notes method with two valid lists """ # GIVEN: two lists of length==2 and a mocked open and get_thumbnail_folder - mocked_open = mock_open() - with patch('builtins.open', mocked_open), patch(FOLDER_TO_PATCH) as mocked_get_thumbnail_folder: + with patch('openlp.plugins.presentations.lib.presentationcontroller.Path.write_text') as mocked_write_text, \ + patch(FOLDER_TO_PATCH) as mocked_get_thumbnail_folder: titles = ['uno', 'dos'] notes = ['one', 'two'] # WHEN: calling save_titles_and_notes - mocked_get_thumbnail_folder.return_value = 'test' + mocked_get_thumbnail_folder.return_value = Path('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='wt', encoding='utf-8') - mocked_open.assert_any_call(os.path.join('test', 'slideNotes1.txt'), mode='wt', encoding='utf-8') - mocked_open.assert_any_call(os.path.join('test', 'slideNotes2.txt'), mode='wt', encoding='utf-8') - 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_any_call('one') - mocked_open().write.assert_any_call('two') + self.assertEqual(mocked_write_text.call_count, 3, 'There should be exactly three files written') + mocked_write_text.assert_has_calls([call('uno\ndos'), call('one'), call('two')]) def test_save_titles_and_notes_with_None(self): """ @@ -107,10 +101,11 @@ class TestPresentationController(TestCase): """ # GIVEN: A mocked open, get_thumbnail_folder and exists - with patch('builtins.open', mock_open(read_data='uno\ndos\n')) as mocked_open, \ + with patch('openlp.plugins.presentations.lib.presentationcontroller.Path.read_text', + return_value='uno\ndos\n') as mocked_read_text, \ patch(FOLDER_TO_PATCH) as mocked_get_thumbnail_folder, \ - patch('openlp.plugins.presentations.lib.presentationcontroller.os.path.exists') as mocked_exists: - mocked_get_thumbnail_folder.return_value = 'test' + patch('openlp.plugins.presentations.lib.presentationcontroller.Path.exists') as mocked_exists: + mocked_get_thumbnail_folder.return_value = Path('test') mocked_exists.return_value = True # WHEN: calling get_titles_and_notes @@ -121,45 +116,36 @@ class TestPresentationController(TestCase): self.assertEqual(len(result_titles), 2, 'There should be two items in the titles') 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'), encoding='utf-8') - mocked_open.assert_any_call(os.path.join('test', 'slideNotes1.txt'), encoding='utf-8') - mocked_open.assert_any_call(os.path.join('test', 'slideNotes2.txt'), encoding='utf-8') - self.assertEqual(mocked_exists.call_count, 3, 'Three files should have been checked') + self.assertEqual(mocked_read_text.call_count, 3, 'Three files should be read') def test_get_titles_and_notes_with_file_not_found(self): """ Test PresentationDocument.get_titles_and_notes method with file not found """ # GIVEN: A mocked open, get_thumbnail_folder and exists - with patch('builtins.open') as mocked_open, \ - patch(FOLDER_TO_PATCH) as mocked_get_thumbnail_folder, \ - patch('openlp.plugins.presentations.lib.presentationcontroller.os.path.exists') as mocked_exists: - mocked_get_thumbnail_folder.return_value = 'test' - mocked_exists.return_value = False + with patch('openlp.plugins.presentations.lib.presentationcontroller.Path.read_text') as mocked_read_text, \ + patch(FOLDER_TO_PATCH) as mocked_get_thumbnail_folder: + mocked_read_text.side_effect = FileNotFoundError() + mocked_get_thumbnail_folder.return_value = Path('test') # WHEN: calling get_titles_and_notes result_titles, result_notes = self.document.get_titles_and_notes() # THEN: it should return two empty lists - self.assertIs(type(result_titles), list, 'result_titles should be of type list') + self.assertIsInstance(result_titles, list, 'result_titles should be of type list') self.assertEqual(len(result_titles), 0, 'there be no titles') - self.assertIs(type(result_notes), list, 'result_notes should be a list') + self.assertIsInstance(result_notes, list, '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 test_get_titles_and_notes_with_file_error(self): """ Test PresentationDocument.get_titles_and_notes method with file errors """ # GIVEN: A mocked open, get_thumbnail_folder and exists - with patch('builtins.open') as mocked_open, \ - patch(FOLDER_TO_PATCH) as mocked_get_thumbnail_folder, \ - patch('openlp.plugins.presentations.lib.presentationcontroller.os.path.exists') as mocked_exists: - mocked_get_thumbnail_folder.return_value = 'test' - mocked_exists.return_value = True - mocked_open.side_effect = IOError() + with patch('openlp.plugins.presentations.lib.presentationcontroller.Path.read_text') as mocked_read_text, \ + patch(FOLDER_TO_PATCH) as mocked_get_thumbnail_folder: + mocked_read_text.side_effect = IOError() + mocked_get_thumbnail_folder.return_value = Path('test') # WHEN: calling get_titles_and_notes result_titles, result_notes = self.document.get_titles_and_notes() @@ -180,18 +166,16 @@ class TestPresentationDocument(TestCase): patch('openlp.plugins.presentations.lib.presentationcontroller.check_directory_exists') self.get_thumbnail_folder_patcher = \ patch('openlp.plugins.presentations.lib.presentationcontroller.PresentationDocument.get_thumbnail_folder') - self.os_patcher = patch('openlp.plugins.presentations.lib.presentationcontroller.os') self._setup_patcher = \ patch('openlp.plugins.presentations.lib.presentationcontroller.PresentationDocument._setup') self.mock_check_directory_exists = self.check_directory_exists_patcher.start() self.mock_get_thumbnail_folder = self.get_thumbnail_folder_patcher.start() - self.mock_os = self.os_patcher.start() self.mock_setup = self._setup_patcher.start() self.mock_controller = MagicMock() - self.mock_get_thumbnail_folder.return_value = 'returned/path/' + self.mock_get_thumbnail_folder.return_value = Path('returned/path/') def tearDown(self): """ @@ -199,7 +183,6 @@ class TestPresentationDocument(TestCase): """ self.check_directory_exists_patcher.stop() self.get_thumbnail_folder_patcher.stop() - self.os_patcher.stop() self._setup_patcher.stop() def test_initialise_presentation_document(self): @@ -227,7 +210,7 @@ class TestPresentationDocument(TestCase): PresentationDocument(self.mock_controller, 'Name') # THEN: check_directory_exists should have been called with 'returned/path/' - self.mock_check_directory_exists.assert_called_once_with(Path('returned', 'path')) + self.mock_check_directory_exists.assert_called_once_with(Path('returned', 'path/')) self._setup_patcher.start() From d801ca9b09010e75769c994ea4e102163e9d4647 Mon Sep 17 00:00:00 2001 From: Philip Ridout Date: Mon, 18 Sep 2017 07:20:06 +0100 Subject: [PATCH 07/12] Test patched which method --- openlp/core/lib/shutil.py | 12 +- openlp/core/ui/servicemanager.py | 5 +- .../presentations/lib/impresscontroller.py | 8 +- .../functional/openlp_core_lib/test_shutil.py | 153 ++++++++++-------- 4 files changed, 97 insertions(+), 81 deletions(-) diff --git a/openlp/core/lib/shutil.py b/openlp/core/lib/shutil.py index 44dea590a..1c7a9a393 100755 --- a/openlp/core/lib/shutil.py +++ b/openlp/core/lib/shutil.py @@ -95,19 +95,17 @@ def rmtree(*args, **kwargs): args, kwargs = replace_params(args, kwargs, ((0, 'path', path_to_str),)) return shutil.rmtree(*args, **kwargs) -# TODO: Test and tidy + + def which(*args, **kwargs): """ - Wraps :func:shutil.rmtree` so that we can accept Path objects. + Wraps :func:shutil.which` so that it return a Path objects. - :param openlp.core.common.path.Path path: Takes a Path object which is then converted to a str object - :return: Passes the return from :func:`shutil.rmtree` back - :rtype: None + :rtype: openlp.core.common.Path See the following link for more information on the other parameters: - https://docs.python.org/3/library/shutil.html#shutil.rmtree + https://docs.python.org/3/library/shutil.html#shutil.which """ - file_name = shutil.which(*args, **kwargs) if file_name: return str_to_path(file_name) diff --git a/openlp/core/ui/servicemanager.py b/openlp/core/ui/servicemanager.py index eb279f267..b393ad736 100644 --- a/openlp/core/ui/servicemanager.py +++ b/openlp/core/ui/servicemanager.py @@ -376,7 +376,10 @@ class ServiceManager(OpenLPMixin, RegistryMixin, QtWidgets.QWidget, Ui_ServiceMa self._file_name = path_to_str(file_path) self.main_window.set_service_modified(self.is_modified(), self.short_file_name()) Settings().setValue('servicemanager/last file', file_path) - self._save_lite = file_path.suffix() == '.oszl' + if file_path and file_path.suffix() == '.oszl': + self._save_lite = True + else: + self._save_lite = False def file_name(self): """ diff --git a/openlp/plugins/presentations/lib/impresscontroller.py b/openlp/plugins/presentations/lib/impresscontroller.py index 472e07801..e4b45465c 100644 --- a/openlp/plugins/presentations/lib/impresscontroller.py +++ b/openlp/plugins/presentations/lib/impresscontroller.py @@ -223,10 +223,9 @@ class ImpressDocument(PresentationDocument): if desktop is None: self.controller.start_process() desktop = self.controller.get_com_desktop() - url = self.file_path.as_uri() else: desktop = self.controller.get_uno_desktop() - url = uno.systemPathToFileUrl(str(self.file_path)) + url = self.file_path.as_uri() if desktop is None: return False self.desktop = desktop @@ -253,10 +252,7 @@ class ImpressDocument(PresentationDocument): if self.check_thumbnails(): return temp_folder_path = self.get_temp_folder() - if is_win(): - thumb_dir_url = temp_folder_path.as_uri() - else: - thumb_dir_url = uno.systemPathToFileUrl(str(temp_folder_path)) + thumb_dir_url = temp_folder_path.as_uri() properties = [] properties.append(self.create_property('FilterName', 'impress_png_Export')) properties = tuple(properties) diff --git a/tests/functional/openlp_core_lib/test_shutil.py b/tests/functional/openlp_core_lib/test_shutil.py index f502e403b..0f6ca9078 100755 --- a/tests/functional/openlp_core_lib/test_shutil.py +++ b/tests/functional/openlp_core_lib/test_shutil.py @@ -3,15 +3,15 @@ from unittest import TestCase from unittest.mock import ANY, MagicMock, patch from openlp.core.common.path import Path -from openlp.core.lib import shutilpatches +from openlp.core.lib.shutil import copy, copyfile, copytree, rmtree, which -class TestShutilPatches(TestCase): +class TestShutil(TestCase): """ Tests for the :mod:`openlp.core.lib.shutil` module """ - def test_pcopy(self): + def test_copy(self): """ Test :func:`copy` """ @@ -19,133 +19,152 @@ class TestShutilPatches(TestCase): with patch('openlp.core.lib.shutil.shutil.copy', return_value=os.path.join('destination', 'test', 'path')) \ as mocked_shutil_copy: - # WHEN: Calling shutilpatches.copy with the src and dst parameters as Path object types - result = shutilpatches.copy(Path('source', 'test', 'path'), Path('destination', 'test', 'path')) + # WHEN: Calling :func:`copy` with the src and dst parameters as Path object types + result = copy(Path('source', 'test', 'path'), Path('destination', 'test', 'path')) - # THEN: `shutil.copy` should have been called with the str equivalents of the Path objects. - # `shutilpatches.copy` should return the str type result of calling `shutil.copy` as a Path - # object. + # THEN: :func:`shutil.copy` should have been called with the str equivalents of the Path objects. + # :func:`copy` should return the str type result of calling :func:`shutil.copy` as a Path object. mocked_shutil_copy.assert_called_once_with(os.path.join('source', 'test', 'path'), os.path.join('destination', 'test', 'path')) self.assertEqual(result, Path('destination', 'test', 'path')) - def test_pcopy_follow_optional_params(self): + def test_copy_follow_optional_params(self): """ Test :func:`copy` when follow_symlinks is set to false """ # GIVEN: A mocked `shutil.copy` with patch('openlp.core.lib.shutil.shutil.copy', return_value='') as mocked_shutil_copy: - # WHEN: Calling shutilpatches.copy with `follow_symlinks` set to False - shutilpatches.copy(Path('source', 'test', 'path'), - Path('destination', 'test', 'path'), - follow_symlinks=False) + # WHEN: Calling :func:`copy` with :param:`follow_symlinks` set to False + copy(Path('source', 'test', 'path'), Path('destination', 'test', 'path'), follow_symlinks=False) - # THEN: `shutil.copy` should have been called with follow_symlinks is set to false + # THEN: :func:`shutil.copy` should have been called with :param:`follow_symlinks` set to false mocked_shutil_copy.assert_called_once_with(ANY, ANY, follow_symlinks=False) - def test_pcopyfile(self): + def test_copyfile(self): """ Test :func:`copyfile` """ - # GIVEN: A mocked `shutil.copyfile` which returns a test path as a string - with patch('openlp.core.lib.shutil.shutil.copyfile', return_value=os.path.join('destination', 'test', 'path')) \ - as mocked_shutil_copyfile: + # GIVEN: A mocked :func:`shutil.copyfile` which returns a test path as a string + with patch('openlp.core.lib.shutil.shutil.copyfile', + return_value=os.path.join('destination', 'test', 'path')) as mocked_shutil_copyfile: - # WHEN: Calling shutilpatches.copyfile with the src and dst parameters as Path object types - result = shutilpatches.copyfile(Path('source', 'test', 'path'), Path('destination', 'test', 'path')) + # WHEN: Calling :func:`copyfile` with the src and dst parameters as Path object types + result = copyfile(Path('source', 'test', 'path'), Path('destination', 'test', 'path')) - # THEN: `shutil.copyfile` should have been called with the str equivalents of the Path objects. - # `shutilpatches.copyfile` should return the str type result of calling `shutil.copyfile` as a Path + # THEN: :func:`shutil.copyfile` should have been called with the str equivalents of the Path objects. + # :func:`copyfile` should return the str type result of calling :func:`shutil.copyfile` as a Path # object. mocked_shutil_copyfile.assert_called_once_with(os.path.join('source', 'test', 'path'), os.path.join('destination', 'test', 'path')) self.assertEqual(result, Path('destination', 'test', 'path')) - def test_pcopyfile_optional_params(self): + def test_copyfile_optional_params(self): """ Test :func:`copyfile` when follow_symlinks is set to false """ - # GIVEN: A mocked `shutil.copyfile` + # GIVEN: A mocked :func:`shutil.copyfile` with patch('openlp.core.lib.shutil.shutil.copyfile', return_value='') as mocked_shutil_copyfile: - # WHEN: Calling shutilpatches.copyfile with `follow_symlinks` set to False - shutilpatches.copyfile(Path('source', 'test', 'path'), - Path('destination', 'test', 'path'), - follow_symlinks=False) + # WHEN: Calling :func:`copyfile` with :param:`follow_symlinks` set to False + copyfile(Path('source', 'test', 'path'), Path('destination', 'test', 'path'), follow_symlinks=False) - # THEN: `shutil.copyfile` should have been called with the optional parameters, with out any of the values - # being modified + # THEN: :func:`shutil.copyfile` should have been called with the optional parameters, with out any of the + # values being modified mocked_shutil_copyfile.assert_called_once_with(ANY, ANY, follow_symlinks=False) - def test_pcopytree(self): + def test_copytree(self): """ Test :func:`copytree` """ - # GIVEN: A mocked `shutil.copytree` which returns a test path as a string - with patch('openlp.core.lib.shutil.shutil.copytree', return_value=os.path.join('destination', 'test', 'path')) \ - as mocked_shutil_copytree: + # GIVEN: A mocked :func:`shutil.copytree` which returns a test path as a string + with patch('openlp.core.lib.shutil.shutil.copytree', + return_value=os.path.join('destination', 'test', 'path')) as mocked_shutil_copytree: - # WHEN: Calling shutilpatches.copytree with the src and dst parameters as Path object types - result = shutilpatches.copytree(Path('source', 'test', 'path'), Path('destination', 'test', 'path')) + # WHEN: Calling :func:`copytree` with the src and dst parameters as Path object types + result = copytree(Path('source', 'test', 'path'), Path('destination', 'test', 'path')) - # THEN: `shutil.copytree` should have been called with the str equivalents of the Path objects. - # `shutilpatches.copytree` should return the str type result of calling `shutil.copytree` as a Path - # object. + # THEN: :func:`shutil.copytree` should have been called with the str equivalents of the Path objects. + # :func:`patches.copytree` should return the str type result of calling :func:`shutil.copytree` as a + # Path object. mocked_shutil_copytree.assert_called_once_with(os.path.join('source', 'test', 'path'), os.path.join('destination', 'test', 'path')) self.assertEqual(result, Path('destination', 'test', 'path')) - def test_pcopytree_optional_params(self): + def test_copytree_optional_params(self): """ Test :func:`copytree` when optional parameters are passed """ - # GIVEN: A mocked `shutil.copytree` + # GIVEN: A mocked :func:`shutil.copytree` with patch('openlp.core.lib.shutil.shutil.copytree', return_value='') as mocked_shutil_copytree: mocked_ignore = MagicMock() mocked_copy_function = MagicMock() - # WHEN: Calling shutilpatches.copytree with the optional parameters set - shutilpatches.copytree(Path('source', 'test', 'path'), - Path('destination', 'test', 'path'), - symlinks=True, - ignore=mocked_ignore, - copy_function=mocked_copy_function, - ignore_dangling_symlinks=True) + # WHEN: Calling :func:`copytree` with the optional parameters set + copytree(Path('source', 'test', 'path'), Path('destination', 'test', 'path'), symlinks=True, + ignore=mocked_ignore, copy_function=mocked_copy_function, ignore_dangling_symlinks=True) - # THEN: `shutil.copytree` should have been called with the optional parameters, with out any of the values - # being modified - mocked_shutil_copytree.assert_called_once_with(ANY, ANY, - symlinks=True, - ignore=mocked_ignore, + # THEN: :func:`shutil.copytree` should have been called with the optional parameters, with out any of the + # values being modified + mocked_shutil_copytree.assert_called_once_with(ANY, ANY, symlinks=True, ignore=mocked_ignore, copy_function=mocked_copy_function, ignore_dangling_symlinks=True) - def test_prmtree(self): + def test_rmtree(self): """ Test :func:`rmtree` """ - # GIVEN: A mocked `shutil.rmtree` - with patch('openlp.core.lib.shutil.shutil.rmtree', return_value=None) as mocked_rmtree: + # GIVEN: A mocked :func:`shutil.rmtree` + with patch('openlp.core.lib.shutil.shutil.rmtree', return_value=None) as mocked_shutil_rmtree: - # WHEN: Calling shutilpatches.rmtree with the path parameter as Path object type - result = shutilpatches.rmtree(Path('test', 'path')) + # WHEN: Calling :func:`rmtree` with the path parameter as Path object type + result = rmtree(Path('test', 'path')) - # THEN: `shutil.rmtree` should have been called with the str equivalents of the Path object. - mocked_rmtree.assert_called_once_with(os.path.join('test', 'path')) + # THEN: :func:`shutil.rmtree` should have been called with the str equivalents of the Path object. + mocked_shutil_rmtree.assert_called_once_with(os.path.join('test', 'path')) self.assertIsNone(result) - def test_prmtree_optional_params(self): + def test_rmtree_optional_params(self): """ - Test :func:`rmtree` when optional parameters are passed + Test :func:`rmtree` when optional parameters are passed """ - # GIVEN: A mocked `shutil.rmtree` + # GIVEN: A mocked :func:`shutil.rmtree` with patch('openlp.core.lib.shutil.shutil.rmtree', return_value='') as mocked_shutil_rmtree: mocked_on_error = MagicMock() - # WHEN: Calling shutilpatches.rmtree with `ignore_errors` set to True and `onerror` set to a mocked object - shutilpatches.rmtree(Path('test', 'path'), ignore_errors=True, onerror=mocked_on_error) + # WHEN: Calling :func:`rmtree` with :param:`ignore_errors` set to True and `onerror` set to a mocked object + rmtree(Path('test', 'path'), ignore_errors=True, onerror=mocked_on_error) - # THEN: `shutil.rmtree` should have been called with the optional parameters, with out any of the values - # being modified + # THEN: :func:`shutil.rmtree` should have been called with the optional parameters, with out any of the + # values being modified mocked_shutil_rmtree.assert_called_once_with(ANY, ignore_errors=True, onerror=mocked_on_error) + + def test_which_no_command(self): + """ + Test :func:`which` when the command is not found. + """ + # GIVEN: A mocked :func:``shutil.which` when the command is not found. + with patch('openlp.core.lib.shutil.shutil.which', return_value=None) as mocked_shutil_which: + + # WHEN: Calling :func:`which` with a command that does not exist. + result = which('no_command') + + # THEN: :func:`shutil.which` should have been called with the command, and :func:`which` should return None. + mocked_shutil_which.assert_called_once_with('no_command') + self.assertIsNone(result) + + def test_which_command(self): + """ + Test :func:`which` when a command has been found. + """ + # GIVEN: A mocked :func:`shutil.which` when the command is found. + with patch('openlp.core.lib.shutil.shutil.which', + return_value=os.path.join('path', 'to', 'command')) as mocked_shutil_which: + + # WHEN: Calling :func:`which` with a command that exists. + result = which('command') + + # THEN: :func:`shutil.which` should have been called with the command, and :func:`which` should return a + # Path object equivalent of the command path. + mocked_shutil_which.assert_called_once_with('command') + self.assertEqual(result, Path('path', 'to', 'command')) From 0ee8ebb1c2f58cb4713d1c27456d6e0f01ba24f5 Mon Sep 17 00:00:00 2001 From: Philip Ridout Date: Mon, 18 Sep 2017 07:32:19 +0100 Subject: [PATCH 08/12] PEP fixes --- tests/functional/openlp_core_lib/test_shutil.py | 8 ++++---- .../presentations/test_presentationcontroller.py | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/functional/openlp_core_lib/test_shutil.py b/tests/functional/openlp_core_lib/test_shutil.py index 0f6ca9078..737f7ce00 100755 --- a/tests/functional/openlp_core_lib/test_shutil.py +++ b/tests/functional/openlp_core_lib/test_shutil.py @@ -13,7 +13,7 @@ class TestShutil(TestCase): def test_copy(self): """ - Test :func:`copy` + Test :func:`copy` """ # GIVEN: A mocked `shutil.copy` which returns a test path as a string with patch('openlp.core.lib.shutil.shutil.copy', return_value=os.path.join('destination', 'test', 'path')) \ @@ -43,7 +43,7 @@ class TestShutil(TestCase): def test_copyfile(self): """ - Test :func:`copyfile` + Test :func:`copyfile` """ # GIVEN: A mocked :func:`shutil.copyfile` which returns a test path as a string with patch('openlp.core.lib.shutil.shutil.copyfile', @@ -75,7 +75,7 @@ class TestShutil(TestCase): def test_copytree(self): """ - Test :func:`copytree` + Test :func:`copytree` """ # GIVEN: A mocked :func:`shutil.copytree` which returns a test path as a string with patch('openlp.core.lib.shutil.shutil.copytree', @@ -112,7 +112,7 @@ class TestShutil(TestCase): def test_rmtree(self): """ - Test :func:`rmtree` + Test :func:`rmtree` """ # GIVEN: A mocked :func:`shutil.rmtree` with patch('openlp.core.lib.shutil.shutil.rmtree', return_value=None) as mocked_shutil_rmtree: diff --git a/tests/functional/openlp_plugins/presentations/test_presentationcontroller.py b/tests/functional/openlp_plugins/presentations/test_presentationcontroller.py index 36ceb6f43..4cf2a1a01 100644 --- a/tests/functional/openlp_plugins/presentations/test_presentationcontroller.py +++ b/tests/functional/openlp_plugins/presentations/test_presentationcontroller.py @@ -39,7 +39,7 @@ class TestPresentationController(TestCase): def setUp(self): self.get_thumbnail_folder_patcher = \ patch('openlp.plugins.presentations.lib.presentationcontroller.PresentationDocument.get_thumbnail_folder', - return_value=Path()) + return_value=Path()) self.get_thumbnail_folder_patcher.start() mocked_plugin = MagicMock() mocked_plugin.settings_section = 'presentations' From 92c6b9c09dd2a5d93b1211d850c30165ac94b3af Mon Sep 17 00:00:00 2001 From: Philip Ridout Date: Mon, 18 Sep 2017 21:08:28 +0100 Subject: [PATCH 09/12] Revert some requested changes --- openlp/core/common/registry.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/openlp/core/common/registry.py b/openlp/core/common/registry.py index 33afe6f21..1894ac458 100644 --- a/openlp/core/common/registry.py +++ b/openlp/core/common/registry.py @@ -138,8 +138,11 @@ class Registry(object): if result: results.append(result) except TypeError: + # Who has called me can help in debugging + trace_error_handler(log) log.exception('Exception for function {function}'.format(function=function)) else: + trace_error_handler(log) log.exception('Event {event} called but not registered'.format(event=event)) return results From b440584cb54a68200bd1d5bd53eea313c9a5b834 Mon Sep 17 00:00:00 2001 From: Philip Ridout Date: Wed, 20 Sep 2017 21:44:57 +0100 Subject: [PATCH 10/12] Moved the patched shuilils to the path module --- openlp/core/__init__.py | 3 +- openlp/core/common/path.py | 117 ++++++++++ openlp/core/lib/__init__.py | 29 --- openlp/core/lib/shutil.py | 112 ---------- openlp/core/ui/lib/filedialog.py | 3 +- openlp/core/ui/mainwindow.py | 3 +- .../presentations/lib/pdfcontroller.py | 3 +- .../lib/presentationcontroller.py | 3 +- .../openlp_core_common/test_path.py | 203 +++++++++++++++++- tests/functional/openlp_core_lib/test_lib.py | 34 +-- .../functional/openlp_core_lib/test_shutil.py | 170 --------------- 11 files changed, 325 insertions(+), 355 deletions(-) delete mode 100755 openlp/core/lib/shutil.py delete mode 100755 tests/functional/openlp_core_lib/test_shutil.py diff --git a/openlp/core/__init__.py b/openlp/core/__init__.py index 0fcea2d1a..8cd62a97f 100644 --- a/openlp/core/__init__.py +++ b/openlp/core/__init__.py @@ -38,10 +38,9 @@ from PyQt5 import QtCore, QtGui, QtWidgets from openlp.core.common import Registry, OpenLPMixin, AppLocation, LanguageManager, Settings, UiStrings, \ check_directory_exists, is_macosx, is_win, translate -from openlp.core.common.path import Path +from openlp.core.common.path import Path, copytree from openlp.core.common.versionchecker import VersionThread, get_application_version from openlp.core.lib import ScreenList -from openlp.core.lib.shutil import copytree from openlp.core.resources import qInitResources from openlp.core.ui import SplashScreen from openlp.core.ui.exceptionform import ExceptionForm diff --git a/openlp/core/common/path.py b/openlp/core/common/path.py index 3c4dd93c9..f11c4bb9f 100644 --- a/openlp/core/common/path.py +++ b/openlp/core/common/path.py @@ -19,6 +19,7 @@ # with this program; if not, write to the Free Software Foundation, Inc., 59 # # Temple Place, Suite 330, Boston, MA 02111-1307 USA # ############################################################################### +import shutil from contextlib import suppress from openlp.core.common import is_win @@ -29,6 +30,121 @@ else: from pathlib import PosixPath as PathVariant +def replace_params(args, kwargs, params): + """ + Apply a transformation function to the specified args or kwargs + + :param tuple args: Positional arguments + :param dict kwargs: Key Word arguments + :param params: A tuple of tuples with the position and the key word to replace. + :return: The modified positional and keyword arguments + :rtype: tuple[tuple, dict] + + + Usage: + Take a method with the following signature, and assume we which to apply the str function to arg2: + def method(arg1=None, arg2=None, arg3=None) + + As arg2 can be specified postitionally as the second argument (1 with a zero index) or as a keyword, the we + would call this function as follows: + + replace_params(args, kwargs, ((1, 'arg2', str),)) + """ + args = list(args) + for position, key_word, transform in params: + if len(args) > position: + args[position] = transform(args[position]) + elif key_word in kwargs: + kwargs[key_word] = transform(kwargs[key_word]) + return tuple(args), kwargs + + +def copy(*args, **kwargs): + """ + Wraps :func:`shutil.copy` so that we can accept Path objects. + + :param src openlp.core.common.path.Path: Takes a Path object which is then converted to a str object + :param dst openlp.core.common.path.Path: Takes a Path object which is then converted to a str object + :return: Converts the str object received from :func:`shutil.copy` to a Path or NoneType object + :rtype: openlp.core.common.path.Path | None + + See the following link for more information on the other parameters: + https://docs.python.org/3/library/shutil.html#shutil.copy + """ + + args, kwargs = replace_params(args, kwargs, ((0, 'src', path_to_str), (1, 'dst', path_to_str))) + + return str_to_path(shutil.copy(*args, **kwargs)) + + +def copyfile(*args, **kwargs): + """ + Wraps :func:`shutil.copyfile` so that we can accept Path objects. + + :param openlp.core.common.path.Path src: Takes a Path object which is then converted to a str object + :param openlp.core.common.path.Path dst: Takes a Path object which is then converted to a str object + :return: Converts the str object received from :func:`shutil.copyfile` to a Path or NoneType object + :rtype: openlp.core.common.path.Path | None + + See the following link for more information on the other parameters: + https://docs.python.org/3/library/shutil.html#shutil.copyfile + """ + + args, kwargs = replace_params(args, kwargs, ((0, 'src', path_to_str), (1, 'dst', path_to_str))) + + return str_to_path(shutil.copyfile(*args, **kwargs)) + + +def copytree(*args, **kwargs): + """ + Wraps :func:shutil.copytree` so that we can accept Path objects. + + :param openlp.core.common.path.Path src : Takes a Path object which is then converted to a str object + :param openlp.core.common.path.Path dst: Takes a Path object which is then converted to a str object + :return: Converts the str object received from :func:`shutil.copytree` to a Path or NoneType object + :rtype: openlp.core.common.path.Path | None + + See the following link for more information on the other parameters: + https://docs.python.org/3/library/shutil.html#shutil.copytree + """ + + args, kwargs = replace_params(args, kwargs, ((0, 'src', path_to_str), (1, 'dst', path_to_str))) + + return str_to_path(shutil.copytree(*args, **kwargs)) + + +def rmtree(*args, **kwargs): + """ + Wraps :func:shutil.rmtree` so that we can accept Path objects. + + :param openlp.core.common.path.Path path: Takes a Path object which is then converted to a str object + :return: Passes the return from :func:`shutil.rmtree` back + :rtype: None + + See the following link for more information on the other parameters: + https://docs.python.org/3/library/shutil.html#shutil.rmtree + """ + + args, kwargs = replace_params(args, kwargs, ((0, 'path', path_to_str),)) + + return shutil.rmtree(*args, **kwargs) + + +def which(*args, **kwargs): + """ + Wraps :func:shutil.which` so that it return a Path objects. + + :rtype: openlp.core.common.Path + + See the following link for more information on the other parameters: + https://docs.python.org/3/library/shutil.html#shutil.which + """ + file_name = shutil.which(*args, **kwargs) + if file_name: + return str_to_path(file_name) + return None + + def path_to_str(path=None): """ A utility function to convert a Path object or NoneType to a string equivalent. @@ -98,3 +214,4 @@ class Path(PathVariant): with suppress(ValueError): path = path.relative_to(base_path) return {'__Path__': path.parts} + diff --git a/openlp/core/lib/__init__.py b/openlp/core/lib/__init__.py index 1d55df497..4602fee2c 100644 --- a/openlp/core/lib/__init__.py +++ b/openlp/core/lib/__init__.py @@ -611,35 +611,6 @@ def create_separated_list(string_list): return list_to_string -def replace_params(args, kwargs, params): - """ - Apply a transformation function to the specified args or kwargs - - :param tuple args: Positional arguments - :param dict kwargs: Key Word arguments - :param params: A tuple of tuples with the position and the key word to replace. - :return: The modified positional and keyword arguments - :rtype: tuple[tuple, dict] - - - Usage: - Take a method with the following signature, and assume we which to apply the str function to arg2: - def method(arg1=None, arg2=None, arg3=None) - - As arg2 can be specified postitionally as the second argument (1 with a zero index) or as a keyword, the we - would call this function as follows: - - replace_params(args, kwargs, ((1, 'arg2', str),)) - """ - args = list(args) - for position, key_word, transform in params: - if len(args) > position: - args[position] = transform(args[position]) - elif key_word in kwargs: - kwargs[key_word] = transform(kwargs[key_word]) - return tuple(args), kwargs - - from .exceptions import ValidationError from .screen import ScreenList from .formattingtags import FormattingTags diff --git a/openlp/core/lib/shutil.py b/openlp/core/lib/shutil.py deleted file mode 100755 index 1c7a9a393..000000000 --- a/openlp/core/lib/shutil.py +++ /dev/null @@ -1,112 +0,0 @@ -# -*- coding: utf-8 -*- -# vim: autoindent shiftwidth=4 expandtab textwidth=120 tabstop=4 softtabstop=4 - -############################################################################### -# OpenLP - Open Source Lyrics Projection # -# --------------------------------------------------------------------------- # -# Copyright (c) 2008-2017 OpenLP Developers # -# --------------------------------------------------------------------------- # -# This program is free software; you can redistribute it and/or modify it # -# under the terms of the GNU General Public License as published by the Free # -# Software Foundation; version 2 of the License. # -# # -# This program is distributed in the hope that it will be useful, but WITHOUT # -# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or # -# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for # -# more details. # -# # -# You should have received a copy of the GNU General Public License along # -# with this program; if not, write to the Free Software Foundation, Inc., 59 # -# Temple Place, Suite 330, Boston, MA 02111-1307 USA # -############################################################################### -""" Patch the shutil methods we use so they accept and return Path objects""" -import shutil - -from openlp.core.common.path import path_to_str, str_to_path -from openlp.core.lib import replace_params - - -def copy(*args, **kwargs): - """ - Wraps :func:`shutil.copy` so that we can accept Path objects. - - :param src openlp.core.common.path.Path: Takes a Path object which is then converted to a str object - :param dst openlp.core.common.path.Path: Takes a Path object which is then converted to a str object - :return: Converts the str object received from :func:`shutil.copy` to a Path or NoneType object - :rtype: openlp.core.common.path.Path | None - - See the following link for more information on the other parameters: - https://docs.python.org/3/library/shutil.html#shutil.copy - """ - - args, kwargs = replace_params(args, kwargs, ((0, 'src', path_to_str), (1, 'dst', path_to_str))) - - return str_to_path(shutil.copy(*args, **kwargs)) - - -def copyfile(*args, **kwargs): - """ - Wraps :func:`shutil.copyfile` so that we can accept Path objects. - - :param openlp.core.common.path.Path src: Takes a Path object which is then converted to a str object - :param openlp.core.common.path.Path dst: Takes a Path object which is then converted to a str object - :return: Converts the str object received from :func:`shutil.copyfile` to a Path or NoneType object - :rtype: openlp.core.common.path.Path | None - - See the following link for more information on the other parameters: - https://docs.python.org/3/library/shutil.html#shutil.copyfile - """ - - args, kwargs = replace_params(args, kwargs, ((0, 'src', path_to_str), (1, 'dst', path_to_str))) - - return str_to_path(shutil.copyfile(*args, **kwargs)) - - -def copytree(*args, **kwargs): - """ - Wraps :func:shutil.copytree` so that we can accept Path objects. - - :param openlp.core.common.path.Path src : Takes a Path object which is then converted to a str object - :param openlp.core.common.path.Path dst: Takes a Path object which is then converted to a str object - :return: Converts the str object received from :func:`shutil.copytree` to a Path or NoneType object - :rtype: openlp.core.common.path.Path | None - - See the following link for more information on the other parameters: - https://docs.python.org/3/library/shutil.html#shutil.copytree - """ - - args, kwargs = replace_params(args, kwargs, ((0, 'src', path_to_str), (1, 'dst', path_to_str))) - - return str_to_path(shutil.copytree(*args, **kwargs)) - - -def rmtree(*args, **kwargs): - """ - Wraps :func:shutil.rmtree` so that we can accept Path objects. - - :param openlp.core.common.path.Path path: Takes a Path object which is then converted to a str object - :return: Passes the return from :func:`shutil.rmtree` back - :rtype: None - - See the following link for more information on the other parameters: - https://docs.python.org/3/library/shutil.html#shutil.rmtree - """ - - args, kwargs = replace_params(args, kwargs, ((0, 'path', path_to_str),)) - - return shutil.rmtree(*args, **kwargs) - - -def which(*args, **kwargs): - """ - Wraps :func:shutil.which` so that it return a Path objects. - - :rtype: openlp.core.common.Path - - See the following link for more information on the other parameters: - https://docs.python.org/3/library/shutil.html#shutil.which - """ - file_name = shutil.which(*args, **kwargs) - if file_name: - return str_to_path(file_name) - return None diff --git a/openlp/core/ui/lib/filedialog.py b/openlp/core/ui/lib/filedialog.py index d4a702e83..0f3ef4058 100755 --- a/openlp/core/ui/lib/filedialog.py +++ b/openlp/core/ui/lib/filedialog.py @@ -22,8 +22,7 @@ """ Patch the QFileDialog so it accepts and returns Path objects""" from PyQt5 import QtWidgets -from openlp.core.common.path import Path, path_to_str, str_to_path -from openlp.core.lib import replace_params +from openlp.core.common.path import Path, path_to_str, replace_params, str_to_path class FileDialog(QtWidgets.QFileDialog): diff --git a/openlp/core/ui/mainwindow.py b/openlp/core/ui/mainwindow.py index 88799b060..4b505b807 100644 --- a/openlp/core/ui/mainwindow.py +++ b/openlp/core/ui/mainwindow.py @@ -39,10 +39,9 @@ from openlp.core.api.http import server from openlp.core.common import Registry, RegistryProperties, AppLocation, LanguageManager, Settings, UiStrings, \ check_directory_exists, translate, is_win, is_macosx, add_actions from openlp.core.common.actions import ActionList, CategoryOrder -from openlp.core.common.path import Path, path_to_str, str_to_path +from openlp.core.common.path import Path, copyfile, path_to_str, str_to_path from openlp.core.common.versionchecker import get_application_version from openlp.core.lib import Renderer, PluginManager, ImageManager, PluginStatus, ScreenList, build_icon -from openlp.core.lib.shutil import copyfile from openlp.core.lib.ui import create_action from openlp.core.ui import AboutForm, SettingsForm, ServiceManager, ThemeManager, LiveController, PluginForm, \ ShortcutListForm, FormattingTagForm, PreviewController diff --git a/openlp/plugins/presentations/lib/pdfcontroller.py b/openlp/plugins/presentations/lib/pdfcontroller.py index a39cce36c..81fa3994a 100644 --- a/openlp/plugins/presentations/lib/pdfcontroller.py +++ b/openlp/plugins/presentations/lib/pdfcontroller.py @@ -27,9 +27,8 @@ from subprocess import check_output, CalledProcessError from openlp.core.common import AppLocation, check_binary_exists from openlp.core.common import Settings, is_win -from openlp.core.common.path import Path, path_to_str +from openlp.core.common.path import which from openlp.core.lib import ScreenList -from openlp.core.lib.shutil import which from openlp.plugins.presentations.lib.presentationcontroller import PresentationController, PresentationDocument if is_win(): diff --git a/openlp/plugins/presentations/lib/presentationcontroller.py b/openlp/plugins/presentations/lib/presentationcontroller.py index 3225eac24..304d70833 100644 --- a/openlp/plugins/presentations/lib/presentationcontroller.py +++ b/openlp/plugins/presentations/lib/presentationcontroller.py @@ -24,9 +24,8 @@ import logging from PyQt5 import QtCore from openlp.core.common import Registry, AppLocation, Settings, check_directory_exists, md5_hash -from openlp.core.common.path import Path +from openlp.core.common.path import Path, rmtree from openlp.core.lib import create_thumb, validate_thumb -from openlp.core.lib.shutil import rmtree log = logging.getLogger(__name__) diff --git a/tests/functional/openlp_core_common/test_path.py b/tests/functional/openlp_core_common/test_path.py index f5abcffd5..0f35319c9 100644 --- a/tests/functional/openlp_core_common/test_path.py +++ b/tests/functional/openlp_core_common/test_path.py @@ -24,8 +24,209 @@ Package to test the openlp.core.common.path package. """ import os from unittest import TestCase +from unittest.mock import ANY, MagicMock, patch -from openlp.core.common.path import Path, path_to_str, str_to_path +from openlp.core.common.path import Path, copy, copyfile, copytree, path_to_str, replace_params, rmtree, str_to_path, \ + which + + +class TestShutil(TestCase): + """ + Tests for the :mod:`openlp.core.common.path` module + """ + def test_replace_params_no_params(self): + """ + Test replace_params when called with and empty tuple instead of parameters to replace + """ + # GIVEN: Some test data + test_args = (1, 2) + test_kwargs = {'arg3': 3, 'arg4': 4} + test_params = tuple() + + # WHEN: Calling replace_params + result_args, result_kwargs = replace_params(test_args, test_kwargs, test_params) + + # THEN: The positional and keyword args should not have changed + self.assertEqual(test_args, result_args) + self.assertEqual(test_kwargs, result_kwargs) + + def test_replace_params_params(self): + """ + Test replace_params when given a positional and a keyword argument to change + """ + # GIVEN: Some test data + test_args = (1, 2) + test_kwargs = {'arg3': 3, 'arg4': 4} + test_params = ((1, 'arg2', str), (2, 'arg3', str)) + + # WHEN: Calling replace_params + result_args, result_kwargs = replace_params(test_args, test_kwargs, test_params) + + # THEN: The positional and keyword args should have have changed + self.assertEqual(result_args, (1, '2')) + self.assertEqual(result_kwargs, {'arg3': '3', 'arg4': 4}) + + def test_copy(self): + """ + Test :func:`openlp.core.common.path.copy` + """ + # GIVEN: A mocked `shutil.copy` which returns a test path as a string + with patch('openlp.core.common.path.shutil.copy', return_value=os.path.join('destination', 'test', 'path')) \ + as mocked_shutil_copy: + + # WHEN: Calling :func:`openlp.core.common.path.copy` with the src and dst parameters as Path object types + result = copy(Path('source', 'test', 'path'), Path('destination', 'test', 'path')) + + # THEN: :func:`shutil.copy` should have been called with the str equivalents of the Path objects. + # :func:`openlp.core.common.path.copy` should return the str type result of calling + # :func:`shutil.copy` as a Path object. + mocked_shutil_copy.assert_called_once_with(os.path.join('source', 'test', 'path'), + os.path.join('destination', 'test', 'path')) + self.assertEqual(result, Path('destination', 'test', 'path')) + + def test_copy_follow_optional_params(self): + """ + Test :func:`openlp.core.common.path.copy` when follow_symlinks is set to false + """ + # GIVEN: A mocked `shutil.copy` + with patch('openlp.core.common.path.shutil.copy', return_value='') as mocked_shutil_copy: + + # WHEN: Calling :func:`openlp.core.common.path.copy` with :param:`follow_symlinks` set to False + copy(Path('source', 'test', 'path'), Path('destination', 'test', 'path'), follow_symlinks=False) + + # THEN: :func:`shutil.copy` should have been called with :param:`follow_symlinks` set to false + mocked_shutil_copy.assert_called_once_with(ANY, ANY, follow_symlinks=False) + + def test_copyfile(self): + """ + Test :func:`openlp.core.common.path.copyfile` + """ + # GIVEN: A mocked :func:`shutil.copyfile` which returns a test path as a string + with patch('openlp.core.common.path.shutil.copyfile', + return_value=os.path.join('destination', 'test', 'path')) as mocked_shutil_copyfile: + + # WHEN: Calling :func:`openlp.core.common.path.copyfile` with the src and dst parameters as Path object + # types + result = copyfile(Path('source', 'test', 'path'), Path('destination', 'test', 'path')) + + # THEN: :func:`shutil.copyfile` should have been called with the str equivalents of the Path objects. + # :func:`openlp.core.common.path.copyfile` should return the str type result of calling + # :func:`shutil.copyfile` as a Path object. + mocked_shutil_copyfile.assert_called_once_with(os.path.join('source', 'test', 'path'), + os.path.join('destination', 'test', 'path')) + self.assertEqual(result, Path('destination', 'test', 'path')) + + def test_copyfile_optional_params(self): + """ + Test :func:`openlp.core.common.path.copyfile` when follow_symlinks is set to false + """ + # GIVEN: A mocked :func:`shutil.copyfile` + with patch('openlp.core.common.path.shutil.copyfile', return_value='') as mocked_shutil_copyfile: + + # WHEN: Calling :func:`openlp.core.common.path.copyfile` with :param:`follow_symlinks` set to False + copyfile(Path('source', 'test', 'path'), Path('destination', 'test', 'path'), follow_symlinks=False) + + # THEN: :func:`shutil.copyfile` should have been called with the optional parameters, with out any of the + # values being modified + mocked_shutil_copyfile.assert_called_once_with(ANY, ANY, follow_symlinks=False) + + def test_copytree(self): + """ + Test :func:`openlp.core.common.path.copytree` + """ + # GIVEN: A mocked :func:`shutil.copytree` which returns a test path as a string + with patch('openlp.core.common.path.shutil.copytree', + return_value=os.path.join('destination', 'test', 'path')) as mocked_shutil_copytree: + + # WHEN: Calling :func:`openlp.core.common.path.copytree` with the src and dst parameters as Path object + # types + result = copytree(Path('source', 'test', 'path'), Path('destination', 'test', 'path')) + + # THEN: :func:`shutil.copytree` should have been called with the str equivalents of the Path objects. + # :func:`openlp.core.common.path.copytree` should return the str type result of calling + # :func:`shutil.copytree` as a Path object. + mocked_shutil_copytree.assert_called_once_with(os.path.join('source', 'test', 'path'), + os.path.join('destination', 'test', 'path')) + self.assertEqual(result, Path('destination', 'test', 'path')) + + def test_copytree_optional_params(self): + """ + Test :func:`openlp.core.common.path.copytree` when optional parameters are passed + """ + # GIVEN: A mocked :func:`shutil.copytree` + with patch('openlp.core.common.path.shutil.copytree', return_value='') as mocked_shutil_copytree: + mocked_ignore = MagicMock() + mocked_copy_function = MagicMock() + + # WHEN: Calling :func:`openlp.core.common.path.copytree` with the optional parameters set + copytree(Path('source', 'test', 'path'), Path('destination', 'test', 'path'), symlinks=True, + ignore=mocked_ignore, copy_function=mocked_copy_function, ignore_dangling_symlinks=True) + + # THEN: :func:`shutil.copytree` should have been called with the optional parameters, with out any of the + # values being modified + mocked_shutil_copytree.assert_called_once_with(ANY, ANY, symlinks=True, ignore=mocked_ignore, + copy_function=mocked_copy_function, + ignore_dangling_symlinks=True) + + def test_rmtree(self): + """ + Test :func:`rmtree` + """ + # GIVEN: A mocked :func:`shutil.rmtree` + with patch('openlp.core.common.path.shutil.rmtree', return_value=None) as mocked_shutil_rmtree: + + # WHEN: Calling :func:`openlp.core.common.path.rmtree` with the path parameter as Path object type + result = rmtree(Path('test', 'path')) + + # THEN: :func:`shutil.rmtree` should have been called with the str equivalents of the Path object. + mocked_shutil_rmtree.assert_called_once_with(os.path.join('test', 'path')) + self.assertIsNone(result) + + def test_rmtree_optional_params(self): + """ + Test :func:`openlp.core.common.path.rmtree` when optional parameters are passed + """ + # GIVEN: A mocked :func:`shutil.rmtree` + with patch('openlp.core.common.path.shutil.rmtree', return_value='') as mocked_shutil_rmtree: + mocked_on_error = MagicMock() + + # WHEN: Calling :func:`openlp.core.common.path.rmtree` with :param:`ignore_errors` set to True and + # :param:`onerror` set to a mocked object + rmtree(Path('test', 'path'), ignore_errors=True, onerror=mocked_on_error) + + # THEN: :func:`shutil.rmtree` should have been called with the optional parameters, with out any of the + # values being modified + mocked_shutil_rmtree.assert_called_once_with(ANY, ignore_errors=True, onerror=mocked_on_error) + + def test_which_no_command(self): + """ + Test :func:`openlp.core.common.path.which` when the command is not found. + """ + # GIVEN: A mocked :func:`shutil.which` when the command is not found. + with patch('openlp.core.common.path.shutil.which', return_value=None) as mocked_shutil_which: + + # WHEN: Calling :func:`openlp.core.common.path.which` with a command that does not exist. + result = which('no_command') + + # THEN: :func:`shutil.which` should have been called with the command, and :func:`which` should return None. + mocked_shutil_which.assert_called_once_with('no_command') + self.assertIsNone(result) + + def test_which_command(self): + """ + Test :func:`openlp.core.common.path.which` when a command has been found. + """ + # GIVEN: A mocked :func:`shutil.which` when the command is found. + with patch('openlp.core.common.path.shutil.which', + return_value=os.path.join('path', 'to', 'command')) as mocked_shutil_which: + + # WHEN: Calling :func:`openlp.core.common.path.which` with a command that exists. + result = which('command') + + # THEN: :func:`shutil.which` should have been called with the command, and :func:`which` should return a + # Path object equivalent of the command path. + mocked_shutil_which.assert_called_once_with('command') + self.assertEqual(result, Path('path', 'to', 'command')) class TestPath(TestCase): diff --git a/tests/functional/openlp_core_lib/test_lib.py b/tests/functional/openlp_core_lib/test_lib.py index 8b46e99c3..96e78e351 100644 --- a/tests/functional/openlp_core_lib/test_lib.py +++ b/tests/functional/openlp_core_lib/test_lib.py @@ -32,7 +32,7 @@ from PyQt5 import QtCore, QtGui from openlp.core.common.path import Path from openlp.core.lib import FormattingTags, build_icon, check_item_selected, clean_tags, compare_chord_lyric, \ create_separated_list, create_thumb, expand_chords, expand_chords_for_printing, expand_tags, find_formatting_tags, \ - get_text_file_string, image_to_byte, replace_params, resize_image, str_to_bool, validate_thumb + get_text_file_string, image_to_byte, resize_image, str_to_bool, validate_thumb TEST_PATH = os.path.abspath(os.path.join(os.path.dirname(__file__), '..', '..', 'resources')) @@ -636,38 +636,6 @@ class TestLib(TestCase): thumb_path.stat.assert_called_once_with() self.assertFalse(result, 'The result should be False') - def test_replace_params_no_params(self): - """ - Test replace_params when called with and empty tuple instead of parameters to replace - """ - # GIVEN: Some test data - test_args = (1, 2) - test_kwargs = {'arg3': 3, 'arg4': 4} - test_params = tuple() - - # WHEN: Calling replace_params - result_args, result_kwargs = replace_params(test_args, test_kwargs, test_params) - - # THEN: The positional and keyword args should not have changed - self.assertEqual(test_args, result_args) - self.assertEqual(test_kwargs, result_kwargs) - - def test_replace_params_params(self): - """ - Test replace_params when given a positional and a keyword argument to change - """ - # GIVEN: Some test data - test_args = (1, 2) - test_kwargs = {'arg3': 3, 'arg4': 4} - test_params = ((1, 'arg2', str), (2, 'arg3', str)) - - # WHEN: Calling replace_params - result_args, result_kwargs = replace_params(test_args, test_kwargs, test_params) - - # THEN: The positional and keyword args should have have changed - self.assertEqual(result_args, (1, '2')) - self.assertEqual(result_kwargs, {'arg3': '3', 'arg4': 4}) - def test_resize_thumb(self): """ Test the resize_thumb() function diff --git a/tests/functional/openlp_core_lib/test_shutil.py b/tests/functional/openlp_core_lib/test_shutil.py deleted file mode 100755 index 737f7ce00..000000000 --- a/tests/functional/openlp_core_lib/test_shutil.py +++ /dev/null @@ -1,170 +0,0 @@ -import os -from unittest import TestCase -from unittest.mock import ANY, MagicMock, patch - -from openlp.core.common.path import Path -from openlp.core.lib.shutil import copy, copyfile, copytree, rmtree, which - - -class TestShutil(TestCase): - """ - Tests for the :mod:`openlp.core.lib.shutil` module - """ - - def test_copy(self): - """ - Test :func:`copy` - """ - # GIVEN: A mocked `shutil.copy` which returns a test path as a string - with patch('openlp.core.lib.shutil.shutil.copy', return_value=os.path.join('destination', 'test', 'path')) \ - as mocked_shutil_copy: - - # WHEN: Calling :func:`copy` with the src and dst parameters as Path object types - result = copy(Path('source', 'test', 'path'), Path('destination', 'test', 'path')) - - # THEN: :func:`shutil.copy` should have been called with the str equivalents of the Path objects. - # :func:`copy` should return the str type result of calling :func:`shutil.copy` as a Path object. - mocked_shutil_copy.assert_called_once_with(os.path.join('source', 'test', 'path'), - os.path.join('destination', 'test', 'path')) - self.assertEqual(result, Path('destination', 'test', 'path')) - - def test_copy_follow_optional_params(self): - """ - Test :func:`copy` when follow_symlinks is set to false - """ - # GIVEN: A mocked `shutil.copy` - with patch('openlp.core.lib.shutil.shutil.copy', return_value='') as mocked_shutil_copy: - - # WHEN: Calling :func:`copy` with :param:`follow_symlinks` set to False - copy(Path('source', 'test', 'path'), Path('destination', 'test', 'path'), follow_symlinks=False) - - # THEN: :func:`shutil.copy` should have been called with :param:`follow_symlinks` set to false - mocked_shutil_copy.assert_called_once_with(ANY, ANY, follow_symlinks=False) - - def test_copyfile(self): - """ - Test :func:`copyfile` - """ - # GIVEN: A mocked :func:`shutil.copyfile` which returns a test path as a string - with patch('openlp.core.lib.shutil.shutil.copyfile', - return_value=os.path.join('destination', 'test', 'path')) as mocked_shutil_copyfile: - - # WHEN: Calling :func:`copyfile` with the src and dst parameters as Path object types - result = copyfile(Path('source', 'test', 'path'), Path('destination', 'test', 'path')) - - # THEN: :func:`shutil.copyfile` should have been called with the str equivalents of the Path objects. - # :func:`copyfile` should return the str type result of calling :func:`shutil.copyfile` as a Path - # object. - mocked_shutil_copyfile.assert_called_once_with(os.path.join('source', 'test', 'path'), - os.path.join('destination', 'test', 'path')) - self.assertEqual(result, Path('destination', 'test', 'path')) - - def test_copyfile_optional_params(self): - """ - Test :func:`copyfile` when follow_symlinks is set to false - """ - # GIVEN: A mocked :func:`shutil.copyfile` - with patch('openlp.core.lib.shutil.shutil.copyfile', return_value='') as mocked_shutil_copyfile: - - # WHEN: Calling :func:`copyfile` with :param:`follow_symlinks` set to False - copyfile(Path('source', 'test', 'path'), Path('destination', 'test', 'path'), follow_symlinks=False) - - # THEN: :func:`shutil.copyfile` should have been called with the optional parameters, with out any of the - # values being modified - mocked_shutil_copyfile.assert_called_once_with(ANY, ANY, follow_symlinks=False) - - def test_copytree(self): - """ - Test :func:`copytree` - """ - # GIVEN: A mocked :func:`shutil.copytree` which returns a test path as a string - with patch('openlp.core.lib.shutil.shutil.copytree', - return_value=os.path.join('destination', 'test', 'path')) as mocked_shutil_copytree: - - # WHEN: Calling :func:`copytree` with the src and dst parameters as Path object types - result = copytree(Path('source', 'test', 'path'), Path('destination', 'test', 'path')) - - # THEN: :func:`shutil.copytree` should have been called with the str equivalents of the Path objects. - # :func:`patches.copytree` should return the str type result of calling :func:`shutil.copytree` as a - # Path object. - mocked_shutil_copytree.assert_called_once_with(os.path.join('source', 'test', 'path'), - os.path.join('destination', 'test', 'path')) - self.assertEqual(result, Path('destination', 'test', 'path')) - - def test_copytree_optional_params(self): - """ - Test :func:`copytree` when optional parameters are passed - """ - # GIVEN: A mocked :func:`shutil.copytree` - with patch('openlp.core.lib.shutil.shutil.copytree', return_value='') as mocked_shutil_copytree: - mocked_ignore = MagicMock() - mocked_copy_function = MagicMock() - - # WHEN: Calling :func:`copytree` with the optional parameters set - copytree(Path('source', 'test', 'path'), Path('destination', 'test', 'path'), symlinks=True, - ignore=mocked_ignore, copy_function=mocked_copy_function, ignore_dangling_symlinks=True) - - # THEN: :func:`shutil.copytree` should have been called with the optional parameters, with out any of the - # values being modified - mocked_shutil_copytree.assert_called_once_with(ANY, ANY, symlinks=True, ignore=mocked_ignore, - copy_function=mocked_copy_function, - ignore_dangling_symlinks=True) - - def test_rmtree(self): - """ - Test :func:`rmtree` - """ - # GIVEN: A mocked :func:`shutil.rmtree` - with patch('openlp.core.lib.shutil.shutil.rmtree', return_value=None) as mocked_shutil_rmtree: - - # WHEN: Calling :func:`rmtree` with the path parameter as Path object type - result = rmtree(Path('test', 'path')) - - # THEN: :func:`shutil.rmtree` should have been called with the str equivalents of the Path object. - mocked_shutil_rmtree.assert_called_once_with(os.path.join('test', 'path')) - self.assertIsNone(result) - - def test_rmtree_optional_params(self): - """ - Test :func:`rmtree` when optional parameters are passed - """ - # GIVEN: A mocked :func:`shutil.rmtree` - with patch('openlp.core.lib.shutil.shutil.rmtree', return_value='') as mocked_shutil_rmtree: - mocked_on_error = MagicMock() - - # WHEN: Calling :func:`rmtree` with :param:`ignore_errors` set to True and `onerror` set to a mocked object - rmtree(Path('test', 'path'), ignore_errors=True, onerror=mocked_on_error) - - # THEN: :func:`shutil.rmtree` should have been called with the optional parameters, with out any of the - # values being modified - mocked_shutil_rmtree.assert_called_once_with(ANY, ignore_errors=True, onerror=mocked_on_error) - - def test_which_no_command(self): - """ - Test :func:`which` when the command is not found. - """ - # GIVEN: A mocked :func:``shutil.which` when the command is not found. - with patch('openlp.core.lib.shutil.shutil.which', return_value=None) as mocked_shutil_which: - - # WHEN: Calling :func:`which` with a command that does not exist. - result = which('no_command') - - # THEN: :func:`shutil.which` should have been called with the command, and :func:`which` should return None. - mocked_shutil_which.assert_called_once_with('no_command') - self.assertIsNone(result) - - def test_which_command(self): - """ - Test :func:`which` when a command has been found. - """ - # GIVEN: A mocked :func:`shutil.which` when the command is found. - with patch('openlp.core.lib.shutil.shutil.which', - return_value=os.path.join('path', 'to', 'command')) as mocked_shutil_which: - - # WHEN: Calling :func:`which` with a command that exists. - result = which('command') - - # THEN: :func:`shutil.which` should have been called with the command, and :func:`which` should return a - # Path object equivalent of the command path. - mocked_shutil_which.assert_called_once_with('command') - self.assertEqual(result, Path('path', 'to', 'command')) From b4a687c85e102088a7f77ad1536942561627fa81 Mon Sep 17 00:00:00 2001 From: Philip Ridout Date: Thu, 21 Sep 2017 07:20:56 +0100 Subject: [PATCH 11/12] PEP fixes --- openlp/core/common/path.py | 1 - 1 file changed, 1 deletion(-) diff --git a/openlp/core/common/path.py b/openlp/core/common/path.py index f11c4bb9f..cdb115940 100644 --- a/openlp/core/common/path.py +++ b/openlp/core/common/path.py @@ -214,4 +214,3 @@ class Path(PathVariant): with suppress(ValueError): path = path.relative_to(base_path) return {'__Path__': path.parts} - From 131e0213e2c93b938a539491e18fca2bf76b1866 Mon Sep 17 00:00:00 2001 From: Philip Ridout Date: Thu, 21 Sep 2017 08:40:41 +0100 Subject: [PATCH 12/12] minor fix --- openlp/plugins/presentations/lib/mediaitem.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openlp/plugins/presentations/lib/mediaitem.py b/openlp/plugins/presentations/lib/mediaitem.py index d9a14e0ed..8061bb193 100644 --- a/openlp/plugins/presentations/lib/mediaitem.py +++ b/openlp/plugins/presentations/lib/mediaitem.py @@ -331,7 +331,7 @@ class PresentationMediaItem(MediaManagerItem): file_path = str_to_path(bitem.data(QtCore.Qt.UserRole)) path, file_name = file_path.parent, file_path.name service_item.title = file_name - if file_path.exists: + if file_path.exists(): if self.display_type_combo_box.itemData(self.display_type_combo_box.currentIndex()) == 'automatic': service_item.processor = self.find_controller_by_type(file_path) if not service_item.processor: