openlp-core team mailing list archive

[Ken Roberts <alisonken1@xxxxxxxxx>]

On Sat, Apr 28, 2018 at 12:15 AM, Phill <phill.ridout@xxxxxxxxx> wrote:
> 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
>>          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.

-1 indicates a socket problem - which is distinct from no data (0 or
empty packet)

>> @@ -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!

This method is only called from within a projector instance - not the
UDP monitor.
However, just noticed it's using the wrong option for {ip} - should be
ip=self.entry.name

>
>>          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?

Since this is in the projector instance, I would like to know if
there's a type issue - if so, then I would like to see it in the log.

>
>>          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
> You are the owner of lp:~alisonken1/openlp/pjlink2-r.



-- 
- Ken
Registered Linux user 296561
Slackin' since 1993
Slackware Linux (http://www.slackware.com)
OpenLP - Church Projection Software
Empower Your Church http://openlp.org

On Sat, Apr 28, 2018 at 12:15 AM, Phill <phill.ridout@xxxxxxxxx> wrote:
> 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
> You are the owner of lp:~alisonken1/openlp/pjlink2-r.



-- 
- Ken
Registered Linux user 296561
Slackin' since 1993
Slackware Linux (http://www.slackware.com)
OpenLP - Church Projection Software
Empower Your Church http://openlp.org

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