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

add FontWidth to support width support introduced in iOS 16 #262

Closed
wants to merge 1 commit into from

Conversation

kirkbig
Copy link
Contributor

@kirkbig kirkbig commented Oct 4, 2023

  • requires updating Package.swift to use swift-tools-version:5.7
  • requires setting minimum supported OS with .width support
  • minimal non-code .md updates

- requires updating `Package.swift` to use `swift-tools-version:5.7`
- requires setting minimum supported OS with `.width` support
- minimal non-code `.md` updates
Copy link
Owner

@gonzalezreal gonzalezreal left a comment

Choose a reason for hiding this comment

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

Hey @kirkbig,

Thanks for your contribution!

We should not change the platform requirements to introduce a feature only available from iOS 16 / macOS 13, etc.

For instance, we currently support table rendering only in macOS 13 / iOS 16 because the implementation uses a SwiftUI view unavailable on earlier versions. However, we did not change the package platform requirements.

You should use @availability and if #available to make this feature available only in the supported platforms without changing the package platform requirements.

@@ -77,6 +77,8 @@ public struct FontProperties: Hashable {
#endif
}

public static var defaultWidth: Font.Width = .standard
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @kirkbig,

Thanks for your contribution!

We should not change the platform requirements to introduce a feature only available from iOS 16 / macOS 13, etc.

For instance, we currently support table rendering only in macOS 13 / iOS 16 because the implementation uses a SwiftUI view unavailable on earlier versions. However, we did not change the package platform requirements.

You should use @availability and if #available to make this feature available only in the supported platforms without changing the package platform requirements.

@gonzalezreal , i did try to use @available, and it worked for creating the struct FontWidth itself and for creating a second .init below with the necessary variables, , but for var instances, the compiler says @available cannot be used. i can certainly investigate other ways to do this. i can also continue to live on my fork in the short term, unless you'd be interested in creating a base-availability-iOS16 branch …

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i have time-boxed this to about an hour, and i've tried a number of things, but without being able mark storied variables (i.e. instance var) with @available , i have not found a way to do this conditionally.

i will plan on maintaining this on my fork, and if you find yourself at some point in the future ready to inform users of this package that they can continue building for iOS 15 up to release 2.2 (or whatever the appropriate backward compatible release is at the time), and that starting with some release, you will add FontWidth support … then at that point it can be merged.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, Swift doesn't allow using @available in stored properties, which makes sense as this would change the parent type layout. But you can use @available in computed properties.

So, for this scenario, you will need two properties. A type-erased stored property to hold the value and a Font.Width computed property available on the supported platforms.

@available(iOS 16.0, macOS 13.0, tvOS 16.0, watchOS 9.0, *)
public var width: Font.Width {
  get { (self.widthStorage as? Font.Width) ?? .standard }
  set { self.widthStorage = newValue }
}

private var widthStorage: AnyHashable?

You don't need the defaultWidth property. This only makes sense for the weight property, as the tvOS platform has a different default weight value.

You can provide an additional initializer in FontProperties that takes a Font.Width parameter, but you may skip this as well, as it is not a requirement for the use case.

@@ -37,10 +37,14 @@ extension Font {
font = font.monospacedDigit()
}

if fontProperties.weight != .regular {
if fontProperties.weight != FontProperties.defaultWeight {
Copy link
Owner

Choose a reason for hiding this comment

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

This compares the weight against the default weight value of a Font value created from scratch, which should be .regular regardless of the platform. With this change, fonts created in tvOS won't be medium by default.

@kirkbig
Copy link
Contributor Author

kirkbig commented Oct 7, 2023

incorporated these suggestions on a branch other than main in my fork. by being on a different branch, i couldn't figure out how to have this PR point to the new branch. so i just created a new PR

@kirkbig kirkbig closed this Oct 7, 2023
@leecaa leecaa mentioned this pull request May 24, 2024
3 tasks
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