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.

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

Fixed an issue where a crash would occur when applying an external update to list content while a live reorder event was occurring #513

Merged
merged 2 commits into from
Nov 30, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
### Fixed

- Fixed an issue where supplementary views (headers or footers) that contained a first responder would result in the view being duplicated when scrolled off-screen.
- Fixed an issue where a crash would occur when applying an external update to list content while a live reorder event was occurring.

### Added

Expand Down
25 changes: 22 additions & 3 deletions Demo/Sources/Demos/Demo Screens/PaymentTypesViewController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,21 @@ import BlueprintUICommonControls

final class PaymentTypesViewController : ListViewController {

override func viewWillAppear(_ animated: Bool) {
super.viewWillAppear(animated)

Timer.scheduledTimer(withTimeInterval: 0.1, repeats: true) { [weak self] _ in
Copy link
Collaborator Author

@kyleve kyleve Nov 28, 2023

Choose a reason for hiding this comment

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

  • Remove this or add a start/stop button to the nav bar

self?.reload(animated: true)
}
}

override func configure(list: inout ListProperties) {

list.layout = .table { table in
table.layout.interSectionSpacingWithNoFooter = 10.0
table.bounds = .init(
padding: UIEdgeInsets(top: 0, left: 0, bottom: 100, right: 0)
padding: UIEdgeInsets(top: 0, left: 0, bottom: 100, right: 0),
width: .atMost(600)
)
}

Expand All @@ -30,21 +39,31 @@ final class PaymentTypesViewController : ListViewController {
list += Section(SectionID.main) { section in

section.header = PaymentTypeHeader(title: SectionID.main.title)

section += types.filter { $0.isEnabled }
.filter { $0.isMain }
.sorted { $0.sortOrder < $1.sortOrder }
.map(makeItem(with:))

if section.items.isEmpty {
section += EmptyRow()
}
}

list += Section(SectionID.more) { section in

section.header = PaymentTypeHeader(title: SectionID.more.title)

section.reordering.minItemCount = 0

section += types.filter { $0.isEnabled }
.filter { $0.isMain == false }
.sorted { $0.sortOrder < $1.sortOrder }
.map(makeItem(with:))

if section.items.isEmpty {
section += EmptyRow()
}
}

list += Section(SectionID.disabled) { section in
Expand Down Expand Up @@ -136,7 +155,7 @@ fileprivate struct PaymentTypeHeader : BlueprintHeaderFooterContent, Equatable {

var elementRepresentation: Element {
Label(text: title) {
$0.font = .systemFont(ofSize: 18.0, weight: .medium)
$0.font = .systemFont(ofSize: 18.0, weight: .bold)
}
.inset(uniform: 15.0)
}
Expand Down
18 changes: 12 additions & 6 deletions ListableUI/Sources/ListView/ListChangesQueue.swift
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import Foundation
/// A queue used to synchronized and serialize changes made to the backing collection view,
/// to work around either bugs or confusing behavior.
///
/// ## Handling Re-ordering (`isQueuingForReorderEvent`)
/// ## Handling Re-ordering (`isQueuingToApplyReorderEvent`)
Copy link
Collaborator Author

@kyleve kyleve Nov 28, 2023

Choose a reason for hiding this comment

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

  • Update this comment

/// Collection View has an issue wherein if you perform a re-order event, and then within
/// the same runloop, deliver an update to the collection view as a result of that re-order event
/// that removes a row or section, the collection view will crash because it's internal index path
Expand Down Expand Up @@ -104,18 +104,24 @@ final class ListChangesQueue {
self.runIfNeeded()
}

/// Set by consumers to enable and disable queueing during a reorder event.
var isQueuingForReorderEvent : Bool = false {
/// Set by consumers to enable and disable queueing when a reorder event is being applied.
var isQueuingToApplyReorderEvent : Bool = false {
didSet {
self.runIfNeeded()
Copy link
Collaborator Author

@kyleve kyleve Nov 28, 2023

Choose a reason for hiding this comment

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

  • Double check we don't need this when a re-order event is abandoned by the user

}
}

/// Prevents processing other events in the queue.
/// Should be set to `{ collectionView.hasUncommittedUpdates }`.
///
/// Note: Right now this just checks `isQueuingForReorderEvent`, but may check more props in the future.
/// When this closure returns `true`, the queue is paused, to avoid crashes when applying
/// content updates while there are index-changing reorder events in process.
var listHasUncommittedReorderUpdates : () -> Bool = {
kyleve marked this conversation as resolved.
Show resolved Hide resolved
fatalError("Must set `listHasUncommittedReorderUpdates` before using `ListChangesQueue`.")
}

/// Prevents processing other events in the queue.
var isPaused : Bool {
self.isQueuingForReorderEvent
self.isQueuingToApplyReorderEvent || self.listHasUncommittedReorderUpdates()
}

var isEmpty : Bool {
Expand Down
2 changes: 1 addition & 1 deletion ListableUI/Sources/ListView/ListView.DataSource.swift
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ internal extension ListView
///
/// See `sendEndQueuingEditsAfterDelay` for a more in-depth explanation.
///
self.view.updateQueue.isQueuingForReorderEvent = true
self.view.updateQueue.isQueuingToApplyReorderEvent = true

/// Perform the change in our data source.

Expand Down
2 changes: 1 addition & 1 deletion ListableUI/Sources/ListView/ListView.Delegate.swift
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ extension ListView
}

func listViewShouldEndQueueingEditsForReorder() {
self.view.updateQueue.isQueuingForReorderEvent = false
self.view.updateQueue.isQueuingToApplyReorderEvent = false
}

// MARK: UIScrollViewDelegate
Expand Down
8 changes: 7 additions & 1 deletion ListableUI/Sources/ListView/ListView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ public final class ListView : UIView

self.closeActiveSwipesGesture = TouchDownGestureRecognizer()

self.updateQueue = ListChangesQueue()

// Super init.

super.init(frame: frame)
Expand All @@ -91,6 +93,10 @@ public final class ListView : UIView
self?.shouldRecognizeCloseSwipeTouch(touch) ?? false
}

self.updateQueue.listHasUncommittedReorderUpdates = { [weak collectionView] in
collectionView?.hasUncommittedUpdates ?? false
}

// Register supplementary views.

SupplementaryKind.allCases.forEach {
Expand Down Expand Up @@ -846,7 +852,7 @@ public final class ListView : UIView
self.configure(with: description)
}

let updateQueue = ListChangesQueue()
let updateQueue : ListChangesQueue
kyleve marked this conversation as resolved.
Show resolved Hide resolved

public func configure(with properties : ListProperties)
{
Expand Down
10 changes: 5 additions & 5 deletions ListableUI/Tests/ListView/ListChangesQueueTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ class ListChangesQueueTests : XCTestCase {
let queue = ListChangesQueue()

XCTAssertFalse(queue.isPaused)
XCTAssertFalse(queue.isQueuingForReorderEvent)
XCTAssertFalse(queue.isQueuingToApplyReorderEvent)

var calls : [Int] = []

Expand All @@ -35,10 +35,10 @@ class ListChangesQueueTests : XCTestCase {
XCTAssertEqual(queue.count, 0)
XCTAssertEqual(calls, [1, 2])

queue.isQueuingForReorderEvent = true
queue.isQueuingToApplyReorderEvent = true

XCTAssertTrue(queue.isPaused)
XCTAssertTrue(queue.isQueuingForReorderEvent)
XCTAssertTrue(queue.isQueuingToApplyReorderEvent)

queue.add {
calls += [3]
Expand All @@ -57,10 +57,10 @@ class ListChangesQueueTests : XCTestCase {
XCTAssertEqual(queue.count, 3)
XCTAssertEqual(calls, [1, 2])

queue.isQueuingForReorderEvent = false
queue.isQueuingToApplyReorderEvent = false

XCTAssertFalse(queue.isPaused)
XCTAssertFalse(queue.isQueuingForReorderEvent)
XCTAssertFalse(queue.isQueuingToApplyReorderEvent)

waitFor {
queue.isEmpty
Expand Down
Loading