Fixes zero size issue when using an attributedTruncationToken.

[dillan]

Fixes sizeThatFits issue where the size returned was (0, 0) when using an attributedTruncationToken.

Addresses: #621 and #678.

I added a unit test to demonstrate the issue and its resolution. You can run the test from commit [2be9753] to see the size being returned as (0, 0). Re-run the test with commit [b82d006] to see the test pass.

[dillan]

The testSizeToFitRequiresNumberOfLines unit test is failing with my change. Does anyone know why TTTAttributedLabel would require the number of lines for sizeToFit to work, or is this perhaps an issue that my change addresses, thus eliminating the number of lines requirement?

[dillan]

@adonoho are you able to shed any light on the number of lines requirement in testSizeToFitRequiresNumberOfLines?

[adonoho]

I look at your patch and see you ignoring the existing -framesetter in favor of a new default CTFramesetterRef. In other words, I think you might have misdiagnosed your problem. You should look to see the settings of the existing -framesetter are before you throw them away. Perhaps the better solution is to just set the framesetter you prefer?

[dillan]

@adonoho -framesetter is created using -renderedAttributedText a colored version of -attributedText thus ignoring the -attributedTruncationToken.

The framesetter that I'm using in -sizeThatFits: uses a concatenated version of both -attributedText and -attributedTruncationToken to calculate the size.

[dillan]

@adonoho Would replacing -renderedAttributedText with the following be more appropriate?

- (NSAttributedString *)renderedAttributedText {
    if (!_renderedAttributedText) {
        NSMutableAttributedString *fullString = [[NSMutableAttributedString alloc] initWithAttributedString:self.attributedText];

        if (self.attributedTruncationToken) {
            [fullString appendAttributedString:self.attributedTruncationToken];
        }

        NSAttributedString *string = [[NSAttributedString alloc] initWithAttributedString:fullString];
        self.renderedAttributedText = NSAttributedStringBySettingColorFromContext(string, self.textColor);
    }

    return _renderedAttributedText;
}

[segiddins]

This will require CI to pass and new test coverage to be added to be mergable

[dillan]

Hey @segiddins I did add test coverage for this issue with testMultilineLabelSizeThatFitsWithTruncationToken in [0763af6]. However, the test that isn't passing is testSizeToFitRequiresNumberOfLines and I'm wondering why sizeToFit should require the number of lines to be greater than zero. Perhaps something vestigial from an earlier version of iOS?

[adonoho]

@dillan Each label can have its own -framesetter. Why don't you just set the one you want in your code and not try to push this case down into the code base?

[dillan]

@adonoho Thanks for your comments. Just to clarify, are you suggesting that -setFramesetter be exposed publicly on TTTAttributedLabel? To me if feels like that should remain a private implementation detail. Maybe I'm misunderstanding your intention, but why wouldn't a user of TTTAttributedLabel want the framesetter to account for the truncation token by default if the -attributedTruncationToken has been set?

Also, to be clear, I'm not trying to add any new behavior, I'm trying to address a regression that was introduced somewhere around 1.13.14.

Thanks @adonoho and @segiddins!

[adonoho]

@dillan I'm suggesting that you try to fix the problem on your side of the library, if possible. Your patch, in specific, overrules an existing property with a newly initialized, transient object. I generally view this pattern as a patch and not a root cause solution. If you do not like the property's value, change it. IMO, making a property public is a much better solution than a creating a transient object in a specific method. For the record though, -framesetter is private for a reason. Hence, pursuing a root cause solution is preferred.

As to the regression since v1.13.14, what changed that introduced this problem? The answer will guide us in finding a root cause solution.

[dillan]

Thanks @adonoho. I ran a git bisect and here is what I found so far:

