From c7615920958f688c73ba3b2b668165c99ce694b5 Mon Sep 17 00:00:00 2001 From: Philip Ridout Date: Sun, 3 Mar 2019 09:49:01 +0000 Subject: [PATCH] Catch some OSErrors to provide user friendly error messages. Few other minor fixes Fixes: https://launchpad.net/bugs/1650910 --- openlp/core/app.py | 7 +++++- openlp/core/common/__init__.py | 6 ++--- openlp/core/lib/__init__.py | 8 +++---- openlp/core/ui/exceptiondialog.py | 6 ++--- openlp/core/ui/exceptionform.py | 21 ++++++++++++------ openlp/core/ui/servicemanager.py | 6 +++-- openlp/core/ui/thememanager.py | 14 ++++++------ openlp/core/version.py | 2 +- openlp/plugins/songs/forms/songimportform.py | 9 ++++++-- .../songusage/forms/songusagedetailform.py | 4 ++-- run_openlp.py | 10 +++++++-- .../openlp_core/common/test_init.py | 22 ++++++++++++------- 12 files changed, 73 insertions(+), 42 deletions(-) diff --git a/openlp/core/app.py b/openlp/core/app.py index 7dafaaf3c..8b274e6b7 100644 --- a/openlp/core/app.py +++ b/openlp/core/app.py @@ -405,7 +405,12 @@ def main(args=None): None, translate('OpenLP', 'Settings Upgrade'), translate('OpenLP', 'Your settings are about to be upgraded. A backup will be created at ' '{back_up_path}').format(back_up_path=back_up_path)) - settings.export(back_up_path) + try: + settings.export(back_up_path) + except OSError: + QtWidgets.QMessageBox.warning( + None, translate('OpenLP', 'Settings Upgrade'), + translate('OpenLP', 'Settings back up failed.\n\nContinuining to upgrade.')) settings.upgrade_settings() # First time checks in settings if not Settings().value('core/has run wizard'): diff --git a/openlp/core/common/__init__.py b/openlp/core/common/__init__.py index 6e9274c10..9796a0f7e 100644 --- a/openlp/core/common/__init__.py +++ b/openlp/core/common/__init__.py @@ -34,7 +34,7 @@ from ipaddress import IPv4Address, IPv6Address, AddressValueError from shutil import which from subprocess import check_output, CalledProcessError, STDOUT -from PyQt5 import QtGui +from PyQt5 import QtGui, QtWidgets from PyQt5.QtCore import QCryptographicHash as QHash from PyQt5.QtNetwork import QAbstractSocket, QHostAddress, QNetworkInterface from chardet.universaldetector import UniversalDetector @@ -474,10 +474,10 @@ def get_file_encoding(file_path): if not chunk: break detector.feed(chunk) - detector.close() - return detector.result except OSError: log.exception('Error detecting file encoding') + finally: + return detector.close() def normalize_str(irregular_string): diff --git a/openlp/core/lib/__init__.py b/openlp/core/lib/__init__.py index 0de575d66..9e084fdd3 100644 --- a/openlp/core/lib/__init__.py +++ b/openlp/core/lib/__init__.py @@ -242,16 +242,16 @@ def image_to_byte(image, base_64=True): """ Resize an image to fit on the current screen for the web and returns it as a byte stream. - :param image: The image to converted. + :param image: The image to be converted. :param base_64: If True returns the image as Base64 bytes, otherwise the image is returned as a byte array. To preserve original intention, this defaults to True """ log.debug('image_to_byte - start') byte_array = QtCore.QByteArray() # use buffer to store pixmap into byteArray - buffie = QtCore.QBuffer(byte_array) - buffie.open(QtCore.QIODevice.WriteOnly) - image.save(buffie, "PNG") + buffer = QtCore.QBuffer(byte_array) + buffer.open(QtCore.QIODevice.WriteOnly) + image.save(buffer, "PNG") log.debug('image_to_byte - end') if not base_64: return byte_array diff --git a/openlp/core/ui/exceptiondialog.py b/openlp/core/ui/exceptiondialog.py index 966166a4a..c5f1c19bf 100644 --- a/openlp/core/ui/exceptiondialog.py +++ b/openlp/core/ui/exceptiondialog.py @@ -77,11 +77,11 @@ class Ui_ExceptionDialog(object): self.save_report_button = create_button(exception_dialog, 'save_report_button', icon=UiIcons().save, click=self.on_save_report_button_clicked) - self.attach_tile_button = create_button(exception_dialog, 'attach_tile_button', + self.attach_file_button = create_button(exception_dialog, 'attach_file_button', icon=UiIcons().open, click=self.on_attach_file_button_clicked) self.button_box = create_button_box(exception_dialog, 'button_box', ['close'], - [self.send_report_button, self.save_report_button, self.attach_tile_button]) + [self.send_report_button, self.save_report_button, self.attach_file_button]) self.exception_layout.addWidget(self.button_box) self.retranslate_ui(exception_dialog) @@ -112,4 +112,4 @@ class Ui_ExceptionDialog(object): ).format(first_part=exception_part1)) self.send_report_button.setText(translate('OpenLP.ExceptionDialog', 'Send E-Mail')) self.save_report_button.setText(translate('OpenLP.ExceptionDialog', 'Save to File')) - self.attach_tile_button.setText(translate('OpenLP.ExceptionDialog', 'Attach File')) + self.attach_file_button.setText(translate('OpenLP.ExceptionDialog', 'Attach File')) diff --git a/openlp/core/ui/exceptionform.py b/openlp/core/ui/exceptionform.py index 9510b8f07..e6089f7b3 100644 --- a/openlp/core/ui/exceptionform.py +++ b/openlp/core/ui/exceptionform.py @@ -95,12 +95,14 @@ class ExceptionForm(QtWidgets.QDialog, Ui_ExceptionDialog, RegistryProperties): """ Saving exception log and system information to a file. """ - file_path, filter_used = FileDialog.getSaveFileName( - self, - translate('OpenLP.ExceptionForm', 'Save Crash Report'), - Settings().value(self.settings_section + '/last directory'), - translate('OpenLP.ExceptionForm', 'Text files (*.txt *.log *.text)')) - if file_path: + while True: + file_path, filter_used = FileDialog.getSaveFileName( + self, + translate('OpenLP.ExceptionForm', 'Save Crash Report'), + Settings().value(self.settings_section + '/last directory'), + translate('OpenLP.ExceptionForm', 'Text files (*.txt *.log *.text)')) + if file_path is None: + break Settings().setValue(self.settings_section + '/last directory', file_path.parent) opts = self._create_report() report_text = self.report_text.format(version=opts['version'], description=opts['description'], @@ -108,8 +110,13 @@ class ExceptionForm(QtWidgets.QDialog, Ui_ExceptionDialog, RegistryProperties): try: with file_path.open('w') as report_file: report_file.write(report_text) - except OSError: + break + except OSError as e: log.exception('Failed to write crash report') + QtWidgets.QMessageBox.warning( + self, translate('OpenLP.ExceptionDialog', 'Failed to Save Report'), + translate('OpenLP.ExceptionDialog', 'The following error occured when saving the report.\n\n' + '{exception}').format(file_name=file_path, exception=e)) def on_send_report_button_clicked(self): """ diff --git a/openlp/core/ui/servicemanager.py b/openlp/core/ui/servicemanager.py index cff91d431..ac179663e 100644 --- a/openlp/core/ui/servicemanager.py +++ b/openlp/core/ui/servicemanager.py @@ -705,11 +705,13 @@ class ServiceManager(QtWidgets.QWidget, RegistryBase, Ui_ServiceManager, LogMixi else: raise ValidationError(msg='No service data found') except (NameError, OSError, ValidationError, zipfile.BadZipFile): + self.application.set_normal_cursor() self.log_exception('Problem loading service file {name}'.format(name=file_path)) critical_error_message_box( message=translate('OpenLP.ServiceManager', - 'The service file {file_path} could not be loaded because it is either corrupt, or ' - 'not a valid OpenLP 2 or OpenLP 3 service file.'.format(file_path=file_path))) + 'The service file {file_path} could not be loaded because it is either corrupt, ' + 'inaccessible, or not a valid OpenLP 2 or OpenLP 3 service file.' + ).format(file_path=file_path)) self.main_window.finished_progress_bar() self.application.set_normal_cursor() self.repaint_service_list(-1, -1) diff --git a/openlp/core/ui/thememanager.py b/openlp/core/ui/thememanager.py index 0c8ab094a..188688c3f 100644 --- a/openlp/core/ui/thememanager.py +++ b/openlp/core/ui/thememanager.py @@ -150,7 +150,7 @@ class ThemeManager(QtWidgets.QWidget, RegistryBase, Ui_ThemeManager, LogMixin, R self.global_theme = Settings().value(self.settings_section + '/global theme') self.build_theme_path() self.load_first_time_themes() - self.upgrade_themes() + self.upgrade_themes() # TODO: Can be removed when upgrade path from OpenLP 2.4 no longer needed def bootstrap_post_set_up(self): """ @@ -570,7 +570,7 @@ class ThemeManager(QtWidgets.QWidget, RegistryBase, Ui_ThemeManager, LogMixin, R with zipfile.ZipFile(str(file_path)) as theme_zip: json_file = [name for name in theme_zip.namelist() if os.path.splitext(name)[1].lower() == '.json'] if len(json_file) != 1: - # TODO: remove XML handling after the 2.6 release. + # TODO: remove XML handling after once the upgrade path from 2.4 is no longer required xml_file = [name for name in theme_zip.namelist() if os.path.splitext(name)[1].lower() == '.xml'] if len(xml_file) != 1: self.log_error('Theme contains "{val:d}" theme files'.format(val=len(xml_file))) @@ -607,12 +607,12 @@ class ThemeManager(QtWidgets.QWidget, RegistryBase, Ui_ThemeManager, LogMixin, R else: with full_name.open('wb') as out_file: out_file.write(theme_zip.read(zipped_file)) - except (OSError, zipfile.BadZipFile): + except (OSError, ValidationError, zipfile.BadZipFile): self.log_exception('Importing theme from zip failed {name}'.format(name=file_path)) - raise ValidationError - except ValidationError: - critical_error_message_box(translate('OpenLP.ThemeManager', 'Validation Error'), - translate('OpenLP.ThemeManager', 'File is not a valid theme.')) + critical_error_message_box( + translate('OpenLP.ThemeManager', 'Import Error'), + translate('OpenLP.ThemeManager', 'There was a problem imoorting {file_name}.\n\nIt is corrupt,' + 'inaccessible or not a valid theme.').format(file_name=file_path)) finally: if not abort_import: # As all files are closed, we can create the Theme. diff --git a/openlp/core/version.py b/openlp/core/version.py index e2b77ee07..198b42df2 100644 --- a/openlp/core/version.py +++ b/openlp/core/version.py @@ -200,7 +200,7 @@ def get_library_versions(): """ library_versions = OrderedDict([(library, _get_lib_version(*args)) for library, args in LIBRARIES.items()]) # Just update the dict for display purposes, changing the None to a '-' - for library, version in library_versions: + for library, version in library_versions.items(): if version is None: library_versions[library] = '-' return library_versions diff --git a/openlp/plugins/songs/forms/songimportform.py b/openlp/plugins/songs/forms/songimportform.py index 2662529c9..6931c3457 100644 --- a/openlp/plugins/songs/forms/songimportform.py +++ b/openlp/plugins/songs/forms/songimportform.py @@ -329,8 +329,13 @@ class SongImportForm(OpenLPWizard, RegistryProperties): importer = self.plugin.import_songs( source_format, file_paths=self.get_list_of_paths(self.format_widgets[source_format]['file_list_widget'])) - importer.do_import() - self.progress_label.setText(WizardStrings.FinishedImport) + try: + importer.do_import() + self.progress_label.setText(WizardStrings.FinishedImport) + except OSError as e: + log.exception('Importing songs failed') + self.progress_label.setText(translate('SongsPlugin.ImportWizardForm', + 'Your Song import failed. {error}').format(error=e)) def on_error_copy_to_button_clicked(self): """ diff --git a/openlp/plugins/songusage/forms/songusagedetailform.py b/openlp/plugins/songusage/forms/songusagedetailform.py index aa8b7c02a..f43ca3b9a 100644 --- a/openlp/plugins/songusage/forms/songusagedetailform.py +++ b/openlp/plugins/songusage/forms/songusagedetailform.py @@ -85,7 +85,7 @@ class SongUsageDetailForm(QtWidgets.QDialog, Ui_SongUsageDetailDialog, RegistryP self.main_window.error_message( translate('SongUsagePlugin.SongUsageDetailForm', 'Output Path Not Selected'), translate('SongUsagePlugin.SongUsageDetailForm', 'You have not set a valid output location for your' - ' song usage report. \nPlease select an existing path on your computer.') + ' song usage report.\nPlease select an existing path on your computer.') ) return create_paths(path) @@ -112,7 +112,7 @@ class SongUsageDetailForm(QtWidgets.QDialog, Ui_SongUsageDetailDialog, RegistryP self.main_window.information_message( translate('SongUsagePlugin.SongUsageDetailForm', 'Report Creation'), translate('SongUsagePlugin.SongUsageDetailForm', - 'Report \n{name} \nhas been successfully created. ').format(name=report_file_name) + 'Report\n{name}\nhas been successfully created.').format(name=report_file_name) ) except OSError as ose: log.exception('Failed to write out song usage records') diff --git a/run_openlp.py b/run_openlp.py index a754b4c78..c48c19d53 100755 --- a/run_openlp.py +++ b/run_openlp.py @@ -24,6 +24,7 @@ The entrypoint for OpenLP """ import faulthandler +import logging import multiprocessing import sys @@ -34,14 +35,19 @@ from openlp.core.common import is_macosx, is_win from openlp.core.common.applocation import AppLocation from openlp.core.common.path import create_paths +log = logging.getLogger(__name__) + def set_up_fault_handling(): """ Set up the Python fault handler """ # Create the cache directory if it doesn't exist, and enable the fault handler to log to an error log file - create_paths(AppLocation.get_directory(AppLocation.CacheDir)) - faulthandler.enable((AppLocation.get_directory(AppLocation.CacheDir) / 'error.log').open('wb')) + try: + create_paths(AppLocation.get_directory(AppLocation.CacheDir)) + faulthandler.enable((AppLocation.get_directory(AppLocation.CacheDir) / 'error.log').open('wb')) + except OSError: + log.exception('An exception occurred when enabling the fault handler') def start(): diff --git a/tests/functional/openlp_core/common/test_init.py b/tests/functional/openlp_core/common/test_init.py index 0c8310f4d..99ef55b01 100644 --- a/tests/functional/openlp_core/common/test_init.py +++ b/tests/functional/openlp_core/common/test_init.py @@ -309,9 +309,9 @@ class TestInit(TestCase, TestMixin): """ # GIVEN: A mocked UniversalDetector instance with done attribute set to True after first iteration with patch('openlp.core.common.UniversalDetector') as mocked_universal_detector, \ - patch.object(Path, 'open', return_value=BytesIO(b"data" * 260)) as mocked_open: + patch.object(Path, 'open', return_value=BytesIO(b'data' * 260)) as mocked_open: encoding_result = {'encoding': 'UTF-8', 'confidence': 0.99} - mocked_universal_detector_inst = MagicMock(result=encoding_result) + mocked_universal_detector_inst = MagicMock(**{'close.return_value': encoding_result}) type(mocked_universal_detector_inst).done = PropertyMock(side_effect=[False, True]) mocked_universal_detector.return_value = mocked_universal_detector_inst @@ -320,7 +320,7 @@ class TestInit(TestCase, TestMixin): # THEN: The feed method of UniversalDetector should only br called once before returning a result mocked_open.assert_called_once_with('rb') - assert mocked_universal_detector_inst.feed.mock_calls == [call(b"data" * 256)] + assert mocked_universal_detector_inst.feed.mock_calls == [call(b'data' * 256)] mocked_universal_detector_inst.close.assert_called_once_with() assert result == encoding_result @@ -331,10 +331,10 @@ class TestInit(TestCase, TestMixin): # GIVEN: A mocked UniversalDetector instance which isn't set to done and a mocked open, with 1040 bytes of test # data (enough to run the iterator twice) with patch('openlp.core.common.UniversalDetector') as mocked_universal_detector, \ - patch.object(Path, 'open', return_value=BytesIO(b"data" * 260)) as mocked_open: + patch.object(Path, 'open', return_value=BytesIO(b'data' * 260)) as mocked_open: encoding_result = {'encoding': 'UTF-8', 'confidence': 0.99} mocked_universal_detector_inst = MagicMock(mock=mocked_universal_detector, - **{'done': False, 'result': encoding_result}) + **{'done': False, 'close.return_value': encoding_result}) mocked_universal_detector.return_value = mocked_universal_detector_inst # WHEN: Calling get_file_encoding @@ -342,7 +342,7 @@ class TestInit(TestCase, TestMixin): # THEN: The feed method of UniversalDetector should have been called twice before returning a result mocked_open.assert_called_once_with('rb') - assert mocked_universal_detector_inst.feed.mock_calls == [call(b"data" * 256), call(b"data" * 4)] + assert mocked_universal_detector_inst.feed.mock_calls == [call(b'data' * 256), call(b'data' * 4)] mocked_universal_detector_inst.close.assert_called_once_with() assert result == encoding_result @@ -352,13 +352,19 @@ class TestInit(TestCase, TestMixin): """ # GIVEN: A mocked UniversalDetector instance which isn't set to done and a mocked open, with 1040 bytes of test # data (enough to run the iterator twice) - with patch('openlp.core.common.UniversalDetector'), \ + with patch('openlp.core.common.UniversalDetector') as mocked_universal_detector, \ patch('builtins.open', side_effect=OSError), \ patch('openlp.core.common.log') as mocked_log: + encoding_result = {'encoding': 'UTF-8', 'confidence': 0.99} + mocked_universal_detector_inst = MagicMock(mock=mocked_universal_detector, + **{'done': False, 'close.return_value': encoding_result}) + mocked_universal_detector.return_value = mocked_universal_detector_inst # WHEN: Calling get_file_encoding result = get_file_encoding(Path('file name')) # THEN: log.exception should be called and get_file_encoding should return None mocked_log.exception.assert_called_once_with('Error detecting file encoding') - assert result is None + mocked_universal_detector_inst.feed.assert_not_called() + mocked_universal_detector_inst.close.assert_called_once_with() + assert result == encoding_result