openlp-core team mailing list archive

[Phill <phill.ridout@xxxxxxxxx>]

Review: Needs Fixing

Just a few minor things

Diff comments:

> 
> === modified file 'openlp/core/api/websockets.py'
> --- openlp/core/api/websockets.py	2017-12-29 09:15:48 +0000
> +++ openlp/core/api/websockets.py	2018-01-07 05:37:57 +0000
> @@ -28,37 +28,88 @@
>  import logging
>  import time
>  
> -import websockets
> -from PyQt5 import QtCore
> +from websockets import serve
>  
>  from openlp.core.common.mixins import LogMixin, RegistryProperties
>  from openlp.core.common.registry import Registry
>  from openlp.core.common.settings import Settings
> +from openlp.core.threading import ThreadWorker, run_thread
>  
>  log = logging.getLogger(__name__)
>  
>  
> -class WebSocketWorker(QtCore.QObject):
> +async def handle_websocket(request, path):
> +    """
> +    Handle web socket requests and return the poll information.
> +    Check ever 0.2 seconds to get the latest position and send if changed.

typo in "ever

> +    Only gets triggered when 1st client attaches
> +
> +    :param request: request from client
> +    :param path: determines the endpoints supported
> +    :return:

Can you remove this ':return:' please

> +    """
> +    log.debug('WebSocket handler registered with client')
> +    previous_poll = None
> +    previous_main_poll = None
> +    poller = Registry().get('poller')
> +    if path == '/state':
> +        while True:
> +            current_poll = poller.poll()
> +            if current_poll != previous_poll:
> +                await request.send(json.dumps(current_poll).encode())
> +                previous_poll = current_poll
> +            await asyncio.sleep(0.2)
> +    elif path == '/live_changed':
> +        while True:
> +            main_poll = poller.main_poll()
> +            if main_poll != previous_main_poll:
> +                await request.send(main_poll)
> +                previous_main_poll = main_poll
> +            await asyncio.sleep(0.2)
> +
> +
> +class WebSocketWorker(ThreadWorker, RegistryProperties, LogMixin):
>      """
>      A special Qt thread class to allow the WebSockets server to run at the same time as the UI.
>      """
> -    def __init__(self, server):
> -        """
> -        Constructor for the thread class.
> -
> -        :param server: The http server class.
> -        """
> -        self.ws_server = server
> -        super(WebSocketWorker, self).__init__()
> -
> -    def run(self):
> -        """
> -        Run the thread.
> -        """
> -        self.ws_server.start_server()
> +    def start(self):
> +        """
> +        Run the worker.
> +        """
> +        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)
> +        # Create the websocker server
> +        loop = 1
> +        self.server = None
> +        while not self.server:
> +            try:
> +                self.server = serve(handle_websocket, address, port)
> +                log.debug('WebSocket server started on {addr}:{port}'.format(addr=address, port=port))
> +            except Exception as e:

'e' unused

> +                log.exception('Failed to start WebSocket server')
> +                loop += 1
> +                time.sleep(0.1)
> +            if not self.server and loop > 3:
> +                log.error('Unable to start WebSocket server {addr}:{port}, giving up'.format(addr=address, port=port))
> +        if self.server:
> +            # If the websocket server exists, start listening
> +            self.event_loop.run_until_complete(self.server)
> +            self.event_loop.run_forever()
> +        self.quit.emit()
>  
>      def stop(self):
> -        self.ws_server.stop = True
> +        """
> +        Stop the websocket server
> +        """
> +        if hasattr(self.server, 'ws_server'):
> +            self.server.ws_server.close()
> +        elif hasattr(self.server, 'server'):
> +            self.server.server.close()
> +        self.event_loop.stop()
> +        self.event_loop.close()
>  
>  
>  class WebSocketServer(RegistryProperties, LogMixin):
> 
> === modified file 'openlp/core/threading.py'
> --- openlp/core/threading.py	2017-12-29 09:15:48 +0000
> +++ openlp/core/threading.py	2018-01-07 05:37:57 +0000
> @@ -51,5 +66,46 @@
>      worker.quit.connect(thread.quit)
>      worker.quit.connect(worker.deleteLater)
>      thread.finished.connect(thread.deleteLater)
> -    if auto_start:
> +    thread.finished.connect(make_remove_thread(thread_name))
> +    if can_start:
>          thread.start()
> +
> +
> +def get_thread_worker(thread_name):
> +    """
> +    Get the worker by the thread name
> +
> +    :param str thread_name: The name of the thread
> +    :returns ThreadWorker: The worker for this thread name
> +    """
> +    return Registry().get('main_window').threads.get(thread_name)
> +
> +
> +def is_thread_finished(thread_name):
> +    """
> +    Check if a thread is finished running.
> +
> +    :param str thread_name: The name of the thread
> +    :returns bool: True if the thread is finished, False if it is still running

Not sure this is valid. Think you need to use :rtype: see my irc comments

> +    """
> +    main_window = Registry().get('main_window')
> +    return thread_name not in main_window.threads or main_window.threads[thread_name]['thread'].isFinished()
> +
> +
> +def make_remove_thread(thread_name):
> +    """
> +    Create a function to remove the thread once the thread is finished.
> +
> +    :param str thread_name: The name of the thread which should be removed from the thread registry.
> +    :returns function: A function which will remove the thread from the thread registry.

Not sure this is valid. Think you need to use :rtype: see my irc comments

> +    """
> +    def remove_thread():
> +        """
> +        Stop and remove a registered thread
> +
> +        :param str thread_name: The name of the thread to stop and remove
> +        """
> +        main_window = Registry().get('main_window')
> +        if thread_name in main_window.threads:
> +            del main_window.threads[thread_name]
> +    return remove_thread


-- 
https://code.launchpad.net/~raoul-snyman/openlp/better-threading/+merge/335801
Your team OpenLP Core is subscribed to branch lp:openlp.