Create ByPropertyIdMap

[Benestar]

This should finally act as a replacement for ByPropertyIdArray. It introduces a new form of indexing using two steps (therefore also two arrays in this class).

First, there is an index for each property id, which states the order of per-property groups.

Second, there is an index for each PropertyIdProvider inside a per-property group.

Both indices can be modified separately, using the move methods. It is also possible to add a new PropertyIdProvider. The finally usable result can be obtained using the method getFlatArray which returns a simple list of PropertyIdProvider instances which are grouped by property id and in the order which has been specified using the move methods.

Tracked in https://phabricator.wikimedia.org/T98375

[JeroenDeDauw]

What are the responsibilities for this class? Just moving stuff around? What about methods that get or remove individual elements or groups?

At the moment I find the class slightly confusing if one just looks at the code. For instance the field names are not entirely self explanatory. Somehow separating the operations on individual elements and on groups would be nice. In the past there was talk about adding a class for representing a group. (See the only not closed off item in #22) Have you thought about that? I'm thinking it might make such code a lot more obvious.

[Benestar]

What are the responsibilities for this class? Just moving stuff around?

The responsibility of this class is indeed to be able to move elements within a group and groups alltogether which is needed in the UI where we want to enable some moving functionality again.

What about methods that get or remove individual elements or groups?

For getting groups we have ByPropertyIdGrouper. I intentionally separated those two classes so that ByPropertyIdGrouper only does the grouping and ByPropertyIdMap the editing/moving of those groups. If more functionality for removing individual elements or groups is needed, it can be added to that class. However, I see no place where we would need such methods.

For instance the field names are not entirely self explanatory.

Can you describe more detailed which field names are not entirely self explanatory and why? For me as I wrote the code it is a bit hard to identify where there might be issues.

Somehow separating the operations on individual elements and on groups would be nice.

I thought I did that by calling them moveGroupToIndex and movePropertyIdProviderToIndex? Maybe use moveElementToIndex instead?

In the past there was talk about adding a class for representing a group.

I think this class is fine the way it is. Statements, the main use case of this class, are stored as a flat list on items/properties (ie all StatementListProvider instances). This class is able to take such a list, does some grouping, allows reordering the elements and returns another flat list. If we once decide to store Statements grouped by property id in items/properties, we can still rework this class but for the time being I think this is a good and valid alternative to ByPropertyIdArray. Remember this class does not aim to be perfect, it just wants to improve ByPropertyIdArray.

[JeroenDeDauw]

Can you describe more detailed which field names are not entirely self explanatory and why?

By this I mean that if one never saw the class before, the field names will not make clear what they contain. One can guess their role after also reading the method names, and confirm that guess after looking at their implementation a bit. At least that was how it went for me. looking at it again, this seems easy to fix: adding a short comment to the $byPropertyId field describing what it contains. I'm happy to add this if you want.

I thought I did that by calling them moveGroupToIndex and movePropertyIdProviderToIndex? Maybe use moveElementToIndex instead

Sure, you have separation by naming. The methods are on the same level though. This is not necessarily bad, and not something we can meaningfully discuss in isolation, as it needs to be compared to an alternative approach.

So you think having a StatementGroup would not help improve the situation? To clarify, such a group would be responsible for its internal consistency, ie making sure there are no statements with a different property id in it. And it would hold methods to move statements within the group, while something else could then just deal with moving groups.

I'm not opposed to this change, though am personally not going to merge this while I'm not more sure about the suitability, or lack thereof, of having a StatementGroup. If you do not want to investigate that, this is fine. If this topic gets stuck, I'll probably do that myself.

[Benestar]

Almost everywhere in the code we just deal with a list of statements so StatementList is enough in those places. There is only one use case in Wikibase Repository which actually requires the grouping by property id and needs sorting and reindexing these groups.

Given this single use case I think it would be too much overhead to create a new datastructure which is only useful in one single place. Of course we can split this class and have to separate classes to handle the indexing of property ids and the indexing within a group. However, I think to cover this rare usecase, this class is sufficient and we don't have to create new datastructures when there is no real need for them.

[JeroenDeDauw]

Sounds reasonable. +1

[JeroenDeDauw]

So am I understanding your comments on #480 correctly by interpreting we might not need this to begin with?

[Benestar]

So am I understanding your comments on #480 correctly by interpreting we might not need this to begin with?

Not sure if I understand your comment correctly. We don't need this class if we just want to drop the index parameter on wbsetclaim completely. If we however want to provide a replacement, we have to use this class in future.