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

Swift 3.0 beta 6 (acegreen's branch, but with fixes for OS X) #1353

Merged
merged 1 commit into from
Sep 5, 2016

Conversation

pixelspark
Copy link
Contributor

As for some reason I can't seem to submit a pull request to @acegreen's branch, I'll submit it here: this is @acegreen's branch for Swift 3 (XCode beta 6), but now with fixes that let it build under OS X.

Further discussion is here: #1342

@liuxuan30
Copy link
Member

liuxuan30 commented Aug 30, 2016

@pixelspark hey thanks. I will merge #1342 soon, could you just pull and update this PR once I merged?

@liuxuan30
Copy link
Member

liuxuan30 commented Aug 30, 2016

I just merged #1342, could you pull the branch again and perhaps double check everything good so I can merge this PR?

@pixelspark
Copy link
Contributor Author

@liuxuan30 thanks, I have just rebased my changes atop of your Swift-3.0 branch (a3c03c2). It builds with these changes on OSX & looks fine to me!

@@ -357,7 +357,7 @@ public class ChartViewBase: NSUIView, ChartDataProvider, ChartAnimatorDelegate
// 23 is the smallest recommended font size on the TV
font = NSUIFont.systemFont(ofSize: 23, weight: UIFontWeightMedium)
#else
font = NSUIFont.systemFont(ofSize: NSUIFont.systemFontSize)
font = NSUIFont.systemFont(ofSize: NSUIFont.nsuiSystemFontSize)
Copy link
Member

Choose a reason for hiding this comment

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

guess it is supposed to start with NSUI ?

@liuxuan30
Copy link
Member

liuxuan30 commented Aug 30, 2016

@pixelspark I have added some line notes. Please review. BTW, some of the changes have weird indents, maybe you want to double check

@@ -200,6 +200,12 @@ types are aliased to either their UI* implementation (on iOS) or their NS* imple
UIGraphicsBeginImageContextWithOptions(size, opaque, scale)
}

extension NSUIFont {
Copy link
Member

Choose a reason for hiding this comment

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

not sure why, but the indent seems not 4 spaces..

@pixelspark
Copy link
Contributor Author

@liuxuan30 Thanks. Will check indenting, probably an issue with my XCode settings between projects. Re the other remarks: will have a look into those later today or tomorrow.

@liuxuan30
Copy link
Member

liuxuan30 commented Aug 31, 2016

@pixelspark One more thing, as SE-1107 introduces open, I have made a PR #1378 to migrate to open. Could you do the same for macOS portion? Or I will do it later merged this.

@pixelspark
Copy link
Contributor Author

@liuxuan30 I don't think that I need to make any classes 'open' in addition to what your commit will fix. The OSX specific parts are mainly internal to the Charts framework (i.e. none of the NSUI classes should be exposed to users of the framework, let alone be declared open...)

@liuxuan30
Copy link
Member

OK, not fully looked into macOS yet, just being careful if it will override access control. It's not only classes but also class members, I have converted all available ones.
Whenever you are ready :)

@pixelspark
Copy link
Contributor Author

@liuxuan30 fixed open/public for OSX (this PR is now based on your latest Swift-3.0 commit, e.g. ce4e180). Feel free to fix any whitespace or code styling issues yourself :)

@liuxuan30
Copy link
Member

Thanks.

@liuxuan30 liuxuan30 merged commit 3e35e32 into ChartsOrg:Swift-3.0 Sep 5, 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.

2 participants