Develop 0.0.4 swift3 #5
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Was a little murky/magic/weird. Replaced it with 2 (saner) free functions in AssociatedObjects.swift
Converted the Subscribable protocol into a class. Rationale: it was becoming very unsustainable to keep adding type constraints everywhere. Additionally, realistically, we don't gain too much (anything) from it being a protocol.
Added the concept of 'BindingHandlers'. The basic idea is they wrap up
the logic involved in updating the UI with a new value. This allows
for client code to customize exactly how the UI is updated.
Some potential uses for custom written binding handlers could include:
- Animating changes
- Reusable transforms (instead of the current transform block on
bind())
- Formatters (ie, could build a BindingHandler that uses a
NSNumberFormatter to transform an Observable<Int> into a String)
- "Debouncing"/delaying when changes are applied to the UI
- Filtering (ie: "only update the UI when value is ...")
- More complex logic (ie: re-rendering a UIImage when a value
changes)
A default BindingHandler (DefaultBindingHandler) is provided that
simply updates
Additionally, refactor BindableProperty.bind(subscribable:transform:)
to use a custom BindingHandler for doing the transform.
Added `BindingHelpers` struct to namespace some BindingHandler creation functions. These helper functions are used because initializers are somewhat constrained (ie, can't specify a same type requirement for 2 generic type parameters)
Begin adding support for using BindingHandlers for BidirectionalBindableProperty's. Currently just hardcoded to DefaultBindingHandler. NOTE: Some of the logic that bubbles changes up through the BindingHandler to the bound Observable are a little murky, might be worth looking at reworking that logic.
BidirectionalBindableProperty's now work with TransformBindingHandler.
Added checks (and a test) to prevent a BidirectionalBinding from infinite looping in the case it uses a BindingHandler that prevents weird values. (ie a TransformBindingHandler where the `transform` & `reverseTransform` blocks don't actually produce the inverse of the other)
Should be using the `.bind(...)` methods
On BindableProperty & BidirectionalBindableProperty, deprecated the follow overloads of `bind()`: - `bind(_, transform:)` (should use a TransformBindingHandler to transform the value) - `bind(block:)` (should use a Computed) Goal is to converge on a single (or as few as possible) `bind(...)` method (as opposed to having these extra overloads & the custom operators)
Binding a Subscribable<T> to a BindableProperty<T?> is now supported. Fixes GH issue #1
BidirectionalBindableProperty<T?> now supports binding to to Observable<T> (or Subscribable<T>) Related to GH issue #1
Updated the UIKit class extensions so that the BindableProperty's (and BidirectionalBindableProperty's) ValueType has the same nullability as the original UIKit property.
Added a BindableProperty for UIImageView.image Additionally, added support for loading images from URL's via LoadImageBindingHandler.
(to allow it to be subclassed outside of the Fisticuffs modulue)
Essentially a Computed, but allows defining a custom setter
Computed/WritableComputed were (incorrectly) tracking additional dependencies (any dependencies accessed inside by its subscribers)
Fixed bug where UITableView/UICollectionView bindings wouldn't select preselected items (because selectRowAtIndexPath was being called before the view had loaded data, so the selection was being discarded by UIKit)
Computed now coalesces updates. So, if 3 of its dependencies change in a single run loop, it'll only recompute itself once. NOTE: Because it uses subscriptions for tracking dependencies, if it depends on a Computed, it'll take 2 runloops to for a change to bubble up. Will be fixing it so it only takes the regular one run loop.
Because Computed's updates are coalesced now, it could have taken 2+ runloops for a change to propagate up. For example, a change to C would take 2 runloops to propagate up to A: A (computed) <- B (computed) <- C (observable) This commit fixes that.
Previously was calculating twice, when it really didn't need to calculate it the second time.
Updated WritableComputed to have the same behaviour as Computed for coalescing updates
Improved performance of Computed / WritableComputed by reducing the number of times their value is recomputed
WritableComputed wasn't immediately recomputing it's value when .value was accessed. Fixed & added tests to verify.
If a Computed's (or WritableComputed's) value was read in a
subscription block, like:
let a = Observable(5)
let result = Computed { a.value }
result.subscribe { [weak result] in
result?.value
}
it would infinitely recurse. This commit fixes that.
If `value` was accessed after a update was scheduled, any observers were being notified twice for the same change; once when accessing `value`, and once when the update happened next run loop. This commit fixes that
DependenciesCollectionStack now uses a custom NSObject subclass as it's key in NSThread.threadDictionary so that it avoids doing string comparison (improves performance significantly)
Now computes the result immediately, fixes a potential flickering issue with reused table/collection view cells
- Update project to Swift 3 - Run Swift migrator & add manual fixes - Update Quick & Nimble to latest versions
tapi
commented
Dec 5, 2016
| var b_title: BindableProperty<UIButton, String?> { | ||
| return associatedObjectProperty(self, &b_title_key) { _ in | ||
| return BindableProperty(self) { control, value in | ||
| control.setTitle(value, for: UIControlState()) |
There was a problem hiding this comment.
@darrenclark This is what I was talking about with the migrator blowing away the enum.
There was a problem hiding this comment.
Ohhhh jeez, that's sneaky
There was a problem hiding this comment.
I think it only seems to happen for the first enum case
There was a problem hiding this comment.
Ahh right, and since UIControlStateNormal = 0, it should turn out okay
There was a problem hiding this comment.
For sure, its just not at all descriptive. And what is the behaviour if your enum starts in the negatives
There was a problem hiding this comment.
Yea true.
For what it's worth, for OptionSet's like this, it should always default to 0, regardless of if it starts in the negatives or not: https://developer.apple.com/reference/swift/optionset/1641258-init (unless someone overrides init())
There was a problem hiding this comment.
Hadn't realized UIControl was an option set, can think of a single time I've ever used more than one option.
Still don't like empty over .normal but I guess thats just a preference thing
…le delay Reduce tests execution time
Implement ThrottleBindingHandler class.
…compiler crash and warning when using Swift 3.1.
If the UICollectionView hadn't been on screen yet & we attempted to insert or delete cells, UICollectionView would raise an exception. This commit fixes the issue by just doing a regular reloadData() if the collection view hasn't loaded data yet. (see comment in code diff for more details)
…before-initial-data-load Fix potential UICollectionView crash
Wrapping all updates in calls to performUpdate(_:completion:) to work around crash that occurred when inserting/removing items before the collection view had done its initial load of section/item counts. See comments in code for specifics. Additionally am calculating the changes using DataSource's `items` array & the new items from the subscribable. Previously it was using `subscribeArray`, which did a diff of the subscribable's old value & new value. Not sure if it was the source of any bugs, but now by using `DataSource.items` & the new items, we are definitely diffing what the UICollectionView will see in terms of updates.
…erting-items-before-initial-data-load Improve fix for UICollectionView crash
|
lol @tapi @darrenclark can we merge this to master? |
|
Is it even needed anymore? |
|
Yea, lets merge it 👍, we've been using it for a while & it hasn't given us any trouble |
ghost
merged commit
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Just creating a PR to highlight the diff