Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve using elements in lists #412

Open
wants to merge 40 commits into
base: main
Choose a base branch
from
Open

Improve using elements in lists #412

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
05cbbfe
Move isEquivalent to its own protocol.
kyleve Jul 25, 2022
ca36edc
Update APIs used to add elements to lists
kyleve Jul 25, 2022
0937a2e
Update single builder
kyleve Jul 25, 2022
fa730f9
Remove unneeded overloads
kyleve Jul 25, 2022
f90444d
Remove no longer needed type erased conversion methods
kyleve Jul 25, 2022
c8f0acc
Ensure we properly respect Equatable or IsEquivalentContent.
kyleve Jul 26, 2022
bcc1074
Also fix header/footer Equatable and IsEquivalent resolution.
kyleve Jul 26, 2022
732e652
Add ListElementNonConvertible
kyleve Jul 26, 2022
89c4d7d
Update section additions
kyleve Jul 26, 2022
1dcaa3d
Additional self review and fixes
kyleve Jul 26, 2022
966d397
Update CHANGELOG
kyleve Jul 26, 2022
1b7d84a
Rename: .item() to .listItem()
kyleve Jul 27, 2022
5b167f1
Implement automatic isEquivalent checking for Elements
kyleve Jul 28, 2022
057e1c9
Docs, add default IsEquivalentContent implementation.
kyleve Jul 29, 2022
b60f091
Rename to areEquatablePropertiesEqual
kyleve Jul 29, 2022
f39e3b4
Update comments and default implementations
kyleve Jul 29, 2022
2d87e95
Comment cleanup
kyleve Jul 29, 2022
d15b036
Remove Property Wrapper
kyleve Jul 29, 2022
090e38a
After chatting with Tim, update how we traverse properties.
kyleve Jul 29, 2022
9778469
Rename IsEquivalentContent, add some more tests.
kyleve Jul 29, 2022
0260289
Peformance: Remove some casting checks
kyleve Jul 29, 2022
d166aa6
Found a Mirror bug, add a test for it
kyleve Jul 30, 2022
6467a3e
Add Swift 5.7 support
kyleve Jul 30, 2022
bd6cc74
Improve error messages
kyleve Jul 30, 2022
01675e4
Address self review
kyleve Aug 8, 2022
2c9da83
fix
kyleve Aug 8, 2022
4294b9b
Add more specific error
kyleve Aug 8, 2022
8fbfa30
Update how we handle backgrounds for elements
kyleve Aug 8, 2022
9ac3840
Update tests to compare equatability performance
kyleve Aug 11, 2022
d65a3d4
Code review
kyleve Feb 2, 2023
964a924
Follow up: Remove automatic equality, replace with easier method to d…
kyleve May 25, 2023
938ef81
Additional renames
kyleve May 25, 2023
58e8a6f
Improve error messages
kyleve May 26, 2023
5c9de6d
Remove applies overrides
kyleve May 26, 2023
59bce3b
Update demos to use key path equivalency
kyleve May 26, 2023
40179f7
Self review and cleanup
kyleve Jun 11, 2023
e3c5ce4
KeyPathEquivalent updates
kyleve Jun 15, 2023
d00c534
Split test case into two
kyleve Jun 15, 2023
dadf9dd
Merge remote-tracking branch 'origin/main' into kve/element-usage-imp…
kyleve Oct 24, 2023
ef78668
Merge remote-tracking branch 'origin/main' into kve/element-usage-imp…
kyleve Aug 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 126 additions & 0 deletions BlueprintUILists/Sources/Element+HeaderFooter.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
//
// Element+HeaderFooter.swift
// BlueprintUILists
//
// Created by Kyle Van Essen on 7/24/22.
//

import BlueprintUI
import ListableUI


// MARK: HeaderFooter / HeaderFooterContent Extensions


extension Element {

/// Ensures that a well-formed error is presented when a non-Equatable or non-LayoutEquivalent element is provided.
@available(*, unavailable, message: "To be directly added to a List, an Element must conform to Equatable or LayoutEquivalent.")
public func listHeaderFooter(
background : @escaping () -> Element? = { nil },
pressedBackground : @escaping () -> Element? = { nil },
configure : (inout HeaderFooter<WrappedHeaderFooterContent<Self>>) -> () = { _ in }
) -> HeaderFooter<WrappedHeaderFooterContent<Self>> {
fatalError()
}
}

