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

Adding Suspendible support #10

Closed
shaps80 opened this issue Aug 28, 2020 · 8 comments
Closed

Adding Suspendible support #10

shaps80 opened this issue Aug 28, 2020 · 8 comments
Assignees
Labels

Comments

@shaps80
Copy link
Collaborator

shaps80 commented Aug 28, 2020

Adding Suspendible support

Introduction

Composed should introduce the concept of a Section being Suspendible in order to prevent user-driven events triggering duplicate events in UIKit.

Motivation

ManagedSection is backed by CoreData and currently provides dedicated functions for suspending updates in order to support user-driven events (e.g. reordering).

However the general pattern used by Composed is to 'discover' features through the use of a protocol. To bring this inline with the rest of Composed, I'm proposing we introduce a protocol to add support for this:

public protocol Suspendible {
    func suspend()
    func resume()
}

In addition I feel we should add similar methods to the appropriate Coordinator's:

This is just to provide a simple convenience for the API consumer. The implementation would be:

extension CollectionCoordinator {
    func suspend() {
        sectionProvider.sections
            .compactMap { $0 as? Suspendible }
            .forEach { $0.suspend() }
    }

    func resume() {
        sectionProvider.sections
            .compactMap { $0 as? Suspendible }
            .forEach { $0.resume() }
    }
}

Currently only ManagedSection would implement this protocol however formalising this approach allows custom Section's to piggy-back on this behaviour as well.

Source compatibility

This should be purely additive.

Alternatives considered

N/A

@shaps80 shaps80 added the enhancement New feature or request label Aug 28, 2020
@shaps80 shaps80 self-assigned this Aug 28, 2020
@shaps80
Copy link
Collaborator Author

shaps80 commented Aug 28, 2020

@JosephDuffy

@shaps80 shaps80 added proposal and removed enhancement New feature or request labels Aug 28, 2020
@JosephDuffy
Copy link
Member

I'm all for this. I see the problem it's fixing and can see how it would be a problem. But I'm wondering exactly how this works.

Is there a method in Core Data to say "stop sending me updates"? If you perform the move while in this stopped state does it map the index paths of the changes performed during the move to what they should be after the move?

If I had:

  • A
  • B
  • C

then I pick up C and move (but don't drop) it:

  • A
  • (C)
  • B

While it's in the this dragging state there's an insert of BB after B. Do I see:

  • A
  • (C)
  • B
  • BB

or

  • A
  • (C)
  • B

?

Once dropped I should see the first.

At the time of the insert – from the perspective of the data source – it was inserted at index 2, but after the drop it would be index 3. If it's not inserting during the drop, does Core Data map to the correct index (2), or does Composed need to do this?

@shaps80
Copy link
Collaborator Author

shaps80 commented Sep 1, 2020

Interesting thought. So when using NSFetchedResultsController you get the following:

willChange
didChangeSection
didChangeItem
didChange

So generally you add a flag via suspend() or wateva and essentially ignore updates to the UI from CoreData during the move.

On drop, you just need to update the model. The actual UI updates are not necessary (hence why you need to pause).

After the drop, you call resume() and everything returns to normal.


Now what you're asking is what happens if an update occurs in the background for example during the move?

I've never actually encountered this but I would imagine this would cause a problem. I would imagine UI updates would be missed for those background updates, so the result would be (incorrectly) A C B.

How do you think we could handle this scenario?

I think this would have to be handled by each section that supports Suspendible. When updates received during a suspended state, would need to be cached and re-applied on resume().

The issue would be that you'd need to ignore the update for the move, since that's the whole point of the suspension.

I think a diff library would best handle this since it would essentially just be comparing snapshots before and after at any stage.

@shaps80
Copy link
Collaborator Author

shaps80 commented Dec 15, 2020

Given the performance stuff you've uncovered as of late, I'm reluctant to continue with this ticket. Close JosephDuffy ?

@JosephDuffy
Copy link
Member

JosephDuffy commented Dec 16, 2020

I was thinking about this while working on #18; the changes in that PR might helps this.

Between that and the performance improvements these things are more feasible.

Focussing on the quality of the existing features is my main priority, but I don't think that means we need to close this, unless you don't plan to implement it for a while.

EDIT: Just fixed the link above.

@shaps80
Copy link
Collaborator Author

shaps80 commented Dec 16, 2020

I was thinking about this while working on #18; the changes in that PR might helps this.

Not sure how that PR is directly associated, would be keen to hear your thoughts, but not a priority?

Focussing on the quality of the existing features is my main priority, but I don't think that means we need to close this, unless you don't plan to implement it for a while.

Yeah my thinking was that this isn't really a 'requirement' as much as I expected it to be. So it might be a while or never even make it in. So cleaning up the issue made sense?

If you really want to keep it open, that's fine, otherwise I think we could close this to keep focus.

@JosephDuffy
Copy link
Member

I was thinking about this while working on #18; the changes in that PR might helps this.

Not sure how that PR is directly associated, would be keen to hear your thoughts, but not a priority?

I meant composed-swift/ComposedUI#15; the suspend and resume calls could use mappingWillBeginUpdating/mappingWillBeginUpdating, since that'll essentially ignore updates (from the UI's perspective)

Focussing on the quality of the existing features is my main priority, but I don't think that means we need to close this, unless you don't plan to implement it for a while.

Yeah my thinking was that this isn't really a 'requirement' as much as I expected it to be. So it might be a while or never even make it in. So cleaning up the issue made sense?

If you really want to keep it open, that's fine, otherwise I think we could close this to keep focus.

Go ahead and close for now, but it's worth revisiting in case it helps and composed-swift/ComposedUI#15 helps.

@shaps80
Copy link
Collaborator Author

shaps80 commented Dec 16, 2020

👍

@shaps80 shaps80 closed this as completed Dec 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

2 participants