Brainstorm improvements to SubModelSelectedItem

[TysonMN]

This is a spinoff of #188

Quoting #188 (comment)

I don't think that these are serious, but they are annoying enough that I would prefer if we could keep subModelSelectedItem while also removing its downsides.

So, what are its downsides?

The main two downsides I see are:

1. Lack of safety;
2. Makes recursive bindings more difficult to implement.

The first issue is that subModelSelectedItem refers to a subModelSeq by name, which might not exist during initialization.

Elmish.WPF/src/Elmish.WPF/ViewModel.fs

Line 394 in c9b6fb5

log "subModelSelectedItem binding referenced binding '%s', but no compatible binding was found with that name" d.SubModelSeqBindingName

In the following example (from the SubModelSelectedItem sample), that name is Entities.

Elmish.WPF/src/Samples/SubModelSelectedItem/Program.fs

Lines 32 to 40 in c9b6fb5

	"Entities" |> Binding.subModelSeq(
	(fun m -> m.Entities),
	(fun e -> e.Id),
	(fun () -> [
	"Name" |> Binding.oneWay (fun (_, e) -> e.Name)
	"SelectedLabel" |> Binding.oneWay (fun (m, e) -> if m.Selected = Some e.Id then " - SELECTED" else "")
	]))
	"SelectedEntity" |> Binding.subModelSelectedItem("Entities", (fun m -> m.Selected), Select)

The second issue is that initializeBinding takes a function getInitializedBindingByName so that the initialized SubModelSeq binding can be obtained when initializing the SubModelSelectedItem binding.

Elmish.WPF/src/Elmish.WPF/ViewModel.fs

Line 219 in c9b6fb5

let initializeBinding name bindingData getInitializedBindingByName =

Elmish.WPF/src/Elmish.WPF/ViewModel.fs

Lines 382 to 384 in c9b6fb5

	| SubModelSelectedItemData d ->
	match getInitializedBindingByName d.SubModelSeqBindingName with
	| Some (SubModelSeq b) ->

---

My idea to address both of these downsides is to "combine" these bindings into one compound binding.

From the user's perspective, it would look something like

("Entities", SelectedEntity) |> Binding.subModelSeqAndSelectedItem( 
   (fun m -> m.Entities), 
   (fun e -> e.Id), 
   (fun () -> [ 
     "Name" |> Binding.oneWay (fun (_, e) -> e.Name) 
     "SelectedLabel" |> Binding.oneWay (fun (m, e) -> if m.Selected = Some e.Id then " - SELECTED" else "") 
   ]),
   (fun m -> m.Selected),
   Select)

There very well might be some syntactic improvements possible there. The main thing that I am trying to suggest there is, compared to the current approach, that both the SubModelSeq binding and the SubModelSelectedItem binding are defined by the user at the same time. This would roughly correspond to changing...

Elmish.WPF/src/Elmish.WPF/Binding.fs

Lines 112 to 123 in c9b6fb5

	and internal BindingData<'model, 'msg> =
	| OneWayData of OneWayData<'model, obj>
	| OneWayLazyData of OneWayLazyData<'model, obj, obj>
	| OneWaySeqLazyData of OneWaySeqLazyData<'model, obj, obj, obj>
	| TwoWayData of TwoWayData<'model, 'msg, obj>
	| TwoWayValidateData of TwoWayValidateData<'model, 'msg, obj>
	| CmdData of CmdData<'model, 'msg>
	| CmdParamData of CmdParamData<'model, 'msg>
	| SubModelData of SubModelData<'model, 'msg, obj, obj>
	| SubModelWinData of SubModelWinData<'model, 'msg, obj, obj>
	| SubModelSeqData of SubModelSeqData<'model, 'msg, obj, obj, obj>
	| SubModelSelectedItemData of SubModelSelectedItemData<'model, 'msg, obj>

...by replacing SubModelSelectedItem with something like SubModelSeqAndSelectedItem.

To be clear, there is no corresponding change to

Elmish.WPF/src/Elmish.WPF/ViewModel.fs

