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

Feature/partial reloading #49

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

simonmitchell
Copy link
Contributor

Adds ability to perform partial reloading and comparison of rows within data

This allows for mutating `self.data` without redrawing the entire table
view for example, and is a working point to being able to mutate `data`
and only reload certain rows
@simonmitchell simonmitchell requested review from BenShutt and removed request for ryanbourneuk January 21, 2020 10:30
public func replace<T: Row & Equatable>(row: T, with otherRow: Row, reloading additionalReloadIndexPaths: [IndexPath] = [], animation: UITableView.RowAnimation = .none) {

guard let indexPath = indexPathFor(row: row) else { return }
guard let tableSection = data[indexPath.section] as? TableSection else { return }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is TableSection required? Or just Section

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TableSection currently... I didn't want to make changes to Section to require rows to be { get set }, but may be okay to do so now?

/// - otherRow: The row that is replacing the original row.
/// - additionalReloadIndexPaths: Additional index paths that should be reloaded at the same time.
/// - animation: The animation to use when reloading the rows
public func replace<T: Row & Equatable>(row: T, with otherRow: Row, reloading additionalReloadIndexPaths: [IndexPath] = [], animation: UITableView.RowAnimation = .none) {
Copy link
Contributor

@BenShutt BenShutt Jul 22, 2020

Choose a reason for hiding this comment

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

Could do fancy setters on subscripts here! But let's leave that for now 😄. Bit OTT

}

var indexPaths = [indexPath]
indexPaths.append(contentsOf: additionalReloadIndexPaths)
Copy link
Contributor

Choose a reason for hiding this comment

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

Happy to leave, but user's could wrap additionalReloadIndexPaths logic into a tableView.beginUpdates() so it doesn't need to go via this method. But let's leave for now

/// A function which allows for mutation of `data` without causing the tableView to reload.
///
/// - Parameter closure: Code to be run without reloading the table view.
public func withoutRedrawing(_ closure: () -> Void) {
Copy link
Contributor

@BenShutt BenShutt Jul 22, 2020

Choose a reason for hiding this comment

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

executeWithoutReloadingData ? Happy to leave

/// The last available indexPath in the tableView
public var lastIndexPath: IndexPath? {
guard let lastSection = data.last(where: { !$0.rows.isEmpty }) else { return nil }
return IndexPath(row: lastSection.rows.count - 1, section: data.count - 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

If there's rows.isEmpty then data.count-1 wouldn't be the correct section

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooooh yikes yep this needs more consideration!

public func redraw<T: Row & Equatable>(row: T, with animation: UITableView.RowAnimation = .none) {

guard let indexPath = indexPathFor(row: row) else { return }
tableView.reloadRows(at: [indexPath], with: animation)
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are using animation shouldn't we wrap in a tableView.beginUpdates - tableView.endUpdates? May be wrong there

/// - otherRow: The row that is replacing the original row.
/// - additionalReloadIndexPaths: Additional index paths that should be reloaded at the same time.
/// - animation: The animation to use when reloading the rows
public func replace<T: Row & Equatable>(row: T, with otherRow: Row, reloading additionalReloadIndexPaths: [IndexPath] = [], animation: UITableView.RowAnimation = .none) {
Copy link
Contributor

@BenShutt BenShutt Jul 22, 2020

Choose a reason for hiding this comment

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

Would be nice to reuse the array option below if we can, save code duplication - happy to leave ofc


replacement.forEach { (index, indexPath) in

guard let tableSection = data[indexPath.section] as? TableSection else { return }
Copy link
Contributor

Choose a reason for hiding this comment

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

Again do we need TableSection ?

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

Successfully merging this pull request may close these issues.

2 participants