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

Remove dependency on ViewModel Class type in Binding Data. #449

Merged
merged 2 commits into from
Mar 30, 2022

Conversation

marner2
Copy link
Collaborator

@marner2 marner2 commented Mar 12, 2022

Allow Statically-typed View Models pt 1

It's not as big as it looks: a major change was to move most of the contents of Bindings.fs into a separate file without modification (first commit). Perhaps that could be a different Pull Request.

The big advantage to this refactor is that it fully decouples all of the binding logic from the actual type of the ViewModel, which is going to be important if we want those to be statically typed later on (See #447 for a very rough proof-of-concept).

@TysonMN

This comment was marked as resolved.

@marner2

This comment was marked as resolved.

@marner2 marner2 force-pushed the feature/allow_static_view_models branch from ade6456 to bcd3d94 Compare March 15, 2022 01:11
@marner2
Copy link
Collaborator Author

marner2 commented Mar 15, 2022

@TysonMN I'm ready for this to be merged in if you think it's an ok change. There are a lot of smaller changes that can be added as a result of this one, such as splitting the ViewModel.fs file, removing all of those and's, etc.

I see two possible paths for the static view model approach. Either I could make a ViewModelBase that copies most of the code from ViewModel and instead of inhering/overriding DynamicObject it would instead offer base class helpers for property implementation that initialize the bindings the first time they are called.

The second option is to double down on the ViewModelHelper concept and move most/all of the bindings management logic inside of it, then reimplement ViewModel in terms of it and just use that as a pattern for other static view models. It would need a mutable Bindings list with members for adding/getting bindings, updating the underlying model (plus processing it), dispatching messages, and registering PropertyChanged and ErrorsChanged events.

@marner2
Copy link
Collaborator Author

marner2 commented Mar 15, 2022

This seems to be related to #418

@marner2
Copy link
Collaborator Author

marner2 commented Mar 16, 2022

I believe this is a pure refactor that does not change the existing external interface of the library at all. Everything is purely additive.

@TysonMN

This comment was marked as outdated.

@TysonMN
Copy link
Member

TysonMN commented Mar 20, 2022

There are a lot of smaller changes that can be added as a result of this one, such as splitting the ViewModel.fs file, removing all of those and's, etc.
...
This seems to be related to #418

Yep, it is related. I would love for ViewModel.fs to be split up.

@TysonMN

This comment was marked as resolved.

@marner2 marner2 force-pushed the feature/allow_static_view_models branch from a86b24c to cc8d4a0 Compare March 20, 2022 01:42
@marner2

This comment was marked as resolved.

@TysonMN

This comment was marked as resolved.

Copy link
Member

@TysonMN TysonMN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the inline comments requests one change be split off into its own PR.

src/Elmish.WPF/ViewModel.fs Show resolved Hide resolved
src/Elmish.WPF/ViewModel.fs Show resolved Hide resolved
src/Elmish.WPF/ViewModel.fs Show resolved Hide resolved
src/Elmish.WPF/ViewModel.fs Show resolved Hide resolved
src/Elmish.WPF/ViewModel.fs Show resolved Hide resolved
src/Elmish.WPF/ViewModel.fs Show resolved Hide resolved
src/Elmish.WPF/ViewModel.fs Outdated Show resolved Hide resolved
src/Elmish.WPF/ViewModel.fs Outdated Show resolved Hide resolved
src/Elmish.WPF/ViewModel.fs Outdated Show resolved Hide resolved
src/Elmish.WPF/BindingInternal.fs Outdated Show resolved Hide resolved
@marner2 marner2 force-pushed the feature/allow_static_view_models branch from cc8d4a0 to 43bfbb3 Compare March 22, 2022 01:38
@marner2
Copy link
Collaborator Author

marner2 commented Mar 22, 2022

@TysonMN see #452 for something approximating a final implementation

@marner2
Copy link
Collaborator Author

marner2 commented Mar 22, 2022

@TysonMN Every time I format this thing with F# formatter it always touches a ton more files than I intended. Does it work better for you? I'm wondering if it has something to do with Fantomas versioning or if somehow it's not picking up the .editorconfig file.

@TysonMN
Copy link
Member

TysonMN commented Mar 22, 2022

I don't use a formatter, but I am open to it. Want to create a PR where you format every F# file in the repository?

@marner2
Copy link
Collaborator Author

marner2 commented Mar 22, 2022

@TysonMN @the-not-mad-psychologist said he could do that. I told him to wait a bit on that, as we're going to have conflicts if we do it while a bunch of PR's are still open.

Copy link
Member

@TysonMN TysonMN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left two comments

src/Elmish.WPF/BindingInternal.fs Outdated Show resolved Hide resolved
src/Elmish.WPF/ViewModel.fs Outdated Show resolved Hide resolved
@marner2 marner2 force-pushed the feature/allow_static_view_models branch from a6239c1 to f3b8a2d Compare March 23, 2022 22:00
@marner2
Copy link
Collaborator Author

marner2 commented Mar 24, 2022

@TysonMN There are two commits here, if you want we can move the second commit to the next PR and then the end of this PR has them all internal

@marner2 marner2 force-pushed the feature/allow_static_view_models branch 2 times, most recently from 8b1dd1a to 34b7970 Compare March 24, 2022 23:59
src/Elmish.WPF.Tests/ViewModelTests.fs Outdated Show resolved Hide resolved
src/Elmish.WPF/ViewModel.fs Outdated Show resolved Hide resolved
src/Elmish.WPF.Tests/ViewModelTests.fs Outdated Show resolved Hide resolved
@marner2 marner2 force-pushed the feature/allow_static_view_models branch from 34b7970 to 74f2194 Compare March 25, 2022 01:37
@marner2 marner2 force-pushed the feature/allow_static_view_models branch from 74f2194 to 7ec115a Compare March 25, 2022 01:41
@marner2 marner2 force-pushed the feature/allow_static_view_models branch from 7ec115a to 9de7dfa Compare March 25, 2022 19:02
@TysonMN TysonMN merged commit 3819514 into elmish:master Mar 30, 2022
@marner2 marner2 deleted the feature/allow_static_view_models branch March 30, 2022 03:30
@marner2 marner2 mentioned this pull request Jun 27, 2022
41 tasks
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

Successfully merging this pull request may close these issues.

2 participants