On Sat, Apr 28, 2018 at 12:15 AM, Phill <phill.rid...@gmail.com> 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.rid...@gmail.com> 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.
_______________________________________________
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help : https://help.launchpad.net/ListHelp
