Jump to content


Photo

ConditionalShowHide and ServiceInfo Converter

enigma2

  • Please log in to reply
9 replies to this topic

#1 mx3L

  • Senior Member
  • 616 posts

+79
Good

Posted 22 June 2016 - 06:23

Hello,

 

When I was adding EPG support for serviceapp I noticed that if this service has EPG available, it always showed also subservices icon in InfoBar as if subservices were available.

In fact subservices are not implemented so this is not correct.

 

I found out, that it's happening because after evUpdatedEventInfo is triggered we check in ServiceInfo for subservices, but since subservices are None it sets boolean for this converter to None, then next downstream element in PLi-HD skin is ConditionalShowHide which transforms this boolean from None to True, which sets visibility for Pixmap renderer to True, which results in shown subservices icon when subservices are not implemented.

 

1. We should make sure to set boolean of ServiceInfo to boolean value and not None:

diff --git a/lib/python/Components/Converter/ServiceInfo.py b/lib/python/Components/Converter/ServiceInfo.py
index 04e0320..3d99ca0 100644
--- a/lib/python/Components/Converter/ServiceInfo.py
+++ b/lib/python/Components/Converter/ServiceInfo.py
@@ -102,12 +102,12 @@ class ServiceInfo(Converter, object):
                        return info.getInfo(iServiceInformation.sAspect) in WIDESCREEN
                elif self.type == self.SUBSERVICES_AVAILABLE:
                        subservices = service.subServices()
-                       return subservices and subservices.getNumberOfSubservices() > 0
+                       return subservices and subservices.getNumberOfSubservices() > 0 or False
                elif self.type == self.HAS_HBBTV:
                        return info.getInfoString(iServiceInformation.sHBBTVUrl) != ""
                elif self.type == self.AUDIOTRACKS_AVAILABLE:
                        audio = service.audioTracks()
-                       return audio and audio.getNumberOfTracks() > 1
+                       return audio and audio.getNumberOfTracks() > 1 or False
                elif self.type == self.SUBTITLES_AVAILABLE:
                        subtitle = service and service.subtitle()
                        subtitlelist = subtitle and subtitle.getSubtitleList()

This basically solves the problem.

 

2. Next thing is that in ConditionalShowHide there was added workaround for sources which boolean's value is None to transform it to True to avoid crash.

https://github.com/O...63bdcbf8bc0ea53

 

Problem is that this workaround for me for unknown reason returns True for None value, also it doesn't respect invert setting.

 

 a] We can fix this by transforming None to False, while invert setting is respected:

diff --git a/lib/python/Components/Converter/ConditionalShowHide.py b/lib/python/Components/Converter/ConditionalShowHi
de.py
index 246fed2..fbccc61 100644
--- a/lib/python/Components/Converter/ConditionalShowHide.py
+++ b/lib/python/Components/Converter/ConditionalShowHide.py
@@ -33,7 +33,7 @@ class ConditionalShowHide(Converter, object):
        def calcVisibility(self):
                b = self.source.boolean
                if b is None:
-                       return True
+                       b = False
                b ^= self.invert
                return b

b] We can remove this workaround, but we would have to fix every element which could be used with ConditionalShowHide and it's boolean returns None otherwise we would have crashes.

c] Don't do anything and make sure that source's boolean is boolean value.

 

I think the first case patch should be OK, In second case I think we should fix workaround a] since b] could potentially bring down custom Elements which we cannot fix. c] option is also good since it's safe if some Elements rely on this behavior and we can fix errors as they appear, maybe show some warning in case boolean value is None?


Edited by mx3L, 22 June 2016 - 06:24.


Re: ConditionalShowHide and ServiceInfo Converter #2 littlesat

  • PLi® Core member
  • 49,729 posts

+543
Excellent

Posted 22 June 2016 - 07:04

And what did you try exactly to find this???

Edited by littlesat, 22 June 2016 - 07:10.

WaveFrontier 28.2E | 23.5E | 19.2E | 16E | 13E | 10/9E | 7E | 5E | 1W | 4/5W | 15W


Re: ConditionalShowHide and ServiceInfo Converter #3 mx3L

  • Senior Member
  • 616 posts

+79
Good

Posted 22 June 2016 - 07:17

 

The suggested changes do make no sense...

X > Y is always a boolean... But what if audio is None? Does that happen... Under which conditions?

>>> print None and True
None
>>> print None and True or False
False

audio is None when iAudioTrackSelection is not implemented, which is not case for any service, but why not make it explicit?

 

