MGLShapeCollection.shapes should be writable

[1ec5]

MGLShapeCollection’s shapes property should be a read-write mutable array, so that the developer can (for example) append a shape to the collection directly rather than having to create a new MGLShapeCollection to change the shapes within an MGLShapeSource.

The implementation would be similar in scope to #6160, although without any C++ wrangling.

/cc @bsudekum

[1ec5]

Making the array mutable would run into the same problem as #7459. But we can still make the property read-write without running into that problem.

[andrewstay]

@1ec5 It is very risky and considered as a bad design to provide mutable containers in the interface. Please consider adding following methods instead:

- (void)addShape:
- (void)removeShape:

[1ec5]

Thanks @andrewstay, in that case, we can certainly provide individual getters and setters without making the array mutable. This will likely be the solution to #7459 as well.

[1ec5]

I think we should consider implementing the KVO methods for mutable containers. That way the developer can call the standard -[NSObject mutableArrayValueForKey:] then efficiently add and remove from that NSMutableArray without having to then set the shapes property at the end. We follow this pattern in MGLStyle.

[andrewgubanov]

@1ec5 The solution suggested by you looks like encapsulation violation as it exposes internal logic to the client.

[1ec5]

Would it be? I thought -mutableArrayValueForKey: etc. are intended to be used that way; implementing KVO methods like -insertObject:inShapesAtIndex: only improves their performance, but the developer doesn’t need to know about the KVO methods. To be clear, shapes would not be declared as mutable.

[andrewgubanov]

To make things cleaner:

1. -mutableArrayValueForKey: is a part of the KVC, not KVO
2. insertObject:inShapesAtIndex: is not a KVO
3. KVC and KVO were intended to be used in unittests initially, but somehow migrated to the production code.

When user of your objects access private property with the knowledge that it is a mutable container and can modify it using KVC it is a direct violation of encapsulation and should be avoided as much as possible.

[1ec5]

Sorry for being thick: I was conflating KVC and KVO. In fact, I’m also overthinking things: the collection methods needed for -mutableArrayValueForKey: (if any) would be autosynthesized anyhow, since shapes itself is autosynthesized. If methods like -appendShape: and -removeShapeAtIndex: would be more discoverable and convenient than -mutableArrayValueForKey:, we can certainly do that – and it’d be consistent with MGLMultiPoint to boot.

[stale]

This issue has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.