ReactiveX / RxSwift

Reactive Programming in Swift
MIT License
24.38k stars 4.17k forks source link

Japanese text would be broken if UITextField has two-way-binding ControlProperty and Variable. #649

Closed tarunon closed 8 years ago

tarunon commented 8 years ago

I use two-way-binding operator UITextField.rx_text and Variable like RxExample. https://github.com/ReactiveX/RxSwift/blob/master/RxExample/RxExample/Examples/APIWrappers/APIWrappersViewController.swift#L142-L149

And please see this capture.  I want to input "あいうえお" at the textField, but we can see a wrong text "ああいあいうあいうえあいうえお". I believe ControlProperty should not receive event about the value inputted from user, because textField would lost the invisible state when set the same text.

And UITextView has the nearly same problem. In UITextView, we cannot use IME character conversion.

This problem would occur in every language that has IME.

I believe the easy way to solve this problem is change the implements of two-way-binding operator. But... I think ControlProperty(or rx_value) might check the observed value is not equal to a value inputted from user.

omochi commented 8 years ago

I am a japanese developer also faced same problem. I have not looked into what is happening in deeply. But I found that I can avoid the problem by blocking event which is comming from ControlProperty itself via Variable. So I made this extension and be using it now. (But it may occurs another problem in some specific cases.) Of course I am happy if it is fixed in RxSwift. This is just information for others with this problem.

extension ControlProperty {
    func cyclePrevented() -> ControlProperty<PropertyType> {
        var fromControl = false
        let observable = Observable<PropertyType>.create { observer in
            let sub = self.subscribe { ev in
                fromControl = true

                observer.on(ev)

                fromControl = false
            }
            return sub
        }
        let observer = AnyObserver<PropertyType> { ev in
            if !fromControl {
                self.on(ev)
            }
        }
        return ControlProperty(values: observable, valueSink: observer)
    }
}

user.name <-> userNameField.rx_text.cyclePrevented()
sergdort commented 8 years ago

Hi, guys

Looks like this is related to #626

I removed distinctUntilChanged in the UIControl.rx_value and it fixed the problem for me

kzaher commented 8 years ago

Thnx for investigation :+1: @sergdort

@tarunon @omochi I've pushed all changes we had to master branch. It would be great if you could verify that current master branch fixes your issue before we create another release.

tarunon commented 8 years ago

Thank you @omochi @sergdort @kzaher

But this problem would occur in latest master branch still... I believe this problem is caused in ControlProperty. In ControlProperty, If it have two-way-binding with Variable, _valueSink receive event that is observed from own _value.

UITextField has a state outside of UITextField.text while we input Japanese. I name that is undetermined text.

For example, I explain the case of I input "あ" in UITextField. UITextField.text is "あ", but it is the undertermined text. And input next character "い", UITextField would replace undetermined text "あ" to new text "あい". And UITextField lose undetermined text when set new text programatically.

In this case, we input "あ" first, then ControlProperty set new text, and UITextField lose undetermined text "あ". We input "い" second, UITextField lost the undetermined text "あ", but new text is "あい". Finally, UITextField would set new text "あ" + "あい".

I think @omochi 's extension is work well, but there is no check the event is realy from ownself.

omochi commented 8 years ago

I try to explain more detail of @tarunon said. UITextField have internal two variable which are determined text and undetermined text. This treats .text property as their concatenation. If we update .text property, it updates determined text and clear undetermined text. The keyboard remembers previous text user inputted and recover this from memory when user input next character. This behavior is need to conversion such like "あい"(hiragana) to "愛"(kanji). For evidence, "愛" is shown in conversion candidates on top of keyboard in his GIF movie.

So his scenario is able to written as below.

First state. (.text: "", det: "", undet: "", keyboard: "") When user inputs "あ", keyboard set undet text to textview. (.text: "あ", det: "", undet: "あ", keyboard: "あ") .text is "あ", so ControlProperty emits "あ" A emitted "あ" loopbacks and set to .text. In this point, the keyboard reminders previous "あ" but does not apply it to textview until user changes keyboard state. (.text: "あ", .det: "あ", undet: "", keyboard: "あ") Then user inputs "い", keyboard updates textviews undet text. (.text: "ああい", .det: "あ", undet: "あい", keyboard: "あい")

@tarunon Is my understanding same to you?

Succete commented 8 years ago

