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

Build via Swift package manager #384

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Build via Swift package manager #384

wants to merge 16 commits into from

Conversation

ephemer
Copy link
Member

@ephemer ephemer commented Dec 9, 2022

Type of change: DX, Performance

Motivation (current vs expected behavior)

Re-allow builds via SwiftPM, which should make it easier for others to integrate the library into their own builds. We also gain improved support for static linking, whole module optimization, etc. by doing this.

Please check if the PR fulfills these requirements

  • Self-review: I am confident this is the simplest and clearest way to achieve the expected behaviour
  • There are no dependencies on other PRs or I have linked dependencies through Zenhub
  • The commit messages are clean and understandable
  • Tests for the changes have been added (for bug fixes / features)

Copy link
Member

@michaelknoch michaelknoch left a comment

Choose a reason for hiding this comment

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

nice stuff! Looks good to me, I left a few comments where things were unclear to me

@@ -0,0 +1,25 @@
#if os(WASI)
Copy link
Member

Choose a reason for hiding this comment

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

nit: WASI shouldn't be needed for UIKit, right?

import SDL

#if os(Android)
@_implementationOnly import SDL
Copy link
Member

Choose a reason for hiding this comment

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

I found some docs about this here but can you explain in our context what this actually changes?

@@ -34,7 +34,7 @@ extension UIScrollView {
let animationTimeConstant = 0.325 * dampingFactor

// This calculation is a weird approximation but it's close enough for now...
let animationTime = log(Double(distanceToBoundsCheckedTarget.magnitude)) * animationTimeConstant
let animationTime = log2(Double(distanceToBoundsCheckedTarget.magnitude)) * animationTimeConstant
Copy link
Member

Choose a reason for hiding this comment

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

is this change intended?

open class UIScrollView: UIView {
open weak var delegate: UIScrollViewDelegate? // TODO: change this to individually settable callbacks
open var panGestureRecognizer = UIPanGestureRecognizer()
open var panGestureRecognizer: UIPanGestureRecognizer
Copy link
Member

Choose a reason for hiding this comment

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

is it not possible anymore to init here in that line when using @mainactor?


init {
arrayOf("JNI", "SDL", "UIKit").forEach { System.loadLibrary(it) }
}
Copy link
Member

Choose a reason for hiding this comment

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

do we use static linking already?

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