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

Fixed an issue where button's setTitle(string:state:) did not respond to a title change. #9

Closed
wants to merge 2 commits into from

Conversation

mixdesign
Copy link
Contributor

Hi, @codeido
Thanks for accepting my PR.

We have a problem in previous PR's implementation 😶:
Everythings works OK if the developer configures the button only through the storyboard.
But, if there is a need to change the title of the button programmatically, then a problem arises:

button.setTitle("Button title", for: .normal) // Won't change the title

The cause of the problem was the getAttributedString() method, which always returned the value .attributedText and ignored the value of .text;

I fixed the issue.
Now we have a flexible function:

// Get the NSMutableAttributedString with kern value: optional("string") ?? "";
// Button's previous attributedText is kept, only mutableString value is changed.
private func getAttributedString(with string:String?, letterSpacing spacing:CGFloat) -> NSMutableAttributedString {
    var attr:NSMutableAttributedString!
    if let string = string, let attrText = self.titleLabel?.attributedText {
        let attrString = NSMutableAttributedString(attributedString:attrText)
        attrString.mutableString.setString(string)
        attr = attrString
    } else {
        attr = NSMutableAttributedString(string:string ?? "")
    }
    
    attr.addAttribute(NSAttributedStringKey.kern, value: spacing, range: NSRange(location: 0, length: attr.length))
    return attr
}

In the future PR with another attributed string features, we may create another common private function which will return titleLabel's NSMutableAttributedString with a single string parameter.

Thanks!

… to a title change.

The cause of the problem was the getAttributedString() method, which always returned the value .attributedText and ignored the value of .text;
@mixdesign
Copy link
Contributor Author

I forgot to mension override method in PMSuperButton, which is self explanatory:

override open func setTitle(_ title: String?, for state: UIControlState) {
    // setTitle always sets attributed text
    self.setAttributedTitle(getAttributedString(with: title, letterSpacing: letterSpacing), for: state)
}

What do you think, is it ok always setting the attributed string for titleLabel ?

@pmusolino
Copy link
Owner

pmusolino commented Oct 16, 2017

Ohh thanks for this fix. What happens if the user wants to set a custom attributed title? Did he lose all the changes?

@pmusolino
Copy link
Owner

I tested your solution, but I think that this can be better:

@IBInspectable open var letterSpacing: CGFloat = 0 {
        didSet {
            self.setTitle(self.titleLabel?.text, for: self.state)
        }
    }

override open func setTitle(_ title: String?, for state: UIControlState) {
        // setTitle always sets attributed text
        if let title = title{
            let attributedStr = NSMutableAttributedString(string: title)
            self.setAttributedTitle(attributedStr, for: self.state)
        }
    }
    
open override func setAttributedTitle(_ title: NSAttributedString?, for state: UIControlState) {
        if let titleAttributed = title{
            let mutableAttributedString = NSMutableAttributedString(attributedString: titleAttributed)
            mutableAttributedString.addAttribute(NSAttributedStringKey.kern, value: letterSpacing, range: NSRange(location: 0, length: mutableAttributedString.length))
            super.setAttributedTitle(mutableAttributedString, for: state)
        }
        else{
            super.setAttributedTitle(title, for: state)
        }
    }

This solution works on all cases that I tested and is more simple and elegant + in future can be added more IBInspectable variables that need use the setAttributedString override method.

What do you think?

@pmusolino
Copy link
Owner

pmusolino commented Oct 16, 2017

In reality, this fixes not manage all the cases, for example, the color change and other adjustments on UIButton. I think we need to remove the letter spacing feature, at least for now.

@mixdesign
Copy link
Contributor Author

@codeido, I think we can solve this problem. Let's continue the discussion until we find the best solution. I will try to prepare another solution till the end of this week.

@pmusolino
Copy link
Owner

Excellent @mixdesign, consider that everything that impacts the text needs to be "converted" in a NSAttributedString with this approach.

1. setTitle(_ title: String?, for state: UIControlState)
2. setAttributedTitle(_ title: NSAttributedString?, for state: UIControlState)
3. setTitleColor(_ color: UIColor?, for state: UIControlState)

to preserve the previous titleLabel set attributes.
@mixdesign
Copy link
Contributor Author

Hey, @codeido
What do you think about this implementation: https://github.com/mixdesign/PMSuperButton/blob/master/Sources/PMSuperButton.swift

It works well with various combinations of programmatically calling setTitle, setAttributedTitle, setTitleColor.

I'm still testing it on my real project, just keep an eye by now.

@pmusolino pmusolino closed this Sep 23, 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