Lines 89 to 101 in c9b6fb5

	and internal VmBinding<'model, 'msg> =
	| OneWay of OneWayBinding<'model, obj>
	| OneWayLazy of OneWayLazyBinding<'model, obj, obj>
	| OneWaySeq of OneWaySeqBinding<'model, obj, obj, obj>
	| TwoWay of TwoWayBinding<'model, 'msg, obj>
	| TwoWayValidate of TwoWayValidateBinding<'model, 'msg, obj>
	| Cmd of CmdBinding<'model, 'msg>
	| CmdParam of cmd: Command
	| SubModel of SubModelBinding<'model, 'msg, obj, obj>
	| SubModelWin of SubModelWinBinding<'model, 'msg, obj, obj>
	| SubModelSeq of SubModelSeqBinding<'model, 'msg, obj, obj, obj>
	| SubModelSelectedItem of SubModelSelectedItemBinding<'model, 'msg, obj, obj, obj>
	| Cached of CachedBinding<'model, 'msg, obj>

Instead, initializeBinding changes by returning a sequence of bindings instead of an optional binding (i.e. at most one binding). The sequence would contain exactly one binding in every case except for SubModelSeqAndSelectedItem when it returns a sequence containing exactly two bindings.

---

I think that this approach removes both of the major drawbacks with SubModelSelectedItem. If it seems like a good idea, then I could try to implement this approach (making various decisions along the way I as I see fit) as a proof of concept.

Are there any other drawbacks that I missed?

Does anyone have any other ideas for improving SubModelSelectedItem?

[BentTranberg]

After I started using subModelSeq, I noticed that SelectedItem, SelectedItems, and various events provided something like - can't remember exactly, but I'm guessing - ViewModel2'<object,object>. And in there somewhere, through the VS debugger, I found e.g. ListBoxItem, the Model and the SubModel.

So I've been wondering why these aren't exposed. I don't doubt there can be really good reasons for that.

I wonder whether a more generic mechanism can be made, that can be used to handle SelectedItem and SelectedItems if they turn out to be the only options, and also properties like IsSelected and events like Selected and Unselected from the sub view. I wonder whether other properties or events exists that will need to be handled in similar ways.

Just loose thoughts, not opinions or propositions. The reason for bringing this up here, is that it may affect whether work is put into improving SubModelSelectedItem as is, or another solution is considered.

[cmeeren]

Just want to let you guys know that I won't have a chance to look at all the recent activity in the repo until this weekend at the earliest, potentially not before the next weekend.

[BentTranberg]

@cmeeren (edit: and @bender2k14), I have no issues that needs resolving, so please don't feel any pressure from me. Just throwing in my two cents worth of thoughts here, but I'll stay quiet for a while now I think.

[TysonMN]

I think that this approach removes both of the major drawbacks with SubModelSelectedItem.

Ah, not exactly. It makes the recursion of initializeBinding more complicated because a list of VmBinding<'model, 'msg> is returned instead of an optional one.

[cmeeren]

Some quick thoughts:

1. Lack of safety;

While I'd ideally like it to be more safe, it must also be balanced against how simple the public API is, and how likely the API is to cause significant problems. I like the idea of not being able to create invalid bindings, but with a new binding that combines subModelSeq and subModelSelectedItem it seems that the API becomes less orthogonal. All the complexities of subModelSeq - easily the most complex binding - will also become part of the new binding. Granted, they are exactly the same complexities, so if you know one, you know the other. Still, I'd say the API is simpler as it is now.

A possible solution is to extend the subModelSeq binding with a new optional parameter for the selected item name. If it's not supplied, it works like it does today. If it's supplied, it works like you suggest. I think this solution would be the least complex to understand. A downside is that the name can not be passed using ("a", "b") |> Binding.subModelSeq.

Another option is of course to just keep subModelSelectedItem as today and decide that the dangers it poses are not likely to cause actual issues. AFAIK no users have complained yet.

2. Makes recursive bindings more difficult to implement.

Ah, not exactly. It makes the recursion of initializeBinding more complicated because a list of VmBinding<'model, 'msg> is returned instead of an optional one.

I'll let you decide what's more or less difficult here. :) Note that, as a general policy, I'd rather not force a worse API on users (if it turns out to be worse) in order to make internals easier, unless it's a significant gain in the internals for a small loss in the public API.

[cmeeren]

For the record, #174 (comment) is relevant here. The controls to be supported may influence the implementations.

[TysonMN]

That's true. For the sake of simplicity though, I think is reasonable to only consider the current implementation for the purposes of this issue.

[cmeeren]

This hasn't seen any activity for a while. Also, I still haven't received any complaints from users regarding the current subModelSelectedItem. Should we close this and keep the current implementation, or do you @bender2k14 want to continue working on ideas here?

[TysonMN]

I am in no rush to change the current implementation, and we can close this. I will comment here if I have any new thoughts.