Skip to content
This repository has been archived by the owner on Feb 17, 2021. It is now read-only.

fix LabelLayout async init used UILabel(). #173

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix LabelLayout async init used UILabel(). #173

wants to merge 1 commit into from

Conversation

ghmake1y
Copy link

If we initialize the LabelLayout asynchronously with an NSAttributedString, most of the time we don't have to set the font, but the default access to UILabel(). Font, this is not safe.I don't quite understand why this is designed in this way.

@jingwei-huang1
Copy link
Contributor

@ghmake1y Thanks for your PR. Could you elaborate why UILabel() isn't safe? We want to match LabelLayout font the same as UILabel by default

@nicksnyder
Copy link
Collaborator

@ghmake1y Does the test you added fail without your change?

@ghmake1y
Copy link
Author

@nicksnyder Yes,the test function named 'testAsyncAttributedLabel' is failed if i don't change default font.

@ghmake1y
Copy link
Author

@jingwei-huang1 You can see code of function testAsyncAttributedLabel in LabelLayoutTests.swit.If async call UILabel() will be crashed,it will be trigger Main Thread Checker: UI API called on a background.

@siuying
Copy link

siuying commented Dec 26, 2017

@jingwei-huang1 UILabel() is not safe because LayoutKit can call it in background queue. When use LabelLayout in ReloadableViewLayoutAdapter, I saw Xcode complaint

Main Thread Checker: UI API called on a background thread: -[UIView init]
PID: 26099, TID: 2008031, Thread name: (none), Queue name: ReloadableViewLayoutAdapter (QOS: USER_INITIATED), QoS: 25

You can reproduce this issue by running the LayoutKitSampleApp and open the "FeedCollectionViewController" example.

@siuying
Copy link

siuying commented Dec 26, 2017

If using system font is not ideal, maybe we can replace it with UIFont.preferredFont(forTextStyle: .body)?

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

Successfully merging this pull request may close these issues.

4 participants