feature: All RoleHandlers supports set/add/remove

[pvojtechovsky]

The list returned by RoleHandler#asList(CtElement) supports

• set(int, Object)
• remove(int)

for all kinds of roles.

The single value roles can use RoleHandler#asList(CtElement) too and you can call

• set(0, value)
• get(0)
• add(value) - if previous value was null
• remove(value) - to set the value to null

[surli]

I think there's already a lot of mixed responsibilities in this file and adding a new anonymous class here increases the complexity. So I'm ok with your changes but I'm in favor of extracting the static classes from this file to put them in their own file: I think it will help the future maintaining. WDYT?

[pvojtechovsky]

I'm in favor of extracting the static classes from this file to put them in their own file: I think it will help the future maintaining. WDYT?

Good idea. I am done with that. It is ready for merge.

[surli]

I find this contract odd: it's not the expected behaviour regarding Java standard usage of lists. Couldn't you check the size of the list? In this case what isEmpty() would return?

[pvojtechovsky]

It is not a normal List, it is wrapper around single value. The problem is
List list=...wrapperAroundSingleValue_x

x = "A"
assertEquals("A", list.get(0))
The problem is that both list operations
list.set(0, null);
list.remove(0);
causes the same result x=null
assertEquals(null, x)
assertEquals(0, list.size())

I have not found better way how to wrap single value as List. In all cases there is some little cosmetic inconsistency, because List has more internal state then single value ...

What do you suggest?

[surli]

What do you suggest?

I was thinking about just calling a check on the get method when you return the AbstractList: you could just check the size of the list and if it's empty you always return an IndexOutOfBoundException.

[pvojtechovsky]

if it's empty you always return an IndexOutOfBoundException.

then this code will throw IndexOutOfBoundException.
list.set(0, null);
list.get(0) -> throws IndexOutOfBoundException.

current implementation passes this:
assertSame(null, list.get(0))

May be I might add an extra internal state into ListWrapper, which would distinguish between
A) list has one null value
B) list has no value
Then the List would behave consistent

[pvojtechovsky]

I am done here.

There are still some "inconsistencies" to standard List behavior, but they were intentional.

add(x) into collection which contains x, keeps size on 1 - so it behaves like a Set
add(null) into collection which contains x, keeps size on 1 - instead of throwing "Too many elements. Max size is 1"
add(null) into empty collection keeps value on null, but sets size on 1

WDYT? Should we make it List compatible or say it is intentional?

[monperrus]

I like the Least Astonishment Principle, so I would go for being List-compatible.

[pvojtechovsky]

It should be list compatible now

[surli]

Thanks @pvojtechovsky for this refactoring :)