elmish / Elmish.WPF

Static WPF views for elmish programs
Other
428 stars 69 forks source link

Propositions for a next version - multi-agent application and dialog/window utilities #52

Closed JDiLenarda closed 5 years ago

JDiLenarda commented 6 years ago

As I said in #24, I wrote a version you can see here.

This development came in 2 steps :

For the first step, I wrote a function that can bind a UserControl to an Elmish program and modified ViewModel so its constructor take a WPF thread dispatcher as a parameter (the AutoCAD thread dispatcher can't be retrieved with Application.Current.Dispatcher).

The second step came when #24 was reopen. It made me theorizing about the way to deal with multi-windowed application and realizing that what I did for user control can be used as well with in-app windows. Most of this work was trying to set up best practices to do so, through writing some sample projects, and also led to include some utilities function for dealing with Elmish loop as DataContext.

See sample projects ModelessWindows.SharedLoop and ModelessWindows.SeparatedLoop. Get a look at the code, this is where I try to find the best way to work with multi window applications. Note that it may work as well with controls. See also OpenDialog, though this one is more raw-coded. Notice that all window management is made in the bindings/views function, not in the update loop.

To boil it down, what does this version allow ?

There are a few thing unfinished where I may need help, notably a function that would be able to apply a XAML binding path-like string to a ViewModel (for now, I use an abstract construct to do so).

Also, check for the ModelessWindows.* projects and tell me if the practices I try to set are good or not.

And let me know if it is worthy of a pull-request.

cmeeren commented 6 years ago

Thank you for the work and thought you have put into this, @JDiLenarda. It is inspiring to see such enthusiasm.

Putting my critical hat on:

Separate Elmish loops represents a shift in thinking of how to structure Elmish.WPF apps, and a deviation from TEA (The Elmish Architecture) even further than we are already at (given that we use static views and that our "view" function just sets up bindings). I'm not immediately convinced this is a move in the right direction.

Given that most developers will happily shoot themselves in the foot with easily accessible anti-patterns (I've seen several people blogging about regretting that they modularized their Elm apps prematurely instead of keeping everything in a single flat model), this change warrants first and foremost a critical high-level overview.

I will have a more careful look at your code and examples when I have the time (not right now unfortunately), but just from the top of my head I have a couple of thoughts and questions based on your post.

I'd also like someone else to take a careful look at this, most importantly @2sComplement who originally wrote Elmish.WPF, and if @et1975 could take a look at this weigh in as well that would be great.

JDiLenarda commented 6 years ago

First, I'll admit that I never considered other Elmish works. I'm a recent F# enthusiast and was seeking for a way to make WPF GUI with it and Elmish.WPF look like the coolest thing to do so. I of course took a look at the Elm Architecture page, and I got that Elmish.WPF is the ugly duckling of the family, but I'd be interested to see other resource, notably user's experience.

For the "keeping models light and focused" part, it is not a performance consideration, I've no worry that it can run smooth on a chunky model. It is more a design concern, but.. well, I tried to oversell it. Anyway, look at my sample projects to see how I use self-looped windows to make a submit palette. I don't see how easily you can do this with a flat model without adding it some "mirror fields".

Also this is the same modification that allow creating library-distributed controls, though I'm probably the only person to have this need for a while (well, honestly, I don't have this need right now, just expecting to have it one day).

Also I know you're not very thrilled, to say the least, about showing new windows in an Elmish.WPF program, but there are guys like me (and I think @giuliohome can back me up) who expect to do anything that can be done with a WPF C# program, including old fashioned GUI with dialogs and windows. Though my take on it may not be the good one, if Elmish.WPF doesn't deal with it, I'm afraid it will just remain a curiosity*. After your great job this summer to make it more useful (let me take a little share of it), let's go forward again.

(*) : no offense for people involved since its inception, if you would not have started it, I wouldn't have started it either.

cmeeren commented 6 years ago

For the record, I am not generally opposed to opening new windows (for certain use-cases), particularly not after @giuliohome showed how easy it actually was. :) (At least in a proof-of-concept way. An architecture around this with proper separation of concerns using a navigation service requires more work.)

JDiLenarda commented 6 years ago

In my samples, all the navigation service is done in the bindings function, which is a good step for separation of concerns, and has also the advantage that you don't have to nest UI calls in a ThreadDispatcher.Invoke function and run asynchronously by default.

JDiLenarda commented 6 years ago

Updated.

@cmeeren

  • Could it also be possible to have this as a separate project with Elmish.WPF as an external dependency? (Not saying I currently think that's the best idea, I'm just wondering if it is possible.)

I gave a thought about that, as long as the Internal module is not internal and my change on ViewModule.fs are accepted, that'd be fine. It would only consist of ViewModelModule.fs.

For my change on this update :

cmeeren commented 6 years ago

Thank you for your continued work. I will get to it sooner or later, hopefully during the coming week/weekend, but I have a lot on my plate right now, so I can't make any promises.

A thought: Instead of having both runWindow and bindFrameWorkElement, could we simply consolidate them and have one for any FrameworkElement, including Window?

JDiLenarda commented 6 years ago

runWindow starts the Window as the app main window. bindFrameworkElement just attach a loop to any FrameworkElement, including Window.

cmeeren commented 6 years ago

Thanks, I see.

I like your refactoring, but it makes it more difficult to see the functional changes. Would you be open to submitting a PR with just the refactorings (no functional changes), to make it easier in the next step to see the functional changes in a new PR?

JDiLenarda commented 6 years ago

OK, I'll try to do this today.

cmeeren commented 6 years ago

Thanks, no hurry. Get it right rather than fast. :)

cmeeren commented 5 years ago

Now that #54 is merged, could you create a PR for the other changes? A PR makes it easier to see what's changed, and is often a better basis for a discussion of concrete changes. If possible though, keep in mind the contributing guidelines - notably, don't make changes in your fork's master branch (which you'll now need to update since #54 is merged, and I might make more changes to it before we're done with the next PR, which would require rebasing on your part); create a dedicated feature branch instead.

JDiLenarda commented 5 years ago

Ok. It may wait for a few day, though :) I surely get a look to the guideline.

cmeeren commented 5 years ago

Thanks, no hurry.

JDiLenarda commented 5 years ago

I didn’t started the develoment yet, but gave a thought about the planning. I will make 2 PR :

About the keyed index part : on my proposition, I made it to work with Binding.subModelSeq. But I’d rather not touch this one and let it respond « naturally » to integer indexed binding path, as it does already. Instead, I will make a Binding.subModelKeyValueSeq working exactly like subModelSeq, using subModelSeqSpec with an added boolean to distinct the 2 use cases. I still have to study how the binding path work with a F# Map or a .NET Dictionary (trying to find corner cases, if any) and make the new subModelKeyValue alike. I may not implement multiple indexes, though.

Your thoughts ?

cmeeren commented 5 years ago

I haven't looked at your code yet, so I have no idea what the keyed index part you talk about is or what use-case it allows. Could you elaborate a bit on how the public API looks, and what function it serves? Give a small (partial) example in a comment here?

JDiLenarda commented 5 years ago

It’s about writing binding path to specific element of a list in the XAML code.

It happens that integer index works on subModelSeq as such.

cmeeren commented 5 years ago

Ah, I had no idea bindings supported such expressions. In order to help me understand the feature, could you give one or two examples of use-cases where key indexing could be helpful or necessary? And also how you envision the key indexing to be used (the syntax) in the public API?

JDiLenarda commented 5 years ago

I use a key index in one of my sample, though I do it through a resolvePath in my utility functions rather than hard coded XAML. Anyway, even if that looks like a vanity, it’s something XAML allows, so why not ?

https://docs.microsoft.com/en-us/dotnet/framework/wpf/data/binding-declarations-overview

If the user just wants the integer index or simply doesn’t mind, he’ll use the subModelSeq. If he wants key index, he’ll use subModelKeyValueSeq (the name is up to debate). Externally, they’ll have the same parameters and work the same way. Internally, I’ll use the getId param to retrieve the element by key. For this, I will wrap the ObservableCollection in a dynamicObject implementing TryGetIndex.

It’s working in my prototype, but I made it for subModelSeq.

For the record, subproperties in binding path (like {Binding Model.SubModel.Property1}) already work as well.

cmeeren commented 5 years ago

Anyway, even if that looks like a vanity, it’s something XAML allows, so why not ?

This is a great comment, because it showed me that I need to clarify my position on what Elmish.WPF is (and I should probably add something like this to the readme or contributing guidelines).

