-
Notifications
You must be signed in to change notification settings - Fork 57
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 performance of height recalculation #178
Conversation
- Fixed performance of height recalculation - Added logging Fixes #63
bbd8fef
to
7c90f78
Compare
@@ -284,7 +298,25 @@ extension BrickFlowLayout { | |||
return contentSize | |||
} | |||
|
|||
func updateDirtyBricks(updatedAttributes: @escaping OnAttributesUpdatedHandler) { | |||
for (section, item) in dirtyMap { | |||
let layoutSection = sections![section]! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this force unwrapping safe?
@@ -352,7 +384,21 @@ extension BrickFlowLayout { | |||
} | |||
let shouldInvalidate = preferredAttributes.frame.height != brickAttribute.originalFrame.height | |||
brickAttribute.isEstimateSize = false | |||
return shouldInvalidate | |||
|
|||
// return shouldInvalidate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix that
open override func invalidateLayout(with context: UICollectionViewLayoutInvalidationContext) { | ||
BrickLogger.logVerbose(message: "Invalidate layout with context \(context)") | ||
guard sections != nil else { // No need to invalidate if there are no sections |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this is guard let sections = sections
then we don't need all the force unwraps below.
@@ -25,7 +26,7 @@ enum BrickLayoutInvalidationContextType { | |||
*/ | |||
var shouldInvalidateAllAttributes: Bool { | |||
switch self { | |||
case .rotation, .invalidate, .creation, .updateVisibility, .updateHeight(_): return true | |||
case .rotation, .invalidate, .creation, .updateVisibility, .updateDirtyBricks /*, .updateHeight(_) */: return true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it intended to not have updateHeight
have this behavior? delete the commented code if so.
} | ||
// guard brickAttributes.originalFrame.height != height else { | ||
// return | ||
// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete this if it's not needed
// if attributes.count < 100 { | ||
// // Prevent that the "Huge" test aren't taking forever to complete | ||
// BrickLogger.logVerbose(message: self.printAttributes()) | ||
// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete this
@@ -307,7 +309,7 @@ internal class BrickLayoutSection { | |||
/// - firstIndex: The index the calculation needs to start from (the main reason is to just calculate the next cells | |||
/// - invalidate: Identifies if the attributes need to be invalidated (reset height etc) | |||
/// - updatedAttributes: Callback for the attributes that have been updated | |||
fileprivate func createOrUpdateCells(from firstIndex: Int, invalidate: Bool, updatedAttributes: OnAttributesUpdatedHandler?) { | |||
public func createOrUpdateCells(from firstIndex: Int, invalidate: Bool, updatedAttributes: OnAttributesUpdatedHandler?) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this meant to be called outside of brickkit? if not, could we make it internal instead?
debugString += "Number of attributes: \(attributes.count) in frameOfInterest \(_dataSource.frameOfInterest)" | ||
debugString += "\n" | ||
debugString += "Total Frame: \(self.frame)" | ||
debugString += "\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should leave a // TODO
in here to use multiline string literals when we can start building this with Xcode 9
Codecov Report
@@ Coverage Diff @@
## master #178 +/- ##
==========================================
- Coverage 93.48% 93.18% -0.31%
==========================================
Files 39 40 +1
Lines 4160 4239 +79
Branches 341 347 +6
==========================================
+ Hits 3889 3950 +61
- Misses 270 288 +18
Partials 1 1
Continue to review full report at Codecov.
|
Fixes #63