Skip to content

Commit

Permalink
Merge pull request #50 from skiptools/pickerperf
Browse files Browse the repository at this point in the history 
Optimize Picker performance for ForEach content without explicit .tags
  • Loading branch information
@aabewhite
aabewhite authored Sep 11, 2024
2 parents 00801e8 + babdf64 commit 7a7f354
Show file tree
Hide file tree
Showing 4 changed files with 223 additions and 84 deletions.
106 changes: 87 additions & 19 deletions Sources/SkipUI/SkipUI/Containers/ForEach.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public final class ForEach : View, LazyItemFactory {
@Composable public override func Compose(context: ComposeContext) -> ComposeResult {
// We typically want to be transparent and act as though our loop were unrolled. The exception is when we need
// to act as a lazy item factory
if context.composer is LazyItemCollectingComposer {
if context.composer is ForEachComposer {
return super.Compose(context: context)
} else {
ComposeContent(context: context)
Expand Down Expand Up @@ -84,24 +84,13 @@ public final class ForEach : View, LazyItemFactory {
}
}

@Composable private func taggedViews(for views: [View], defaultTag: Any?, context: ComposeContext) -> [View] {
return views.map { view in
if let taggedView = TagModifierView.strip(from: view, role: ComposeModifierRole.tag) {
return taggedView
} else if let defaultTag {
return TagModifierView(view: view, value: defaultTag, role: ComposeModifierRole.tag)
} else {
return view
}
}
}

@Composable func appendLazyItemViews(to views: MutableList<View>, appendingContext: ComposeContext) -> ComposeResult {
// ForEach views might contain nested lazy item factories such as Sections or other ForEach instances. They also
// might contain more than one view per iteration, which isn't supported by Compose lazy processing. We execute
// our content closure for the first item in the ForEach and examine its content to see if it should be unrolled
// If it should, we perform the full ForEach to append all items. If not, we append ourselves instead so that we
// can take advantage of Compose's ability to specify ranges of items
let isTagging = EnvironmentValues.shared._placement.contains(ViewPlacement.tagged)
var isFirstView = true
if let indexRange {
for index in indexRange {
Expand All @@ -112,7 +101,7 @@ public final class ForEach : View, LazyItemFactory {
} else {
isFirstView = false
}
if let identifier {
if isTagging {
contentViews = taggedViews(for: contentViews, defaultTag: index, context: appendingContext)
}
contentViews.forEach { $0.Compose(appendingContext) }
Expand All @@ -126,7 +115,7 @@ public final class ForEach : View, LazyItemFactory {
} else {
isFirstView = false
}
if let identifier {
if isTagging, let identifier {
contentViews = taggedViews(for: contentViews, defaultTag: identifier(object), context: appendingContext)
}
contentViews.forEach { $0.Compose(appendingContext) }
Expand All @@ -141,7 +130,7 @@ public final class ForEach : View, LazyItemFactory {
} else {
isFirstView = false
}
if let identifier {
if isTagging, let identifier {
contentViews = taggedViews(for: contentViews, defaultTag: identifier(objects[i]), context: appendingContext)
}
contentViews.forEach { $0.Compose(appendingContext) }
Expand All @@ -150,6 +139,53 @@ public final class ForEach : View, LazyItemFactory {
return ComposeResult.ok
}

/// If there aren't explicit `.tag` modifiers on `ForEach` content, we can potentially find the matching view for a tag
/// without having to unroll the entire loop.
///
/// - Seealso: `Picker`
@Composable func untaggedView(forTag tag: Any?, context: ComposeContext) -> View? {
// Evaluate the view generated by the first item to see if our body produces tagged views
var firstView: View? = nil
if let indexRange, let first = indexRange.first {
firstView = indexedContent!(first)
} else if let objects, let first = objects.first {
firstView = objectContent!(first)
} else if let objectsBinding {
let objects = objectsBinding.wrappedValue
if !objects.isEmpty {
firstView = objectsBindingContent!(objectsBinding, 0)
}
}
guard let firstView else {
return nil
}
let firstViews = collectViews(from: firstView, context: context)
guard !firstViews.contains(where: { TagModifierView.strip(from: $0, role: .tag) != nil }) else {
return nil
}

// If we do not produce tagged views, then we can match the supplied tag against our id function
if let indexRange, let index = tag as? Int, indexRange.contains(index) {
return indexedContent!(index)
} else if let objects, let identifier {
for object in objects {
let id = identifier(object)
if id == tag {
return objectContent!(object)
}
}
} else if let objectsBinding, let identifier {
let objects = objectsBinding.wrappedValue
for i in 0..<objects.count {
let id = identifier(objects[i])
if id == tag {
return objectsBindingContent!(objectsBinding, i)
}
}
}
return nil
}

@Composable private func isUnrollRequired(contentViews: [View], isFirstView: Bool, context: ComposeContext) -> Bool {
// If we're past the first view where we make the unroll decision, we must be unrolling
guard isFirstView else {
Expand All @@ -162,17 +198,46 @@ public final class ForEach : View, LazyItemFactory {

override func composeLazyItems(context: LazyItemFactoryContext) {
if let indexRange {
context.indexedItems(indexRange, identifier, onDeleteAction, onMoveAction, indexedContent!)
let factory: (Int) -> View = context.isTagging ? { index in
return TagModifierView(view: indexedContent!(index), value: index, role: ComposeModifierRole.tag)
} : indexedContent!
context.indexedItems(indexRange, identifier, onDeleteAction, onMoveAction, factory)
} else if let objects {
context.objectItems(objects, identifier!, onDeleteAction, onMoveAction, objectContent!)
let factory: (Any) -> View = context.isTagging ? { object in
let view = objectContent!(object)
guard let tag = identifier!(object) else {
return view
}
return TagModifierView(view: view, value: tag, role: ComposeModifierRole.tag)
} : objectContent!
context.objectItems(objects, identifier!, onDeleteAction, onMoveAction, factory)
} else if let objectsBinding {
context.objectBindingItems(objectsBinding, identifier!, editActions, onDeleteAction, onMoveAction, objectsBindingContent!)
let factory: (Binding<any RandomAccessCollection<Any>>, Int) -> View = context.isTagging ? { objects, index in
let view = objectsBindingContent!(objects, index)
guard let tag = identifier!(objects.wrappedValue[index]) else {
return view
}
return TagModifierView(view: view, value: tag, role: ComposeModifierRole.tag)
} : objectsBindingContent!
context.objectBindingItems(objectsBinding, identifier!, editActions, onDeleteAction, onMoveAction, factory)
}
}

@Composable private func collectViews(from view: any View, context: ComposeContext) -> [View] {
return (view as? ComposeBuilder)?.collectViews(context: context) ?? [view]
}

@Composable private func taggedViews(for views: [View], defaultTag: Any?, context: ComposeContext) -> [View] {
return views.map { view in
if let taggedView = TagModifierView.strip(from: view, role: ComposeModifierRole.tag) {
return taggedView
} else if let defaultTag {
return TagModifierView(view: view, value: defaultTag, role: ComposeModifierRole.tag)
} else {
return view
}
}
}
#else
public var body: some View {
stubView()
Expand All @@ -181,6 +246,9 @@ public final class ForEach : View, LazyItemFactory {
}

#if SKIP
/// Mark composers that should not unroll `ForEach` views.
protocol ForEachComposer {
}

// Kotlin does not support generic constructor parameters, so we have to model many ForEach constructors as functions

Expand Down
7 changes: 6 additions & 1 deletion Sources/SkipUI/SkipUI/Containers/LazySupport.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ public final class LazyItemFactoryContext {

/// Initialize the content factories.
func initialize(
isTagging: Bool = false,
startItemIndex: Int,
item: (View) -> Void,
indexedItems: (Range<Int>, ((Any) -> AnyHashable?)?, Int, ((IndexSet) -> Void)?, ((IndexSet, Int) -> Void)?, (Int) -> View) -> Void,
Expand All @@ -45,6 +46,7 @@ public final class LazyItemFactoryContext {
sectionHeader: (View) -> Void,
sectionFooter: (View) -> Void
) {
self.isTagging = isTagging
self.startItemIndex = startItemIndex

content.removeAll()
Expand Down Expand Up @@ -95,6 +97,9 @@ public final class LazyItemFactoryContext {
}
}

/// Whether we're in a tagging view placement.
private(set) var isTagging = false

/// The current number of content items.
var count: Int {
var itemCount = 0
Expand Down Expand Up @@ -291,7 +296,7 @@ public final class LazyItemFactoryContext {
private var content: [Content] = []
}

final class LazyItemCollectingComposer: SideEffectComposer {
final class LazyItemCollectingComposer: SideEffectComposer, ForEachComposer {
let views: MutableList<View> = mutableListOf() // Use MutableList to avoid copies

@Composable override func Compose(view: View, context: (Bool) -> ComposeContext) -> ComposeResult {
Expand Down
20 changes: 15 additions & 5 deletions Sources/SkipUI/SkipUI/Containers/List.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,9 @@ let listSectionnCornerRadius = 8.0
public final class List : View {
let fixedContent: ComposeBuilder?
let forEach: ForEach?
let itemTransformer: ((any View) -> any View)?

init(fixedContent: (any View)? = nil, identifier: ((Any) -> AnyHashable?)? = nil, indexRange: Range<Int>? = nil, indexedContent: ((Int) -> any View)? = nil, objects: (any RandomAccessCollection<Any>)? = nil, objectContent: ((Any) -> any View)? = nil, objectsBinding: Binding<any RandomAccessCollection<Any>>? = nil, objectsBindingContent: ((Binding<any RandomAccessCollection<Any>>, Int) -> any View)? = nil, editActions: EditActions = []) {
init(fixedContent: (any View)? = nil, identifier: ((Any) -> AnyHashable?)? = nil, itemTransformer: ((any View) -> any View)? = nil, indexRange: Range<Int>? = nil, indexedContent: ((Int) -> any View)? = nil, objects: (any RandomAccessCollection<Any>)? = nil, objectContent: ((Any) -> any View)? = nil, objectsBinding: Binding<any RandomAccessCollection<Any>>? = nil, objectsBindingContent: ((Binding<any RandomAccessCollection<Any>>, Int) -> any View)? = nil, editActions: EditActions = []) {
if let fixedContent {
self.fixedContent = fixedContent as? ComposeBuilder ?? ComposeBuilder(view: fixedContent)
} else {
Expand All @@ -86,6 +87,7 @@ public final class List : View {
} else {
self.forEach = nil
}
self.itemTransformer = itemTransformer
}

public convenience init(@ViewBuilder content: () -> any View) {
Expand Down Expand Up @@ -218,6 +220,7 @@ public final class List : View {
forceUnanimatedItems.value = false
}

let isTagging = EnvironmentValues.shared._placement.contains(ViewPlacement.tagged)
LazyColumn(state: reorderableState.listState, modifier: modifier) {
let sectionHeaderContext = context.content(composer: RenderingComposer { view, context in
ComposeSectionHeader(view: view, context: context(false), styling: styling, isTop: false)
Expand All @@ -242,6 +245,7 @@ public final class List : View {
startItemIndex += 1 // Search field
}
factoryContext.value.initialize(
isTagging: isTagging,
startItemIndex: startItemIndex,
item: { view in
item {
Expand Down Expand Up @@ -383,7 +387,7 @@ public final class List : View {
$0.set_placement(placement.union(ViewPlacement.listItem))
} in: {
// Note that we're calling the same view's Compose function again with a new context
view.Compose(context: context.content(composer: ListItemComposer(contentModifier: Self.contentModifier)))
view.Compose(context: context.content(composer: ListItemComposer(contentModifier: Self.contentModifier, itemTransformer: itemTransformer)))
}
if itemModifierView?.separator != Visibility.hidden {
Self.ComposeSeparator()
Expand Down Expand Up @@ -680,17 +684,23 @@ protocol ListItemAdapting {
#if SKIP
final class ListItemComposer: RenderingComposer {
let contentModifier: Modifier
let itemTransformer: ((View) -> View)?

init(contentModifier: Modifier) {
init(contentModifier: Modifier, itemTransformer: ((View) -> View)? = nil) {
self.contentModifier = contentModifier
self.itemTransformer = itemTransformer
}

@Composable override func Compose(view: View, context: (Bool) -> ComposeContext) {
let view = itemTransformer?(view) ?? view
if let listItemAdapting = view as? ListItemAdapting, listItemAdapting.shouldComposeListItem() {
listItemAdapting.ComposeListItem(context: context(false), contentModifier: contentModifier)
} else if view is ComposeModifierView || !view.isSwiftUIModuleView {
} else if view is ComposeModifierView || view is ComposeBuilder || !view.isSwiftUIModuleView {
// Allow content of modifier views and custom views to adapt to list item rendering
view.ComposeContent(context: context(true))
var contentContext = context(true)
// Erase item transformer, as we've already applied it
contentContext.composer = ListItemComposer(contentModifier: contentModifier)
view.ComposeContent(context: contentContext)
} else {
Box(modifier: contentModifier, contentAlignment: androidx.compose.ui.Alignment.CenterStart) {
view.ComposeContent(context: context(false))
Expand Down
Loading

0 comments on commit 7a7f354

Please sign in to comment.