Calculating destination offset from source offset

[ivopintodasilva]

Hello 👋

While using FlexibleDiff I ran into a situation where the generated SectionedChangeset seems to not be correct.

I've added the following test which is failing:

it("should reflect three removed sections") {
    diffTest(
        previous: [
            Section(identifier: "A", items: [Section.Item(identifier: "A0", value: .max)]),
            Section(identifier: "B", items: [Section.Item(identifier: "B0", value: .max)]),
            Section(identifier: "C", items: [Section.Item(identifier: "C0", value: .max)]),
            Section(identifier: "D", items: [Section.Item(identifier: "D0", value: .max)])
        ],
        current: [
            Section(identifier: "C", items: [Section.Item(identifier: "C0", value: .max)])
        ],
        expected: SectionedChangeset(
            sections: Changeset(removals: [0, 1, 3]),
            mutatedSections: []
        )
    )
}

The previous test fails with the following error:

expected to equal <- inserted 0 item(s) at []- deleted 3 item(s) at [0, 1, 3]- mutated 0 item(s) at []- moved 0 item(s) at []- changesets of mutated sections: <<<>>>>, got <- inserted 0 item(s) at []- deleted 3 item(s) at [0, 1, 3]- mutated 1 item(s) at [1]- moved 0 item(s) at []- changesets of mutated sections: <<<section 1 -> 0- inserted 1 item(s) at [0]- deleted 1 item(s) at [0]- mutated 0 item(s) at []- moved 0 item(s) at []>>>>

Which means that the generated SectionedChangeset is something like

SectionedChangeset(
    sections: Changeset(removals: [0, 1, 3], mutations: [1]),
    mutatedSections: [
        SectionedChangeset.MutatedSection(
            source: 1,
            destination: 0,
            changeset: Changeset(inserts: [0], removals: [0])
        )
    ])

This tells us that:

1. Section 1 is being both deleted and mutated at the same time;
2. There are insertions and removals of items when no insertion/removal is performed.

The problem seems to be that the predeletionOffset on the SectionedChangeset's init method is not being calculated correctly for this specific case.

My suggestion in this PR is to start traversing the previous collection of sections and calculate a destinationOffset for that section.
By doing this we are able to know how many deletions and insertions will be performed up until the source index and we can calculate correctly what will be the section's index on the current collection.

Great library 👏 Keep up the good work 😃

[andersio]

Thanks for using FlexibleDiff & raising this. I did run into a similar problem lately, and changing the source of derivation does not seem to be the solution. Apparently, this way of deriving indices is busted by complications of moves, and it is unfortunate that the test coverage is not sufficient in this area.

As an example:

ABC
BDCEFA

Neither the current implementation nor that in this PR would correct the issue when, say, attempting to derive C's index in both direction.

At this point I think a more rigid approach is to give SectionedChangeset access to the internal data structures constructed by the diff algorithm, which gives direct information about the index mapping in both directions. I will try to work something out before 2018 ends. 🤔

[ivopintodasilva]

Right, I didn't take that case into account 😛

I managed to create that test and check why the index derivation was not correct and was able to fix it.

However, probably I'm missing some scenarios and your suggestion of giving access to the diffing algorithm's data structures would be safer.

[andersio]

Hmm I think this is solid enough for the time being. Can't think of other boundary cases just yet.

[andersio]

Could you break the loop when the predicate is not satisfied? There is little value to continue iterating afterwards.

[andersio]

[Nit] I guess one of the tab vs space argument is that Xcode sucks with handling tabs...

[andersio]

Maybe just a counter instead of an IndexSet. We do not care about the actual indices here.

[ivopintodasilva]

Updated PR with code review suggestions and branch update 😃