Styles are no longer refreshed when `setText:` is called

[wprater]

In version 2.1.0 when you'd call setText: on a styles label, the styles will keep, on the current HEAD build, they are lost and I have to call updateStyles on the view. Is this an intentional change?

It's work noting the UILabel that is losing it's styles is making use of attributedText and the attributed-text child selector.

[pcolton]

So you are setting setText on a label that has attributed text? If so, then that's expected, you'd want to call setAttributedText instead. (setText overrides attributed text settings).

[wprater]

Just wanted to confirm this was a change from previous behavior. I used to be able to rely on the stylesheet to style the text when setText was called. Now if Im ever to change a UILabel that was styled using a attributed-text child selector, then I need to grab the existing attributes and reset them with using setAttributedText:. If this behavior change is somehow done for performance reasons, Im all for it. Just wanted to be sure this change was intentional.

On Mar 19, 2014, at 6:25 PM, Paul Colton notifications@github.com wrote:

So you are setting setText on a label that has attributed text? If so, then that's expected, you'd want to call setAttributedText instead. (setText overrides attributed text settings).

— Reply to this email directly or view it on GitHub.

[pcolton]

Here's the implementation:

-(void)setText:(NSString *)text
{
    callSuper1(SUPER_PREFIX, _cmd, text);

    if([self preventStyling] == NO)
    {
        [PXStyleUtils invalidateStyleableAndDescendants:self];
        [self updateStylesNonRecursively];
    }
}

As you can see, it doesn't style if preventStyling returns YES, and it does so when the label is inside of a UIButton. Otherwise, the only change I think is calling updateStylesNonRecursively rather than recursively (i.e. updateStyles). This change was submitted by another user as it has other ill side-effects.

I think the other change is that many properties were added into attributed-text, so things like shadows and color are now all settable that way, which means calling setText clears more things out.

[wprater]

I do recall reading through that PR #50 . Just wonder what is to be expected when one sets up styles with attributed-text. Will updating labels text also preserve set styles, or do we need to also manage how they were set through Pixate?

I know we've had this discussion before, so I don't want to run it into the ground.. as long as there is an expectation of the developer to know that one needs to either call updateStyles on a label, or it's parent after using setText:, or realize that they need to switch to grabbing all the attributes and use setAttributedText:. I'd argue the latter is much easier to implement for them, but may may deserve a documentation note.

I know it's a slippery slope to fuss with Apple's APIs, but I feel if the developer never used setAttributedText: to style the text originally, they should not have to use it when styles change. With that said, the simple solution is to remember to call updateStyles: after using setText: if you're using the attributed-text child selector to style your UILabel.

Where I ran into this issues was drawing table cells and I need to update visible cells with a timer. Every time the timer is called, the same draw function that was used to initially setup the cells gets called. Now I'll need to add an additional call to updateStyles on the table or the cell. Both are acceptable, but just wanted to gather thoughts.

[pcolton]

This are good points. I think the issue is that since we no longer style recursively by default, the attributed-text child doesn't get styled when you call setText. I'll have to review the PR for that change to remind myself why it was changed.

[pcolton]

This is the pull request: https://github.com/Pixate/pixate-freestyle-ios/pull/50 that made the change.

The issue is what I described in one of the last comments. Some 'children' are 'real' and some are not. attributed-text is not really a child, but rather a property on UILabel. Right now, there's no distinction, so we either have to style all children or none. We might want to consider reverting that change.

[wprater]

Can we always recursively style 'virtual' children? If we're able to create a distinction from virtual (style-only type) and real children (UIView type), then we can always recursively style virtual children and keep the other rules for non-recursive on real children.

[pcolton]

Yes, I believe that's the fix. I'd have to expand updateStyles to perhaps add a version for virtual children only and not subviews. Ideally that could be done without adding new methods. One idea is that updateStylesNonRecursive will still walk the virtual children but not real subviews. I'm open to suggestions as well.

[angelolloqui]

@wprater Do you have an example? I am trying to reproduce this issue but I see my label been style with or without the use of attributed-tex. Maybe I am missing something?. This is the piece of code I wrote to "see" it in action:

- (void)changeText {

    if (_attritributedUsed) {
        self.storiesLabel.text = @"Another regular text";
    }
    else {
        NSMutableAttributedString *text = [[NSMutableAttributedString alloc] initWithString:@"This is a test of attributted label"];
        NSRange range = NSMakeRange(text.length - 5, 5);
        [text addAttribute:NSFontAttributeName
                     value:[UIFont fontWithName:@"Helvetica" size:18]
                     range:range];
        [text addAttribute:NSBackgroundColorAttributeName
                     value:[UIColor redColor]
                     range:range];
        self.storiesLabel.attributedText = text;

    }
    _attritributedUsed = !_attritributedUsed;
    [self performSelector:@selector(changeText) withObject:nil afterDelay:3];
}

So the label is changed between the attributed text version and a regular text. In both cases the text is changed and styled as expected.

Apart from that, I would say that if the PR causes any trouble then I would switch it back to the recursive version because the performance gain is so minimal that it worths nothing to take the risk. But I would like to understand and reproduce the issue first if possible, so next time I better understand the implications of such a change.

Thanks guys! and sorry if this is all due to my PR.

[angelolloqui]

@pcolton I looked over your comment on virtual children but I still see no difference (except maybe on the textfield placeholder). The attributed-text virtual control in PXUILabel seems to do nothing, an a text change should not require any style update on childrens (if any) other than modifying the text itself. I can understand that if the virtual control has some styling applied then the change could break the styling, but not really with the current virtual control on the components and methods changed. As I said, Pixate is quite a complex library, so I might be missing something, it would be nice to now :)

Anyway, I thought was very "naive" but now it is making me feel uncomfortable. We probably should revert it anyway, even if this issue is finally not caused by it.

[wprater]

@angelolloqui the issue is not about removing the ability for one to use the setAttributedText: API, rather that if you rely on styles that are using the API, they are no longer styled when setText: is called. This is because when the virtual child and selector attributed-text is used, it's not necessary clear to a developer what is happening under the hood. Since other attributes of the UILabel do get styled when they use setText: but not all, notably the attributed ones.

@pcolton it seems to me that your solution of updating all virtual children in updateStylesNonRecursive is a good one. Unless you think we may need virtual children accessors in the future? But essentially, since they are virtually part of the parent view, it makes sense for them to be styled.

[pcolton]

I'm able to reproduce the issue, so i'm looking at the least impactful way to fix it. Probably by styling virtual children (perhaps just one level) when calling updateStylesNonRecursively. updateStyles would recursive all children.

[pcolton]

Ok, I've made a minor change whereby when styling non-recursively, virtual children still get updated (but non-virtual do not). This resolves the issue in terms of using setText but still doesn't walk all children when updating styles for the controls that were change in #50 PR.

[pcolton]

Give it a shot: https://github.com/Pixate/pixate-freestyle-ios/releases/tag/v2.1.3RC1

[pcolton]

I've made one more small change that prevents double styling when doing recursive styling (it would style the virtual children and then style all children (styling the virtuals again)). I just updated the binary if you want to give it a quick check.

[wprater]

@pcolton still looking goof over here.