jurialmunkey / script.skinvariables

A helper script for Kodi skinners to construct multiple variables
GNU General Public License v3.0
35 stars 17 forks source link

Possible bug? Expression parent #17

Closed roidy closed 2 years ago

roidy commented 2 years ago

Using the following expression template with a parent:-

<expression name="showInfoUpdate" containers="9101...9105" parent="Control.HasFocus({id})"> [Container({id}).IsUpdating + Integer.IsEqual(Container({id}).NumItems,0)] </expression>

I get this:-

<includes>
    <expression name="showInfoUpdate_C9101">
        [Container(9101).IsUpdating + Integer.IsEqual(Container(9101).NumItems,0)]
    </expression>
    <expression name="showInfoUpdate_C9102">
        [Container(9102).IsUpdating + Integer.IsEqual(Container(9102).NumItems,0)]
    </expression>
    <expression name="showInfoUpdate_C9103">
        [Container(9103).IsUpdating + Integer.IsEqual(Container(9103).NumItems,0)]
    </expression>
    <expression name="showInfoUpdate_C9104">
        [Container(9104).IsUpdating + Integer.IsEqual(Container(9104).NumItems,0)]
    </expression>
    <expression name="showInfoUpdate_C9105">
        [Container(9105).IsUpdating + Integer.IsEqual(Container(9105).NumItems,0)]
    </expression>
    <expression name="showInfoUpdate">
        [Container().IsUpdating + Integer.IsEqual(Container().NumItems,0)]
    </expression>
    <variable name="showInfoUpdate_Parent">
        <value condition="Control.HasFocus(9101)">$VAR[showInfoUpdate_C9101]</value>
        <value condition="Control.HasFocus(9102)">$VAR[showInfoUpdate_C9102]</value>
        <value condition="Control.HasFocus(9103)">$VAR[showInfoUpdate_C9103]</value>
        <value condition="Control.HasFocus(9104)">$VAR[showInfoUpdate_C9104]</value>
        <value condition="Control.HasFocus(9105)">$VAR[showInfoUpdate_C9105]</value>
        <value condition="True">$VAR[showInfoUpdate]</value>
    </variable>
</includes>

Clearly not correct as it creates a variable that references variables.

I would expect the resultant parent expression to be:-

<expression name="showInfoUpdate_Parent">
    [Control.HasFocus(9101) + $EXP[showInfoUpdate_C9101]] |
    [Control.HasFocus(9102) + $EXP[showInfoUpdate_C9102]] |
    [Control.HasFocus(9103) + $EXP[showInfoUpdate_C9103]] |
    [Control.HasFocus(9104) + $EXP[showInfoUpdate_C9104]] |
    [Control.HasFocus(9105) + $EXP[showInfoUpdate_C9105]]
</expression>

Is this a bug or was the script never designed to create parents for expressions?

jurialmunkey commented 2 years ago

Nope, wasn't designed for expressions to use the parent option. I honestly didn't even think of this type of approach at all.

Happy to consider adding something like this but I think it is likely to be pretty bad performance wise to have so many control.hasfocus() conditions.

Trying to understand the type of scenario where this wouldn't be better handled with includes/params e.g. doing $EXP[showinfoupdate_$PARAM[id]] instead.

roidy commented 2 years ago

Nope, wasn't designed for expressions to use the parent option. I honestly didn't even think of this type of approach at all.

That's what I figured. No problem, it's a single expression that's easy to write by hand.

Happy to consider adding something like this but I think it is likely to be pretty bad performance wise to have so many control.hasfocus() conditions.

No, that's ok, as I said it's a single use case and not worth changing the script for, I just didn't know if that was the expected behaviour of the script.

As for performance that's true for any of these generated variables, take for instance your own 'Defs_PercentPlayed_Progress'

https://github.com/jurialmunkey/skin.arctic.horizon.2/blob/e9e704d6d612379c47b94c7d99d61ce969c39845/1080i/script-skinvariables-includes.xml#L130

Worst case scenario you're doing 100 Integer.IsEqual comparisons. I know most of the time the variable will resolve well before the 100th while my expression always has to evaluate all of the comparisons, but it's only 5 Control.HasFocus comparisons so I'm not too concerned with the performance. Now if I was doing 100 every time then I'd worried :D

Trying to understand the type of scenario where this wouldn't be better handled with includes/params e.g. doing $EXP[showinfoupdate_$PARAM[id]] instead.

I've got a unique situation where I've got an information panel that's separate from my Widgets and thus has no access to the Widgets control 'id' but I need to change it's visibility based on weather the Widget is updating and already has content. I can't think of any other solution.

jurialmunkey commented 2 years ago

Worst case scenario you're doing 100 Integer.IsEqual comparisons. I know most of the time the variable will resolve well before the 100th while my expression always has to evaluate all of the comparisons, but it's only 5 Control.HasFocus comparisons so I'm not too concerned with the performance. Now if I was doing 100 every time then I'd worried :D

Yeah I hear you. I really wish that var didn't need to exist. However, in general IsEqual should be relatively fast because it is just checking that int == int which is about the fastest comparison operation possible.

It's more that I find all the Control.HasFocus() and Control.IsVisible() type conditions to be a bit performance hungry so generally I try to limit their usage. I'm not sure what goes on with Control.HasFocus() as I would expect it to simply check that system current control ID matches but the performance hits feels like a lot more than that is going on (it's not a massive hit to performance but I found when I used a lot of them in the past I would start to notice it - also possibly placebo, or very situationally specific )

But yeah, only 5 is going to be fine either way. I thought it was more you using it as an example for what would be a much bigger expression overall.

I've got a unique situation where I've got an information panel that's separate from my Widgets and thus has no access to the Widgets control 'id' but I need to change it's visibility based on weather the Widget is updating and already has content. I can't think of any other solution.

Why can't you just use Container.IsUpdating and Container.NumItems without any ID? Or is this in a separate custom dialog?

If you only need it visible for specific containers then you can set a window property and clear it for those containers. That way you're only checking one condition rather than five.

EDIT: Never mind me. I forgot that Container.NumItems only works without an ID in the MyNav windows.

roidy commented 2 years ago

Why can't you just use Container.IsUpdating and Container.NumItems without any ID? Or is this in a separate custom dialog?

EDIT: Never mind me. I forgot that Container.NumItems only works without an ID in the MyNav windows.

Yep, as you said Container.NumItems without an id only works in MyNav windows. A crude alternative to

Integer.IsEqual(Container.NumItems,0) is String.IsEmpty(ListItem.Label) and just hope every item has a label, not good :(

If you only need it visible for specific containers then you can set a window property and clear it for those containers. That way you're only checking one condition rather than five.

Yep, that was my first idea, however I couldn't get it to work.

In my Widgets focuseditem I had a hidden button with an onfocus to set a Window property to the current control id

<onfocus>SetProperty(WidgetID,$PARAM[id],home)</onfocus>

That way I'd have the control id of the widget, however on initial load when the widget is empty there's no focuseditem to set the property.

Edit:- Actually String.IsEmpty(ListItem.UniqueID()) should work as that should always have a value if there's an item.