I try it too. And I found an interesting result.

first case:

textField.rx_text
            .debug("text")
            .subscribeNext { text in
                print(text)
            }
            .addDisposableTo(disposeBag)

It's true.

second case

let textValue = Variable("")
        textField.rx_text <-> textValue

        textValue.asObservable()
            .subscribeNext { text in
                print(text)
            }
            .addDisposableTo(disposeBag)

        textField.rx_text
            .subscribeNext { text in
                print(text)
            }
            .addDisposableTo(disposeBag)

they are both false.

and there are snapshots first case simulator screen shot 2016 4 28 11 49 50 second case simulator screen shot 2016 4 28 11 50 28

It seems that in first case keyboard is waiting my confirm, but second case not. I think maybe the <-> operator is the reason for the bug. let bindToUIDisposable = variable.asObservable() .bindTo(property) this code bind variable's value to property. When I type "あ", the value has been set to property(but it should wait for my confirm before it is set).

tarunon commented 8 years ago

@omochi Thanks for your explain, yes it is just my thinking.

@Succete Yes, I believe we can fix two-way-binding operator, and It is easy way to solve this problem. On the other hand, I worry about specification of ControlProperty. "ControlProperty should (or not) receive event that is notified ownself."

omochi commented 8 years ago

@Succete Thank you for a suggestive report. I was able to reproduce it, and tap to "愛" in conversion candidates. Then fields presents "ああい愛". Keyboard may replace undetermined text in UITextView to its determined conversion result in ordinary cases. If we set UITextView.text, undetermined text is cleared and selecting with blue shadow is reset. So conversion result is just appended without replacing.

Succete commented 8 years ago

@tarunon @omochi Thank you guys for replying.

Succete commented 8 years ago

I think I know how to solve it. It's not the two way binding that need to be changed. this is the problem

public var rx_nomarkedtext: ControlProperty<String> {
        return UIControl.rx_value(
            self,
            getter: { textField in
                textField.text ?? ""
            }, setter: { textField, value in
                if textField.markedTextRange == nil {
                    textField.text = value
                }
            }
        )
    }

then it's correct

let textValue = Variable("")
        textField.rx_nomarkedtext <-> textValue

        textValue.asObservable()
            .debug("value")
            .subscribeNext { text in
                print(text)
            }
            .addDisposableTo(disposeBag)

        textField.rx_text
            .debug("rx_text")
            .subscribeNext { text in
                print(text)
            }
            .addDisposableTo(disposeBag)
kzaher commented 8 years ago

Hi guys,

That's an interesting discussion for sure :)

It seems to me that adding something like

/**
    Reactive wrapper for `text` property. Notifications are fired only when when there
    isn't any marked text.
    */
    public var rx_nonMarkedText: ControlProperty<String> {
        let textProperty = self.rx_text
        return ControlProperty(
            values: textProperty._values.filter { [weak self] _ in self?.markedTextRange == nil },
            valueSink: textProperty._valueSink
        )
    }
         let textValue = Variable("")
        textField.rx_nonMarkedText <-> textValue

        textValue.asObservable()
            .subscribeNext { [weak self] x in
                self?.debug("UITextField text \(x)")
            }
            .addDisposableTo(disposeBag)

would solve your problems. I don't understand the full culture/language context so if you could provide feedback, that would be great.

Succete commented 8 years ago

Hi @kzaher , it's great. I've tried all Chinese and Japanese Keyboards, and it works fine. I think we can add it to UITextView too.

tarunon commented 8 years ago

Hi @kzaher Thank you for your great solution. It's seems solve this problem. I found a problem in this solution. In the case of save text, this solution is perfect. But in the case of query search using suggest list, the user might expect that they can get suggest list from undetermined text.

I think the best of solve this problem is using properly rx_text and rx_nonMarkedText.

kzaher commented 8 years ago

Hi @tarunon @Succete @omochi ,

yeah I was thinking of having two properties. Both for UITextField and UITextView.

The other one being

/**
    Reactive wrapper for `text` property. Notifications are fired only when when there
    isn't any marked text.
    */
    public var rx_nonMarkedText: ControlProperty<String> {
        let textProperty = self.rx_text
        return ControlProperty(
            values: textProperty._values.filter { [weak self] _ in self?.markedTextRange == nil },
            valueSink: textProperty._valueSink
        )
    }