11a4d4a1f4d24f7683a002a7df1a08377400b516 is the first bad commit
commit 11a4d4a1f4d24f7683a002a7df1a08377400b516
Author: Jonathan Hersh <jon@her.sh>
Date:   Tue Jun 2 12:05:21 2015 -0700

    [Sizing] consider truncation token (fixes #491)

:040000 040000 2250ef643f799c07bbb14cf1755f767fbc9f3e7a d1350c8438778d3381b1e44fe6a2305dcafba9e0 M  Example
:040000 040000 8ab44334b26181f5daddd482777b716825254f24 c0a9054519ab1894e49304e4645d6dce65dae322 M  TTTAttributedLabel

[dillan]

@adonoho I believe that the root cause of the issue is that the attributed string that -framesetter is configured with does not match the attributed string that is being passed in as a parameter to CTFramesetterSuggestFrameSizeForAttributedStringWithConstraints().

I've pushed some further changes that may be more appropriate to addressing the root cause by configuring the -renderedAttributedText with the -attributedTruncationTokenwhen one exists. @adonoho I would appreciate your feedback on this proposed fix.

The question still remains about why the -testSizeToFitRequiresNumberOfLines test asserts that a number of lines greater than 0 should be required. @jhersh do you recall the rationale behind this test? I don't want to remove or alter this test without understanding the original intent. See #491.

[adonoho]

@dillan I am happier with your new fix. Thank you for putting up with my objections. Together we make a better library.

As to the -testSizeToFitRequiresNumberOfLines needing to be greater than zero condition, I suspect the reason is pretty simple from an API design point of view: why have a label with zero lines? Kinda missing the point. But from an engineering perspective, zero probably isn't a problem. But these kinds of assumptions get baked into code in odd and unexpected places. Why do you want to relax the 1 or greater lines requirement?

[dillan]

Thanks @adonoho.

Setting the number of lines to zero is consistent with the behavior of UILabel. Consider the following from the documentation:

This property controls the maximum number of lines to use in order to fit the label’s text into its bounding rectangle. The default value for this property is 1. To remove any maximum limit, and use as many lines as needed, set the value of this property to 0.

https://developer.apple.com/reference/uikit/uilabel/1620539-numberoflines

So setting the number of lines to any number other than zero is essentially telling the label that it should be the specified number of lines, where setting 0 means the number of lines should be dynamically calculated.

[adonoho]

@dillan UILabel is not TTTAttributedLabel. This library predated the link and attributed string version of UILabel. Hence, it may not match exactly to the idioms adopted by UILabel. My advice? Because TTTAttributedLabel tests against at least one line, it almost certainly doesn't support the dynamic calculation mode that UILabel does. But I don't know for sure. I do know that I would encourage you to have a good reason to open up this bit of the code. You may not like the results of hidden assumptions being revealed. TTTAttributedLabel is an excellent class that is basically in maintenance mode. It has a long life ahead of it. As a workhorse class, most people probably like it as it is.

[dillan]

@adonoho All very good points.

In my testing setting the number of lines to 0, does match the dynamic height behavior I would expect. Further the -testSizeToFitRequiresNumberOfLines test was introduced in the same commit [11a4d4a] where the regression was introduced. Hence my question for @jhersh to clarify the underlying intent behind the test. Per my earlier comments I want to make sure I understand this intent before modifying or removing the test, because currently with my other real world testing it's unclear why the requirement for the number of lines to be greater than zero would exist. I'm sure that @jhersh has a good reason, I just want make sure we understand those reasons.

[jhersh]

-[TTTAttributedLabel sizeThatFits:] calls through to CTFramesetterSuggestFrameSizeForAttributedStringWithConstraints, which has behavior that varies with the number of lines passed in. This test likely depends on that underlying behavior. It would be good to try 0, 1, and 2+ lines under the current SDK and, if necessary, update the test.

[codecov-io]

Current coverage is 77.71% (diff: 100%)

No coverage report found for master at 981772d.

Powered by Codecov. Last update 981772d...3871c95

[dillan]

@jhersh Correct. It is currently implemented as:

static inline CGSize CTFramesetterSuggestFrameSizeForAttributedStringWithConstraints(CTFramesetterRef framesetter, NSAttributedString *attributedString, CGSize size, NSUInteger numberOfLines) {
    CFRange rangeToSize = CFRangeMake(0, (CFIndex)[attributedString length]);
    CGSize constraints = CGSizeMake(size.width, TTTFLOAT_MAX);

    if (numberOfLines == 1) {
        // If there is one line, the size that fits is the full width of the line
        constraints = CGSizeMake(TTTFLOAT_MAX, TTTFLOAT_MAX);
    } else if (numberOfLines > 0) {
        // If the line count of the label more than 1, limit the range to size to the number of lines that have been set
        CGMutablePathRef path = CGPathCreateMutable();
        CGPathAddRect(path, NULL, CGRectMake(0.0f, 0.0f, constraints.width, TTTFLOAT_MAX));
        CTFrameRef frame = CTFramesetterCreateFrame(framesetter, CFRangeMake(0, 0), path, NULL);
        CFArrayRef lines = CTFrameGetLines(frame);

        if (CFArrayGetCount(lines) > 0) {
            NSInteger lastVisibleLineIndex = MIN((CFIndex)numberOfLines, CFArrayGetCount(lines)) - 1;
            CTLineRef lastVisibleLine = CFArrayGetValueAtIndex(lines, lastVisibleLineIndex);

            CFRange rangeToLayout = CTLineGetStringRange(lastVisibleLine);
            rangeToSize = CFRangeMake(0, rangeToLayout.location + rangeToLayout.length);
        }

        CFRelease(frame);
        CGPathRelease(path);
    }

    CGSize suggestedSize = CTFramesetterSuggestFrameSizeWithConstraints(framesetter, rangeToSize, NULL, constraints, NULL);

    return CGSizeMake(CGFloat_ceil(suggestedSize.width), CGFloat_ceil(suggestedSize.height));
}

I've updated the unit test to exercise the three possible code paths in the above code:

1. Number of lines = 1 - falls into the first if (numberOfLines == 1) statement, setting the constraints to TTTFLOAT_MAX in both dimensions and using the default rangeToSize.
2. Number of lines = 2 - falls into the else if (numberOfLines > 0) statement, setting the rangeToSize, and using the default constraints.
3. Number of lines = 0 - uses the default rangeToSize and constraints.

All three of these possibilities call through to CTFramesetterSuggestFrameSizeWithConstraints() to get a suggested size. Note: CoreText's CTFramesetterSuggestFrameSizeWithConstraints() will return a non-zero size even when the number of lines is set to 0. This is the expected behavior when the frame setter has been configured with a valid attributed string with non-zero length.

[dillan]

@jhersh @adonoho @segiddins Is there anything else you need from me on this?

[dillan]

@jhersh @adonoho @segiddins just checking to see if there is anything left that I can address for this issue.

[segiddins]

I think it's just awaiting review

[dillan]

Thanks @jhersh @segiddins!

[JoeSzymanski]

We've recently hit this issue also. IS there any way this can get reviews done to be merged?

[adonoho]

I apologize for the delay getting the review done. Being persistent helps. We're all volunteers here.

[JoeSzymanski]

No problem. That's why I thought I would ask. Thanks @adonoho!

[dillan]

Thanks @adonoho!

[JoeSzymanski]

Just a heads up, I'm still seeing issues with truncation tokens and single line labels. I'm digging into the issue now to try to figure out what the problem is, but I think it has to do with usage of CTLineGetStringRange and the expectation that the range returned includes truncation (which according to research, is not the case).

[flexme]

Can we push a new version tag to include this fix?