elmish / Elmish.WPF

Static WPF views for elmish programs
Other
430 stars 71 forks source link

Non-nullable F# types not fully supported by Binding.twoWay? #75

Closed mbenoni7 closed 5 years ago

mbenoni7 commented 5 years ago

I have a rather basic scenario: I've bound a collection from my model to a ListView, and mirrored the ListView's SelectedItem to my model using Binding.twoWay.

The trouble begins when the ListView's selection is cleared, say by calling TheListView.UnselectAll() in the code behind.

When my collection is a collection of strings, there's no issue - the deselection will cause my twoWay binding's set function to be called, passing in null for the value. My set function converts the null to None and triggers the model update - all good.

When my collection is composed of F# records, however, my twoWay binding's setter is no longer called. "[VM] TrySetMember [BindingName]" is printed to the console, but that is the only indication that the deselection occurred.

Now, I can see why this might be an issue for the framework: what value is supposed to be passed to the binding's set function, given that null is not a valid value for an F# record type?

The only workaround I can see here is to make my F# types nullable (with [AllowNullLiteral]), or to wrap them in nullable wrappers (like Nullable[T]). Clearly this is sub-optimal from an F# perspective. Perhaps a new Binding flavor should be introduced that converts the implicitly nullable values coming from WPF into Option[T]s?

mbenoni7 commented 5 years ago

If code would help, this is very close to what I'm talking about. The twoWay binding's setter will never be called if the value coming from WPF is null (as in the case of deselection). (Note that in the binding's getter I'm returning an empty value on None since I can't pass null.)

type Rec =
    { value1 : string
      value2 : string }

type Model =
    { recs : Rec array
      selectedRec : Rec option }

let init () =
    { recs = [| { value1 = "a"; value2 = "a" }; { value1 = "b"; value2 = "b" } |]
      selectedRec = None }

type Msg =
    | UpdateSelectedRec of Rec option

let update msg model =
    match msg with
    | UpdateSelectedRec newRec ->
        { model with selectedRec = newRec }

let bindings _ _ =
    [
        "Recs" |> Binding.oneWay (fun m -> m.recs)
        "SelectedRec" |> 
            Binding.twoWay 
                (fun m -> m.selectedRec |> Option.getOrElse { value1 = ""; value2 = "" }) 
                (fun v _ -> UpdateSelectedRec (Some v))
    ]
cmeeren commented 5 years ago

F# records not having null as a valid value is only an F# compiler check. AFAIK there's no reason that should cause problems at runtime. I'll investigate what could cause this.

cmeeren commented 5 years ago

The way this works is that the view can accept and return a value that may or may not be null (if no item is selected). We can't return an option from the getter, because WPF has no idea what to do with an option. So we must return the raw underlying type. However, we can't just call Option.toObj directly, because the record type can't be null (and you can't add [<AllowNullLiteral>] even if you wanted to).

The solution

The solution, thankfully, is very simple - we just need to add Option.map box in the getter and Option.map unbox in the setter:

"SelectedRec" |> Binding.twoWay
  (fun m -> m.selectedRec |> Option.map box |> Option.toObj)
  (fun v _ -> v |> Option.ofObj |> Option.map unbox |> UpdateSelectedRec)

As you can see, this can be solved in terms of the existing twoWay function.

If you find yourself needing this a lot, you can even define your own binding helper for it:

module Binding =
  let twoWayOpt (get: 'model -> 'a option) (set: 'a option -> 'model -> 'msg) name =
    twoWay
      (get >> Option.map box >> Option.toObj)
      (Option.ofObj >> Option.map unbox >> set)
      name

Using this helper is as simple as using twoWay with non-option values:

"SelectedRec" |> Binding.twoWay
  (fun m -> m.selectedRec)
  (fun v _ -> UpdateSelectedRec v)

Should this be in the main library?

It's been a while since I've actively used Elmish.WPF, so I'd like a second opinion or two.

It seems to me that Elmish.WPF is currently missing built-in helpers for binding to option values (apart from Binding.subModelOpt). The twoWayOpt helper above could be added, but for completeness we'd also need oneWayOpt, oneWayLazyOpt, and perhaps also twoWayValidateOpt and twoWayIfValidOpt. Since they can all be defined in terms of the existing binding functions, there would be almost no added maintenance involved. However, I'm afraid of adding functionality that might end up pushing users toward antipatterns. I can't immediately see any downsides, but I haven't thought this through yet.

As I said, second opinions are welcome.


A side note

For your particular case, I don't recommend you keep the actual selected object in the model. What if the object needs to be updated while it is selected? Then you need to remember to update it both in recs and in selectedRec. Such state duplication is discouraged in Elm architectures. For more information, I recommend the talk Immutable Relational Data by Richard Feldman.

A better solution might be to use SelectedValue and SelectedValuePath instead of SelectedItem. Set SelectedValuePath to an identifier property for Rec (assuming there is one), and bind using SelectedValue instead. In your model, only keep the actual ID in selectedRec, not the whole Rec object. Then, whenever you need to grab the respective record, just look it up in Rec (which should probably be a Map<RecId, Rec> or some such).

And as a side note to this side note, unless you have measured and know you'll gain performance using array, I recommend using e.g. list or other immutable data structures. That makes you more sure that your model doesn't mutate under you.

mbenoni7 commented 5 years ago

Great answer, thanks.

Can't say whether it should be in the library, but the basic idea (how to correctly two-way bind an option) seems important enough to be in the FAQ. It feels like a very natural problem to run into when porting standard WPF/MVVM patterns into the Elmish world.

cmeeren commented 5 years ago

After a little bit of thought I've come to the conclusion that it should be in the library. Will add.

cmeeren commented 5 years ago

Released in 2.0.0-beta-9. I only added oneWayOpt and twoWayOpt since I'm not sure how much sense it makes for the lazy and validation bindings, and there are further design decisions there I can't really take a stand on without knowing of any concrete use-cases.