Skip to content

Conversation

@tonik173
Copy link

  • method RNTweetShadowView.computeIntrinsicSize has to make sure, that sizeThatFits gets invoked in the main thread.
  • _bridge property removed. Shadows native implementation.
  • (dispatch_queue_t)methodQueue added to RNTwitterKitViewManager, since this module is supposed to run in main queue

@ZdravkoN ZdravkoN requested review from ZdravkoN and ognen November 21, 2017 10:36
TWTRTweetView *view = [self tweetViewForMeasuring];
defaultSize = [view sizeThatFits:CGSizeMake(width, CGFLOAT_MAX)]; // 200 is the minimum width required for a tweet
defaultSize.width = UIViewNoIntrinsicMetric;
dispatch_async(dispatch_get_main_queue(), ^{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are calling dispatch_async and in the block you are modifying deafultSize. defaultSize then gets immediately returned, i.e.you don't wait for your async call to finish.

Perhaps dispatch_sync is in order here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, you’re right.

static TWTRTweetView *tweetView = nil;
static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
tweetView = [[TWTRTweetView alloc] initWithTweet:nil style:TWTRTweetViewStyleCompact];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem also arises here, dispatch_sync should be called instead of dispatch_once.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, alloc can be safely called in a bkgnd thread. The initWithTweet is bkgnd thread is safe also – at least thats what Apple’s code analyser says.

@implementation RNTwitterKitViewManager

// this module is supposed to run in the main queue
- (dispatch_queue_t)methodQueue
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for moving the execution of the view manager on the main queue? It is not apparent.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it is indeed not required at the moment. However, it will be, once the library exports constants. I added it to prevent future thread problems. Since you might need it in the future anyway, the run time model remains the same. But of course, you can remove it if you wish. Should’t hurt at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants