Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RunLoopMood changes for Swift 3.0 & Xcode 8.0 Beta 3 #1266

Closed
wants to merge 1 commit into from

Conversation

atetlaw
Copy link

@atetlaw atetlaw commented Jul 23, 2016

  • RunLoop.main is now a class var instead of a method
  • Thread.isMainThread is now a class var instead of a method
  • display link method now take the RunLoopMode struct instead of a string

It should be this way:
_displayLink.remove(from: RunLoop.main, forMode: RunLoopMode.commonModes)

This way is just redundant:
_displayLink.remove(from: RunLoop.main, forMode: RunLoopMode(rawValue: RunLoopMode.commonModes.rawValue))

- RunLoop.main is now a class var instead of a method
- Thread.isMainThread is now a class var instead of a method
- display link method now take the RunLoopMode struct instead of a string
- set the “use legacy swift” setting to NO
@liuxuan30
Copy link
Member

There are two PRs about swift 3.0 - #1230, #1258 pending merge.
One question: why you have changes in ChartPlatform.swift, while those two don't?

@atetlaw
Copy link
Author

atetlaw commented Jul 25, 2016

Hi @liuxuan30, It contains the NSUIDisplayLink class. That class had an API the same as CADisplayLink. The old API took a String for the forMode arg. I changed it to take the RunLoopMode.

@liuxuan30
Copy link
Member

@atetlaw what's the purpose of changing to RunLoopMode?

@atetlaw
Copy link
Author

atetlaw commented Jul 25, 2016

A couple of reasons,

  • matches the `CADisplayLink API
  • avoids the "stringiness"

On the stringliness point: the old version took a string, which meant that to call it you'd need to pass RunLoopMode.commonModes.rawValue, and then in the NSUIDisplayLink class it took the string and created a RunLoopMode via RunLoopMode(rawValue: stringArg).

It seemed redundant to do that, when passing RunLoopMode.commonModes provides additional compile-time protection that you're passing the right type to the method.

@liuxuan30
Copy link
Member

liuxuan30 commented Jul 26, 2016

@atetlaw ok, I have to let @danielgindi take a look at this, not sure why he uses string at first place.
BTW, I have merged #1258, could you please resolve the conflict? Thanks

@atetlaw
Copy link
Author

atetlaw commented Jul 26, 2016

👍 no problem

@danielgindi
Copy link
Collaborator

@liuxuan30 Could you please take all of those issues & PRs regarding Swift 3 and leave only one? It will be finalized after the Swift 3 release, as Swift changes all the time... And will probably sneak in Charts v3 release...

@liuxuan30
Copy link
Member

liuxuan30 commented Aug 3, 2016

@danielgindi sure, but I need you to take a look at this PR regarding RunLoopMode, is it right to use RunLoopMode instead of 'string'? I'm not sure because it seems related to NSUIDisplayLink class and you intend to use String

Current swift 2.3 and swift 3.0 branchs are suited for test purpose only, once Swift 3.0 is released, it is much easier for us to convert directly at that time than merging these branches.

However I observed people using the same Xcode beta can generate slightly different PRs.. So I am careful with these differences

@liuxuan30
Copy link
Member

I'm not sure if this is a mistake while merging Swift 3.0 code.. It seems intended to use string for NSUIDisplayLink and later convert to RunLoopMode for UIDisplayLink.
I will look into this this weekend.

@liuxuan30
Copy link
Member

I looked into this, it's valid. Since it has conflicts, I have manually merged it in #1296

@liuxuan30 liuxuan30 closed this Aug 6, 2016
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.

None yet

3 participants