Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Best practices for mutable objects in model? #113

Closed
reinux opened this issue Sep 12, 2019 · 31 comments
Closed

Best practices for mutable objects in model? #113

reinux opened this issue Sep 12, 2019 · 31 comments

Comments

@reinux
Copy link

reinux commented Sep 12, 2019

I'm working with a text/code editor control called AvalonEdit, which has a TextDocument class that contains all the text as well as methods for manipulating the text and events for changes to the text/filename/etc.

I thought about just wrapping it in a control along with the editor, and binding to a collection of lines, but it turns out there's no way to two-way bind to a collection, which is sensible.

So now I'm putting the TextDocument inside the model, which means my model is now mutable. I have a few questions about this:

  • What are the caveats to having a mutable object in the model?
  • How do I subscribe to events coming from an object in the model?
  • Is this the right approach overall?

Thanks in advance.

@cmeeren
Copy link
Member

cmeeren commented Sep 12, 2019

So now I'm putting the TextDocument inside the model

Ideally the model and the update function would be agnostic to particular UI technologies. If a single model/update is able to drive e.g. Fable.React and console apps as well as WPF, you're doing very well.

What are the caveats to having a mutable object in the model?

I haven't checked out AvalonEdit, so I can't comment directly on that. But in general, mutable objects in the model should be avoided (that is, objects which are actually mutated, e.g. not array manipulated immutably with Array module functions).

  • Testing is more difficult
  • Memoization and lazy bindings may not work due to reference equality checks no longer being sufficient to determine if something has changed
  • One of the main benefits of Elmish is the simple unidirectional architecture, and having complex mutable/event-based classes flies completely in the face of that
  • I've probably forgot something

You may not care about the first point, and you may be able to work around the second using appropriate lazy binding overloads with a custom equality. But in general I would strongly discourage mutating the model.

How do I subscribe to events coming from an object in the model?

I'm having as much trouble wrapping my head around this as I would making toast with a hammer. Like, I'm sure it's possible (use a torch to heat the hammer?), but I haven't given much thought to it and I think and hope there are better ways of solving the problem. Also see the point above regarding simplicity.

Is this the right approach overall?

Again, I haven't investigated AvalonEdit, but in general, no.


In general, anything designed to be MVVM-friendly (using data bindings) should be compatible with Elmish.WPF. It may seem like AvalonEdit requires a complex class (TextDocument) to be in the view model (or indeed, that TextDocument perhaps is the view model, supplied by the control library). If that's the case, I'm not sure AvalonEdit is a good choice for Elmish.WPF.

@reinux
Copy link
Author

reinux commented Sep 12, 2019

Thanks for the quick reply.

I've managed to get a subscription to the TextDocument events in the model using Cmd.ofSub and wiring that through to init in Program.mkProgramWpf.

It seems like it should work fine, but if there are unforeseen risks, I should probably abandon that idea.

And yeah, the TextDocument is the model for the editor control.

I guess I could wrap the TextDocument along with the control and expose an event that would pass an immutable model that mirrors the document any time something changes. The wrapper could get a bit hairy, but at least then I would be able to keep the MVU section fairly untainted, I think?

@cmeeren
Copy link
Member

cmeeren commented Sep 12, 2019

I think what I'd try is to create a custom control that wraps the UI stuff and the TextDocument, and from that wrapper control, only surface bindable properties. Convert between these properties and the TextDocument's setters and events in the control's code. Then, from the point of view of Elmish.WPF (and the XAML usage), the control is just like any other MVVM-friendly control.

@reinux
Copy link
Author

reinux commented Sep 12, 2019

I think the only issue with that approach is that in order to avoid having to work with a string that's potentially thousands of lines of text long any time the user hits a key, I have to be able to expose a bindable collection, and as far as I know, there isn't a way to bind from a collection in the view to a collection in the model.

@cmeeren
Copy link
Member

cmeeren commented Sep 12, 2019

I'd try it before passing judgement on the performance. But if you want to bind to each line separately, you can probably do it with Binding.subModelSeq.

@reinux
Copy link
Author

reinux commented Sep 12, 2019

I'm not sure I've grasped subModelSeq 100% yet, but it doesn't look like there's any way to add or remove items from the binding to the model?

I've actually tried re-parsing entire documents before, and it gets too sluggish, which is why I do individual lines. I guess I could do some heavy-duty diffing, but that could potentially get even more complicated.

@cmeeren
Copy link
Member

cmeeren commented Sep 12, 2019

Adding/removing items is possible but not from within a "sub-model" itself; it must be done at the level where you have the subModelSeq binding. Check out the SubModelSeq sample in this repo for an example. (It's recursive, so a bit more complicated, but hopefully it can be useful.)

Each text line (i.e. sub-model) would have to have an ID, so I'm unsure how you can solve that. Line number / index is probably not a good idea, but I can't think of anything else.

@JDiLenarda
Copy link
Contributor

I had a case with Live-Charts that looks like yours. Live-Charts comes with its own MVVM mutable objects and UI elements can only be bound to them rather than to raw collections. In my case, I conventionally choose to use a list in my model and deal with the MVVM class (a module level instance) in my binding function (creation and synchronization, which was fortunately easy since this MVVM class seems to update its points cleverly — and there were load of points). Lazy bindings have been helpful for performance, though. (Also, It was just for displaying after loading them, no modifications.)

After a fast overview of AvalonEdit, my approach would be to create an app level instance of TextDocument reachable by your bindings and cmds functions and deal with it the OOP-way it is intended to be used.

  • I’d let the control and the mutable model deal with the user input. I don’t think rewriting the logic it embeds is worth the hassle.
  • I’d subscribe to the Changing or Changed event to update the text as a simple string in the elmish model. I suppose you need it in the model for loading or saving it, but the same could be said about the model : no need to double it for the sake of having a whole immutable copy of it.
  • If I have, say, a « Erase the 13th line » button, I would use a cmd calling myTextDocument.RemoveLine(12) rather than modifying the elmish model. That is : updating the elmish model from the mutable model rather than the opposite.
  • use lazy binding and update only if the elmish value is different from the mutable value, so as to avoid update loop feedback.

Of course, that’s unorthodox and if your app is nothing more than the text control alone, that make the elmish take-on rather moot. But if there’s more around, that would be still interesting.

@reinux
Copy link
Author

reinux commented Sep 13, 2019

I'm looking at the subModelSeq sample, but I can't quite see how I would use this to be able to update the model with new/removed lines from the UI element.

    "ChildCounters" |> Binding.subModelSeq(
      (fun (m, c) -> m |> childrenCountersOf c.Id),
      (fun ((m, _), childCounter) -> (m, childCounter)),
      (fun (_, c) -> c.Id),
      snd,
      counterBindings)   // I'm guessing there'd have to be some trickery setting the child bindings?

my approach would be to create an app level instance of TextDocument reachable by your bindings and cmds functions and deal with it the OOP-way it is intended to be used.

That seems reasonable. I actually need the text in the model for a bunch of things, including undo/redo for the text and other state, parsing out each line to display individually with formatting both within the text editor and in the output control, and keeping track of various markers and anchors.

So I can do most of the massaging in a changing/changed handler, and just feed whatever I need back to Elm through a subscription. If I ever need the full text, I can just access it from the TextDocument directly.

I could probably just keep the undo stack in the control side as well, and just give each operation a sequence token to make sure everything happens in order.

This is for an application that'll be displaying text in soft real-time. It's not exactly a full-fledged RTF editor, but it's pretty involved, and all the VMs were starting to get a bit unwieldy. React can only do so much to wrangle all the state propagation, so Elmish's single-source-of-truth is really attractive.

@TysonMN
Copy link
Member

TysonMN commented Sep 13, 2019

Who is this program for? For example, is this a personal project or are you creating it at work as part of your job?

@reinux
Copy link
Author

reinux commented Sep 13, 2019

Personal project. Mostly just tinkering and having fun. I'll use it for work if I can get it to maturity, but we do have our own tools that work already.

@cmeeren
Copy link
Member

cmeeren commented Sep 13, 2019

I'm looking at the subModelSeq sample, but I can't quite see how I would use this to be able to update the model with new/removed lines from the UI element.

You can try to look at how the removal of the counters is handled.

In any case, I haven't looked at AvalonEdit, so this might not be feasible at all. Perhaps try the suggestion by @JDiLenarda?

@reinux
Copy link
Author

reinux commented Sep 14, 2019

Yeah, that seems to be the best approach. Haven't run into any horrible roadblocks yet.

Will post here if I notice anything that might be worth mentioning.

Thanks for all the input!

@JDiLenarda
Copy link
Contributor

@reinux : can we have a feedback, please ? I’m interested to know how you dealt with it.

@TysonMN
Copy link
Member

TysonMN commented Oct 3, 2019

The general feedback would be not to do it.

Why are you considering using mutuality mutability?

@JDiLenarda
Copy link
Contributor

I genuinely don’t get if it’s a technical opinion or if I’m told I broke some etiquette. If so, I apologize.

@TysonMN
Copy link
Member

TysonMN commented Oct 3, 2019

Oh, sorry. I thought you were requesting general feedback about having the model contain mutable data. I was stating a technical opinion. To be clear, you are welcome to ask any question. We will do our best to answer it.

@reinux, do you have an update that you can share about your use of mutability in the model?

@JDiLenarda
Copy link
Contributor

So you mean mutability, not mutuality ? This word lost me. No worry, though.

More specifically, I’d like to know how reinux dealt with its use of a third-party component.

@TysonMN
Copy link
Member

TysonMN commented Oct 3, 2019

Oh, yes. I corrected my "mutuality" for "mutability" typo. Thanks :)

@reinux
Copy link
Author

reinux commented Oct 3, 2019

@JDiLenarda I think I've stumbled on a few repeatable patterns for factoring mutable state out of the model and dealing with non-MVVM stuff. @bender2k14 I would love to have your input to know if I'm on the right track.

The editor and the document live happily in XAML now, and there are no direct references to it in the model or in update.

The general idea is that there are only a handful of reasons to interact with a mutable object, and only a handful of ways to deal with each:

  • You need to instantiate it. Do it in XAML if possible, bind to it if possible, but otherwise instantiate after the window constructor and use Program.withSubscription.
  • You need it to do something. That can be done with a Cmd.OfFunc, which can be given to the update function through a Msg constructor parameter when the object is initialized, and stored in the model for use later.
  • You need to react to something that happened on the mutable object. This is pretty straightforward: if Binding.twoWay won't work, add an event handler in Program.withSubscription. In many cases, if you need to call a method on the mutable object immediately in response, you can just pass in a Cmd.OfFunc at that point, use it in the update, and never bother storing it in the model.
  • You need to ask it something, i.e. you need a return value or a property value, and that value changes frequently. This is the hardest case. I think there are only two options:
    • Update the model each time there's a PropertyChanged or equivalent, which means a lot of updates and in the worst case, a lot of battery drain.
    • Update the model just before you need the value, by making a Cmd.OfFunc that'll do the query on the mutable and return a Cmd.batch that updates the model and then does the desired operation. That's two extra update/view calls. You also have to be aware that your model can potentially be stale, and you have to be careful not to call the Msg that does the operation without first updating it.

The last case was the big sticking point for AvalonEdit. I optimized the heck out of the update loop using lazy bindings. refEq is useful for this, since it's the fastest, and occasional false positives are relatively harmless, and false negatives will never occur as long as the field is immutable (i.e. use a list or map, not an array or dictionary).

Assuming this is the right way to think about the problem, I think a simple media player would be a good place to illustrate these ideas, since MediaElement has frequently updating state and isn't very MVVM-friendly. If there's interest, I think I can whip something up, and we can polish it into a useful sample.

Also, this is probably dangerous information, but it turns out that a giant performance bottleneck in WPF when you have a ton of elements bound to F# objects is accessibility, because it calls Object.Equals on everything, and F# does structural comparison by default. If you have even just a hundred items bound to single-field records, and something (Narrator, some UI automation program or, for whatever reason, Google Chrome) causes your program to load Accessibility.dll, certain controls, notably ListView, will slow to a crawl. Since my application will be of no use to the visually impaired anyway, I ended up using this workaround to disable it.

@BentTranberg
Copy link

BentTranberg commented Oct 4, 2019

I use DevExpress WPF components heavily, and have exactly these kind problems to solve, all the time. I mostly use EventToCommand and in a few cases also KeyToCommand in the XAML. There's also other tools from DevExpress, but there's no point mentioning them I believe.

xmlns:dxmvvm="http://schemas.devexpress.com/winfx/2008/xaml/mvvm"

    <dxmvvm:Interaction.Behaviors>
        <dxmvvm:EventToCommand EventName="Loaded" Command="{Binding WhenLoaded}"/>
        <dxmvvm:EventToCommand EventName="Closed" Command="{Binding WhenClosed}"/>
        <dxmvvm:KeyToCommand KeyGesture="F7" Command="{Binding ShowStopTime}"/>
    </dxmvvm:Interaction.Behaviors>

These are DevExpress specific helpers, but I believe similar functionality exists in some Microsoft library that I can't remember the name of right now, but it deals with MVVM. Also elsewhere I would guess it can be found.

@reinux
Copy link
Author

reinux commented Oct 4, 2019

That looks to be comparable to Microsoft.Xaml.Behaviors.

                <Button x:Name="button" Content="Click Me" HorizontalAlignment="Stretch" Grid.Row="1" Margin="0,10,0,10">
                    <Behaviors:Interaction.Triggers>
                        <Behaviors:EventTrigger EventName="Click" SourceObject="{Binding ElementName=button}">
                            <Behaviors:InvokeCommandAction Command="{Binding ColorCommand}" CommandParameter="{Binding Grid.Background}" />
                        </Behaviors:EventTrigger>
                    </Behaviors:Interaction.Triggers>
                </Button>

@cmeeren
Copy link
Member

cmeeren commented Oct 4, 2019

false negatives will never occur as long as the field is immutable (i.e. use a list or map, not an array or dictionary).

In fact, it is sufficient that an object is never mutated. If you use Array.map etc. then it's no problem using an array.

Assuming this is the right way to think about the problem, I think a simple media player would be a good place to illustrate these ideas, since MediaElement has frequently updating state and isn't very MVVM-friendly. If there's interest, I think I can whip something up, and we can polish it into a useful sample.

If you think that can be a useful sample, you're more than welcome to create a PR! 😊

it turns out that a giant performance bottleneck in WPF when you have a ton of elements bound to F# objects is accessibility, because it calls Object.Equals on everything, and F# does structural comparison by default

How about not binding to F# records and using subModelSeq instead? Or alternatively only binding to specially designed F# records that use reference equality? I think that in general, it would not be wise to bind to domain objects if that's what was happening.

@reinux
Copy link
Author

reinux commented Oct 4, 2019

How about not binding to F# records and using subModelSeq instead? Or alternatively only binding to specially designed F# records that use reference equality? I think that in general, it would not be wise to bind to domain objects if that's what was happening.

Huh, that worked. I would never have suspected subModelSeq would be faster, since it's an additional layer, but I guess it makes sense that it would do equality faster. Are there other reasons not to use oneWaySeq for model objects?

@cmeeren
Copy link
Member

cmeeren commented Oct 4, 2019

To be honest I'm not sure exactly why I created oneWaySeq or in which situations it's faster than subModelSeq. If you just need to bind to a single value for each item, then it may be useful, but oneWaySeqLazy may improve performance.

In any case, If each item has several bindings, then in general I would always reach for subModelSeq first.

@reinux
Copy link
Author

reinux commented Oct 4, 2019

To be honest I'm not sure exactly why I created oneWaySeq or in which situations it's faster than subModelSeq. If you just need to bind to a single value for each item, then it may be useful, but oneWaySeqLazy may improve performance.

Ah, good to know.

Are there any plans for a subModelSeqLazy? Or I wonder if I could combine it with oneWayLazy somehow to achieve the same effect.

@cmeeren
Copy link
Member

cmeeren commented Oct 4, 2019

I literally just made an issue for it 😆 #143

I personally have no immediate plans to implement it. Let me know in that issue if you have measured subModelSeq to be a performance bottleneck and know you need a lazy version. It would make the issue a higher priority.

Not sure if combining with oneWayLazy would work. It'd be an ugly hack anyway.

@reinux
Copy link
Author

reinux commented Oct 4, 2019

Sweet :D

I'll have something working soon which will require 60fps rendering, with a 1000+ element list in the model which doesn't need to be updated all the time. I also happen to have a version that doesn't use Elmish, so I can get a pretty good comparison of real world-ish performance.

Will test it on my old-ish desktop and mid-range i5 Surface Book and get back to you.

But first, I'll get that MediaElement sample going.

@cmeeren
Copy link
Member

cmeeren commented Oct 4, 2019

I'll have something working soon which will require 60fps rendering, with a 1000+ element list in the model which doesn't need too be updated all the time. I also happen to have a version that doesn't use Elmish, so I can get a pretty good comparison of real world-ish performance.

That's very, very interesting. Looking forward to hearing about it!

@reinux
Copy link
Author

reinux commented Oct 11, 2019

So now that I have a better grasp of Elmish, the media player sample I've been working on seems weirdly trivial. I'll probably return to it sometime if I can think of a way to make it more compelling.

Meanwhile, having 1000+ items bound using subModelSeq turns out to be prohibitively expensive, taking several hundred milliseconds on my machine. The slowdown happens even if the sequence isn't actually bound to anything in the xaml. The update is fine.

Elmish.WPF.ViewModel``2[System.__Canon,System.__Canon]::updateValue is on the hottest path, specifically the Seq.find call. I've used an int64 for the ID, so it's not the eq that's taking long. Maybe the ObservableCollection can be paired with a dictionary or something?

For the time being, I'll just not use a binding for the giant list.

@cmeeren
Copy link
Member

cmeeren commented Oct 11, 2019

@bender2k14, perhaps some of that magical fairy dust in #137 could help significantly here?

Also probably relevant: #143. Tips/PRs welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants