Closed shaps80 closed 3 years ago
@josephduffy I'd love your input on this if you have the time to review it.
I'm not sure that function builders will be useful or possible in they ways you're outlining. I think the motivation for this could be fairly easily solved by using a closure-based section:
open class ClosureSection: Section {
public let wrappedSection: Section
public var numberOfElements: Int { wrappedSection.numberOfElements }
open weak var updateDelegate: SectionUpdateDelegate?
open required init(wrappedSection: Section) {
self.wrappedSection = wrappedSection
}
}
extension ClosureSection: SectionUpdateDelegate {
// Forward all `SectionUpdateDelegate` functions to `wrappedSection`. Ideally the `updateDelegate` would simply `get`/`set` on `wrappedSection`, but `===` is used in a few places internally that cause weird crashes or incorrect data.
}
The name isn't really right but Section
is already taken.
This by itself is not that useful, but can then be subclassed to provide similar functionality to what you're saying in the motivation:
open class CollectionClosureSection<Cell: UICollectionViewCell>: ClosureSection, CollectionSectionProvider {
public typealias ConfigureCell = (_ cell: Cell, _ index: Int, _ section: Section) -> Void
public let dequeueMethod: DequeueMethod<Cell>
public let cellConfigure: ConfigureCell
open required init<Section: Composed.Section>(wrappedSection: Section, dequeueMethod: DequeueMethod<Cell>, cellConfigure: @escaping ConfigureCell) {
self.cell = cell
self.dequeueMethod = dequeueMethod
self.cellConfigure = cellConfigure // Here you would do similar to what's in `CollectionCellElement` to force cast the section to `Section`
super.init(wrappedSection: wrappedSection)
}
open section(with traitCollection: UITraitCollection) -> CollectionSection {
let cell = CollectionCellElement(section: self, dequeueMethod: dequeueMethod, configure: cellConfigure>
return CollectionSection(section: self, cell: cell)
}
}
The caller can then:
let peopleSection = CollectionClosureSection(
wrappedSection: ArraySection([Person(name: "Foo"), Person(name: "Bar")]),
dequeueMethod: .nib(PersonCell.self)
) { cell, index, section in
let element = section.element(at: index)
cell.prepare(with: element)
}
Extensions could be added to add an init
that takes an array to remove the need for ArraySection
.
Of course it would need fleshing out to provide e.g. headers and footers but I think it fixes:
The current implementation requires the creation of a class with the inclusion of at least 1 method.
It does not fix:
In addition, it is required to create at least 1 cell class (and associated XIB?).
Although I don't think that's possible to fix via the API (with out or without function builders), are you suggesting allowing the view to be any UIView
then automatically embedding that view in a cell?
I think that functions builders aren't the silver bullet that can fix the API for this. A big part of this is because SwiftUI makes heavy use of some
, which is great when the base protocol is all that's required (I believe it's some View
in SwiftUI) but is a lot less useful when there are other pieces of information needed (such as being able to provide collection view elements, if it supports drag and drop etc.).
Your first example could be quite easily implemented using regular closures and I do not see the advantage of using function builders:
Section(people) { index in
Cell(.fromNib(PersonCell.self) { cell in
cell.prepare(people[index])
}
}
This is essentially the CollectionClosureSection
with an init<Model>(_ models: [Model], cellProvider: @escaping (_ index: Int) -> Cell)
.
If this were to a function builder like the example:
Section {
Cell(.fromNib(PersonNameCell.self) { cell in
cell.prepare(person.name)
}
Cell(.fromNib(PersonAgeCell.self) { cell in
cell.prepare(person.age)
}
}
I don't see how Composed can know which cell to show for each row. As you say you could implement ForEach
but even that would be quite complex to implement.
If you were to implement this:
@_functionBuilder
struct SectionBuilder {
static func buildBlock(_ cells: Cell...) -> [Cell] {
cells
}
}
struct Section {
let cells: [Cell]
init(@SectionBuilder _ content: () -> [Cell]) {
cells = content()
}
}
What do you do with the array of cells here? It could be useful for registration but how do you get a cell for a given row?
Another issue I see is diffing. SwiftUI is not slow because it does very good diffing of the view tree. The first step would be to iterate over every section and row to get the section and row counts, but at this point at lot of work has been done inside the Section
s. One of the advantages now is that things like cell creation/registration is delayed until necessary.
Making the function builder API in your examples (with struct
s) would require each section to be diffable, which is an inversion of the current model (currently the section notifies the delegate of specific changes).
When considering:
Is this API more of a sugar-candy DSL that just makes it simpler to define your sections
I don't think function builders can be a piece of syntactic sugar atop the existing API, but I do think using closures for more simple sections would help.
Is this a complete re-architecture of how Composed is implemented
I think this would be a monumental task that would pretty much be recreating SwiftUI but with less features.
First off, thank you for the detailed input. Really great points @JosephDuffy
I'm not sure that function builders will be useful or possible in they ways you're outlining.
This was my concern as well, I said 'proposal' however I probably should've called it 'exploration' as that's more inline with what it is. I agree, it may not be possible, I just think the output/design can be a great driver towards for the conversation. Which I feel has served its purpose here.
I think the motivation for this could be fairly easily solved by using a closure-based section.
That's a great idea, and might be a little more composable on top of the existing implementation.
The name isn't really right but
Section
is already taken.
True, but I'm up for renaming in v2.0 since its mostly used internally. Yes, you and other did/could create custom Section
's but for the most part I don't think it'll have as big an impact given Composed has mostly been adopted by only a handful of people to date.
Extensions could be added to add an
init
that takes an array to remove the need forArraySection
.
Yeah, that's when it becomes more beneficial IMHO. I like how you're 'wrapping' the Section
. Its a decent idea. I'll flesh this out a little more.
Of course it would need fleshing out to provide e.g. headers and footers but I think it fixes:
Agreed.
It does not fix:
In addition, it is required to create at least 1 cell class (and associated XIB?).
I definitely wasn't suggesting this needed fixing, just that its a constraint we have to work within 👍
And no, I definitely don't want to do embedding UIView
's, its tricky particularly when it comes to cell reuse.
I think that functions builders aren't the silver bullet that can fix the API for this. A big part of this is because SwiftUI makes heavy use of
some
, which is great when the base protocol is all that's required (I believe it'ssome View
in SwiftUI) but is a lot less useful when there are other pieces of information needed (such as being able to provide collection view elements, if it supports drag and drop etc.).
This is a really great observation actually. I was aware (have been using SwiftUI a lot) of the heavy usage of some
but I hadn't yet considered the implication well enough (probably should have 😬).
Your first example could be quite easily implemented using regular closures and I do not see the advantage of using function builders:
Section(people) { index in Cell(.fromNib(PersonCell.self) { cell in cell.prepare(people[index]) } }
This is a fair point, but it's also the most trivial example. We should review this again with a more complex structure once we have some initial implementation details in place.
I don't see how Composed can know which cell to show for each row. As you say you could implement
ForEach
but even that would be quite complex to implement.
Yeah I wasn't sure about this yet either.
What do you do with the array of cells here? It could be useful for registration but how do you get a cell for a given row?
Another valid point.
Another issue I see is diffing. SwiftUI is not slow because it does very good diffing of the view tree. The first step would be to iterate over every section and row to get the section and row counts, but at this point at lot of work has been done inside the
Section
s. One of the advantages now is that things like cell creation/registration is delayed until necessary.
Good point but I did have some thoughts on that. So I wasn't advocating that was stop deferring resources until required. I felt that the new API could instead be more of a DSL (a definition of a Section
) rather than the actual resulting instance itself. Therefore under-the-hood I think we could still defer
where possible. That being said, your point above about the use of some
would make this impossible (or at least very difficult at best). I hadn't thought it through enough at the time of writing, so it's a very good point.
Making the function builder API in your examples (with
struct
s) would require each section to be diffable, which is an inversion of the current model (currently the section notifies the delegate of specific changes).
Actually I don't that's entirely true. My thinking was that Content
or Data
would need to be diffable, but I do agree this is still an inversion to some degree. That being said, I'm not exactly against it, given Apple's new APIs for diffing entire structures quite efficiently. I think their implementation is extremely fast even given fairly large datasets, but I don't think ArraySection
for example is often the right choice for large datasets, not just for diffing but even memory usage.
I think this would be a monumental task that would pretty much be recreating SwiftUI but with less features.
100% agreed!
I think you've made some extremely valid point overall, so I'm going to explore the closure based approach. Its more of an enhancement for starters, but also, I like this for another reason – it doesn't require us to drop support for older iOS versions.
The primary driver for this conversation is to address 3 key areas:
Usability
I feel like at the moment, one of the main pain points is being forced to create a subclass for even trivial usage. I like the closure based wrapper idea, because it potentially solves this well. But as you mentioned, it definitely requires some fleshing out to see where its limitations fall and how we make that clear to an API consumer. Also when they hit those boundaries, how do they know what approach should be taken to resolve.
Discoverability
Another pain point is that you have to look up the docs or the headers to know what protocols you can conform to, in order to add additional behaviour to your sections.
Furthermore, even if you conform to the protocol, the library generally provides reasonable default implementations of many methods to appease UIKit delegate/datasource methods. As a result it can be difficult to know what you need to implement to get your desired behaviour.
Another side affect I've found that I'm not happy about is that if you update Composed and we've deprecated a method on a protocol, you likely won't get any help from the compiler, since you're not the one calling it. I've had this a few times during the development, where I simply changed the order of the arguments for example, and couldn't work out why the function was never called.
Using protocol composition is great, but its problematic for the API consumer in these scenarios.
Readability
I think a subclassed Section
is fine. Its general 1 method returning your section, plus any protocol, you've added conformance to. That being said, this could definitely be improved.
However the main issue I find, is that defining the Section
's themselves (say in a view controller) isn't quite as easy to understand in some scenarios. Particularly larger complex structures.
Just look at the tests and you can see that it requires careful 'reading' to work out if you've defined the structure correctly.
Improving the readability for as a result improve the ability to reason about the structure.
Summary
With those targets in mind, I think the closure based approach is a worthy contender for the moment. I'll flesh it out some more and then perhaps make a Draft PR so we can discuss around something with more context. This issue can remain open for further discussions and explorations as well.
Sound good @JosephDuffy ?
So I've explored this a little more and found some challenges already.
For reference, the implementation introduces a new type UICollectionSection
that takes a 'data' section, and essentially augments it for UI purposes.
An issue arises where the closures need to reference the internal Section
however since UICollectionSection
is providing the UI elements, the coordinator needs to reference that. This means the coordinator needs to know about this specific type.
There are a couple solutions:
We could slowly move towards using this type at some point, such that CollectionCoordinator
would always look for a UICollectionSection
rather than any section conforming to CollectionSectionProvider
. The protocols, provides and handlers would essentially become internal types and their public references deprecated.
Alternatively we could create a brand new Coordinator that's designed to work with UICollectionSection
. This approach however would prevent slow migration by the API consumer, since they would either have to switch all of their sections over, or not use the new Coordinator.
One other issues I've found is correctly propagating updateDelegate
updates.
I'd love any thoughts/feedback you might have @JosephDuffy – you can checkout the code on the branch eature/diffable
(yes I made a typo mistake when creating the branch)
As part of the diffing I think using the identifier of the section – rather than the section itself – would be a good approach. This would make some of the delegate calls easier to understand and would probably help with debugging.
It's a fairly large change from an API standpoint so I would guess starting with a new coordinator will be easier. Especially to build in more debugging.
Totally agree! And also yeah, I did add a section identifier too. I also think it'll just make the whole DiffableDatasource integration simpler as a result as well.
So although I got a fair way into this. Ultimately it's not looking likely due to the code-generation that would need to be included to make this work nicely.
I'm keen to explore closure based sections but my early experiments hit a few difficult to solve issues.
Closing for now.
Adding
FunctionBuilder
supportIntroduction
I'd like to add support for the new function builder APIs.
Motivation
The current implementation requires the creation of a
class
with the inclusion of at least 1method
. In addition, it is required to create at least 1 cell class (and associatedXIB
?).This doesn't sound like a lot but in practice it can still quickly become cumbersome when building reusable code.
Most often its necessary to define your own generic types on top of Composed's options:
Even in this rudimentary example, that's still a fair amount of code. What's important to capture here is:
Array
UICollectionView
PersonCell
Proposed solution
Add a
FunctionBuilder
style DSL to simplify usage for an API consumer as well reduce the code required to achieve the same result.There are a few added benefits to this approach:
Array
)Presentation
One thing to note,
Cell
needs to be inferred so the above wouldn't compile until it's inserted into a view. A nice solution to this might look like the following:Using this approach we can see:
struct
removes reference semantics and the potential for retain cyclesUICollectionView
is intended for the presentationUICollectionViewCell
is what we're expecting for ourCell
configurationHeaders & Footers
However another added benefit to using the view to infer our
Section
type is that we can also now determine what 'features' ourSection
might have.For example, we know that our
UICollectionView
supports header/footer elements:We could even go further, providing a convenience header type:
Static Views
One final advantage over this API is that it makes it trivial to build both dynamic as well as static
Section
's.Existing Sections
In the current implementation we have a few
Section
types that define the backing data that will be used for that section.SingleElementSection
ArraySection
ManagedSection
As well, we have two
SectionProvider
types that help us compose sections:ComposedSectionProvider
SegmentedSectionProvider
Using the proposed implementation, we can see all of these become completely unnecessary.
SingleElementSection
A
SingleElementSection
can be replaced by simply defining a static section:ArraySection & ManagedSection
Both can be defined by simply providing the relevant data to the
Section
ComposedSectionProvider
Simply including multiple sections removes the need for a specific type as well.
SegmentedSectionProvider
That leaves us with just one final type. The purpose of this type is to hold onto 1 or more child
Section
's while keeping only 1 active at a time.Well, this is easily achieved with the above implementation simply with the use of conditional statements.
Conclusion
It's now apparent that this implementation should remove the need for an API consumer (at least) to ever have any knowledge of multiple
Section
types. Instead focus on providing the data (or not) for aSection
and specifying whichCell
they want to be used for representing the element at a specified index.As for composition, using Swift knowledge you already have, you should easily be able to build composable
Section
's using nothing more than conditional statements.The use of
FunctionBuilder
APIs also removes the additional syntax of brackets and comma's that would otherwise (and currently) litter your code.Behaviours
ComposedUI
also extends yourSection
through the use of protocol conformance to provide behaviours like editing, selection, drag and drop and more.One potential solution here would be the use of a
Modifier
-style API as seen inSwiftUI
:There are a few obvious benefits to this approach:
Composed
will likely support SwiftUI and this API will 'feel' a lot more familiar and consistent.Final Thoughts
This approach is not only simpler and cleaner, its also much more user friendly and discoverable. It removes a few key pain points for users as well:
One last thing...
There are 2 other areas that still need to be thought through.
Layout, can likely be solved by initialising the view with a layout (if required). The bigger question is how to tie individual section layouts to the outer view layout. However we've solved it already, so I'm sure this can be worked through.
Environment variables are passed to functions or closures currently. These could replaced by something more like
@Environment
available in SwiftUI where Composed ensures the variables are set automatically for you at the right moment. I'm not sure atm if this is possible or something the compiler does for free in SwiftUI alone. I will need to investigate this further. Alternatively, the functions/closures could continue to provide these values as required.Considerations
The current approach to handle section updates and ensure they get propagated up to the coordinator is to use the delegate pattern. In order to support this 1 of 2 things should be considered.
My current instincts are to consider the usage of
Combine
and the new Swift diffing APIs, however this would mean dropping support for iOS <13. I'm not adverse to this, but it should be considered carefully.Source compatibility
This is likely to cause breaking API changes and require significant renaming in both ComposedUI and Composed. As such its planned for 2.0.
Alternatives considered
N/A