-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve using elements in lists #412
Open
kyleve
wants to merge
40
commits into
main
Choose a base branch
from
kve/element-usage-improvements
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 7 commits
Commits
Show all changes
40 commits
Select commit
Hold shift + click to select a range
05cbbfe
Move isEquivalent to its own protocol.
kyleve ca36edc
Update APIs used to add elements to lists
kyleve 0937a2e
Update single builder
kyleve fa730f9
Remove unneeded overloads
kyleve f90444d
Remove no longer needed type erased conversion methods
kyleve c8f0acc
Ensure we properly respect Equatable or IsEquivalentContent.
kyleve bcc1074
Also fix header/footer Equatable and IsEquivalent resolution.
kyleve 732e652
Add ListElementNonConvertible
kyleve 89c4d7d
Update section additions
kyleve 1dcaa3d
Additional self review and fixes
kyleve 966d397
Update CHANGELOG
kyleve 1b7d84a
Rename: .item() to .listItem()
kyleve 5b167f1
Implement automatic isEquivalent checking for Elements
kyleve 057e1c9
Docs, add default IsEquivalentContent implementation.
kyleve b60f091
Rename to areEquatablePropertiesEqual
kyleve f39e3b4
Update comments and default implementations
kyleve 2d87e95
Comment cleanup
kyleve d15b036
Remove Property Wrapper
kyleve 090e38a
After chatting with Tim, update how we traverse properties.
kyleve 9778469
Rename IsEquivalentContent, add some more tests.
kyleve 0260289
Peformance: Remove some casting checks
kyleve d166aa6
Found a Mirror bug, add a test for it
kyleve 6467a3e
Add Swift 5.7 support
kyleve bd6cc74
Improve error messages
kyleve 01675e4
Address self review
kyleve 2c9da83
fix
kyleve 4294b9b
Add more specific error
kyleve 8fbfa30
Update how we handle backgrounds for elements
kyleve 9ac3840
Update tests to compare equatability performance
kyleve d65a3d4
Code review
kyleve 964a924
Follow up: Remove automatic equality, replace with easier method to d…
kyleve 938ef81
Additional renames
kyleve 58e8a6f
Improve error messages
kyleve 5c9de6d
Remove applies overrides
kyleve 59bce3b
Update demos to use key path equivalency
kyleve 40179f7
Self review and cleanup
kyleve e3c5ce4
KeyPathEquivalent updates
kyleve d00c534
Split test case into two
kyleve dadf9dd
Merge remote-tracking branch 'origin/main' into kve/element-usage-imp…
kyleve ef78668
Merge remote-tracking branch 'origin/main' into kve/element-usage-imp…
kyleve File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,106 @@ | ||
// | ||
// Element+HeaderFooter.swift | ||
// BlueprintUILists | ||
// | ||
// Created by Kyle Van Essen on 7/24/22. | ||
// | ||
|
||
import BlueprintUI | ||
import ListableUI | ||
|
||
|
||
// MARK: HeaderFooter / HeaderFooterContent Extensions | ||
|
||
|
||
extension Element { | ||
|
||
/// Converts the given `Element` into a Listable `HeaderFooter`. You many also optionally | ||
/// configure the header / footer, setting its values such as the `onTap` callbacks, etc. | ||
/// | ||
/// ```swift | ||
/// MyElement(...) | ||
/// .headerFooter { header in | ||
/// header.onTap = { ... } | ||
/// } | ||
/// ``` | ||
/// | ||
/// ## ⚠️ Performance Considerations | ||
/// Unless your `Element` conforms to `Equatable` or `IsEquivalentContent`, | ||
/// it will return `false` for `isEquivalent` for each content update, which can dramatically | ||
/// hurt performance for longer lists (eg, more than 20 items): it will be re-measured for each content update. | ||
/// | ||
/// It is encouraged for these longer lists, you ensure your `Element` conforms to one of these protocols. | ||
public func headerFooter( | ||
configure : (inout HeaderFooter<WrappedHeaderFooterContent<Self>>) -> () = { _ in } | ||
) -> HeaderFooter<WrappedHeaderFooterContent<Self>> { | ||
HeaderFooter( | ||
WrappedHeaderFooterContent(represented: self), | ||
configure: configure | ||
) | ||
} | ||
} | ||
|
||
|
||
extension Element where Self:Equatable { | ||
|
||
public func headerFooter( | ||
configure : (inout HeaderFooter<WrappedHeaderFooterContent<Self>>) -> () = { _ in } | ||
) -> HeaderFooter<WrappedHeaderFooterContent<Self>> { | ||
HeaderFooter( | ||
WrappedHeaderFooterContent(represented: self), | ||
configure: configure | ||
) | ||
} | ||
} | ||
|
||
|
||
extension Element where Self:IsEquivalentContent { | ||
|
||
public func headerFooter( | ||
configure : (inout HeaderFooter<WrappedHeaderFooterContent<Self>>) -> () = { _ in } | ||
) -> HeaderFooter<WrappedHeaderFooterContent<Self>> { | ||
HeaderFooter( | ||
WrappedHeaderFooterContent(represented: self), | ||
configure: configure | ||
) | ||
} | ||
} | ||
|
||
|
||
public struct WrappedHeaderFooterContent<ElementType:Element> : BlueprintHeaderFooterContent | ||
{ | ||
public let represented : ElementType | ||
|
||
private let isEquivalent : (Self, Self) -> Bool | ||
|
||
init(represented : ElementType) { | ||
self.represented = represented | ||
|
||
self.isEquivalent = { _, _ in false } | ||
} | ||
|
||
init(represented : ElementType) where ElementType:Equatable { | ||
self.represented = represented | ||
|
||
self.isEquivalent = { | ||
$0.represented == $1.represented | ||
} | ||
} | ||
|
||
init(represented : ElementType) where ElementType:IsEquivalentContent { | ||
self.represented = represented | ||
|
||
self.isEquivalent = { | ||
$0.represented.isEquivalent(to: $1.represented) | ||
} | ||
} | ||
|
||
public func isEquivalent(to other: Self) -> Bool { | ||
isEquivalent(self, other) | ||
} | ||
|
||
public var elementRepresentation: Element { | ||
represented | ||
} | ||
} | ||
|
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,134 @@ | ||
// | ||
// Element+Item.swift | ||
// BlueprintUILists | ||
// | ||
// Created by Kyle Van Essen on 7/24/22. | ||
// | ||
|
||
import BlueprintUI | ||
import ListableUI | ||
|
||
|
||
// MARK: Item / ItemContent Extensions | ||
|
||
extension Element { | ||
|
||
/// Converts the given `Element` into a Listable `Item` with the provided ID. You can use this ID | ||
/// to scroll to or later access the item through the regular list access APIs. | ||
/// You many also optionally configure the item, setting its values such as the `onDisplay` callbacks, etc. | ||
kyleve marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// | ||
/// ```swift | ||
/// MyElement(...) | ||
/// .item(id: "my-provided-id") { item in | ||
/// item.insertAndRemoveAnimations = .scaleUp | ||
/// } | ||
/// ``` | ||
/// | ||
/// ## ⚠️ Performance Considerations | ||
/// Unless your `Element` conforms to `Equatable` or `IsEquivalentContent`, | ||
/// it will return `false` for `isEquivalent` for each content update, which can dramatically | ||
/// hurt performance for longer lists (eg, more than 20 items): it will be re-measured for each content update. | ||
/// | ||
/// It is encouraged for these longer lists, you ensure your `Element` conforms to one of these protocols. | ||
public func item( | ||
kyleve marked this conversation as resolved.
Show resolved
Hide resolved
|
||
id : AnyHashable = ObjectIdentifier(Self.Type.self), | ||
kyleve marked this conversation as resolved.
Show resolved
Hide resolved
|
||
configure : (inout Item<WrappedElementContent<Self>>) -> () = { _ in } | ||
) -> Item<WrappedElementContent<Self>> { | ||
Item( | ||
WrappedElementContent( | ||
identifierValue: id, | ||
represented: self | ||
), | ||
configure: configure | ||
) | ||
} | ||
} | ||
|
||
extension Element where Self:Equatable { | ||
|
||
public func item( | ||
id : AnyHashable = ObjectIdentifier(Self.Type.self), | ||
configure : (inout Item<WrappedElementContent<Self>>) -> () = { _ in } | ||
) -> Item<WrappedElementContent<Self>> { | ||
Item( | ||
WrappedElementContent( | ||
identifierValue: id, | ||
represented: self | ||
), | ||
configure: configure | ||
) | ||
} | ||
} | ||
|
||
|
||
extension Element where Self:IsEquivalentContent { | ||
|
||
public func item( | ||
id : AnyHashable = ObjectIdentifier(Self.Type.self), | ||
configure : (inout Item<WrappedElementContent<Self>>) -> () = { _ in } | ||
) -> Item<WrappedElementContent<Self>> { | ||
Item( | ||
WrappedElementContent( | ||
identifierValue: id, | ||
represented: self | ||
), | ||
configure: configure | ||
) | ||
} | ||
} | ||
|
||
|
||
public struct WrappedElementContent<ElementType:Element> : BlueprintItemContent | ||
{ | ||
public let identifierValue: AnyHashable | ||
|
||
public let represented : ElementType | ||
|
||
private let isEquivalent : (Self, Self) -> Bool | ||
|
||
init( | ||
identifierValue: AnyHashable, | ||
represented: ElementType | ||
) { | ||
self.represented = represented | ||
self.identifierValue = identifierValue | ||
|
||
self.isEquivalent = { _, _ in false } | ||
} | ||
|
||
init( | ||
identifierValue: AnyHashable, | ||
represented: ElementType | ||
) where ElementType:Equatable { | ||
self.represented = represented | ||
self.identifierValue = identifierValue | ||
|
||
self.isEquivalent = { | ||
$0.represented == $1.represented | ||
} | ||
} | ||
|
||
init( | ||
identifierValue: AnyHashable, | ||
represented: ElementType | ||
) where ElementType:IsEquivalentContent { | ||
self.represented = represented | ||
self.identifierValue = identifierValue | ||
|
||
self.isEquivalent = { | ||
$0.represented.isEquivalent(to: $1.represented) | ||
} | ||
} | ||
|
||
public func isEquivalent(to other: Self) -> Bool { | ||
isEquivalent(self, other) | ||
} | ||
|
||
public func element(with info: ApplyItemContentInfo) -> Element { | ||
represented | ||
} | ||
|
||
public var reappliesToVisibleView: ReappliesToVisibleView { | ||
kyleve marked this conversation as resolved.
Show resolved
Hide resolved
|
||
.ifNotEquivalent | ||
} | ||
} |
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
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
// | ||
// ListableBuilder+Element.swift | ||
// BlueprintUILists | ||
// | ||
// Created by Kyle Van Essen on 7/24/22. | ||
// | ||
|
||
import BlueprintUI | ||
import ListableUI | ||
|
||
|
||
/// Adds `Element` support when building `AnyItemConvertible` arrays, which allows: | ||
/// | ||
/// ```swift | ||
/// Section("3") { section in | ||
/// TestContent1() // An ItemContent | ||
/// | ||
/// Element1() // An Element | ||
/// Element2() // An Element | ||
/// } | ||
/// ``` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the summary. |
||
/// | ||
/// ## Note | ||
/// Takes advantage of `@_disfavoredOverload` to avoid ambiguous method resolution with the default implementations. | ||
kyleve marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// See more here: https://github.com/apple/swift/blob/main/docs/ReferenceGuides/UnderscoredAttributes.md#_disfavoredoverload | ||
/// | ||
public extension ListableBuilder where ContentType == AnyItemConvertible { | ||
|
||
static func buildExpression<ElementType:Element>(_ expression: ElementType) -> Component { | ||
[expression.item()] | ||
} | ||
|
||
static func buildExpression<ElementType:Element>(_ expression: ElementType) -> Component where ElementType:Equatable { | ||
[expression.item()] | ||
} | ||
|
||
static func buildExpression<ElementType:Element>(_ expression: ElementType) -> Component where ElementType:IsEquivalentContent { | ||
[expression.item()] | ||
} | ||
} | ||
|
||
|
||
public extension ListableOptionalBuilder where ContentType == AnyHeaderFooterConvertible { | ||
|
||
static func buildBlock<ElementType:Element>(_ content: ElementType) -> ContentType { | ||
return content.headerFooter() | ||
} | ||
|
||
static func buildBlock<ElementType:Element>(_ content: ElementType) -> ContentType where ElementType:Equatable { | ||
return content.headerFooter() | ||
} | ||
|
||
static func buildBlock<ElementType:Element>(_ content: ElementType) -> ContentType where ElementType:IsEquivalentContent { | ||
return content.headerFooter() | ||
} | ||
} |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kylebshr Creating a thread to reply to stuff, so we can talk in a thread vs top level comments:
Hard to say – there's a bunch of existing
ElementItem
usages in POS now, and they're all pretty much wrong – the API was hard to use, so there's a lot of this:But in this case the underlying value is
"formFields-element"
, aka a static string; soisEquivalent
always returns true, and the item will never re-size even when it should – which is already a foot gun and breaks things in weird ways.Identifiers? No – the list smart enough to do a "best attempt" at creating stable identifiers when there's duplicate IDs (and identifiers are already salted with the
ItemContent
type), so even without explicitly provided IDs, you'll get stable IDs across updates. Eg, if this is your list, these will be the identifiers:The main benefit to providing IDs is during mutative diffs, the list can more intelligently manage the changes.
Same thing as you'd do before with
ItemContent
/BlueprintItemContent
; you need to manually implementEquatable
orisEquivalent
, and compare the equatable parameters that you can. There are some clever things we can do here to avoid needing this, but I didn't explore it: We can usemirror
to find equatable parameters and compare them