/// Ensures that the `Equatable` initializer for `WrappedHeaderFooterContent` is called.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with this, but it also might be a good place to pump the breaks on overloading and provide an argument label or some such to disambiguate.

This would also solve the potential issue of branching implementations which could in pathological cases cause unpredictable behavior for types which adopt multiple (non-inherited) protocols.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed for the result builders in particular – so it's not that we can provide an overload that's useful – it's that we need to signal to the compiler that this is the method set, etc to use for these passing types

extension Element where Self:Equatable {

public func listHeaderFooter(
background : @escaping () -> Element? = { nil },
pressedBackground : @escaping () -> Element? = { nil },
configure : (inout HeaderFooter<WrappedHeaderFooterContent<Self>>) -> () = { _ in }
) -> HeaderFooter<WrappedHeaderFooterContent<Self>> {
HeaderFooter(
WrappedHeaderFooterContent(
represented: self,
background: background,
pressedBackground: pressedBackground
),
configure: configure
)
}
}


/// Ensures that the `LayoutEquivalent` initializer for `WrappedHeaderFooterContent` is called.
extension Element where Self:LayoutEquivalent {

@_disfavoredOverload
public func listHeaderFooter(
background : @escaping () -> Element? = { nil },
pressedBackground : @escaping () -> Element? = { nil },
configure : (inout HeaderFooter<WrappedHeaderFooterContent<Self>>) -> () = { _ in }
) -> HeaderFooter<WrappedHeaderFooterContent<Self>> {
HeaderFooter(
WrappedHeaderFooterContent(
represented: self,
background: background,
pressedBackground: pressedBackground
),
configure: configure
)
}
}


public struct WrappedHeaderFooterContent<ElementType:Element> : BlueprintHeaderFooterContent
{
public let represented : ElementType

private let isEquivalent : (Self, Self) -> Bool

init(
represented : ElementType,
background : @escaping () -> Element?,
pressedBackground : @escaping () -> Element?
) where ElementType:Equatable
{
self.represented = represented

self.backgroundProvider = background
self.pressedBackgroundProvider = pressedBackground

self.isEquivalent = {
$0.represented == $1.represented
}
}

init(
represented : ElementType,
background : @escaping () -> Element?,
pressedBackground : @escaping () -> Element?
) where ElementType:LayoutEquivalent
{
self.represented = represented

self.backgroundProvider = background
self.pressedBackgroundProvider = pressedBackground

self.isEquivalent = {
$0.represented.isEquivalent(to: $1.represented)
}
}

public func isEquivalent(to other: Self) -> Bool {
isEquivalent(self, other)
}

public var elementRepresentation: Element {
represented
}

var backgroundProvider : () -> Element? = { nil }

public var background: Element? {
backgroundProvider()
}

var pressedBackgroundProvider : () -> Element? = { nil }

public var pressedBackground: Element? {
pressedBackgroundProvider()
}
}
149 changes: 149 additions & 0 deletions BlueprintUILists/Sources/Element+Item.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,149 @@
//
// Element+Item.swift
Copy link
Collaborator Author

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:

a) Is this an even bigger foot gun than what we have now? Many people probably won't make their element equatable or IsEquivalentItem

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:

ElementItem("formFields-element", id: \.self) { _, _ in
   // Return an element
}

ElementItem(transaction.token, id: \.self) { _, _ in ... }

ElementItem("error-state", id: \.self) { _, _ in ... }

ElementItem("business-organization-name-content", id: \.self) { _, _ in ... }

Etc...

But in this case the underlying value is "formFields-element", aka a static string; so isEquivalent 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.

b) Is there a performance concern from losing identifiers (most people won't add one)

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:

MyElement1() -> Identifier(MyElement1, 1)
MyElement2() -> Identifier(MyElement2, 1)
MyElement2() -> Identifier(MyElement2, 2)
MyElement1() -> Identifier(MyElement1, 2)

The main benefit to providing IDs is during mutative diffs, the list can more intelligently manage the changes.

c) Most elements have closures, which make equatability impossible. What's the correct thing to do there. Should there be more helpers for interacting with the row tap handlers, so that the element doesn't have to be re-applied for the closure to be correct?

