Dwifft -> DifferenceKit #136

benasher44 · 2018-08-20T23:55:20Z

Changes in this pull request

This swaps out Dwifft for DifferenceKit, which uses Paul Heckel's more efficient diffing algo (same one used by IGListKit). Most of the work here is adding layers to avoid Self/associatedtype requirements issues with our protocols. These layers could be removed in the future though, pending changes that would allow this in Swift 5 or generics changes to TableViewModel/CollectionViewModel.

This also fixes #126

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

benasher44 · 2018-08-20T23:56:02Z

Preliminary tests using this with problem projects in PG look really good. Going to hold off putting in more effort here, until we can do more thorough testing internally.

benasher44 · 2018-08-21T05:15:46Z

Sources/Diffing.swift

+/// Creates a type-erased Differentiable for DiffableViewModel.
+/// These are only created internally from either `TableCellViewModel` or `CollectionCellViewModel`,
+/// so that we can safely force cast the models back to those types.
+public struct AnyDiffableViewModel {


Can some of these helper types be made non-public?

Nope. They're used in public methods/vars that are requirements of public protocols.

If this struct is only used internally, can it be internal instead of public?

Maybe I'm misreading your comment?

AnyDiffableViewModel is used in a decl that satisfies a protocol for a public type, so this has to be public.

Ah, got it 👍

jessesquires

Overall looks good! 💯

jessesquires · 2018-08-27T17:27:48Z

Sources/CollectionViewDriver.swift

            }
+            self.refreshViews()


👌 Looks much cleaner

jessesquires · 2018-08-27T17:29:02Z

Sources/Diffing.swift

+/// Creates a type-erased Differentiable for DiffableViewModel.
+/// These are only created internally from either `TableCellViewModel` or `CollectionCellViewModel`,
+/// so that we can safely force cast the models back to those types.
+public struct AnyDiffableViewModel {


If this struct is only used internally, can it be internal instead of public?

Maybe I'm misreading your comment?

jessesquires · 2018-08-27T17:35:24Z

Sources/Diffing.swift

+            diffingKey: sectionModel.diffingKey
+        )
+    }
+}


benasher44 · 2018-08-27T17:42:28Z

Sources/Diffing.swift

