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

Fix crash when deleting a row from @ObservedSectionedResults #8295

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

leemaguire
Copy link
Contributor

@leemaguire leemaguire commented Jul 10, 2023

Observing the sectioned results directly doesnt allow the SwiftUI diff to work correctly as the previous state of the sectioned results will have the new values.

An example of when this is an issue is when an item is deleted in a List containing sectioned results, the diff needs a stable state of the previous transaction but due to the observation callback calling calculate_sections the collection will be brought up to date.

The solution around this is to store a frozen copy of the sectioned results and observe the parent Results instead. Each time the results observation callback is invoked and the SwiftUI View is redrawn the sectioned results will be updated.

@leemaguire leemaguire force-pushed the lm/sectioned-results-swiftui branch from 72f0611 to ee11fcc Compare July 11, 2023 16:10
@@ -1,11 +1,12 @@
x.y.z Release notes (yyyy-MM-dd)
=============================================================
### Enhancements
* None.
* Add `@ObservedSectionedResults.remove(atOffsets:, section:)` which adds the ability to
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* Add `@ObservedSectionedResults.remove(atOffsets:, section:)` which adds the ability to
* Add `@ObservedSectionedResults.remove(atOffsets:section:)` which adds the ability to

collection.realm!.delete(offsets.map { section[$0].thaw()! })
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could also add an append function, something like this

public func append(_ value: Value.Element.Element) where Value.Element.Element: Object {
        write(wrappedValue) { collection in
            collection.realm!.add(value)
        }
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Users would be able to append objects to the collection used within @ObservedResultsSection without having a reference to the realm or having to thaw the realm from the wrappedValue

let reminderList = ReminderList()
$reminders.append(reminderList)

value = results

/*
Observing the sectioned results directly doesnt allow the SwiftUI diff to work
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Observing the sectioned results directly doesnt allow the SwiftUI diff to work
Observing the sectioned results directly doesn't allow the SwiftUI diff to work

Each time the results observation callback is invoked and the SwiftUI View is redrawn the sectioned results will be updated.
*/
sectionedResults = value.sectioned(sortDescriptors: sortDescriptors, sectionBlock).freeze()
token = self.objectWillChange.sink { [weak self] _ in
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really need to add an observation here as well and update the sectionedResults value if there is change in value in results, isn't the above code doing it already, it seems like you are duplicating updating sectionedResults

Copy link
Collaborator

Choose a reason for hiding this comment

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

I tested this and seems to be making this workaround to work.

@GriffinMeyer
Copy link

Is there an ETA on this getting released? We'd love to be able to implement grouped lists!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants