TextChanged is incorrectly fired when changing the model

[praeclarum]

The event is fired even though the user has not input anything when the model is switching.

type Model = 
    {
        ShowGreeting : bool
        Value : string
    }

type Msg = 
    | ShowGreeting
    | ShowValue
    | SetValue of string

let initModel = { ShowGreeting = false; Value = "text" }

let init () = initModel, Cmd.none

let update msg model =
    match msg with
    | ShowGreeting -> { model with ShowGreeting = true }, Cmd.none
    | ShowValue -> { model with ShowGreeting = false }, Cmd.none
    | SetValue v -> { model with Value = v}, Cmd.none

let view (model: Model) dispatch =
    let text = if model.ShowGreeting then "Hello, World!"
               else model.Value
    let onTextChanged (e : TextChangedEventArgs) =
        printfn "TEXT CHANGED from:%s to:%s" e.OldTextValue e.NewTextValue
        e.NewTextValue |> SetValue |> dispatch
    View.ContentPage(
      content = View.StackLayout(padding = 44.0,
        children = [ 
            View.Button(text = "Show Greeting", command = (fun () -> dispatch ShowGreeting))
            View.Button(text = "Show Value", command = (fun () -> dispatch ShowValue))
            View.Entry(text = text, textChanged = onTextChanged)
        ]))

Repro:

1. Run the given app
2. Note it says "text" because ShowGreeting = false
3. Tap "Show Greeting" and note correct text
4. Tap "Show Value" and not incorrect text it show "Hello, World!" instead of "text"

This is because the ShowGreeting forced a SetValue message. This can be seen in the trace:

Message: ShowGreeting
Updated model: {ShowGreeting = true; Value = "text";}
View, model = {ShowGreeting = true; Value = "text";}
View result: ContentPage(...)@-1009560494

TEXT CHANGED from:text to:Hello, World!  <-- Should not be called

Message: SetValue "Hello, World!"
Updated model: {ShowGreeting = true; Value = "Hello, World!";}
View, model = {ShowGreeting = true; Value = "Hello, World!";}
View result: ContentPage(...)@-1762026536

The bug is in UpdateEntry (and UpdateSearchBar, Editor). The Text property is set before the TextChanged event. This causes TextChanged to fire using old model data.

https://github.com/fsprojects/Fabulous/blob/a16e11eeadf6c70479b38315f31ffa7c65e0ff83/src/Fabulous.Core/Xamarin.Forms.Core.fs#L7508-L7564

To fix it, TextChanged should be unsubscribed from before setting Text. In general, this pattern should be followed for all PropertyChanged events (I have not audited the code). The event should first be unsubscribed, then the property set, then re-subscribed if needed.

I ran into this bug while building a data entry UI in my app.

[TimLariviere]

I would have expected the behavior to be correct. The text has changed, even if only programmatically.

But the more I think about it, the more I can see the benefits from preventing a TextChanged event to occur when setting a new value programmatically.

The developer loses the ability to factor logic for all text changes (from user or code), but he is still in full control and can update manually any state that needs updating. (especially true in MVU) At the same time, this modification might drastically reduce the number of events we fire.

Is there some kind of best practice regarding if a XXXChanged event should exclude updates from code?

[praeclarum]

@TimLariviere This isn't really a behavioral decision, it's pretty much just a bug. The wrong event handler is getting called. Even if you want to argue that programmatic (non-user) changes should count as a change, then it's still calling the wrong event handler (since it binds the handler after setting the text).

That said, I agree with you that programmatic changes should not bubble up this event in this architecture - it would be a complete waste.

For reference, implement the same code in Elm or React and you will see it behaving correctly.

Thanks for looking into it!

[praeclarum]

Given this architecture, the XXXChanged events should only fire on user input since updating the UI should be non-eventful.

^ see what I did there?

[TimLariviere]

Ah yes indeed, in the case the event handler and the text are set at the same time, the old event handler will be fired instead. We need to fix that.

I think we need to proceed in 2 steps:

• First: A quick fix for the event handler bug is to invert the event handler subscription and the update of its related property. (switch them in the JSON bindings file is enough) This way, when changing the Text property, the latest event handler will be used.

• Second: Update the generator to unsubscribe the event before setting the value, then resubscribe. This will change the current behavior and will only fire TextChanged (and others) when the user is inputting something, not when the value is changed by the app itself. We may need to wait for #296 before starting.

[willsam100]

I tested this manually by adding the following code

        match prevTextChangedOpt with
        | ValueSome prevValue -> 
            System.Diagnostics.Debug.WriteLine "Unsubscribing"    
            target.TextChanged.RemoveHandler(prevValue)
        | ValueNone -> ()  

above the exisitng code for the entry

        match prevTextOpt, currTextOpt with
        | ValueSome prevValue, ValueSome currValue when prevValue = currValue -> ()
        | _, ValueSome currValue -> 
            System.Diagnostics.Debug.WriteLine <| sprintf "Setting text to new value: %s" currValue
            target.Text <-  currValue
        | ValueSome _, ValueNone -> target.Text <- null
        | ValueNone, ValueNone -> ()

in Xamarin.Forms.Core.fs and can confirm this fixes the bug with the given demo.

The TextChanged handler is also subscribed to again as the generator already had the following code further down in the file.

        match prevTextChangedOpt, currTextChangedOpt with
        | ValueSome prevValue, ValueSome currValue when identical prevValue currValue -> ()
        | ValueSome prevValue, ValueSome currValue -> target.TextChanged.RemoveHandler(prevValue); target.TextChanged.AddHandler(currValue)
        | ValueNone, ValueSome currValue -> target.TextChanged.AddHandler(currValue)
        | ValueSome prevValue, ValueNone -> target.TextChanged.RemoveHandler(prevValue)
        | ValueNone, ValueNone -> ()

For the generator, it seems logical to code gen something to similar to the sample I have provided for every XXXChanged property. Inserting below the binding for each prevXXXChangedOpt item.

@praeclarum and @TimLariviere does this seem to right to you?

[TimLariviere]

@willsam100 Yes, seems good. Changing the generator shouldn't be hard. We need to differentiate the concept of events and properties (today it's only "members"); as well as introduce the concept of relation between an event and one or more properties.

A thing we can do to improve performance is to avoid unsubscribing/resubscribing for nothing in the case the handler and its related properties don't change.

[babusri]

Wish I had seen this ticket before spending lot of time debugging the issue.

I have a model which has an array of gliders and an enum called currentGlider. When the model changes from one glider to another, I get a textChanged message. By itself it shouldn't cause a problem except for unnecessary computation. But a weird thing happens. I have two variables pilotMass and baggageMass. When I switch models, I get a textChanged message UpdateBaggageMass with the value of pilotMass! This is a serious bug.

[babusri]

Note: I changed textChanged to completed, and it works fine. So, I have a good workaround.

[TimLariviere]

When I switch models, I get a textChanged message UpdateBaggageMass with the value of pilotMass! This is a serious bug.

@babusri Could you open another issue with a repro if possible, please?

[pauldorehill]

I think I've just run into this issue when using dependent pickers - I end up in an endless loop of messages when changing the first picker if there is a selected value in the second picker. I agree with the above comments that firing events on programmatic changes was somewhat unexpected.

Simplified example here: https://gist.github.com/pauldorehill/59546150443307ea110bb485760db9a5

[TimLariviere]

It's finally fixed in v0.50.0-alpha.2

[TimLariviere]

This will be released in v0.50.0 which will arrive in the coming days (once we add support for XF 4.3)