so if we did that, would that make sense and solve all issues?

Succete commented 8 years ago

Hi @kzaher , I think that would solve all issues.

tarunon commented 8 years ago

Hi @kzaher , I think we can solve all issues. If possible, I recommend using properly rx_text(rx_nonMarkedText) and rx_markedText for convenience and safety.

/**
 Reactive wrapper for `text` property.
*/
private var rx_textInternal: ControlProperty<String> {
    return UIControl.rx_value(
        self,
        getter: { textField in
            textField.text ?? ""
        }, setter: { textField, value in
            textField.text = value
        }
    )
}

/**
 Reactive wrapper for `text` property. Notifications are fired only when when there
 isn't any marked text.
 */
public var rx_text: ControlProperty<String> {
    let textProperty = self.rx_textInternal
    return ControlProperty(
        values: textProperty._values.filter { [weak self] _ in self?.markedTextRange == nil },
        valueSink: textProperty._valueSink
    )
}

/**
 Reactive wrapper for `text` property. Notifications are fired only when when there
 is marked text.
 */
public var rx_markedText: ControlProperty<String> {
    let textProperty = self.rx_textInternal
    return ControlProperty(
        values: textProperty._values.filter { [weak self] _ in self?.markedTextRange != nil },
        valueSink: textProperty._valueSink
    )
}

I think we satisfy using rx_nonMarkedText in many cases, and we need rx_markedText in the limited cases.

omochi commented 8 years ago

@tarunon When we use one-way binding, there is no problem. The problem is in two-way binding only. And current rx_text behavior is useful for searching text field. But your proposal bring change of behavior in such existing code. And I don't want to merge your rx_text and your rx_markedText to recover original function. Its confusing.

@kzaher Your proposal can solve all problems if programmers know rx_nonMarkedText. I think that some Japanese RxSwift beginners (recently increasing) will step on trap of rx_text. (like me) It is better that default option (rx_text) is safer than specific option (rx_nonMarkedText). But as noted above, changing rx_text brings another problem. So I propose to add information about rx_nonMarkedText in summary source comment on rx_text.

tarunon commented 8 years ago

@omochi Yes, I can understand you say. But, UITextView has same problem, and now, we must not use two-way binding at rx_text and any Variable. I think it is bug, and we can fix it. I recommended using rx_markedText a few days ago. But now I think rx_markedText type might be Observable. (Because we would not use two-way binding).

Sorry I misunderstood PublishSubject. It is not appropriate in this case.

kzaher commented 8 years ago

Hi guys,

I'm still thinking how to best handle this. What ever solution I come up with, it has it's drawbacks.

Any solution is this form

public var rx_nonMarkedText: ControlProperty<String>
public var rx_markedText: ControlProperty<String>

I've thought about solutions in this form

public var rx_textAndMarkedRange: ControlProperty<(text: String, markedRange: UITextRange?)>

Then I've thought about solutions in this form

public var rx_textAndMarkedRange: ControlProperty<(text: String, markedRange: NSRange?)>

Then I thought, well ok, why don't we just do

public var rx_textFromContainer: ControlProperty<UITextInput>

My suggestion would be ...

I'm not too happy with what I'm about to say, but wouldn't it make sense if we would just add that convenience in front of <-> operator.

    protocol RxTextInput {
         var rx_text: ControlProperty<String> { get }
    }
    extension UITextField : RxTextInput {
    }
    extension UITextView: RxTextInput {
    }
    extension RxTextInput {
    /**
    Reactive wrapper for `text` property. Notifications are fired only when when there
    isn't any marked text.
    */
    public var rx_nonMarkedText: ControlProperty<String> {
        let textProperty = self.rx_text
        return ControlProperty(
            values: textProperty.asObservable().filter { [weak self] _ in self?.markedTextRange == nil },
            valueSink: textProperty.asObserver()
        )
    }
    }

// Two way binding operator between control property and variable, that's all it takes (well, almost) {

infix operator <-> {
}

func <-> <T>(property: ControlProperty<T>, variable: Variable<T>) -> Disposable {
    let bindToUIDisposable = variable.asObservable()
        .bindTo(property)
    let bindToVariable = property
        .subscribe(onNext: { n in
            variable.value = n
        }, onCompleted:  {
            bindToUIDisposable.dispose()
        })

    return StableCompositeDisposable.create(bindToUIDisposable, bindToVariable)
}