Same thing as you'd do before with ItemContent / BlueprintItemContent; you need to manually implement Equatable or isEquivalent, 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 use mirror to find equatable parameters and compare them

// BlueprintUILists
//
// Created by Kyle Van Essen on 7/24/22.
//

import BlueprintUI
import ListableUI


// MARK: Item / ItemContent Extensions


extension Element {

/// Ensures that a well-formed error is presented when a non-Equatable or non-LayoutEquivalent element is provided.
@available(*, unavailable, message: "To be directly added to a List, an Element must conform to Equatable or LayoutEquivalent.")
public func listItem(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the right direction, and only question if we should require the presence of an id.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I subsequently understood that this was meant to improve the builder experience. I have mixed feelings about that but do think it matches the standard folks have come to expect from SwiftUI.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My 2c / thoughts are that, since you only really need to provide an ID as an optimization (eg if your list is changing a lot, or you're reading data back out – eg for reordering), the framework can already make a pretty good guess what you meant, so requiring it isn't super useful

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(And indeed, people get the ID wrong pretty often – eg just passing a UUID() every time – which is very destabilizing / a perf issue)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey its been over a year, I am going back on this and requiring the ID! If we see it really getting abused again, we can remove this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, hmm, this does make quite a few places less ergonomic. I'm going to ruminate on this one a bit more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I went back on this again. Not doing an ID.

id : AnyHashable? = nil,
selection: ItemSelectionStyle = .notSelectable,
background : @escaping (ApplyItemContentInfo) -> Element? = { _ in nil },
selectedBackground : @escaping (ApplyItemContentInfo) -> Element? = { _ in nil },
configure : (inout Item<WrappedElementContent<Self>>) -> () = { _ in }
) -> Item<WrappedElementContent<Self>> {
fatalError()
}
}


/// Ensures that the `Equatable` initializer for `WrappedElementContent` is called.
extension Element where Self:Equatable {

public func listItem(
id : AnyHashable? = nil,
selection: ItemSelectionStyle = .notSelectable,
background : @escaping (ApplyItemContentInfo) -> Element? = { _ in nil },
selectedBackground : @escaping (ApplyItemContentInfo) -> Element? = { _ in nil },
configure : (inout Item<WrappedElementContent<Self>>) -> () = { _ in }
) -> Item<WrappedElementContent<Self>> {
Item(
WrappedElementContent(
identifierValue: id,
represented: self,
background: background,
selectedBackground: selectedBackground
),
configure: {
$0.selectionStyle = selection

configure(&$0)
}
)
}
}


/// Ensures that the `LayoutEquivalent` initializer for `WrappedElementContent` is called.
extension Element where Self:LayoutEquivalent {

@_disfavoredOverload
public func listItem(
id : AnyHashable? = nil,
selection: ItemSelectionStyle = .notSelectable,
background : @escaping (ApplyItemContentInfo) -> Element? = { _ in nil },
selectedBackground : @escaping (ApplyItemContentInfo) -> Element? = { _ in nil },
configure : (inout Item<WrappedElementContent<Self>>) -> () = { _ in }
) -> Item<WrappedElementContent<Self>> {
Item(
WrappedElementContent(
identifierValue: id,
represented: self,
background: background,
selectedBackground: selectedBackground
),
configure: {
$0.selectionStyle = selection

configure(&$0)
}
)
}
}


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,
background : @escaping (ApplyItemContentInfo) -> Element?,
selectedBackground : @escaping (ApplyItemContentInfo) -> Element?
) where ElementType:Equatable
{
self.represented = represented
self.identifierValue = identifierValue

self.backgroundProvider = background
self.selectedBackgroundProvider = selectedBackground

self.isEquivalent = {
$0.represented == $1.represented
}
}

