Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[guiinfo] extend ranges for listitem properties #8922

Closed
wants to merge 1 commit into from

Conversation

phil65
Copy link
Contributor

@phil65 phil65 commented Jan 19, 2016

Since the amount of properties skins are using grew quite significantly over the last years, the ranges we set back then for the listitem properties are not large enough anymore. This should fix that by approx. doubling the ranges.
As a testcase you can try running a simple script

import xbmc
for i in range(1, 90):
    InfoLabel = xbmc.getInfoLabel("Container(9000).ListItem.Property(%i)" % (i))

or
https://www.dropbox.com/s/z4xhfy7caeipfkd/script.bug.zip?dl=0

and see that this breaks confluence without this commit (no fanart visible anymore in library for example)

@mkortstiege @ronie

@ronie
Copy link
Member

ronie commented Jan 20, 2016

thanx, i'll test it.

i wonder why it reaches the limit so soon though...
looking at the code, we should be able to get 1300 listitem properties,
out of which 500 are reserved for art/rating/votes.
but that still leaves 800 to use... i wonder what those are being used by.

while you're at it, perhaps we should also bump LISTITEM_PROPERTY_START?
there's currently only 39 spaces left for adding new infolabels.

@ronie
Copy link
Member

ronie commented Jan 20, 2016

hmm... something else may be going wrong. it indeed breaks after 90,
but i need to up it to 1290 before i get the error message:
ERROR: AddListItemProp - not enough listitem property space!

@phil65
Copy link
Contributor Author

phil65 commented Jan 20, 2016

Perhaps it is worth reworking that part so that we consistently use _START / _END instead of _OFFSET? it felt quite conflusing to me when i looked at it first time.

@mkortstiege
Copy link
Member

Can't really remember my findings but i agree that it is kinda weird to read. @Paxxi, do you remember the offset issue? I think we talked about it at some point.

@DaveTBlake
Copy link
Member

Perhaps it is worth reworking that part so that we consistently use _START / _END instead of _OFFSET? it felt quite conflusing to me when i looked at it first time.

Definitely not obvious how to add to it at the moment. Hands up - it is possible that I have broken it in ignorance. :(

I have introduced new listitem properties without allocating a sub range or offset, the Role.XXXXX properties. Could someone look at how I have implemented it and tell me what I should have done?

@ronie
Copy link
Member

ronie commented Jan 20, 2016

it is possible that I have broken it in ignorance. :(

nah, problem exists in isengard as well.

@phil65 after applying this patch, change your script to get 200 properties
and things will break again :(

@DaveTBlake
Copy link
Member

nah, problem exists in isengard as well.

That is a relief, not down to me!

All the same I wonder why votes property has a range of IDs? A list item only has one votes value doesn't it? And I am now questioning my implementation of the role property. I'm not sure I understand how this is all meant to work.

@razzeee
Copy link
Member

razzeee commented Jan 20, 2016

There are more multiple ratings so there are votes for each of them

@phil65
Copy link
Contributor Author

phil65 commented Jan 20, 2016

@ronie i know that it still breaks with higher numbers, but it at least gives us some more room. As said, I have hit that limit in real-life already when using several scripts in a row. Doubling that amount will already help a lot.
Youre right of course that we should still investigate why the allowed amount is that low.

Perhaps we should take this fix for backporting at least?

@MartijnKaijser
Copy link
Member

ping

@ronie
Copy link
Member

ronie commented Jun 22, 2016

a proper fix would be more than welcome...
perhaps @notspiff could provide some pointers

@notspiff
Copy link
Contributor

well, the first thing that hits me is that the logic is completely broken.

consider when we have 100 "normal" props. there is suddenly no space for any music props because we use

return LISTITEM_PROPERTY_START + offset + m_listitemProperties.size() - 1; as index.

since m_listitemProperties.size() == 100, we hit the video label range immediately! so for >= 100 props we are SOL.

that being said i cannot reproduce your errors without pushing a lot more info labels in the dummy script (1300-ish). neither with confluence nor estuary. is this on windows? if so, try postfixing the defines with L or the likes. maybe something ends up as a 8/16 bit var and overflow is the issue.

@ronie
Copy link
Member

ronie commented Jun 23, 2016

it happens on linux as well.
to reproduce it (in Confluence) i had to up the number of labels in the script to 99.
after running the script, navigate to the movie library and you should notice the fanart isn't displayed in the background anymore.

@phil65
Copy link
Contributor Author

phil65 commented Sep 26, 2016

superseded by #10527

@phil65 phil65 closed this Sep 26, 2016
@phil65 phil65 deleted the allow_more_props branch December 15, 2016 05:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants