Iglistkit: IGListIndexSetResult and IGListIndexPathResult docs say updates use new index

[diogot]

IGListIndexSetResult updates contains index of old collection

New issue checklist

• [X] I have reviewed the README and documentation
• [X] I have searched existing issues and this is not a duplicate

General information

• IGListKit version: 2.1.0
• iOS version(s): 9,10
• CocoaPods/Carthage version: 1.1.1
• Xcode version: 8.2.1
• Devices/Simulators affected: All tested
• Reproducible in the demo project?: Yes

Description

In IGListIndexSetResult documentation can be read:

/**
 The indexes in the new collection that need updated.
 */
@property (nonatomic, strong, readonly) NSIndexSet *updates;

So it's expected that all indexes on updates refer to objects in new collection, although if you have an old collection with two elements and a new collection only with the second element updated we got an index on updates that are out of bounds the new collection. This test will fail on the last assert:

func testDiffingSwiftClassWithDeleteAndUpdates() {
    let o = [SwiftClass(id: 0, value: "a"), SwiftClass(id: 1, value: "b")]
    let n = [SwiftClass(id: 1, value: "c")]
    let result = IGListDiff(o, n, .equality)
    XCTAssertEqual(result.deletes, IndexSet(integer: 0))
    XCTAssertEqual(result.inserts.count, 0)
    XCTAssertEqual(result.moves.count, 0)
    XCTAssertEqual(result.updates, IndexSet(integer: 0))
}

In this case results.updates contains IndexSet(integer: 1), which can only refers to the old collection.

My doubt here is if the issue is on the documentation or this is a bug.

[jessesquires]

Thanks @diogot ! Sounds like a bug. 👍

cc @rnystrom

[jessesquires]

@diogot - Do you want to submit a PR with a fix?

(we should add this test, too 😄 )

[diogot]

The fix was easy than I though, just change this line to:

addIndexToCollection(mUpdates, toSection, i);

Although this expects that the index was from old collection.

So what should I do, change the documentation or change the behavior on all places until the tests are green?

[Adlai-Holler]

Typically reloads are expressed using indexes from before the update – e.g. if you call reloadItems: UICollectionView expects old indexes. Maybe the documentation should be changed? You can always map to the new indexes if needed.

[rnystrom]

@diogot @Adlai-Holler is correct, the documentation (here and here) is out of date, would love a PR to fix that!

The diff result is designed for UICollectionView and UITableView batch updates where the reload index is relative to the _old_ index. Though we don't use reloadItemsAtIndexPaths: explicitly, it's still relevant.

We have this expectation covered in this unit test.

[diogot]

Done in #413