+
+/// MARK: - DifferenceKit Protocol Conformance
+
+extension TableSectionViewModel: DifferentiableSection {


@jessesquires AnyDiffableViewModel is used to satisfy the DifferentiableSection protocol. Because TableSectionViewModel is public, this protocol implementation must be made public (and cascades back to AnyDiffableViewModel)

Ah, got it 👍

jessesquires · 2018-08-27T18:09:25Z

I'm assuming this fixes #126 ?

benasher44 · 2018-08-28T16:36:29Z

@jessesquires possibly? TBH, I can't tell what should be broken about UICollectionView diffing. AFAICT, the test issue I fixed in #131 was a test-only problem.

benasher44 · 2018-08-28T22:04:41Z

@jessesquires this is ready for final review! I think we're happy with internal testing

cc @sasilukr

benasher44 · 2018-08-28T22:47:26Z

@jessesquires oh wait no this still needs tests and changelog entry. will update again once that's done.

benasher44 · 2018-08-29T17:12:29Z

@jessesquires DifferenceKit properly checks for the presence of the view's window before attempting to perform animated updates, which breaks tests (e.g. means reload data gets called instead of calls to delete sections). Some options:

Delete tests that test the "what is the difference," since we have relegated the responsibility of diffing to a library and therefore falls out of scope of our testing. Instead, we could test "did we attempt to diff as opposed to not diffing".
Figure out how to get a window involved in our tests

Thoughts?

jessesquires · 2018-08-29T19:55:43Z

@benasher44 Either approach sounds reasonable. You can add the table/collection views to a UIWindow in the tests (I'm pretty sure).

But, I'm leaning toward (1) — seems like we shouldn't be writing tests for the actual diffing lib.

benasher44 · 2018-09-04T17:45:17Z

I think best way forward to fixing tests is leveraging protocol-based mocking. I've started with a PR for DifferenceKit: ra1028/DifferenceKit#27

benasher44 · 2018-09-10T00:05:51Z

@jessesquires adding the window, properly overriding performBatchUpdates, and remove some super calls gets tests to pass. This is ready for final approval!

Also, confirmed #126 is fixed.

benasher44 · 2018-09-10T00:33:56Z

Travis appears to be failing because their specs are out of date.

PlanBot-iOS · 2018-09-10T01:14:22Z

	1 Warning
⚠️	Big PR

Generated by 🚫 Danger

benasher44 · 2018-09-10T01:14:54Z

Added pod repo update as a before_install step.

jessesquires · 2018-09-10T17:26:47Z

Sources/Diffing.swift

+        self.model = model
+
+        let differenceIdentifier = model.diffingKey
+        /// Only compares diff identifiers. This means we'll never get "reload"-type `Changeset`s


hm... isn't this a problem? what if a cell updates it's content and needs to be refreshed? (but it's diff identifier wouldn't change...)

We do it manually for the cells on screen in ReactiveLists, which is how it works today (Dwifft doesn't do reloads at all).

We could revisit this in the future to see if this is something we want to support (using diffing to animate reloading of cells), but right now it would break forms (e.g. jumping fields because of keyboard dismissal)

ah, right 👍 sounds good to me

jessesquires

LGTM! 💯 🥇

benasher44 added the in-progress label Aug 20, 2018

benasher44 added this to the 0.2.0 milestone Aug 20, 2018

benasher44 commented Aug 21, 2018

View reviewed changes

benasher44 force-pushed the basher/differencekit branch from da98482 to 6d24e30 Compare August 24, 2018 17:08

jessesquires reviewed Aug 27, 2018

View reviewed changes

benasher44 commented Aug 27, 2018

View reviewed changes

benasher44 force-pushed the basher/add-collection-conformance branch from e69beed to 404dbc3 Compare August 28, 2018 16:39

benasher44 force-pushed the basher/differencekit branch from 6d24e30 to 48423ca Compare August 28, 2018 16:51

Ben Asher added 2 commits August 28, 2018 14:15

Use DifferenceKit

040bfc6

more cleanup

f155143

benasher44 force-pushed the basher/differencekit branch from 48423ca to f155143 Compare August 28, 2018 21:17

benasher44 changed the base branch from basher/add-collection-conformance to master August 28, 2018 21:17

changelog entry

4063c45

benasher44 mentioned this pull request Sep 4, 2018

Add protocols to allow mocking UITableView and UICollectionView ra1028/DifferenceKit#27

Closed

benasher44 mentioned this pull request Sep 5, 2018

Add TableView and CollectionView protocol #139

Closed

5 tasks

Fix tests

5e4ce7d

Upgrade to DifferenceKit 0.5.3

10f7ab1

add pod repo update

aed76d4

benasher44 force-pushed the basher/differencekit branch from 897ba81 to aed76d4 Compare September 10, 2018 01:09

improve window hack

c4cb560

benasher44 force-pushed the basher/differencekit branch from c9d02ae to c4cb560 Compare September 10, 2018 01:21

benasher44 removed the in-progress label Sep 10, 2018

jessesquires reviewed Sep 10, 2018

View reviewed changes

jessesquires approved these changes Sep 10, 2018

View reviewed changes

update changelog for fix

c4d6a22

benasher44 merged commit f3c8d00 into master Sep 11, 2018

benasher44 deleted the basher/differencekit branch September 11, 2018 15:54

ronaldsmartin mentioned this pull request Oct 11, 2018

[Proposal / Investigation] Switch from using Dwiftt to IGListKit/Diffing #8

Closed


		/// MARK: - DifferenceKit Protocol Conformance

		extension TableSectionViewModel: DifferentiableSection {

Dwifft -> DifferenceKit #136

Dwifft -> DifferenceKit #136

Uh oh!

Conversation

benasher44 commented Aug 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes in this pull request

Checklist

Uh oh!

benasher44 commented Aug 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jessesquires left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jessesquires commented Aug 27, 2018

Uh oh!

benasher44 commented Aug 28, 2018

Uh oh!

benasher44 commented Aug 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benasher44 commented Aug 28, 2018

Uh oh!

benasher44 commented Aug 29, 2018

Uh oh!

jessesquires commented Aug 29, 2018

Uh oh!

benasher44 commented Sep 4, 2018

Uh oh!

benasher44 commented Sep 10, 2018

Uh oh!

benasher44 commented Sep 10, 2018

Uh oh!

PlanBot-iOS commented Sep 10, 2018

Uh oh!

benasher44 commented Sep 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benasher44 Sep 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jessesquires left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

benasher44 commented Aug 20, 2018 •

edited

Loading

benasher44 commented Aug 28, 2018 •

edited

Loading

benasher44 commented Sep 10, 2018 •

edited

Loading

benasher44 Sep 10, 2018 •

edited

Loading