From 3f41c852f275b29f433242294298cfbb7459dc26 Mon Sep 17 00:00:00 2001 From: Tim Bentley Date: Thu, 2 Apr 2020 18:48:27 +0000 Subject: [PATCH] Fix some race conditions, remove calls to ImageManager, fix a crash in the remote API, fix some more test issues. --- openlp/core/api/http/__init__.py | 6 +++--- openlp/core/api/http/server.py | 4 ++-- openlp/core/api/lib.py | 10 +++++----- openlp/core/api/poll.py | 11 ++++++++--- openlp/core/api/versions/v1/controller.py | 8 ++++---- openlp/core/api/versions/v2/controller.py | 8 ++++---- openlp/core/api/versions/v2/core.py | 8 ++++---- openlp/core/api/websockets.py | 5 +++-- openlp/core/api/zeroconf.py | 4 ++-- openlp/core/app.py | 6 +++++- openlp/core/common/registry.py | 2 ++ openlp/core/loader.py | 2 -- openlp/core/ui/mainwindow.py | 2 +- openlp/plugins/images/imageplugin.py | 13 +------------ tests/conftest.py | 5 +++++ tests/functional/openlp_core/api/test_websockets.py | 2 +- tests/functional/openlp_core/api/v2/test_core.py | 2 +- .../openlp_core/lib/test_image_manager.py | 4 +++- tests/functional/openlp_core/ui/test_mainwindow.py | 5 +++-- 19 files changed, 57 insertions(+), 50 deletions(-) diff --git a/openlp/core/api/http/__init__.py b/openlp/core/api/http/__init__.py index 25b75cabf..d5d82d9e0 100644 --- a/openlp/core/api/http/__init__.py +++ b/openlp/core/api/http/__init__.py @@ -34,8 +34,8 @@ def check_auth(auth): :param auth: the authorisation object which needs to be tested :return Whether authentication have been successful """ - auth_code = "{user}:{password}".format(user=Registry().get('settings').value('api/user id'), - password=Registry().get('settings').value('api/password')) + auth_code = "{user}:{password}".format(user=Registry().get('settings_thread').value('api/user id'), + password=Registry().get('settings_thread').value('api/password')) try: auth_base = base64.b64encode(auth_code) except TypeError: @@ -64,7 +64,7 @@ def requires_auth(f): """ @wraps(f) def decorated(*args, **kwargs): - if not Registry().get('settings').value('api/authentication enabled'): + if not Registry().get('settings_thread').value('api/authentication enabled'): return f(*args, **kwargs) req = args[0] if not hasattr(req, 'authorization'): diff --git a/openlp/core/api/http/server.py b/openlp/core/api/http/server.py index 45660abcd..bc89bf682 100644 --- a/openlp/core/api/http/server.py +++ b/openlp/core/api/http/server.py @@ -47,8 +47,8 @@ class HttpWorker(ThreadWorker): """ Run the thread. """ - address = Registry().get('settings').value('api/ip address') - port = Registry().get('settings').value('api/port') + address = Registry().get('settings_thread').value('api/ip address') + port = Registry().get('settings_thread').value('api/port') Registry().execute('get_website_version') try: application.static_folder = str(AppLocation.get_section_data_path('remotes') / 'static') diff --git a/openlp/core/api/lib.py b/openlp/core/api/lib.py index 8d730c3ff..357597c39 100644 --- a/openlp/core/api/lib.py +++ b/openlp/core/api/lib.py @@ -27,7 +27,7 @@ from openlp.core.common.registry import Registry def login_required(f): @wraps(f) def decorated(*args, **kwargs): - if not Registry().get('settings').value('api/authentication enabled'): + if not Registry().get('settings_thread').value('api/authentication enabled'): return f(*args, **kwargs) token = request.headers.get('Authorization', '') if token == Registry().get('authentication_token'): @@ -38,7 +38,7 @@ def login_required(f): def old_success_response(): - return jsonify({'results': {'sucess': True}}) + return jsonify({'results': {'success': True}}) def extract_request(json_string, name): @@ -52,14 +52,14 @@ def extract_request(json_string, name): # ----------------------- OLD AUTH SECTION --------------------- def check_auth(username, password): - return Registry().get('settings').value('api/user id') == username and \ - Registry().get('settings').value('api/password') == password + return Registry().get('settings_thread').value('api/user id') == username and \ + Registry().get('settings_thread').value('api/password') == password def old_auth(f): @wraps(f) def decorated(*args, **kwargs): - if not Registry().get('settings').value('api/authentication enabled'): + if not Registry().get('settings_thread').value('api/authentication enabled'): return f(*args, **kwargs) auth = request.authorization if not auth or not check_auth(auth.username, auth.password): diff --git a/openlp/core/api/poll.py b/openlp/core/api/poll.py index 1beadfe3d..75b263ce5 100644 --- a/openlp/core/api/poll.py +++ b/openlp/core/api/poll.py @@ -21,6 +21,7 @@ import json +from openlp.core.common.registry import Registry from openlp.core.common.httputils import get_web_page from openlp.core.common.mixins import RegistryProperties @@ -37,6 +38,10 @@ class Poller(RegistryProperties): self.live_cache = None self.stage_cache = None self.chords_cache = None + settings = Registry().get('settings_thread') + self.address = settings.value('api/ip address') + self.ws_port = settings.value('api/websocket port') + self.http_port = settings.value('api/port') def raw_poll(self): return { @@ -87,7 +92,7 @@ class Poller(RegistryProperties): """ if self.stage_cache is None: try: - page = get_web_page("http://localhost:4316/stage") + page = get_web_page(f'http://{self.address}:{self.http_port}/stage') except Exception: page = None if page: @@ -103,7 +108,7 @@ class Poller(RegistryProperties): """ if self.live_cache is None: try: - page = get_web_page("http://localhost:4316/main") + page = get_web_page(f'http://{self.address}:{self.http_port}/main') except Exception: page = None if page: @@ -119,7 +124,7 @@ class Poller(RegistryProperties): """ if self.chords_cache is None: try: - page = get_web_page("http://localhost:4316/chords") + page = get_web_page(f'http://{self.address}:{self.http_port}/chords') except Exception: page = None if page: diff --git a/openlp/core/api/versions/v1/controller.py b/openlp/core/api/versions/v1/controller.py index 090cd2d32..e6a58e232 100644 --- a/openlp/core/api/versions/v1/controller.py +++ b/openlp/core/api/versions/v1/controller.py @@ -55,12 +55,12 @@ def controller_text_api(): item['text'] = frame['text'] item['html'] = current_item.get_rendered_frame(index) elif current_item.is_image() and not frame.get('image', '') and \ - Registry().get('settings').value('api/thumbnails'): + Registry().get('settings_thread').value('api/thumbnails'): thumbnail_path = os.path.join('images', 'thumbnails', frame['title']) full_thumbnail_path = AppLocation.get_data_path() / thumbnail_path if not full_thumbnail_path.exists(): create_thumb(Path(current_item.get_frame_path(index)), full_thumbnail_path, False) - Registry().get('image_manager').add_image(str(full_thumbnail_path), frame['title'], None, 88, 88) + # Registry().get('image_manager').add_image(str(full_thumbnail_path), frame['title'], None, 88, 88) item['img'] = urllib.request.pathname2url(os.path.sep + str(thumbnail_path)) item['text'] = str(frame['title']) item['html'] = str(frame['title']) @@ -71,12 +71,12 @@ def controller_text_api(): if current_item.is_capable(ItemCapabilities.HasNotes): item['slide_notes'] = str(frame['notes']) if current_item.is_capable(ItemCapabilities.HasThumbnails) and \ - Registry().get('settings').value('api/thumbnails'): + Registry().get('settings_thread').value('api/thumbnails'): # If the file is under our app directory tree send the portion after the match data_path = str(AppLocation.get_data_path()) if frame['image'][0:len(data_path)] == data_path: item['img'] = urllib.request.pathname2url(frame['image'][len(data_path):]) - Registry().get('image_manager').add_image(frame['image'], frame['title'], None, 88, 88) + # Registry().get('image_manager').add_image(frame['image'], frame['title'], None, 88, 88) item['text'] = str(frame['title']) item['html'] = str(frame['title']) data.append(item) diff --git a/openlp/core/api/versions/v2/controller.py b/openlp/core/api/versions/v2/controller.py index ca49bbb94..0b4394040 100644 --- a/openlp/core/api/versions/v2/controller.py +++ b/openlp/core/api/versions/v2/controller.py @@ -52,12 +52,12 @@ def controller_text_api(): item['text'] = frame['text'] item['html'] = current_item.get_rendered_frame(index) elif current_item.is_image() and not frame.get('image', '') and \ - Registry().get('settings').value('api/thumbnails'): + Registry().get('settings_thread').value('api/thumbnails'): thumbnail_path = os.path.join('images', 'thumbnails', frame['title']) full_thumbnail_path = AppLocation.get_data_path() / thumbnail_path if not full_thumbnail_path.exists(): create_thumb(Path(current_item.get_frame_path(index)), full_thumbnail_path, False) - Registry().get('image_manager').add_image(str(full_thumbnail_path), frame['title'], None, 88, 88) + # Registry().get('image_manager').add_image(str(full_thumbnail_path), frame['title'], None, 88, 88) item['img'] = urllib.request.pathname2url(os.path.sep + str(thumbnail_path)) item['text'] = str(frame['title']) item['html'] = str(frame['title']) @@ -68,12 +68,12 @@ def controller_text_api(): if current_item.is_capable(ItemCapabilities.HasNotes): item['slide_notes'] = str(frame['notes']) if current_item.is_capable(ItemCapabilities.HasThumbnails) and \ - Registry().get('settings').value('api/thumbnails'): + Registry().get('settings_thread').value('api/thumbnails'): # If the file is under our app directory tree send the portion after the match data_path = str(AppLocation.get_data_path()) if frame['image'][0:len(data_path)] == data_path: item['img'] = urllib.request.pathname2url(frame['image'][len(data_path):]) - Registry().get('image_manager').add_image(frame['image'], frame['title'], None, 88, 88) + # Registry().get('image_manager').add_image(frame['image'], frame['title'], None, 88, 88) item['text'] = str(frame['title']) item['html'] = str(frame['title']) data.append(item) diff --git a/openlp/core/api/versions/v2/core.py b/openlp/core/api/versions/v2/core.py index 3d98840be..28ef99d9e 100644 --- a/openlp/core/api/versions/v2/core.py +++ b/openlp/core/api/versions/v2/core.py @@ -61,8 +61,8 @@ def plugin_list(): @core.route('/system') def system_information(): data = {} - data['websocket_port'] = Registry().get('settings').value('api/websocket port') - data['login_required'] = Registry().get('settings').value('api/authentication enabled') + data['websocket_port'] = Registry().get('settings_thread').value('api/websocket port') + data['login_required'] = Registry().get('settings_thread').value('api/authentication enabled') return jsonify(data) @@ -73,8 +73,8 @@ def login(): abort(400) username = data.get('username', '') password = data.get('password', '') - if username == Registry().get('settings').value('api/user id') and \ - password == Registry().get('settings').value('api/password'): + if username == Registry().get('settings_thread').value('api/user id') and \ + password == Registry().get('settings_thread').value('api/password'): return jsonify({'token': Registry().get('authentication_token')}) else: return '', 401 diff --git a/openlp/core/api/websockets.py b/openlp/core/api/websockets.py index 6f10fa37f..0b271fdf5 100644 --- a/openlp/core/api/websockets.py +++ b/openlp/core/api/websockets.py @@ -75,8 +75,9 @@ class WebSocketWorker(ThreadWorker, RegistryProperties, LogMixin): """ Run the worker. """ - address = Registry().get('settings').value('api/ip address') - port = Registry().get('settings').value('api/websocket port') + settings = Registry().get('settings_thread') + address = settings.value('api/ip address') + port = settings.value('api/websocket port') # Start the event loop self.event_loop = asyncio.new_event_loop() asyncio.set_event_loop(self.event_loop) diff --git a/openlp/core/api/zeroconf.py b/openlp/core/api/zeroconf.py index 8871b424c..abc034df0 100644 --- a/openlp/core/api/zeroconf.py +++ b/openlp/core/api/zeroconf.py @@ -107,7 +107,7 @@ def start_zeroconf(): # When we're running tests, just skip this set up if this flag is set if Registry().get_flag('no_web_server'): return - http_port = Registry().get('settings').value('api/port') - ws_port = Registry().get('settings').value('api/websocket port') + http_port = Registry().get('settings_thread').value('api/port') + ws_port = Registry().get('settings_thread').value('api/websocket port') worker = ZeroconfWorker([iface['ip'] for iface in get_network_interfaces().values()], http_port, ws_port) run_thread(worker, 'api_zeroconf') diff --git a/openlp/core/app.py b/openlp/core/app.py index 6e0ba7834..5d3a919e4 100644 --- a/openlp/core/app.py +++ b/openlp/core/app.py @@ -311,6 +311,7 @@ def main(): """ The main function which parses command line options and then runs """ + log.debug(f'Entering function - main') args = parse_options() qt_args = ['--disable-web-security'] # qt_args = [] @@ -373,12 +374,15 @@ def main(): # Initialise the Registry Registry.create() settings = Settings() + Registry().register('settings', settings) + # Need settings object for the threads. + settings_thread = Settings() + Registry().register('settings_thread', settings_thread) Registry().register('application-qt', application) Registry().register('application', app) Registry().set_flag('no_web_server', args.no_web_server) # Upgrade settings. app.settings = settings - Registry().register('settings', settings) application.setApplicationVersion(get_version()['version']) # Check if an instance of OpenLP is already running. Quit if there is a running instance and the user only wants one server = Server() diff --git a/openlp/core/common/registry.py b/openlp/core/common/registry.py index 3b2413a0d..1ff479eba 100644 --- a/openlp/core/common/registry.py +++ b/openlp/core/common/registry.py @@ -70,6 +70,7 @@ class Registry(metaclass=Singleton): :param key: The service to be created this is usually a major class like "renderer" or "main_window" . :param reference: The service address to be saved. """ + log.debug(f'Registering object {key}') if key in self.service_list: trace_error_handler(log) log.error('Duplicate service exception {key}'.format(key=key)) @@ -95,6 +96,7 @@ class Registry(metaclass=Singleton): recipients. :param function: The function to be called when the event happens. """ + log.debug(f'Registering event {event}') if event in self.functions_list: self.functions_list[event].append(function) else: diff --git a/openlp/core/loader.py b/openlp/core/loader.py index 43538119e..04d15fde6 100644 --- a/openlp/core/loader.py +++ b/openlp/core/loader.py @@ -26,7 +26,6 @@ from openlp.core.state import State from openlp.core.ui.media.mediacontroller import MediaController from openlp.core.lib.pluginmanager import PluginManager from openlp.core.display.render import Renderer -from openlp.core.lib.imagemanager import ImageManager from openlp.core.ui.slidecontroller import LiveController, PreviewController @@ -40,7 +39,6 @@ def loader(): MediaController() PluginManager() # Set up the path with plugins - ImageManager() Renderer() # Create slide controllers PreviewController() diff --git a/openlp/core/ui/mainwindow.py b/openlp/core/ui/mainwindow.py index 63d0c5217..dfaf6f6e3 100644 --- a/openlp/core/ui/mainwindow.py +++ b/openlp/core/ui/mainwindow.py @@ -998,7 +998,7 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, LogMixin, RegistryPropert The screen has changed so we have to update components such as the renderer. """ self.application.set_busy_cursor() - self.image_manager.update_display() + # self.image_manager.update_display() self.renderer.resize(self.live_controller.screens.current.display_geometry.size()) self.preview_controller.screen_size_changed() self.live_controller.setup_displays() diff --git a/openlp/plugins/images/imageplugin.py b/openlp/plugins/images/imageplugin.py index 26cdd04f9..214fe4c89 100644 --- a/openlp/plugins/images/imageplugin.py +++ b/openlp/plugins/images/imageplugin.py @@ -21,11 +21,9 @@ import logging -from PyQt5 import QtGui - from openlp.core.state import State from openlp.core.common.i18n import translate -from openlp.core.lib import ImageSource, build_icon +from openlp.core.lib import build_icon from openlp.core.lib.db import Manager from openlp.core.lib.plugin import Plugin, StringContent from openlp.core.ui.icons import UiIcons @@ -90,12 +88,3 @@ class ImagePlugin(Plugin): 'service': translate('ImagePlugin', 'Add the selected image to the service.') } self.set_plugin_ui_text_strings(tooltips) - - def config_update(self): - """ - Triggered by saving and changing the image border. Sets the images in image manager to require updates. Actual - update is triggered by the last part of saving the config. - """ - log.info('Images config_update') - background = QtGui.QColor(self.settings.value(self.settings_section + '/background color')) - self.image_manager.update_images_border(ImageSource.ImagePlugin, background) diff --git a/tests/conftest.py b/tests/conftest.py index d3b1fc205..63792b870 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -74,10 +74,13 @@ def settings(qapp, registry): sets = Settings() sets.setValue('themes/global theme', 'my_theme') Registry().register('settings', sets) + Registry().register('settings_thread', sets) Registry().register('application', qapp) qapp.settings = sets yield sets del sets + Registry().remove('settings') + Registry().remove('settings_thread') os.close(fd) os.unlink(Settings().fileName()) @@ -89,8 +92,10 @@ def mock_settings(qapp, registry): mk_settings = MagicMock() Registry().register('settings', mk_settings) Registry().register('application', qapp) + Registry().register('settings_thread', mk_settings) yield mk_settings Registry().remove('settings') + Registry().remove('settings_thread') del mk_settings diff --git a/tests/functional/openlp_core/api/test_websockets.py b/tests/functional/openlp_core/api/test_websockets.py index 65fdac822..c8156a95f 100644 --- a/tests/functional/openlp_core/api/test_websockets.py +++ b/tests/functional/openlp_core/api/test_websockets.py @@ -30,7 +30,7 @@ from openlp.core.common.registry import Registry @pytest.yield_fixture -def poller(registry): +def poller(settings): poll = Poller() yield poll diff --git a/tests/functional/openlp_core/api/v2/test_core.py b/tests/functional/openlp_core/api/v2/test_core.py index f883d08a4..5f2428489 100644 --- a/tests/functional/openlp_core/api/v2/test_core.py +++ b/tests/functional/openlp_core/api/v2/test_core.py @@ -47,7 +47,7 @@ def test_plugins_returns_list(flask_client): def test_system_information(flask_client, settings): - settings.setValue('api/authentication enabled', False) + Registry().get('settings_thread').setValue('api/authentication enabled', False) res = flask_client.get('/api/v2/core/system').get_json() assert res['websocket_port'] > 0 assert not res['login_required'] diff --git a/tests/functional/openlp_core/lib/test_image_manager.py b/tests/functional/openlp_core/lib/test_image_manager.py index 79c49cddb..693380cb3 100644 --- a/tests/functional/openlp_core/lib/test_image_manager.py +++ b/tests/functional/openlp_core/lib/test_image_manager.py @@ -39,6 +39,7 @@ from tests.utils.constants import RESOURCE_PATH TEST_PATH = str(RESOURCE_PATH) +@skip('Probably not going to use ImageManager') class TestImageWorker(TestCase, TestMixin): """ Test all the methods in the ImageWorker class @@ -88,6 +89,7 @@ class TestImageWorker(TestCase, TestMixin): assert mocked_image_manager.stop_manager is True, 'mocked_image_manager.stop_manager should have been True' +@skip('Probably not going to use ImageManager') class TestPriorityQueue(TestCase, TestMixin): """ Test the PriorityQueue class @@ -131,7 +133,7 @@ class TestPriorityQueue(TestCase, TestMixin): mocked_queue.remove.assert_called_once_with((Priority.High, Priority.Normal, mocked_image)) -@skip('Probably not going to use ImageManager in WebEngine/Reveal.js') +@skip('Probably not going to use ImageManager') class TestImageManager(TestCase, TestMixin): def setUp(self): diff --git a/tests/functional/openlp_core/ui/test_mainwindow.py b/tests/functional/openlp_core/ui/test_mainwindow.py index a931340e0..2a8cb7b05 100644 --- a/tests/functional/openlp_core/ui/test_mainwindow.py +++ b/tests/functional/openlp_core/ui/test_mainwindow.py @@ -151,8 +151,9 @@ def test_mainwindow_configuration(main_window): # WHEN: you check the started functions # THEN: the following registry functions should have been registered - expected_service_list = ['settings', 'application', 'main_window', 'http_server', 'authentication_token', - 'settings_form', 'service_manager', 'theme_manager', 'projector_manager'] + expected_service_list = ['settings', 'settings_thread', 'application', 'main_window', 'http_server', + 'authentication_token', 'settings_form', 'service_manager', 'theme_manager', + 'projector_manager'] expected_functions_list = ['bootstrap_initialise', 'bootstrap_post_set_up', 'bootstrap_completion', 'theme_update_global', 'config_screen_changed'] assert list(Registry().service_list.keys()) == expected_service_list, \