And the second one when there is no source boolean... You do not need to set it and possibly invert something... When does ut happen that source has no boolean?

If there is no source boolean you would get AttributeError:

>>> object().boolean
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'object' object has no attribute 'boolean'

As I described in case you have ServiceInfo->ConditionalShowHide converters ServiceInfo object will have boolean set to None if subservices are not available, this is relevant part of Pli-HD skin

    <widget source="session.CurrentService" render="Pixmap" pixmap="PLi-HD/infobar/ico_sub_on.png" position="839,588"
      <convert type="ServiceInfo">SubservicesAvailable</convert>
      <convert type="ConditionalShowHide" />
    </widget>


Re: ConditionalShowHide and ServiceInfo Converter #4 mx3L

  • Senior Member
  • 616 posts

+79
Good

Posted 22 June 2016 - 07:31

And what did you try exactly to find this???

What do you mean? I looked for subservices in Converters found ServiceInfo add some debug messages then looked in ConditionalShowHide and see what it does with None value..



Re: ConditionalShowHide and ServiceInfo Converter #5 littlesat

  • PLi® Core member
  • 49,729 posts

+543
Excellent

Posted 22 June 2016 - 08:32

Can you push a merge suggestion?


WaveFrontier 28.2E | 23.5E | 19.2E | 16E | 13E | 10/9E | 7E | 5E | 1W | 4/5W | 15W


Re: ConditionalShowHide and ServiceInfo Converter #6 littlesat

  • PLi® Core member
  • 49,729 posts

+543
Excellent

Posted 22 June 2016 - 08:37

Just another simplify suggestion... Sometimes less is more.... ;)

def calcVisibility(self):
    return (self.source.boolean or False) ^ self.invert

Edited by littlesat, 22 June 2016 - 08:40.

WaveFrontier 28.2E | 23.5E | 19.2E | 16E | 13E | 10/9E | 7E | 5E | 1W | 4/5W | 15W


Re: ConditionalShowHide and ServiceInfo Converter #7 mx3L

  • Senior Member
  • 616 posts

+79
Good

Posted 22 June 2016 - 10:09

Nice, maybe even better?

def calcVisibility(self):
    return bool(self.source.boolean) ^ self.invert

Will create pull



Re: ConditionalShowHide and ServiceInfo Converter #8 mx3L

  • Senior Member
  • 616 posts

+79
Good

Posted 22 June 2016 - 11:02

Thinking about it, I think it's not such a good idea, since we basically extend this workaround not only for None but boolean value can be anything like:

>>> bool([])
False
>>> bool(['a'])
True
>>> bool(0)
False
>>> bool(1)
True
>>> bool("")
False
>>> bool("test")
True

We should be stricter about this and only allow None as was before, other cases should end with crash since it's clearly mistake to set boolean other then bool value and should be fixed in source.

So I would like to push original suggestion.

 

What do you think?



Re: ConditionalShowHide and ServiceInfo Converter #9 littlesat

  • PLi® Core member
  • 49,729 posts

+543
Excellent

Posted 22 June 2016 - 13:23

Why make it more complicated.... I do not expect [] ["x"]  "" "text" or 0 1 there... Just None True, False or None... And we do not even catch that with the IF... the result is the same so....

 

i liked your bool(self.source.boolean) :D


Edited by littlesat, 22 June 2016 - 13:25.

WaveFrontier 28.2E | 23.5E | 19.2E | 16E | 13E | 10/9E | 7E | 5E | 1W | 4/5W | 15W


Re: ConditionalShowHide and ServiceInfo Converter #10 mx3L

  • Senior Member
  • 616 posts

+79
Good

Posted 22 June 2016 - 16:29

 

Why make it more complicated.... I do not expect [] ["x"]  "" "text" or 0 1 there... Just None True, False or None...

Me neither, but Source and Converter can be written by anybody and their boolean might by mistake return non-boolean value, which might bring unexpected results, since there would be no apparent error shown because of converting everything with bool().

 

And we do not even catch that with the IF... the result is the same so....

If boolean is not None,False or True, it will crash since xor cannot be done on string, array....:

>>> [] ^ True
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unsupported operand type(s) for ^: 'list' and 'bool'
>>> "" ^ True
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unsupported operand type(s) for ^: 'str' and 'bool'

Still I missed one since xor can be done on numbers so it's still not good.

 

Thanks for merge :)







1 user(s) are reading this topic

0 members, 1 guests, 0 anonymous users