Fixed Potential truncation issue in LabelLayout #229
Conversation
PrkumariLI
commented
Aug 14, 2018
PrkumariLI
commented
- Added line break mode support in LabelLayout to calculate correct size based on line break mode.
PrkumariLI
force-pushed
the
HUED-8605-LabelLayoutHeightCalculationFix
branch
from
2343c3f
to
1e91330
Compare
PrkumariLI
changed the title
|
| @@ -11,6 +11,12 @@ import LayoutKit | |||
|
|
|||
| class LabelLayoutTests: XCTestCase { | |||
|
|
|||
| // For the defined `sampleText` and `labelLayoutMaxWidth` combination, `LabelLayout` requires 2 line of | |||
| // text for char-wrapping and 3 lines for word-wrapping/truncating-tail. | |||
| // So don't change this combination as the tests are based on these values. | |||
There was a problem hiding this comment.
Could we add a test that verifies that char-wrapping and word-wrapping do indeed return different values? That way we'd catch the issue if these constants are indeed changed. That test then can explain that other tests are relying on this difference.
Looking further down, I see that there are already tests that verify this, so it'd be nice to mention that in the comment so that the reader understands that this is verified by tests and not that fragile. The current comment makes it sound like this is easy to break.
| // Use different line break mode for `LabelLayout` and dummy label. | ||
| // So in this case, size calculation should not match with dummy label's size calculation. | ||
| let label = UILabel(text: LabelLayoutTests.sampleText, numberOfLines: 0, lineBreakMode: .byCharWrapping) | ||
| let layout = LabelLayout(text: LabelLayoutTests.sampleText, config: { label in |
There was a problem hiding this comment.
We should be able to remove this config parameter and the two similar ones below, yes?
| func testAttributedTextSizeCalculationWithWordWrapping() { | ||
| // Use same line break mode for `LabelLayout` and dummy label and then match LabelLayout's size with dummy label's size calculation. | ||
| let attributedText = NSAttributedString(string: LabelLayoutTests.sampleText) | ||
| verifyAttributedTextSizeCalculation(with: attributedText, lineBreakMode: .byWordWrapping) |
There was a problem hiding this comment.
Would it make sense to iterate through all the different line break modes to confirm that the sizes match for all of them?
Sources/Text.swift
Outdated
| let options: NSStringDrawingOptions = [ | ||
| .usesLineFragmentOrigin | ||
| ] | ||
|
|
||
| /** |
There was a problem hiding this comment.
Please use // comments to be consistent with the other comments in this method. Also, /** comments usually are meant for doc comments that get parsed and processed and formatted by tools to produce API documentation, and this comment is internal information.
Thank you for adding this comment, by the way!
Sources/Text.swift
Outdated
| So use `paragraphStyle` only in case when UILabel/UITextView's mode is `byCharWrapping` because if we use `paragraphStyle` | ||
| with `byTruncatingTail`, `boundingRect(with:options:attributes:)` always give single line height. | ||
| */ | ||
| var attributes: [NSAttributedStringKey: Any] = [NSAttributedStringKey.font: font] |
There was a problem hiding this comment.
Minor issue, but can we make this a let instead of a var? slightly easier to read if one doesn't have to trace through the code looking to see if there are any more spots where it might get updated.
| @objc public let numberOfLines: Int | ||
| @objc public let alignment: LOKAlignment | ||
| @objc public let viewClass: UILabel.Type | ||
| @objc public let configure: ((UILabel) -> Void)? | ||
|
|
||
| // TODO: Remove this init once all consumers are switch to `init(attributedString:font:lineBreakMode:lineHeight:numberOfLines:alignment:flexibility:viewReuseId:viewClass:configure:)` |
There was a problem hiding this comment.
I think we should just add the new parameter, since it's optional.
There was a problem hiding this comment.
- We have added a new parameter in LOKLabelLayout.swift but it will be a breaking change because every Objective-C caller will have to pass the new parameter else it won't build. Also we made it non-optional parameter as optional enum won't be accessible in Objective-C.
Let us know if this should be fine.
- Added LabelLayout unit test to verify that size calculation logic considers line break mode while calculating LabelLayout size. - Added line break mode support in LabelLayout to calculate correct size based on line break mode.
PrkumariLI
force-pushed
the
HUED-8605-LabelLayoutHeightCalculationFix
branch
from
1e91330
to
089149f
Compare
|
|
These changes are being tracked in pull request: #260 |
