Merge branch 'fix-passed-params' into 'master'

Fix loading service files from the command line

Closes #993 and #992

See merge request openlp/openlp!401

openlp/core/app.py

@@ -383,7 +383,7 @@ def main():
     Registry.create()
     settings = Settings()
     Registry().register('settings', settings)
-    log.warning(f'Arguments passed {args}')
+    log.info(f'Arguments passed {args}')
     # Need settings object for the threads.
     settings_thread = Settings()
     Registry().register('settings_thread', settings_thread)

openlp/core/ui/mainwindow.py

@@ -21,7 +21,6 @@
 """
 This is the main window, where all the action happens.
 """
-import os
 import shutil
 from datetime import datetime, date
 from pathlib import Path
@@ -642,7 +641,7 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, LogMixin, RegistryPropert
         # We have -disable-web-security added by our code.
         # If a file is passed in we will have that as well so count of 2
         # If not we need to see if we want to use the previous file.so count of 1
-        self.log_warning(self.application.args)
+        self.log_info(self.application.args)
         if self.application.args and len(self.application.args) > 1:
             self.open_cmd_line_files(self.application.args)
         elif self.settings.value('core/auto open'):
@@ -1406,15 +1405,49 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, LogMixin, RegistryPropert
             self.settings.remove('advanced/data path')
         self.application.set_normal_cursor()
 
-    def open_cmd_line_files(self, args):
+    def open_cmd_line_files(self, args: list):
         """
         Open files passed in through command line arguments
 
         :param list[str] args: List of remaining positional arguments
         """
+        self.log_info(args)
+        # Drop this argument, it's obvs not a filename
+        if '--disable-web-security' in args:
+            args.remove('--disable-web-security')
+        # It has been known for Microsoft to double quote the path passed in and then encode one of the quotes.
+        # Remove these to get the correct path.
+        args = list(map(lambda x: x.replace('"', ''), args))
+        # Loop through the parameters, and see if we can pull a file out
+        file_path = None
         for arg in args:
-            self.log_warning(arg)
-            file_name = os.path.expanduser(arg)
-            if os.path.isfile(file_name):
-                self.log_warning("File name found")
-                self.service_manager_contents.load_file(Path(file_name))
+            try:
+                # Resolve the file, and use strict mode to throw an exception if the file does not exist
+                file_path = Path(arg).resolve(strict=True)
+                # Check if this is actually a file
+                if file_path.is_file():
+                    break
+                else:
+                    file_path = None
+            except FileNotFoundError:
+                file_path = None
+        # If none of the individual components are files, let's try pulling them together
+        if not file_path:
+            path_so_far = []
+            for arg in args:
+                path_so_far.append(arg)
+                try:
+                    file_path = Path(' '.join(path_so_far)).resolve(strict=True)
+                    if file_path.is_file():
+                        break
+                    else:
+                        file_path = None
+                except FileNotFoundError:
+                    file_path = None
+            else:
+                file_path = None
+        if file_path and file_path.suffix in ['.osz', '.oszl']:
+            self.log_info("File name found")
+            self.service_manager_contents.load_file(file_path)
+        else:
+            self.log_error(f"File {file_path} not found for arg {arg}")

tests/openlp_core/ui/test_mainwindow.py

@@ -119,19 +119,72 @@ def test_cmd_line_file(main_window):
     mocked_load_file.assert_called_with(Path(service))
 
 
-@patch('openlp.core.ui.servicemanager.ServiceManager.load_file')
-def test_cmd_line_arg(mocked_load_file, main_window):
+def test_cmd_line_file_encoded(main_window):
+    """
+    Test that passing a service file from the command line loads the service where extra encoded quotes are added
+    """
+    # GIVEN a service as an argument to openlp
+    service_base = os.path.join(TEST_RESOURCES_PATH, 'service', 'test.osz')
+    service = f'"{service_base}"'
+
+    # WHEN the argument is processed
+    with patch.object(main_window.service_manager, 'load_file') as mocked_load_file:
+        main_window.open_cmd_line_files([service])
+
+    # THEN the service from the arguments is loaded
+    mocked_load_file.assert_called_with(Path(service_base))
+
+
+def test_cmd_line_arg_non_service(main_window):
     """
     Test that passing a non service file does nothing.
     """
     # GIVEN a non service file as an argument to openlp
-    service = 'run_openlp.py'
+    args = ['run_openlp.py']
 
     # WHEN the argument is processed
-    main_window.open_cmd_line_files(service)
+    with patch.object(main_window.service_manager, 'load_file') as mocked_load_file:
+        main_window.open_cmd_line_files(args)
 
-    # THEN the file should not be opened
-    assert mocked_load_file.called is False, 'load_file should not have been called'
+        # THEN the file should not be opened
+        assert mocked_load_file.called is False, 'load_file should not have been called'
+
+
+def test_cmd_line_arg_other_args(main_window):
+    """
+    Test that passing a non service file does nothing.
+    """
+    # GIVEN a non service file as an argument to openlp
+    service_file = os.path.join(TEST_RESOURCES_PATH, 'service', 'test.osz')
+    args = ['--disable-web-security', service_file]
+
+    # WHEN the argument is processed
+    with patch.object(main_window.service_manager, 'load_file') as mocked_load_file:
+        main_window.open_cmd_line_files(args)
+
+        # THEN the file should not be opened
+        assert mocked_load_file.called is True, 'load_file should have been called'
+        mocked_load_file.assert_called_with(Path(service_file))
+
+
+@patch('openlp.core.ui.mainwindow.Path')
+def test_cmd_line_filename_with_spaces(MockPath, main_window):
+    """
+    Test that passing a service file with spaces loads.
+    """
+    # GIVEN a set of arguments with a file separated by spaces
+    mocked_path_is_file = MagicMock(**{'is_file.return_value': True, 'suffix': '.osz'})
+    MockPath.return_value.resolve.side_effect = [FileNotFoundError, FileNotFoundError,
+                                                 FileNotFoundError, mocked_path_is_file]
+    args = ['Service', '2022-02-06.osz']
+
+    # WHEN the argument is processed
+    with patch.object(main_window.service_manager, 'load_file') as mocked_load_file:
+        main_window.open_cmd_line_files(args)
+
+        # THEN the file should be looked for
+        assert MockPath.return_value.resolve.call_count == 4
+        mocked_load_file.assert_called_with(mocked_path_is_file)
 
 
 def test_main_window_title(main_window):
@@ -142,12 +195,12 @@ def test_main_window_title(main_window):
 
     # WHEN no changes are made to the service
 
-    # THEN the main window's title shoud be the same as the OpenLP string in the UiStrings class
+    # THEN the main window's title should be the same as the OpenLP string in the UiStrings class
     assert main_window.windowTitle() == UiStrings().OpenLP, \
         'The main window\'s title should be the same as the OpenLP string in UiStrings class'
 
 
-def test_set_service_modifed(main_window):
+def test_set_service_modified(main_window):
     """
     Test that when setting the service's title the main window's title is set correctly
     """