// }

We don't bundle that <-> operator anyway, so if you are copy pasting, it's not too hard to copy paste a couple of lines more.

What we could include in the RxCocoa project is maybe just this part

    protocol RxTextInput {
         var rx_text: ControlProperty<String> { get }
    }
    extension UITextField : RxTextInput {
    }
    extension UITextView: RxTextInput {
    }

so you would just need to write

extension RxTextInput {
    /**
    Reactive wrapper for `text` property. Notifications are fired only when when there
    isn't any marked text.
    */
    public var rx_nonMarkedText: ControlProperty<String> {
        let textProperty = self.rx_text
        return ControlProperty(
            values: textProperty.asObservable().filter { [weak self] _ in self?.markedTextRange == nil },
            valueSink: textProperty.asObserver()
        )
    }
    }

// Two way binding operator between control property and variable, that's all it takes (well, almost) {

infix operator <-> {
}

func <-> <T>(property: ControlProperty<T>, variable: Variable<T>) -> Disposable {
    let bindToUIDisposable = variable.asObservable()
        .bindTo(property)
    let bindToVariable = property
        .subscribe(onNext: { n in
            variable.value = n
        }, onCompleted:  {
            bindToUIDisposable.dispose()
        })

    return StableCompositeDisposable.create(bindToUIDisposable, bindToVariable)
}

// }
omochi commented 8 years ago

@kzaher

There are 2 major problems. One is not solved at all.

First, Japanese developers need to handle this issue. Yes, we can address it with various ways such like shown in this thread. In this angle, your opinion is good to keep things simply. But, there is a lot of document about usage of RxSwift in internet written by Non-Japanese developers. They don't know about this issue so documents may include this issue. Some Japanese developer will take their time to fight and solve it.

Second, Japanese user of application made with RxSwift by Non-Japanese developers can not use it correctly. This issue of input process is very critical as user can not input they want at all. Please imagine if this happen, Japanese users may report issue to developer of app. But it is difficult to understand and debug about problem for Non-Japanese developer (they may does not know about input conversion at all) and also difficult to report problem for Japanese non-developer users with English. If one developer understand topic and solve it, others does not relate. So this hard report and handle story may occurs multiple places for each apps. So we need solve this at root of this, in RxSwift itself. not in individual app developers.

Sorry for my lack of best solution proposal and many negative comments. This thread looks at first problem mainly. But I think this second problem is more important than first actually. Because it has possibility that cause more unhappiness.

kzaher commented 8 years ago

Hi @omochi ,

Thnx for expressing your concerns. I think you are right and we haven't considered problem 2 sufficiently.

Yes, it looks like not only Japanese developers need to take care of that, but all developers. Because if you have app built in Croatia that some Japanese user is using with that type of keyboard, it will fail for them also.

We also agree that I would like to improve situation regarding IME methods.

I don't think that we can change behavior of rx_text just by adding marked text filters like I've tried to explain before:

I think there are two different problems that we should answer.

I don't currently see any way how to solve these two points with any solution, no matter how complex.

What I was maybe thinking of doing is:

I'm not sure it is possible for us to control what people write on the internet, or create an API so that how ever people use it, it just works in all cases.

I'm open to suggestions as always, but currently I don't see how we can solve this in the rx_text extension itself.

kzaher commented 8 years ago

This should work for second part of the problem :)

extension UITextField {

    /**
    Reactive wrapper for `text` property.
    */
    public var rx_text: ControlProperty<String> {
        return UIControl.rx_value(
            self,
            getter: { textField in
                textField.text ?? ""
            }, setter: { textField, value in
                if textField.text != value {
                    textField.text = value
                }
            }
        )
    }
}
kzaher commented 8 years ago

Changes that fix this problem have been release. Feel free to reopen this issue if needed.

apinhopt commented 5 years ago

Hey there guys,

I have the same issue when binding to a BehaviourRelay. The Japanese highlighting stops working.

let value: BehaviorRelay<String> = .init(value: "")

self.textField.textField?.rx.text.orEmpty
            .bind(to: self.value)
            .disposed(by: self.disposeBag)

//same for this
 self.textField.textField?.rx.text.orEmpty.asDriver()
            .drive(onNext: { (value) in
                self.value.accept(value)
        })
        .disposed(by: disposeBag)