Pass old sections to refresh. Minor podspec updates to remove deprecated settings. #144
Conversation
colinhumber
changed the title
colinhumber
force-pushed
the
refresh-fixes
branch
from
f2a14d1
to
3e705a3
Compare
colinhumber
changed the title
|
Hi @colinhumber , thanks for the contribution. Given the empty PR notes, could I ask the use case this is addressing? and few more notes for thorough understanding? Issue/Problem/Improvement Goal: |
|
@dmiluski Oh shoot, my bad. Issue/Problem: Solution: |
|
@dmiluski Just curious when/if this will be merged. Thanks! |
|
Hi @colinhumber , sorry for the delay, pulling and taking a peak now. |
|
@dmiluski No worries! Thanks for checking it out. |
| guard let tableView = tableView else { return } | ||
| guard let oldSections = oldSections else { | ||
| guard !oldSections.isEmpty else { |
There was a problem hiding this comment.
Thanks for the PR @colinhumber ,
Given the move from Optional to isEmpty check, why the hard reload?
If moving from old to new, would it be beneficial to fall through to the code below no matter what (Given the newly assumed [])
cc: @hyperspacemark you might have some context on this?
There was a problem hiding this comment.
That's a good point. Having the default value as nil instead of [] could help differentiate between the first time the table view is loaded (oldSections == nil) and we'd want a hard reload vs subsequent changes where we may have removed all sections and are inserting new ones and would want those animated.
Thoughts?
There was a problem hiding this comment.
Given sections is non-nil (thus nil doesn't appear an option), I'm thinking all of these can just fall through to the add/remove/update animation rather than defaulting to reloadData(). Unless I'm missing some historical context here.
There was a problem hiding this comment.
The more I think about it, I think having the distinction between the table view being loaded for the first time (oldSections = nil) and the sections changing (oldSections = non-nil) makes sense.
sections is a non-optional property, but the DataSource initializer allows us to pass in nil for sections. We're just defaulting to [] in the case where nil is passed in.
If we did a fall through on the if oldSections is empty, the first time the table view is loaded, the sections would animate in, which likely wouldn't be the desired behaviour.
If that makes sense, I'll make the changes allowing sections to be an optional property.
There was a problem hiding this comment.
Looking at this again, I think you're right with the fall through. If we're setting the sections for the first time, or if the section count hasn't changed, we just call reloadSections and nothing would be animated. Tested this out with the example project and things were looking good.
There was a problem hiding this comment.
Do you think we still need the is empty check? Which determines reloadData() call?
There was a problem hiding this comment.
Removing the isEmpty check and relying solely on the add/remove/update/reload animations is throwing some memory errors when I run the tests.
Given that the initializer allows for nil sections to be passed in, part of me is thinking it would be more explicit to work with nullable sections, like we also handle the case where tableView is nil.
| @@ -151,8 +151,7 @@ public class DataSource: NSObject { | |||
| tableView.insertSections(IndexSet(integersIn: range), with: animation) | |||
| } else { | |||
| // Remove sections | |||
| let start = oldCount - 1 | |||
| let range: Range<IndexSet.Element> = start..<(start - delta) | |||
| let range: Range<IndexSet.Element> = newCount..<(newCount - delta) | |||
There was a problem hiding this comment.
Reading through this, why the move from start to newCount? Given the difference from index vs count, is this - 1 now no longer necessary?
There was a problem hiding this comment.
With the additional tests added, if the sections are being cleared, the range passed into deleteSections was incorrect and causing a crash when going from 2 sections to 0 sections.
The range created with start is 1..<3, which would crash with attempt to delete section 2, but there are only 2 sections before the update.
With newCount, the correct range is generated, 0..<2, and both sections are deleted with passing tests.
|
@dmiluski Anything missing to get this merged? This would be a nice addition to the library. |
|
Hi @colinhumber , I'm no longer a maintainer here. @danhollywells @factotvm for a review? |
| private func refresh() { | ||
| refreshTableSections() | ||
| private func refresh(oldSections: [Section] = []) { | ||
| refreshTableSections(oldSections: oldSections) |
There was a problem hiding this comment.
Hey guys, just FYI I'm testing this PR on my end in a project and these changes have surfaced a small bug related to dequeuing cells that have yet to be registered in the table view. If we just swap these two lines the issue is resolved (i.e. call refreshRegisteredCells() before refreshTableSections()). By calling refreshRegisteredCells() first, we ensure any new cells that have been added to the sections are registered before updating the layout.
This affects sections that use different cells within the same section.
No description provided.