init(
identifierValue: AnyHashable?,
represented: ElementType,
background : @escaping (ApplyItemContentInfo) -> Element?,
selectedBackground : @escaping (ApplyItemContentInfo) -> Element?
) where ElementType:LayoutEquivalent
{
self.represented = represented
self.identifierValue = identifierValue

self.backgroundProvider = background
self.selectedBackgroundProvider = selectedBackground

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
}

var backgroundProvider: (ApplyItemContentInfo) -> Element?

public func backgroundElement(with info: ApplyItemContentInfo) -> Element? {
backgroundProvider(info)
}

var selectedBackgroundProvider: (ApplyItemContentInfo) -> Element?

public func selectedBackgroundElement(with info: ApplyItemContentInfo) -> Element? {
selectedBackgroundProvider(info)
}
}
6 changes: 5 additions & 1 deletion BlueprintUILists/Sources/HeaderFooter.swift → ...UILists/Sources/ElementHeaderFooter.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
//
// HeaderFooter.swift
// ElementHeaderFooter.swift
// BlueprintUILists
//
// Created by Kyle Van Essen on 10/9/20.
Expand All @@ -9,6 +9,8 @@ import ListableUI
import BlueprintUI


///
/// ⚠️ This method is soft-deprecated! Consider using `myElement.listHeaderFooter(...)` instead.
///
/// Provides a way to create a `HeaderFooter` for your Blueprint elements without
/// requiring the creation of a new `BlueprintHeaderFooterContent` struct.
Expand Down Expand Up @@ -62,6 +64,8 @@ public func ElementHeaderFooter<Represented>(
)
}

///
/// ⚠️ This method is soft-deprecated! Consider using `myElement.listHeaderFooter(...)` instead.
///
/// Provides a way to create a `HeaderFooter` for your Blueprint elements without
/// requiring the creation of a new `BlueprintHeaderFooterContent` struct.
Expand Down
6 changes: 5 additions & 1 deletion BlueprintUILists/Sources/Item.swift → BlueprintUILists/Sources/ElementItem.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
//
// Item.swift
// ElementItem.swift
// BlueprintUILists
//
// Created by Kyle Van Essen on 9/10/20.
Expand All @@ -9,6 +9,8 @@ import ListableUI
import BlueprintUI


///
/// ⚠️ This method is soft-deprecated! Consider using `myElement.listItem(...)` instead.
///
/// Provides a way to create an `Item` for your Blueprint elements without
/// requiring the creation of a new `BlueprintItemContent` struct.
Expand Down Expand Up @@ -68,6 +70,8 @@ public func ElementItem<Represented, IdentifierValue:Hashable>(


///
/// ⚠️ This method is soft-deprecated! Consider using `myElement.listItem(...)` instead.
///
/// Provides a way to create an `Item` for your Blueprint elements without
/// requiring the creation of a new `BlueprintItemContent` struct.
///
Expand Down
21 changes: 17 additions & 4 deletions BlueprintUILists/Sources/List.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public struct List : Element
//
// MARK: Initialization
//

/// Create a new list, configured with the provided properties,
/// configured with the provided `ListProperties` builder.
public init(
Expand All @@ -76,13 +76,26 @@ public struct List : Element
public init(
measurement : List.Measurement = .fillParent,
configure : ListProperties.Configure = { _ in },
@ListableBuilder<Section> sections : () -> [Section]
@ListableArrayBuilder<Section> sections : () -> [Section],
@AnyHeaderFooterBuilder containerHeader : () -> AnyHeaderFooterConvertible? = { nil },
@AnyHeaderFooterBuilder header : () -> AnyHeaderFooterConvertible? = { nil },
@AnyHeaderFooterBuilder footer : () -> AnyHeaderFooterConvertible? = { nil },
@AnyHeaderFooterBuilder overscrollFooter : () -> AnyHeaderFooterConvertible? = { nil }
) {
self.measurement = measurement

self.properties = .default(with: configure)
var properties = ListProperties.default {
$0.sections = sections()

$0.containerHeader = containerHeader()
$0.header = header()
$0.footer = footer()
$0.overscrollFooter = overscrollFooter()
}

configure(&properties)

self.properties.sections += sections()
self.properties = properties
}

//
Expand Down
Loading
Loading