Skip to content

Highlighting with NSAttributedString's #15

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

Merged
merged 4 commits into from
Feb 3, 2018
Merged

Highlighting with NSAttributedString's #15

merged 4 commits into from
Feb 3, 2018

Conversation

nathantannar4
Copy link
Contributor

@nathantannar4 nathantannar4 commented Jan 31, 2018

This is for #8 and adds the ability to have text highlighting for autocomplete. This is done by switching the text replacement to NSAttributedString's with custom attributes.

Management of autocompleted substrings was done by applying a custom NSAttributedStringKey.

Open to all feedback 😊

Breaking Change

  • Added a new delegate method to MessageTextViewListener, func willChangeRange(textView: MessageTextView, to range: NSRange) which allowed for the observation of text range changes such that the entire autocomplete string is deleted rather than character by character

@@ -29,6 +29,27 @@ public final class MessageAutocompleteController: MessageTextViewListener {
public let range: NSRange
}
public private(set) var selection: Selection?

/// Adds an additional space after the autocompleted text when true. Default value is `TRUE`
Copy link
Member

Choose a reason for hiding this comment

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

Reminder for me: I should start adding docs 😝

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😝 Every devs favorite task

/// The default text attributes
open var defaultTextAttributes: [NSAttributedStringKey: Any] =
[NSAttributedStringKey.font: UIFont.preferredFont(forTextStyle: .body),
NSAttributedStringKey.foregroundColor: UIColor.black]
Copy link
Member

Choose a reason for hiding this comment

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

nit: omit the NSAttributedStringKey since it can be inferred from the type

Copy link
Member

Choose a reason for hiding this comment

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

Remind to myself, add assert(Thread.isMainThread() to every public API w/ mutations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

open var autocompleteTextAttributes: [String: [NSAttributedStringKey: Any]] = [:]

/// A key used for referencing which substrings were autocompletes
private let NSAttributedAutocompleteKey = NSAttributedStringKey.init("com.system.autocompletekey")
Copy link
Member

Choose a reason for hiding this comment

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

nit: Could we namespace w/ the project name? So

private let NSAttributedAutocompleteKey = 
  NSAttributedStringKey.init("com.messageviewcontroller.autocompletekey")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

textView.attributedText = textView.attributedText.replacingCharacters(in: highlightedRange, with: newAttributedString) + NSAttributedString(string: " ", attributes: typingTextAttributes)
} else {
textView.attributedText = textView.attributedText.replacingCharacters(in: highlightedRange, with: newAttributedString)
}
Copy link
Member

Choose a reason for hiding this comment

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

style nit: can we put params on newlines to keep the columns <120?

It looks like the line

textView.attributedText.replacingCharacters(in: highlightedRange, with: newAttributedString)

Is included twice. Could you put that in a var before entering the if statement?

Or simply:

textView.attributedText = textView.attributedText.replacingCharacters(in: highlightedRange, with: newAttributedString)
if appendSpaceOnCompletion {
  textView.attributedText += NSAttributedString(string: " ", attributes: typingTextAttributes)
}

Copy link
Member

Choose a reason for hiding this comment

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

Reminder: SwiftLint

Copy link
Collaborator

Choose a reason for hiding this comment

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

Via Danger!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Private API function?

///
/// - Parameter textView: The `UITextView` to apply `typingTextAttributes` to
internal func preserveTypingAttributes(for textView: UITextView) {

Copy link
Member

Choose a reason for hiding this comment

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

nit: remove whitespace

// Backspace/removing text
let attribute = textView.attributedText.attributes(at: range.lowerBound, longestEffectiveRange: nil, in: range).filter { return $0.key == NSAttributedAutocompleteKey }
let isAutocompleted = (attribute[NSAttributedAutocompleteKey] as? Bool ?? false) == true
if isAutocompleted {
Copy link
Member

Choose a reason for hiding this comment

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

merge isAutocompleted into the if-statement? variable isn't used anywhere else

Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like it improves readability here as it's a quite complicatedly (is that a word?) computed Bool.

if isAutocompleted {
// Starting to delete autocompleted text, so remove the autocompleted substring
let lowerRange = NSRange(location: 0, length: range.location + 1)
textView.attributedText.enumerateAttribute(NSAttributedAutocompleteKey, in: lowerRange, options: .reverse, using: { (_, range, stop) in
Copy link
Member

Choose a reason for hiding this comment

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

same nit about column length

textView.attributedText.enumerateAttribute(NSAttributedAutocompleteKey, in: lowerRange, options: .reverse, using: { (_, range, stop) in

// Only delete the first found range
defer { stop.pointee = true }
Copy link
Member

Choose a reason for hiding this comment

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

❤️

return ns
}

static func +(lhs: NSAttributedString, rhs: NSAttributedString) -> NSAttributedString {
Copy link
Member

Choose a reason for hiding this comment

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

Love it!!

}

}

Copy link
Member

Choose a reason for hiding this comment

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

Yaaaay tests 🎉

Suggestions from @rnystrom in PR and edits in the README to show users how to use text highlighting
@nathantannar4
Copy link
Contributor Author

Solid, thanks for the feedback! 💯 Edits made

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants