Make sure collection have unique ListDiffable objects on initial data… #993

yemodin · 2017-10-29T07:39:47Z

… source setup too

Changes in this pull request

Issue fixed: #815

Checklist

All tests pass. Demo project builds and runs.
I added tests, an experiment, or detailed why my change isn't tested.
I added an entry to the CHANGELOG.md for any breaking changes, enhancements, or bug fixes.
I have reviewed the contributing guide

… source setup too

facebook-github-bot · 2017-10-29T07:39:52Z

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. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

yemodin · 2017-10-29T07:41:11Z

@rnystrom can we use something like this for #815? Or better handle this at AdapterUpdater?

yemodin · 2017-10-29T09:38:00Z

Not sure about logic behind this test, when started separately looks like it always fails

IGListKit/Tests/IGListDebuggerTests.m

Line 48 in a6f264f

XCTAssertEqual(descriptions.count, 4);

facebook-github-bot · 2017-10-29T09:45:48Z

@yemodin has updated the pull request. View: changes

facebook-github-bot · 2017-10-29T18:29:34Z

@yemodin has updated the pull request. View: changes

Actually throw on removed test "test_whenPassingNonUniqueIdentifiers_adapterShouldAssert" make extra one description hanging around

facebook-github-bot · 2017-10-29T18:48:02Z

@yemodin has updated the pull request. View: changes

rnystrom · 2017-11-03T22:02:57Z

Source/IGListAdapter.m

    }
 #endif

+    NSArray *uniqueObjects = objectsWithDuplicateIdentifiersRemoved(objects);


While not opposed to this change, this does make deduping in the updater redundant.

Maybe instead the deduping should be done here:

https://github.com/Instagram/IGListKit/blob/master/Source/IGListAdapter.m#L146

@rnystrom If I dedupe at https://github.com/Instagram/IGListKit/blob/master/Source/IGListAdapter.m#L146 we still have the same issue reappear on reloadDataWithCompletion for adapter:

IGListKit/Source/IGListAdapter.m

Line 368 in b3ffafc

[weakSelf updateObjects:newItems dataSource:dataSource];

Oooooooh, now I see it! Wo this is an awesome find @yemodin! How about we:

Move the dedupe to public settings change and reloadData

We make a followup issue to consolidate and dedupe in one place

I actually want to move object generation entirely so that "to" object arrays are only constructed once. Atm they are built w/ every call to performUpdatesAnimated:completion:, even if a bunch of calls are updated into a single batch.

Done. Also made deduping before performUpdatesAnimated

IGListKit/Source/IGListAdapter.m

Line 322 in b3ffafc

NSArray *newObjects = [dataSource objectsForListAdapter:self];

And test for such case.

Added test for updates

facebook-github-bot · 2017-11-06T19:04:53Z

@yemodin has updated the pull request. View: changes

facebook-github-bot · 2017-11-06T19:13:53Z

@yemodin has updated the pull request. View: changes

rnystrom · 2017-11-07T21:20:20Z

@yemodin would love to import this, but we still need you to sign the CLA, then we're good to go! https://code.facebook.com/cla

facebook-github-bot · 2017-11-07T21:28:48Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

rnystrom · 2017-11-07T22:14:24Z

Source/IGListAdapter.m

@@ -319,14 +321,14 @@ - (void)performUpdatesAnimated:(BOOL)animated completion:(IGListUpdaterCompletio
    }

    NSArray *fromObjects = self.sectionMap.objects;
-    NSArray *newObjects = [dataSource objectsForListAdapter:self];
+    NSArray *uniqueObjects = objectsWithDuplicateIdentifiersRemoved([dataSource objectsForListAdapter:self]);


If we're going to put this here (which we don't need to yet), we should remove it from here since that'll be wasteful. I lean towards not messing w/ it here since it'll be extra work (dedupe every update call instead of at coalescence).

I'm actually going to patch that change then import this!

facebook-github-bot · 2017-11-07T22:17:36Z

@yemodin has updated the pull request. View: changes

facebook-github-bot

@rnystrom has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

rnystrom · 2017-11-08T16:03:11Z

Internal CI is reporting a test failure for -[IGListAdapterTests test_whenPassingNonUniqueIdentifiers_adapterUpdatesShouldSkipDuplicates]. I'll need to figure out what's going on there before we can land.

yemodin · 2017-11-08T16:16:49Z

@rnystrom It fails because "revert dedupe in performUpdatesAnimated". If only set objects twice and perform update on adapter it will pass duplicates too

drinkius · 2017-11-09T11:36:19Z

Is this pull request going to be merged to master? Currently in our project we face an issue with the Identifiers must be unique! assert, in our setup having several object with the same IDs is expected and having this Assert doesn't bring much value

yemodin · 2017-11-09T12:25:06Z

@rnystrom Should I return dedupe to performUpdatesAnimated? Let me know if we have better options.

rnystrom · 2017-11-09T14:08:28Z

@drinkius it'll be in master momentarily. Bear in mind that this will assert more if your objects aren't unique. The assert isn't going away. In fact, your objects w/ duped IDs are being stripped.

@yemodin no worries! Patching internally then landing 😄

drinkius · 2017-11-09T14:22:14Z

@rnystrom From what I see in the current implementation in @yemodin 's repo it just strips the duplicates (I indeed need this behaviour) but you can continue working well with the stripped list, is it going to change?

rnystrom · 2017-11-09T14:41:28Z

