Copy method calls cleanup & IGListIndexSetResult hasChanges set to read only #403
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks! If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions. |
|
@AdamRobertsEF updated the pull request - view changes |
AdamRobertsEF
changed the title
There was a problem hiding this comment.
There are some examples in IGListAdapter that I linked to in #384 that we should also hit.
Some of these we actually need to keep. Anything coming from a public API should be copied so the consumer doesn't accidentally break something.
| @@ -47,7 +47,7 @@ + (void)cleanIndexPathsWithMap:(const std::unordered_map<NSUInteger, IGListMoveI | |||
| indexPaths:(NSMutableSet<NSIndexPath *> *)indexPaths | |||
| deletes:(NSMutableIndexSet *)deletes | |||
| inserts:(NSMutableIndexSet *)inserts { | |||
| for (NSIndexPath *path in [indexPaths copy]) { | |||
There was a problem hiding this comment.
This one has to stay b/c otherwise we're mutating the array while enumerating
There was a problem hiding this comment.
I made a similar change to another indexSet method and undo it, missed this one.
| _moveSections = mMoveSections; | ||
| _deleteIndexPaths = mDeleteIndexPaths; | ||
| _insertIndexPaths = mInsertIndexPaths; | ||
| _reloadIndexPaths = mReloadIndexPaths; |
| _updates = updates; | ||
| _moves = moves; | ||
| _oldIndexPathMap = oldIndexPathMap; | ||
| _newIndexPathMap = newIndexPathMap; |
There was a problem hiding this comment.
Normally I'd say this is good practice, but since this is part of the private API we're fine I think
|
|
||
| /** | ||
| Returns whether the result has any changes or not. | ||
|
|
||
| @return `YES` if the result has changes, `NO` otherwise. | ||
| */ | ||
| - (BOOL)hasChanges; | ||
| @property (nonatomic, assign, readonly) BOOL hasChanges; |
There was a problem hiding this comment.
This is a breaking change. We should probably revert this and handle #402 as a separate PR. Will require a changelog entry, etc.
| _configureBlock = [configureBlock copy]; | ||
| _sizeBlock = [sizeBlock copy]; | ||
| _configureBlock = configureBlock; | ||
| _sizeBlock = sizeBlock; |
There was a problem hiding this comment.
nit: these get copied anyways
There was a problem hiding this comment.
I removed the copy, then reverted my changes.
Same for the one below.
| _configureBlock = [configureBlock copy]; | ||
| _sizeBlock = [sizeBlock copy]; | ||
| _configureBlock = configureBlock; | ||
| _sizeBlock = sizeBlock; |
| @@ -63,9 +63,9 @@ - (instancetype)initWithStoryboardCellIdentifier:(NSString *)identifier | |||
| IGParameterAssert(configureBlock != nil); | |||
| IGParameterAssert(sizeBlock != nil); | |||
| if (self = [super init]) { | |||
| _identifier = [identifier copy]; | |||
There was a problem hiding this comment.
This needs to stay b/c it is coming from consumers of the API. They could pass in an NSMutableString and change it later, breaking things.
| @@ -185,7 +185,7 @@ - (UICollectionViewCell *)cellForItemAtIndex:(NSInteger)index sectionController: | |||
| [cells addObject:cell]; | |||
| } | |||
| } | |||
| return [cells copy]; | |||
| return cells; | |||
| @@ -55,7 +55,7 @@ - (NSInteger)sectionForSectionController:(IGListSectionController <IGListSection | |||
| - (void)updateWithObjects:(NSArray *)objects sectionControllers:(NSArray *)sectionControllers { | |||
| IGParameterAssert(objects.count == sectionControllers.count); | |||
|
|
|||
| self.objects = [objects copy]; | |||
| self.objects = objects; | |||
There was a problem hiding this comment.
Since this is a private class it'll be fine
| @@ -108,7 +108,7 @@ - (void)updateObject:(id)object { | |||
|
|
|||
| NSMutableArray *mobjects = [self.objects mutableCopy]; | |||
| mobjects[section] = object; | |||
| self.objects = [mobjects copy]; | |||
| self.objects = mobjects; | |||
There was a problem hiding this comment.
Also ok b/c its private. 👍
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
@AdamRobertsEF updated the pull request - view changes |
|
@AdamRobertsEF updated the pull request - view changes |
|
@AdamRobertsEF updated the pull request - view changes |
|
@AdamRobertsEF are you splitting this into different PRs? Did you mean to close this one? |
|
I'm having fun with git. I submitted the first pull request and the GitHub app put it into the prior pull request, when I submitted the second. I reversed the commit, to put it into a new PR. Then pulled everything from master to get my fork in sync with this one, and it cancelled the PR. Let's close this one and I'll figure out how to do it better on the next pull request. :-) |
|
Quick note, need a new PR for this:
|
|
I'll create a new PR when the my last PR has been accepted. The GitHub app doesn't handle multiple PRs at the same time. |
|
@AdamRobertsEF which PR is that? You don't have any open atm. |
Removed copy method calls from objects and properties that aren’t weak references or blocks.