Add new infolabel ListItem.CurrentItem and new Boolean expression IsEven/IsOdd #15527

a1rwulf · 2019-02-15T20:39:54Z

Description

Looks like we have no way to just print the position
of an item within it's container.
Adds a new ListItem property to obtain the current
position within a container.
Additionally it adds new boolean conditions to check if an item is on an even or odd place, so one can easily apply different textures to every second row in a list.

Types of change

Bug fix (non-breaking change which fixes an issue)
Clean up (non-breaking change which removes non-working, unmaintained functionality)
Improvement (non-breaking change which improves existing functionality)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that will cause existing functionality to change)
Cosmetic change (non-breaking change that doesn't touch code)
None of the above (please explain below)

Checklist:

My code follows the Code Guidelines of this project
My change requires a change to the documentation, either Doxygen or wiki
I have updated the documentation accordingly
I have read the Contributing document
I have added tests to cover my change
All new and existing tests passed

ronie · 2019-02-15T21:31:19Z

can't tell if it should be an infolabel or a property... the difference has never been clear to me.

could you please use Number instead of Position?
Position is used in other infolabels (Container(id).Position / Container(id).ListItemPosition(id).[infolabel]) to refer to the focused position.

jurialmunkey · 2019-02-17T03:19:55Z

Unless there is a specific reason for using a property, my preference is for an infolabel as they are easier to read in the skin xml

+1 for ListItem.Number

Hitcher · 2019-02-17T07:36:36Z

Or ListItem.CurrentItem as it does the same as Container(id).CurrentItem?

Wintermute0110 · 2019-02-17T09:26:20Z

One question, I think related to this conversation. Is it possible from Python to set the focus of a ListItem on a particular item row?

jurialmunkey · 2019-02-17T10:16:58Z

@HitcherUK - I agree, ListItem.CurrentItem is better.

@Wintermute0110 - you can set focus with the builtin SetFocus(ID,POSITION,absolute) - so it's definitely possible in python. How does that affect returning the absolute position as an infolabel though?

Hitcher

+1

Wintermute0110 · 2019-02-18T07:29:42Z

@jurialmunkey Thanks a lot! I though it was not possible to set the position so if someone was proposing to return the position I was about to propose to be able to also set the position.

ronie

agreed, ListItem.CurrentItem would be the best name for this new infolabel.

a1rwulf · 2019-07-10T16:22:41Z

@ronie OK, I finally got some time to change the PR.
I've converted it into an infolabel with the requested name.
Can you check if my documentation is done right and possibly give it a try at some point?

Rechi · 2019-07-10T18:38:43Z

xbmc/guilib/GUIListItem.cpp

+unsigned int CGUIListItem::GetCurrentItem() const
+{
+  return m_currentItem;
+}


add an empty line at the end of file

DaveTBlake · 2019-07-11T06:45:47Z

can't tell if it should be an infolabel or a property... the difference has never been clear to me.

At a guess I think the difference was once related to where the data was from, with infolabels relating to data held in one of the CFileItem infotags (music or video) and properties comming from item properties that had been appended to the item. Not sure how much we now diverge from this, CFiteItem is a monster.

a1rwulf · 2019-07-11T07:20:57Z

can't tell if it should be an infolabel or a property... the difference has never been clear to me.

At a guess I think the difference was once related to where the data was from, with infolabels relating to data held in one of the CFileItem infotags (music or video) and properties comming from item properties that had been appended to the item. Not sure how much we now diverge from this, CFiteItem is a monster.

When you request additional properties through json rpc, they are directly translated into fileitem-properties.
My guess is, that this is the reason why properties were introduced.
It made it easy to write generic code to extract whatever you need, by just looking up a std::map.

ksooo

I'm uncertain whether we actually need this.

First, it increases the size of every CGUIListItem instance, second I'm not sure that this functionality does actually not exist.

ksooo · 2019-07-15T12:53:28Z

xbmc/GUIInfoManager.cpp

@@ -5687,6 +5687,14 @@ const infomap container_str[]  = {{ "property",         CONTAINER_PROPERTY },
 ///     @return The parental rating of the list item (PVR).
 ///     <p>
 ///   }
+///   \table_row3{   <b>`ListItem.CurrentItem`</b>,
+///                  \anchor ListItem_CurrentItem
+///                  _integer_,


Type 'integer' does not match implementation, which returns a string. (GUIInfoManager.cpp line 9838). Do you only want to to display the value, then string is correct. If you want to do 'greater', 'less' etc math with the value, then you should go with integer.

I think display is enough.
I can't see a benefit of comparing them.

Given the latest conversation I found it doesn't matter if its _integer_.
The sample with ListItem.Year is also a _string_ and can be used in comparisons.
So I changed the documentation to align with the code.
CurrentItem is now _string_.

ksooo · 2019-07-15T12:57:06Z

xbmc/guilib/GUIListItem.h

@@ -172,6 +186,7 @@ class CGUIListItem
  CGUIListItemLayoutPtr m_layout;
  CGUIListItemLayoutPtr m_focusedLayout;
  bool m_bSelected;     // item is selected or not
+  unsigned int m_currentItem; // current item number within container (starting at 1)


CGUIListItems are mass objects, for instance every PVR EPG event is a CFileItem which is derived from CGUIListItem. One can easily have tens of thousands of those instances in one Kodi process. Thus, we should think twice before adding new members to this class.

I had an implementation based on ListItem.Properties first - you point is valid, where could I put this new infolabel instead?

Would it be possible to ignore the epggrid layout altogether?

No not in this case.
The CGUIListItem has no idea about grids, panels, lists or whatever.

Also tbh I think it's kinda neglectable.
Given an unsigend int takes up around 4 byte of memory, it may be more due to padding though, 1mio items would take up addionally 4mb of ram.
I really can't imagine, that this would break any platform.

a1rwulf · 2019-07-15T13:03:31Z

I'm uncertain whether we actually need this.

First, it increases the size of every CGUIListItem instance, second I'm not sure that this functionality does actually not exist.

I could not figure out how to do it without my change.
According to a slack conversation with our skinners, it was said that it isn't possible yet and that it was asked in the forums at some point.

ksooo · 2019-07-15T13:05:52Z

https://github.com/xbmc/xbmc/blob/master/xbmc/GUIInfoManager.cpp#L3439 ff.

Container(id).Position , Container(id).CurrentItem, ... ???

ksooo · 2019-07-15T13:10:24Z

Adds a new ListItem property to obtain the current
position within a container.

Container(id).CurrentItem - "@return The current item in the container or grouplist with given id."

Doesn't that fit?

a1rwulf · 2019-07-15T13:25:56Z

Adds a new ListItem property to obtain the current
position within a container.

Container(id).CurrentItem - "@return The current item in the container or grouplist with given id."

Doesn't that fit?

Description wise, yes.
But it gives me random numbers that change when scrolling.
So not sure what it's supposed to do.

a1rwulf · 2019-07-15T13:38:49Z

Both methods seem to operate on the current selection or focus, so it's not what I want.
I basically need the index of an item to show it in the GUI.
But now that we speak of it, I think it would better fit into the container section and it would be less intrusive.
Something like Container(id).CurrentIndex?

ksooo · 2019-07-15T13:52:53Z

Seems to fit better, yes.

a1rwulf · 2019-07-15T13:59:56Z

I just talked to @HitcherUK and I was told the Container(Id).... thing, is meant to be used outside of containers.
Now I remember that we had this discussion and this was the reason I came up with this PR.

ksooo · 2019-07-15T14:04:14Z

Not sure I get what you exactly want to achieve, but I'm not a skinner, so feel free to ignore me. 😃

a1rwulf · 2019-07-15T14:08:11Z

Basically I want to be able to not only show a list, but also number the items.
Like:

1. Item1
2. Item2
3. Item3
4. etc
5. etc

ksooo · 2019-07-15T14:23:27Z

AFAIK, an item instance can be shared among different containers, no? If so, the index the item has in an container cannot be a member of the item, because it can have another index in another container it might be contained as well.

ksooo · 2019-07-15T14:27:19Z

But if i see 'CGUILIstItem' members like 'm_bSelected' I might be wrong with my previous comment. Time to shut up, I guess. ;-)

jurialmunkey · 2019-07-15T14:30:41Z

It needs to be a listitem so that you can use it inside <itemlayout>
For instance, say you want to number the items in the list
<label>$INFO[ListItem.CurrentItem,My , label]</label> will now generate:
My 1 label
My 2 label
My 3 label

Having this infolabel will also allow skinners to do things like apply a texture only to odd rows by using something like
<visible>String.EndsWith(ListItem.CurrentItem,1) | String.EndsWith(ListItem.CurrentItem,3) | String.EndsWith(ListItem.CurrentItem,5) | String.EndsWith(ListItem.CurrentItem,7) | String.EndsWith(ListItem.CurrentItem,9)</visible>

Having a way to apply textures only to specific rows inside itemlayout and have those move only when the list scrolls has not been possible previously because options for comparing labels based upon itemlayout position are either:

String.IsEqual(ListItemPosition(1).Label,ListItem.Label) - which is a static position that doesn't move when the list scrolls.
String.IsEqual(ListItem(1).Label,ListItem.Label) - which is an offset position from focused item so it always moves when focus position changes in the list.
String.IsEqual(ListItemAbsolute(1).Label,ListItem.Label) - which is an absolute position and so requires as many conditions as there are items in the list.

None of these options provides a sane way to have a condition in the itemlayout based upon the absolute position of the item in the list (the 3rd option comes closest, but requires a condition for every single possible position just to do something simple like highlighting every second row).

a1rwulf · 2019-07-16T08:41:32Z

@jurialmunkey the even/odd usecase for textures is an interesting case.
The optimal implementation for this however would be, to make ListItem.CurrentItem be integer instead of string as per @ksooo 's comment and implement an even/odd method.
Then you could write something like:

<visible>Integer.IsOdd(ListItem.CurrentItem)</visible>
<visible>Integer.IsEven(ListItem.CurrentItem)</visible>

This infolabel allows a skinner to print the current index of an item in within it's container.

a1rwulf · 2019-07-16T16:04:07Z

Inspired by @jurialmunkey's usecase I checked the code and found it's easy to implement the odd/even boolean condition.
See updated commit.

jurialmunkey · 2019-07-20T08:09:10Z

I gave the testbuild a spin. Working well afaict. 👍

I tested with/without parent folder items, in panels, in wrap/fixedlists, and referencing other containers and all worked as expected. The IsOdd/Even conditions worked correctly everywhere I tried them.

a1rwulf · 2019-07-20T09:58:15Z

I gave the testbuild a spin. Working well afaict.

I tested with/without parent folder items, in panels, in wrap/fixedlists, and referencing other containers and all worked as expected. The IsOdd/Even conditions worked correctly everywhere I tried them.

Awesome news!
Thanks for testing.

a1rwulf · 2019-07-22T08:03:27Z

As testing have been successful, I changed the PR description and will merge this week if there are no objections.

ksooo · 2019-08-20T10:15:30Z

As testing have been successful, I changed the PR description and will merge this week if there are no objections.

This was written 29 days ago. As there were no objections, let's do it now. :-)

a1rwulf added Type: Improvement non-breaking change which improves existing functionality Component: GUI engine v19 Matrix labels Feb 15, 2019

a1rwulf requested review from Hitcher, ronie and ksooo February 15, 2019 20:39

Hitcher approved these changes Feb 17, 2019

View reviewed changes

ronie requested changes Feb 18, 2019

View reviewed changes

jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Apr 10, 2019

a1rwulf force-pushed the listitem-position branch from 35743f3 to 8664b85 Compare July 10, 2019 15:01

jenkins4kodi removed the Rebase needed PR that does not apply/merge cleanly to current base branch label Jul 10, 2019

a1rwulf changed the title ~~Add ListItem.Property(Position)~~ Add new infolabel ListItem.CurrentItem Jul 10, 2019

a1rwulf marked this pull request as ready for review July 10, 2019 16:20

a1rwulf added the Wiki: Needed label Jul 10, 2019

a1rwulf added this to the Matrix 19.0-alpha 1 milestone Jul 10, 2019

Rechi reviewed Jul 10, 2019

View reviewed changes

a1rwulf force-pushed the listitem-position branch from 8664b85 to 8f2141c Compare July 10, 2019 20:40

ksooo reviewed Jul 15, 2019

View reviewed changes

a1rwulf added 2 commits July 16, 2019 17:46

Add new infolabel ListItem.CurrentItem

3a372a2

This infolabel allows a skinner to print the current index of an item in within it's container.

Guilib: Add IsOdd/IsEven boolean condition

13b1667

a1rwulf force-pushed the listitem-position branch from 8f2141c to 13b1667 Compare July 16, 2019 15:58

a1rwulf changed the title ~~Add new infolabel ListItem.CurrentItem~~ Add new infolabel ListItem.CurrentItem and new Boolean expression IsEven/IsOdd Jul 22, 2019

ksooo merged commit e48fb2a into xbmc:master Aug 20, 2019

KarellenX added Wiki: Done and removed Wiki: Needed labels Aug 4, 2021

Add new infolabel ListItem.CurrentItem and new Boolean expression IsEven/IsOdd #15527

Add new infolabel ListItem.CurrentItem and new Boolean expression IsEven/IsOdd #15527

Conversation

a1rwulf commented Feb 15, 2019 • edited Loading

Description

Types of change

Checklist:

ronie commented Feb 15, 2019

jurialmunkey commented Feb 17, 2019

Hitcher commented Feb 17, 2019

Wintermute0110 commented Feb 17, 2019

jurialmunkey commented Feb 17, 2019

Hitcher left a comment

Choose a reason for hiding this comment

Wintermute0110 commented Feb 18, 2019

ronie left a comment

Choose a reason for hiding this comment

a1rwulf commented Jul 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveTBlake commented Jul 11, 2019

a1rwulf commented Jul 11, 2019

ksooo left a comment • edited Loading

Choose a reason for hiding this comment

ksooo Jul 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a1rwulf commented Jul 15, 2019

ksooo commented Jul 15, 2019

ksooo commented Jul 15, 2019

a1rwulf commented Jul 15, 2019

a1rwulf commented Jul 15, 2019

ksooo commented Jul 15, 2019

a1rwulf commented Jul 15, 2019

ksooo commented Jul 15, 2019

a1rwulf commented Jul 15, 2019

ksooo commented Jul 15, 2019

ksooo commented Jul 15, 2019

jurialmunkey commented Jul 15, 2019 • edited Loading

a1rwulf commented Jul 16, 2019

a1rwulf commented Jul 16, 2019

jurialmunkey commented Jul 20, 2019

a1rwulf commented Jul 20, 2019 • edited Loading

a1rwulf commented Jul 22, 2019

ksooo commented Aug 20, 2019

a1rwulf commented Feb 15, 2019 •

edited

Loading

ksooo left a comment •

edited

Loading

ksooo Jul 15, 2019 •

edited

Loading

jurialmunkey commented Jul 15, 2019 •

edited

Loading

a1rwulf commented Jul 20, 2019 •

edited

Loading