Nope, this PR adds deduping to a few places where it was missing and causing inconsistencies. The assert is staying put tho as a warning to the developer that the data you're putting into IGLK isn't what you're going to see.

rnystrom · 2017-11-17T20:24:04Z

Need to unland and re-evaluate, this caused some regressions internally

rnystrom · 2017-11-20T15:24:26Z

False alarm, something else started crashing in a similar spot at almost the exact same time. Will be reverting my revert this week 😅

Summary: … source setup too Issue fixed: #815 - [x] All tests pass. Demo project builds and runs. - [x] I added tests, an experiment, or detailed why my change isn't tested. - [x] I added an entry to the `CHANGELOG.md` for any breaking changes, enhancements, or bug fixes. - [x] I have reviewed the [contributing guide](https://github.com/Instagram/IGListKit/blob/master/.github/CONTRIBUTING.md) Closes #993 Reviewed By: manicakes Differential Revision: D6388270 Pulled By: rnystrom fbshipit-source-id: e5e7e047bad5f21b81b562ebd586f7f5036325ff

jessesquires · 2017-11-24T02:19:10Z

@rnystrom -- re-opened this so we don't loose track. close once you successfully un-revert? 😄

rnystrom · 2017-11-27T17:36:21Z

Added in c7d4dc2

Make sure collection have unique ListDiffable objects on initial data…

0a1472f

… source setup too

add test for setup with non unique identifiers

d03ac34

Update changelog with fix info

d3aa167

Fix test for debugger

e26f1cb

Actually throw on removed test "test_whenPassingNonUniqueIdentifiers_adapterShouldAssert" make extra one description hanging around

rnystrom suggested changes Nov 3, 2017

View reviewed changes

Deduping before updateObjects

2316281

Added test for updates

rnystrom added this to the 3.2.0 milestone Nov 6, 2017

Cleanup test for deduping on reload adapter

a02f5a8

facebook-github-bot added the CLA Signed label Nov 7, 2017

rnystrom reviewed Nov 7, 2017

View reviewed changes

revert dedupe in performUpdatesAnimated

7faefe0

facebook-github-bot reviewed Nov 7, 2017

View reviewed changes

facebook-github-bot closed this in 319928f Nov 15, 2017

jessesquires reopened this Nov 24, 2017

rnystrom closed this Nov 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure collection have unique ListDiffable objects on initial data… #993

Make sure collection have unique ListDiffable objects on initial data… #993

yemodin commented Oct 29, 2017 •

edited

Loading

facebook-github-bot commented Oct 29, 2017

yemodin commented Oct 29, 2017

yemodin commented Oct 29, 2017 •

edited

Loading

facebook-github-bot commented Oct 29, 2017

facebook-github-bot commented Oct 29, 2017

facebook-github-bot commented Oct 29, 2017

rnystrom Nov 3, 2017

yemodin Nov 6, 2017

rnystrom Nov 6, 2017

yemodin Nov 6, 2017 •

edited

Loading

facebook-github-bot commented Nov 6, 2017

facebook-github-bot commented Nov 6, 2017

rnystrom commented Nov 7, 2017

facebook-github-bot commented Nov 7, 2017

rnystrom Nov 7, 2017

facebook-github-bot commented Nov 7, 2017

facebook-github-bot left a comment

rnystrom commented Nov 8, 2017

yemodin commented Nov 8, 2017

drinkius commented Nov 9, 2017

yemodin commented Nov 9, 2017

rnystrom commented Nov 9, 2017

drinkius commented Nov 9, 2017

rnystrom commented Nov 9, 2017

rnystrom commented Nov 17, 2017

rnystrom commented Nov 20, 2017

jessesquires commented Nov 24, 2017

rnystrom commented Nov 27, 2017

Make sure collection have unique ListDiffable objects on initial data… #993

Make sure collection have unique ListDiffable objects on initial data… #993

Conversation

yemodin commented Oct 29, 2017 • edited Loading

Changes in this pull request

Checklist

facebook-github-bot commented Oct 29, 2017

yemodin commented Oct 29, 2017

yemodin commented Oct 29, 2017 • edited Loading

facebook-github-bot commented Oct 29, 2017

facebook-github-bot commented Oct 29, 2017

facebook-github-bot commented Oct 29, 2017

rnystrom Nov 3, 2017

Choose a reason for hiding this comment

yemodin Nov 6, 2017

Choose a reason for hiding this comment

rnystrom Nov 6, 2017

Choose a reason for hiding this comment

yemodin Nov 6, 2017 • edited Loading

Choose a reason for hiding this comment

facebook-github-bot commented Nov 6, 2017

facebook-github-bot commented Nov 6, 2017

rnystrom commented Nov 7, 2017

facebook-github-bot commented Nov 7, 2017

rnystrom Nov 7, 2017

Choose a reason for hiding this comment

facebook-github-bot commented Nov 7, 2017

facebook-github-bot left a comment

Choose a reason for hiding this comment

rnystrom commented Nov 8, 2017

yemodin commented Nov 8, 2017

drinkius commented Nov 9, 2017

yemodin commented Nov 9, 2017

rnystrom commented Nov 9, 2017

drinkius commented Nov 9, 2017

rnystrom commented Nov 9, 2017

rnystrom commented Nov 17, 2017

rnystrom commented Nov 20, 2017

jessesquires commented Nov 24, 2017

rnystrom commented Nov 27, 2017

yemodin commented Oct 29, 2017 •

edited

Loading

yemodin commented Oct 29, 2017 •

edited

Loading

yemodin Nov 6, 2017 •

edited

Loading