DeclarativeHub / Bond

A Swift binding framework
MIT License
4.23k stars 361 forks source link

Possible issue with NSTableView binding #653

Open killev opened 5 years ago

killev commented 5 years ago

Working on collections/changesets I found a possible issue:

                case .next(let element):
                    let newCollection = element
                    if let collection = collection {
                        let diff = generateDiff(collection, newCollection)
                        observer.receive(OrderedCollectionChangeset(collection: newCollection, patch: [], diff: diff))
                    } else {
                        observer.receive(OrderedCollectionChangeset(collection: newCollection, patch: []))
                    }
                    collection = newCollection

Line:

observer.receive(OrderedCollectionChangeset(collection: newCollection, patch: [], diff: diff))

I think this is a mistake. You shouldn't pass patch option there. Then this patch will be used here:

 open func apply(changeset: Changeset) {
        guard let tableView = tableView else { return }
        let patch = changeset.patch
        if patch.isEmpty {
            collection = clone(changeset.collection)
            tableView.reloadData()
        } else {
            tableView.beginUpdates()
            patch.forEach(apply)
            tableView.endUpdates()
        }
    }

And as far as it's empty it always reloadData.

srdanrasic commented 5 years ago

Good catch 👍

killev commented 5 years ago

Fixed in https://github.com/DeclarativeHub/Bond/pull/654

killev commented 5 years ago

How about removing the constructor which accepts both arguments:

    public init(collection: Collection, patch: [Operation], diff: Diff) {
        self.collection = collection
        self.precalculatedPatch = patch
        self.precalculatedDiff = diff
    }