Catch some OSErrors to provide user friendly error messages. Few other minor fixes

Fixes: https://launchpad.net/bugs/1650910

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'):

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):

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)
+    buffer = QtCore.QBuffer(byte_array)
-    buffie.open(QtCore.QIODevice.WriteOnly)
+    buffer.open(QtCore.QIODevice.WriteOnly)
-    image.save(buffie, "PNG")
+    image.save(buffer, "PNG")
     log.debug('image_to_byte - end')
     if not base_64:
         return byte_array

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'))

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(
+        while True:
-            self,
+            file_path, filter_used = FileDialog.getSaveFileName(
-            translate('OpenLP.ExceptionForm', 'Save Crash Report'),
+                self,
-            Settings().value(self.settings_section + '/last directory'),
+                translate('OpenLP.ExceptionForm', 'Save Crash Report'),
-            translate('OpenLP.ExceptionForm', 'Text files (*.txt *.log *.text)'))
+                Settings().value(self.settings_section + '/last directory'),
-        if file_path:
+                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):
         """

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 '
+                                  'The service file {file_path} could not be loaded because it is either corrupt, '
-                                  'not a valid OpenLP 2 or OpenLP 3 service file.'.format(file_path=file_path)))
+                                  '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)

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
+            critical_error_message_box(
-        except ValidationError:
+                translate('OpenLP.ThemeManager', 'Import Error'),
-            critical_error_message_box(translate('OpenLP.ThemeManager', 'Validation Error'),
+                translate('OpenLP.ThemeManager', 'There was a problem imoorting {file_name}.\n\nIt is corrupt,'
-                                       translate('OpenLP.ThemeManager', 'File is not a valid theme.'))
+                                                 '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.

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

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()
+        try:
-        self.progress_label.setText(WizardStrings.FinishedImport)
+            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):
         """

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')

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))
+    try:
-    faulthandler.enable((AppLocation.get_directory(AppLocation.CacheDir) / 'error.log').open('wb'))
+        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():

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