openlp-core team mailing list archive

[Phill <phill.ridout@xxxxxxxxx>]

Review: Needs Fixing

Ran out of time to complete a review, but a few in lines to start.

Diff comments:

> 
> === modified file 'openlp/core/projectors/manager.py'
> --- openlp/core/projectors/manager.py	2018-04-20 06:04:43 +0000
> +++ openlp/core/projectors/manager.py	2018-04-28 02:41:38 +0000
> @@ -513,6 +519,18 @@
>              projector.socket_timer.timeout.disconnect(projector.link.socket_abort)
>          except (AttributeError, TypeError):
>              pass
> +        # Disconnect signals from projector being deleted
> +        try:
> +            self.pjlink_udp.data_received.disconnect(projector.get_buffer)

He's disconnecting a slot here, so TypeError is the correct exception to catch. Not sure it AttributeError is needed though

> +        except (AttributeError, TypeError):
> +            pass
> +        try:
> +            if self.pjlink_udp[projector.port]:
> +                self.pjlink_udp[projector.port].data_received.disconnect(projector.get_buffer)
> +        except (AttributeError, TypeError):
> +            pass
> +
> +        # Rebuild projector list
>          new_list = []
>          for item in self.projector_list:
>              if item.link.db_item.id == projector.link.db_item.id:
> 
> === modified file 'openlp/core/projectors/pjlink.py'
> --- openlp/core/projectors/pjlink.py	2018-04-20 06:04:43 +0000
> +++ openlp/core/projectors/pjlink.py	2018-04-28 02:41:38 +0000
> @@ -108,22 +111,20 @@
>          if read_size < 0:
>              log.warning('(UDP) No data (-1)')
>              return
> -        if read_size < 1:
> +        elif read_size < 1:

given the < 0 condition above, this condition will only be True when read_size == 0, which to me is a bit readable / obvious at a quick glance.

>              log.warning('(UDP) get_datagram() called when pending data size is 0')
>              return
> -        data, peer_address, peer_port = self.readDatagram(self.pendingDatagramSize())
> +        elif read_size > PJLINK_MAX_PACKET:
> +            log.warning('(UDP) UDP Packet too large ({size} bytes)- ignoring'.format(size=read_size))
> +            return
> +        data_in, peer_host, peer_port = self.readDatagram(read_size)
> +        data = data_in.decode('utf-8') if isinstance(data_in, bytes) else data_in
>          log.debug('(UDP) {size} bytes received from {adx} on port {port}'.format(size=len(data),
> -                                                                                 adx=peer_address,
> -                                                                                 port=peer_port))
> +                                                                                 adx=peer_host.toString(),
> +                                                                                 port=self.port))
>          log.debug('(UDP) packet "{data}"'.format(data=data))
> -        # Send to appropriate instance to process packet
> -        log.debug('(UDP) Checking projector list for ip {host} to process'.format(host=peer_address))
> -        for projector in self.projector_list:
> -            if peer_address == projector.ip:
> -                # Dispatch packet to appropriate remote instance
> -                log.debug('(UDP) Dispatching packet to {host}'.format(host=projector.entry.name))
> -                return projector.get_data(buff=data, ip=peer_address, host=peer_address, port=peer_port)
> -        log.warning('(UDP) Could not find projector with ip {ip} to process packet'.format(ip=peer_address))
> +        log.debug('(UDP) Sending data_received signal to projectors')
> +        self.data_received.emit(peer_host, self.localPort(), data)
>          return
>  
>      def search_start(self):
> @@ -654,10 +651,10 @@
>          :param host: IP address of sending host
>          :param port: Port received on
>          """
> -        log.warning('(UDP) SRCH packet received from {host} - ignoring'.format(host=host))
> +        log.warning("({ip}) I don't do SRCH packets - ignoring".format(ip=self.ip))

Why this change? the original is more understandable to me!

>          return
>  
> -    def process_sver(self, data, *args, **kwargs):
> +    def process_sver(self, data):
>          """
>          Software version of projector
>          """
> @@ -1181,7 +1187,7 @@
>          try:
>              self.readyRead.disconnect(self.get_socket)
>          except TypeError:
> -            pass
> +            log.debug('({ip}) disconnect_from_host(): TypeError detected'.format(ip=self.entry.name))

You're just disconnecting a slot here right? Do we need to log that?

>          log.debug('({ip}) disconnect_from_host() '
>                    'Current status {data}'.format(ip=self.entry.name, data=self._get_status(self.status_connect)[0]))
>          if abort:


-- 
https://code.launchpad.net/~alisonken1/openlp/pjlink2-r/+merge/344795
Your team OpenLP Core is subscribed to branch lp:openlp.