Add rx.attributedText to UITextView and UITextField #1249

carlosypunto · 2017-05-12T16:47:54Z

Requested in issue #1246

RxPullRequestBot · 2017-05-12T17:03:15Z

	2 Warnings
⚠️	Please target PRs to `develop` branch
⚠️	No CHANGELOG changes made

Generated by 🚫 Danger

kzaher · 2017-05-13T23:14:40Z

RxCocoa/iOS/UITextView+Rx.swift

+            // when IME input method is used.
+            if textView.attributedText != attributedText {
+                print(textView.attributedText)
+                print(attributedText)


I think these are accidental prints.

removed in las commit

kzaher · 2017-05-13T23:15:17Z

RxCocoa/iOS/UITextView+Rx.swift

+                .observeOn(MainScheduler.asyncInstance)
+                .map { _ in
+                    guard let textStorage = textView?.textStorage else { return nil }
+                    return textStorage.attributedSubstring(from: NSRange(location: 0, length: textStorage.string.utf8.count))


I'm not sure what is the length here. Why do you think it's textStorage.string.utf8.count?

In last commit is changed to NSRange(location: 0, length: textStorage.length) the full range

kzaher · 2017-05-13T23:21:28Z

RxCocoa/iOS/UITextField+Rx.swift

+    /// Bindable sink for `attributedText` property.
+    public var attributedText: UIBindingObserver<Base, NSAttributedString?> {
+        return UIBindingObserver(UIElement: self.base) { textField, attributedText in
+            textField.attributedText = attributedText


I think we need to find way to make sure this isn't updated in case attributed text is the same and test this.

if textField.attributedText != attributedText { textField.attributedText = attributedText }

We had issues with IME.

// This check is important because setting text value always clears control state // including marked text selection which is imporant for proper input // when IME input method is used.

kzaher · 2017-05-13T23:42:03Z

Tests/RxCocoaTests/UITextField+RxTests.swift

+        XCTAssertTrue(textField.settedText)
+    }
+
+    func testLabel_attributedTextObserver() {


I think this is copied code from UILabel, but nothing is updated, it doesn't test UITextField.

fixed in last commit

kzaher · 2017-05-13T23:42:39Z

Tests/RxCocoaTests/UITextField+RxTests.swift

    }
 }

 final class UITextFieldSubclass : UITextField {
-    var set: Bool = false
-
+    var settedText = false


I think maybe didSetText would be more descriptive.

kzaher · 2017-05-13T23:43:37Z

Tests/RxCocoaTests/UITextView+RxTests.swift

-    var set: Bool = false
-
+    var settedText = false
+    var settedAttributedText = false


The same like for UITextField. This should probably be didSetText and didSetAttributedText.

carlosypunto · 2017-05-14T03:27:01Z

I think now is correct.
Maybe you want APIWrapper RxExample will log attributedText changes, I can add to PR changing this with:

        let attributedTextValue = textView.rx.attributedText.asObservable()
        Observable.zip(textViewValue.asObservable(), attributedTextValue, resultSelector: { text, attributedString -> String in
            return "UITextView text: \(text)\nUITextView attributedText: \(attributedString?.description ?? "nil")"
        })
            .subscribe(onNext: { [weak self] x in
                self?.debug("UITextView text \(x)")
            })
            .disposed(by: disposeBag)

kzaher · 2017-05-15T10:48:45Z

Yeah, we can do this in a separate PR.

kzaher · 2017-05-18T11:04:29Z

Tests/RxCocoaTests/UITextField+RxTests.swift

+        XCTAssertTrue(textField.isSettedText)
+    }
+
+    func testLabel_attributedTextObserver() {


Test title says testLabel. It should probably be just test because there is no label that we are testing inside this test.

kzaher · 2017-05-18T11:04:30Z

Tests/RxCocoaTests/UITextField+RxTests.swift

        textField.text = "Text1"
-        textField.set = false
+        textField.isSettedText = false


I don't think isSettedText is formulated correctly. Maybe use didSetText.

kzaher · 2017-05-18T11:08:22Z

Tests/RxCocoaTests/UITextField+RxTests.swift

+    var textFieldAttributedString: NSAttributedString {
+        let tf = UITextField()
+        tf.attributedText = NSAttributedString(string: self)
+        return tf.attributedText!


I don't understand this code. UITextField is being created and then discarded immediately.

Is there something wrong with just using NSAttributedText(string: "Hello!")?

The NSAttributedText(string: "Hello!") style can't have style info. When we set this to a Textfield or Textview, they overwrite its internal NSAttributedString properties in the case only text, then, if we get the control's attributedStyle is not equal to the setted. So this function returns and NSAttributedText with all properties with effect in these controls being it comparable with getted style from control.

kzaher · 2017-05-18T11:09:27Z

Tests/RxCocoaTests/UITextField+RxTests.swift

    override var text: String? {
        get {
            return super.text
        }
        set {
-            set = true
+            isSettedText = true


This has the same grammar issue.

kzaher · 2017-05-18T11:10:05Z

Tests/RxCocoaTests/UITextView+RxTests.swift

+    var textViewAttributedString: NSAttributedString {
+        let tf = UITextView()
+        tf.attributedText = NSAttributedString(string: self)
+        return tf.attributedText!


The same problem with redundant extension.

Same explanation than TextField case

kzaher · 2017-05-18T11:10:21Z

Tests/RxCocoaTests/UITextView+RxTests.swift


+final class UITextViewSubclass2 : UITextView {
+    var isSettedText = false
+    var isSettedAttributedText = false


This has the same grammar issue.

kzaher

Hm. I think some of the previous comments weren't addressed, and there are some new ones.

sergdort · 2017-07-15T18:55:03Z

RxCocoa/iOS/UITextField+Rx.swift

+    /// Bindable sink for `attributedText` property.
+    public var attributedText: UIBindingObserver<Base, NSAttributedString?> {
+        return UIBindingObserver(UIElement: self.base) { textField, attributedText in
+            if textField.attributedText != attributedText {


Hm. Why do we need this check?

kzaher requested changes May 13, 2017

View reviewed changes

kzaher reviewed May 18, 2017

View reviewed changes

kzaher requested changes May 18, 2017

View reviewed changes

sergdort reviewed Jul 15, 2017

View reviewed changes

carlosypunto and others added 4 commits October 1, 2017 13:45

Add rx.attributedText to UITextView and UITextField

5500d1f

fix attributtedString: end of range - don’t emit next if equal an tests

a492cdd

fix variable names

355db94

fix test

9923b8c

kzaher force-pushed the addAttributedString branch from 9147175 to bd3e2d5 Compare October 1, 2017 11:46

kzaher changed the base branch from develop to rxswift4.0-swift4.0 October 1, 2017 11:47

kzaher force-pushed the addAttributedString branch 2 times, most recently from 9dfb3e6 to d2ccc22 Compare October 1, 2017 12:43

Polishes attributedText.

981b98a

kzaher force-pushed the addAttributedString branch from d2ccc22 to 981b98a Compare October 1, 2017 12:45

kzaher merged commit c29e8f9 into rxswift4.0-swift4.0 Oct 1, 2017

kzaher deleted the addAttributedString branch October 1, 2017 17:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rx.attributedText to UITextView and UITextField #1249

Add rx.attributedText to UITextView and UITextField #1249

carlosypunto commented May 12, 2017

RxPullRequestBot commented May 12, 2017 •

edited

Loading

kzaher May 13, 2017

carlosypunto May 21, 2017

kzaher May 13, 2017

carlosypunto May 21, 2017

kzaher May 13, 2017

kzaher May 13, 2017

carlosypunto May 21, 2017

kzaher May 13, 2017

kzaher May 13, 2017

carlosypunto commented May 14, 2017 •

edited

Loading

kzaher commented May 15, 2017

kzaher May 18, 2017

kzaher May 18, 2017

kzaher May 18, 2017

carlosypunto May 21, 2017

kzaher May 18, 2017

kzaher May 18, 2017

carlosypunto May 21, 2017

kzaher May 18, 2017

kzaher left a comment

sergdort Jul 15, 2017

Add rx.attributedText to UITextView and UITextField #1249

Add rx.attributedText to UITextView and UITextField #1249

Conversation

carlosypunto commented May 12, 2017

RxPullRequestBot commented May 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosypunto commented May 14, 2017 • edited Loading

kzaher commented May 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzaher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RxPullRequestBot commented May 12, 2017 •

edited

Loading

carlosypunto commented May 14, 2017 •

edited

Loading