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

Fix crash caused by nil lineView and titleLabel #216

Merged
merged 2 commits into from
Jul 3, 2018
Merged

Fix crash caused by nil lineView and titleLabel #216

merged 2 commits into from
Jul 3, 2018

Conversation

alextov
Copy link

@alextov alextov commented Jun 21, 2018

When creating SkyFloatingLabelTextField from storyboard and overriding certain properties, it is possible to get an Unexpectedly found nil while unwrapping an Optional value fatal error in updateLineColor() and some other methods.

Here is a minimal working example:

import SkyFloatingLabelTextField
import UIKit

final class ViewController: UIViewController {

    @IBOutlet private weak var textField: SkyFloatingLabelTextField!

}

extension SkyFloatingLabelTextField {

    override open var isSecureTextEntry: Bool {
        didSet {
            textColor = isSecureTextEntry ? .blue : .red
        }
    }

}

(Full Xcode project.)

The proposed pull request solves this issue by performing optional unwrapping with guards.

I didn’t check all places where implicit option unwrapping occurs, so there may be other crashes left related to this issue. To properly address it I was thinking to rewrite the component to have pairs of optional and non-optional properties (e.g., optional customLineView and non-optional lineView). That way user will still be able to customize the respective views and there will be a reliable fallback option. It is out of scope of this pull request, but if you deem it useful I can explore it further and prepare a separate proposal.

@k0nserv
Copy link
Contributor

k0nserv commented Jun 21, 2018

Thanks @alextov this looks good, can you add a line to CHANGELOG.md crediting yourself too?

@alextov
Copy link
Author

alextov commented Jun 21, 2018

@k0nserv Done.

@k0nserv k0nserv merged commit d492acb into Skyscanner:master Jul 3, 2018
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

2 participants