Elmish.WPF is not about supporting anything XAML/WPF supports just for its own sake. Elmish.WPF is an opinionated framework and should provide necessary and orthogonal (non-overlapping) features to facilitate simple development of WPF apps using TEA (The Elmish Architecture), which itself is a highly opinionated architecture. If a new feature has significant overlap with an existing way to do things, and does not provide anything other than a syntax alternative (and that syntax alternative is not better in any way), then I'm not likely to accept it.

A good litmus test is this: If someone proposes a new feature to a library, I always think of how the new feature should be documented to users. If I find that I can not explain adequately to users why and in which situations they should prefer the new solution over an already existing solution (such as making a new use-case possible or simplifying an existing use-case), then either I have misunderstood something (to be clarified by the person suggesting the feature), or the new feature does not provide any new functionality nor any benefits over existing methods.

This is, at the moment, my impression of key-based XAML binding indexing. It seems to me to confer absolutely no benefits over simply using sub-properties and the existing subModel (or, alternatively, oneWaySeq with a simple record type as output if one wants sub-properties but not a new set of Model/Msg/update/bindings). If you think I'm wrong and that explicit support for key-based indexing either makes something new possible, or simplifies some existing way to do things, then please do enlighten me. 🙂

JDiLenarda commented 5 years ago

I'm afraid we're there in a practical vs theoretical debate.

I'll try to rewrite my sample using this construct to see if there is not another way to do it (it comes to my mind it must break many other Elmish rules). This is the ModelessWindow.SharedLoop, if you want to look at it.

I won't hide I'm a little disappointed, but be sure I'm acting in good faith (I don't want to heat the debate as it has been seen on other topics).

For now, I suggest we temporary close this issue but expect me to come back to it later :)

cmeeren commented 5 years ago

I won't hide I'm a little disappointed, but be sure I'm acting in good faith (I don't want to heat the debate as it has been seen on other topics).

Rest assured I hold nothing against you. This is, and continues to be, a civil discussion, and I am fully open to the fact that I may be wrong - I'm merely asking you to succinctly explain how I'm wrong (in a way that doesn't require me to spend time delving into a fork hunting for unknown changes) and clearly explain why this change benefits Elmish.WPF users. 🙂 Help me help you, in other words.

A few clarifications:

I'm afraid we're there in a practical vs theoretical debate.

I'm open to counter-arguments, but I initially disagree with this statement: It is, in my eyes, very practical to avoid user confusion due to having several ways of doing the same thing if there is no real preference for one or the other in any situation (again, if you believe this is wrong in this specific case, I'll happily accept an explanation of where this benefits users). Furthermore, I (and others) have to maintain the code, which has highly practical consequences down the road if, for example during some later refactor, I still don't understand why some addition was made and thus where it should fit in context of the refactor, or why it should be kept around at all.

In short: We welcome useful contributions, but as a maintainer I need to understand what the changes entail and why they are needed.

I'll try to rewrite my sample using this construct to see if there is not another way to do it (it comes to my mind it must break many other Elmish rules). This is the ModelessWindow.SharedLoop, if you want to look at it.

Note that my previous post concerned the key-based indexing in isolation. If key-based indexing somehow turns out be required in order to accomplish your other goal, or at least makes it simpler for end users, then of course key-based indexing may be a relevant and helpful addition to Elmish.WPF. (But if it's only relevant to the other goal, and provides no benefits in itself, then it should be part of the same PR as the other goal, and likely be integrated into the API surfaced by your other goal.)

For now, I suggest we temporary close this issue but expect me to come back to it later :)

As you wish. Feel free to continue the discussion at your leisure, either here or in another issue (whichever you find best), and let me know if you want this issue re-opened.

JDiLenarda commented 5 years ago

I just don't exclude I'm using it wrong as well. Let me a few days to study it in depht. As I said elsewhere, I never get a look at other Elmish implementations, so I may miss some clue about the good practices.

cmeeren commented 5 years ago

If you want, you can check out the "Recommended sources" section of the readme.

Also, the /r/elm subreddit can be a good resource. I did some searches there a while back in order to understand how Elm apps are structured, which is a significant difference between normal Elm apps and Elmish.WPF apps (due to Elmish.WPF using static views and just setting up bindings in the view function, see https://github.com/elmish/Elmish.WPF/issues/26#issuecomment-413626608 for some thoughts of mine on this matter). See for example this thread and this thread for some good guidelines on the structure of Elm apps and how to scale them (which is not directly relevant for Elmish.WPF, but still a nice educational exercise to understand the elm architecture).