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

Fixes UISegmentedControl extension crash and improves efficiency #398

Merged
merged 7 commits into from
Jun 30, 2021
3 changes: 2 additions & 1 deletion BonMot.podspec
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ Pod::Spec.new do |s|
s.license = 'MIT'
s.author = { "Zev Eisenberg" => "zev@zeveisenberg.com" }
s.source = { :git => "https://github.com/Rightpoint/BonMot.git", :tag => s.version.to_s }
s.social_media_url = 'https://twitter.com/ZevEisenberg'
# Setting the twitter url is causing builds to fail due to not being able to reach twitter.
# s.social_media_url = 'https://twitter.com/ZevEisenberg'
s.requires_arc = true

s.ios.deployment_target = '10.0'
Expand Down
28 changes: 14 additions & 14 deletions Sources/UIKit/AdaptableTextContainer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -146,20 +146,20 @@ extension UISegmentedControl {

// `UISegmentedControl` has terrible generics ([NSObject: AnyObject]? or [AnyHashable: Any]?) on
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this comment need to be updated at all in light of these changes?

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 fixed an error inside that method. When I changed the return type back to being non-optional I forgot to default to an empty dictionary when nil. I also added a test for segmented control similar to the other UIKit tests.

As for the comment, I can't speak to whether it is still appropriate or not, although I can say in Swift 5 titleTextAttributes(for:) returns [NSAttributedFont.Key: Any]?.

/// `titleTextAttributes`, so use a helper in Swift 3+
@nonobjc final func bon_titleTextAttributes(for state: UIControl.State) -> StyleAttributes {
let attributes = titleTextAttributes(for: state) ?? [:]
var result: StyleAttributes = [:]
for value in attributes {
#if swift(>=4.2)
result[value.key] = value
#else
guard let string = value.key as? StyleAttributes.Key else {
fatalError("Can not convert key \(value.key) to String")
@nonobjc final func bon_titleTextAttributes(for state: UIControl.State) -> StyleAttributes? {
Copy link
Contributor

Choose a reason for hiding this comment

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

This changes the public API, would it be possible keep this the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I have reverted the changes to the signature of that method.

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 just realized that method is actually internal, so if we wanted to keep that change it wouldn't affect the public API. I'm fine with either direction.

Copy link
Contributor Author

@rzulkoski rzulkoski Dec 18, 2020

Choose a reason for hiding this comment

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

Also it looks like the tests are failing because Zev's twitter account is unreachable.

 ** BUILD SUCCEEDED **
    
   Testing with `xcodebuild`. 
 -> BonMot (5.6.0)
    - WARN  | url: The URL (https://twitter.com/ZevEisenberg) is not reachable.
    - NOTE  | xcodebuild:  note: Using new build system
    - NOTE  | xcodebuild:  note: Building targets in parallel
    - NOTE  | xcodebuild:  note: Using codesigning identity override: -
    - NOTE  | xcodebuild:  note: Planning build
    - NOTE  | xcodebuild:  note: Constructing build description
    - NOTE  | xcodebuild:  warning: Capabilities for Signing & Capabilities may not function correctly because its entitlements use a placeholder team ID. To resolve this, select a development team in the App editor. (in target 'App' from project 'App')
    - NOTE  | xcodebuild:  warning: Skipping code signing because the target does not have an Info.plist file and one is not being generated automatically. (in target 'App' from project 'App')
    - NOTE  | xcodebuild:  note: Using codesigning identity override: 

[!] BonMot did not pass validation, due to 1 warning (but you can use `--allow-warnings` to ignore it).
You can use the `--no-clean` option to inspect any issue.

Exited with code exit status 1
CircleCI received exit code 1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might be related to #373 @ZevEisenberg

guard let attributes = titleTextAttributes(for: state) else { return nil }
#if swift(>=4.2)
return attributes
#else
var result: StyleAttributes = [:]
for attribute in attributes {
guard let string = attribute.key as? StyleAttributes.Key else {
fatalError("Can not convert key \(attribute.key) to String")
}
result[string] = value
#endif
}
return result
result[string] = attribute.value
}
return result
#endif
}

/// Adapt `attributedTitle`, for all control states, to the specified trait collection.
Expand All @@ -168,7 +168,7 @@ extension UISegmentedControl {
@objc(bon_updateTextForTraitCollection:)
public func adaptText(forTraitCollection traitCollection: UITraitCollection) {
for state in UIControl.State.commonStates {
let attributes = bon_titleTextAttributes(for: state)
guard let attributes = bon_titleTextAttributes(for: state) else { continue }
let newAttributes = NSAttributedString.adapt(attributes: attributes, to: traitCollection)
setTitleTextAttributes(newAttributes, for: state)
}
Expand Down
25 changes: 24 additions & 1 deletion Tests/UIKitBonMotTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,30 @@ class UIKitBonMotTests: XCTestCase {
}
}

func writeTestSegmentedControl() {}
func testSegmentedControl() {
let segmentedControl = UISegmentedControl()
// Make sure the test is valid and the title text attributes are not defined for the normal state
XCTAssertNil(segmentedControl.titleTextAttributes(for: .normal))

segmentedControl.insertSegment(withTitle: ".", at: 0, animated: false)

// Assign the title text attributes for the normal state and ensure original values match
segmentedControl.setTitleTextAttributes(adaptiveStyle.attributes, for: .normal)

var attributes = segmentedControl.titleTextAttributes(for: .normal)
XCTAssertEqual(attributes?[.font] as? UIFont, expectedFont)
BONAssertColor(inAttributes: attributes, key: .foregroundColor, color: adaptiveStyle.color!)
BONAssert(attributes: attributes, query: { $0.pointSize }, float: expectedFont.pointSize)

// Update the trait collection and ensure the font grows.
if #available(iOS 10.0, tvOS 10.0, *) {
segmentedControl.adaptText(forTraitCollection: UITraitCollection(preferredContentSizeCategory: UIContentSizeCategory.extraLarge))
attributes = segmentedControl.titleTextAttributes(for: .normal)
BONAssert(attributes: attributes, query: { $0.pointSize }, float: expectedFont.pointSize + 2)
BONAssertColor(inAttributes: attributes, key: .foregroundColor, color: adaptiveStyle.color!)
}
}

func writeTestNavigationBar() {}
func writeTestToolbar() {}
func writeTestViewController() {}
Expand Down