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

feature: Add winforms auto binding to Tables/Flow based controls #1956

Merged
merged 2 commits into from Feb 21, 2019

Conversation

Projects
None yet
3 participants
@glennawatson
Copy link
Contributor

glennawatson commented Feb 21, 2019

The following has been added:

  • A interface called ISetMethodBindingConverter which allows the bind engine to perform a custom set operation. This is because WinForms often gives you collections where you are meant to do 'Add' operations directly rather than setting them.
  • Added two new implementations of the ISetMethodBindingConverter. One for general ContentControls, the other for tables.
  • Added a winforms demo based on the main sample.

@glennawatson glennawatson requested review from reactiveui/core-team as code owners Feb 21, 2019

@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 21, 2019

Codecov Report

Merging #1956 into master will decrease coverage by 0.63%.
The diff coverage is 59.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1956      +/-   ##
==========================================
- Coverage   59.01%   58.38%   -0.64%     
==========================================
  Files         117      120       +3     
  Lines        4780     4803      +23     
  Branches      678      689      +11     
==========================================
- Hits         2821     2804      -17     
- Misses       1786     1822      +36     
- Partials      173      177       +4
Impacted Files Coverage Δ
...veUI/Platforms/uap10.0.16299/WinRTAppDataDriver.cs 0% <0%> (ø) ⬆️
...c/ReactiveUI.Winforms/ContentControlBindingHook.cs 0% <0%> (ø)
...rc/ReactiveUI/Bindings/Reactive/ReactiveBinding.cs 75% <0%> (-21.43%) ⬇️
src/ReactiveUI/Expression/Reflection.cs 62.85% <100%> (ø) ⬆️
src/ReactiveUI.Winforms/Registrations.cs 70% <100%> (+7.5%) ⬆️
....Winforms/TableContentSetMethodBindingConverter.cs 8.33% <8.33%> (ø)
...ctiveUI.Winforms/PanelSetMethodBindingConverter.cs 8.33% <8.33%> (ø)
.../Bindings/Property/PropertyBinderImplementation.cs 83.91% <86.76%> (-3.59%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e29503f...eca8fa3. Read the comment docs.

@gpriaulx
Copy link
Contributor

gpriaulx left a comment

Looks good; nice simplifications and cleanup.

@glennawatson glennawatson merged commit 2b62d1d into master Feb 21, 2019

3 checks passed

codecov/patch 59.25% of diff hit (target 59.01%)
Details
codecov/project Absolute coverage decreased by -0.63% but relative coverage increased by +0.24% compared to 6a66115
Details
license/cla All CLA requirements met.
Details

@delete-merged-branch delete-merged-branch bot deleted the glennawatson-sample-winforms branch Feb 21, 2019

@axgdev

This comment has been minimized.

Copy link

axgdev commented Feb 21, 2019

Hi there @glennawatson @gpriaulx! Thank you for the work on ReactiveUI it is great! :)

This commit unfortunately breaks some functionality. If a property value of a binding was null, the framework could (in v9.10.7) cleanly convert it into something appropriate. Example:

this.WhenActivated(disposable =>
{
    this.WhenAnyValue(x => x.FormText)
        .BindTo(this, x => x.Text)
        .DisposeWith(disposable);
});

Please note that this refers to a Form. If for some reason the property FormText is null then an exception is thrown like:

PropertyBinderImplementation: x.Text Binding received an Exception!: System.NullReferenceException: Object reference not set to an instance of an object.
   at ReactiveUI.PropertyBinderImplementation.<>c__DisplayClass10_0`3.<BindToDirect>g__SetThenGet|0(Object paramTarget, Object paramValue, Object[] paramParams) in somepath\src\ReactiveUI\Bindings\Property\PropertyBinderImplementation.cs:line 459
   at ReactiveUI.PropertyBinderImplementation.<>c__DisplayClass10_0`3.<BindToDirect>b__1(TObs x) in somepath\src\ReactiveUI\Bindings\Property\PropertyBinderImplementation.cs:line 475
   at System.Reactive.Linq.ObservableImpl.Select`2.Selector._.OnNext(TSource value) in somepath\src\System.Reactive\Linq\Observable\Select.cs:line 39
@glennawatson

This comment has been minimized.

Copy link
Contributor Author

glennawatson commented Feb 21, 2019

It's worth probably taking this sort of feedback straight to an issue btw. Closed PR's have a habit of getting lost. Moved it over to one ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.