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

[BUG] WPF: BindCommand cause warning in log for button declared in XAML #2037

Closed
mgnslndh opened this issue May 18, 2019 · 6 comments

Comments

Projects
None yet
2 participants
@mgnslndh
Copy link
Contributor

commented May 18, 2019

Describe the bug

If I use BindCommand to bind a view model command to a button declared in XAML like this:

this.BindCommand(ViewModel, vm => vm.DecreaseSpeed, view => view.DecreaseSpeed);

I will then get a warning in the log like this:

POCOObservableForProperty: The class ...PreviewView property DecreaseSpeed is a POCO type and won't send change notifications, WhenAny will only return a single value!

The warning is wrong since the button DecreaseSpeed is not a property on the view but a field, declared like this in XAML:

<RepeatButton Name="DecreaseSpeed" Content="Decrease"/>

Steps To Reproduce

  1. Create a view model with single command property
  2. Create a corresponding view to the view model and implement IViewFor
  3. Declare a named button in view XAML
  4. Add call to BindCommand in view constructor to bind the view model command to the button
  5. When the view model is assigned to the view the BindCommand will execute and the warning will appear in the log

I'm working on a test case to show this.

Expected behavior
I expect not to see any warnings in the log when doing something expected by the library, like binding a command to a button declared in XAML.

Environment

  • OS: Windows 10
  • Version: 9.12.0 to 9.15.4
  • Device: PC

Additional context

I have chatted with @glennawatson on slack about this issue https://reactivex.slack.com/archives/C02AJB872/p1558086054213400

@mgnslndh mgnslndh added the bug label May 18, 2019

@open-collective-bot

This comment has been minimized.

Copy link

commented May 18, 2019

Hey @mgnslndh 👋,

Thank you for opening an issue. We will get back to you as soon as we can. Also, check out our Open Collective and consider backing us.

https://opencollective.com/reactiveui

PS.: We offer priority support for all backers. Don't forget to add priority label when you start backing us 😄

An advanced, composable, functional reactive model-view-viewmodel framework for all .NET platforms!
@glennawatson

This comment has been minimized.

Copy link
Contributor

commented May 18, 2019

As discussed we probably want to suppress the message in the case of WPF BindCommand on the view property. We still want to attempt using RxUI binding on both sides since there may be more unusual situations when both properties are notifiable.

@mgnslndh

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2019

The following code in ReactiveNotifyPropertyChangedMixin.cs will try to find an appropriate implementation of ICreatesObservableForProperty but in my case DecreaseSpeed is not a property but a field.

private static IObservable<IObservedChange<object, object>> NotifyForProperty(object sender, Expression expression, bool beforeChange)
{
    var propertyName = expression.GetMemberInfo().Name;
    var result = notifyFactoryCache.Get(Tuple.Create(sender.GetType(), propertyName, beforeChange));

    if (result == null)
    {
        throw new Exception($"Could not find a ICreatesObservableForProperty for {sender.GetType()} property {propertyName}. This should never happen, your service locator is probably broken. Please make sure you have installed the latest version of the ReactiveUI packages for your platform. See https://reactiveui.net/docs/getting-started/installation/nuget-packages for guidance.");
    }

    return result.GetNotificationForProperty(sender, expression, propertyName, beforeChange);
}

Since there is no appropriate implementation the POCOObservableForProperty will be selected and it will naturally log the warning.

I'm not 100% sure I understand all the code but either ReactiveUI should see that it is a FieldExpression and not try to resolve an ICreatesObservableForProperty but I suspect it is needed to get at least the "first" value. Or, ReactiveUI could have a specific implementation of ICreatesObservableForProperty for fields (a DependencyObject with both Command and CommandParameter properties) declared in views.

@mgnslndh

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2019

How would POCOObservableForProperty check to find out if it should suppress the message? It does not know it executes in the context of a BindCommand call, I think.

@mgnslndh

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2019

I have a test case in my local repo for this issue and I'm willing to do a PR but I need guidance on the best approach of fixing this. Thanks for helping out @glennawatson!

@glennawatson

This comment has been minimized.

Copy link
Contributor

commented May 18, 2019

ICreatesObservableForProperty.GetNotificationForProperty

Add the ability to suppress parameters by adding a new parameter with a default value of false

I would change the following

IObservable<IObservedChange<object, object>> GetNotificationForProperty(object sender, Expression expression, string propertyName, bool beforeChanged = false);

to:

IObservable<IObservedChange<object, object>> GetNotificationForProperty(object sender, Expression expression, string propertyName, bool beforeChanged = false, bool supressWarnings = false);

ReactiveNotifyPropertyChangedMixin.NotifyForProperty

private static IObservable<IObservedChange<object, object>> NotifyForProperty(object sender, Expression expression, bool beforeChange)

I would add the parameter bool suppressMessage and pass that into GetNotificationForProperty

ReactiveNotifyPropertyChangedMixin.NestedObservedChanges

private static IObservable<IObservedChange<object, object>> NestedObservedChanges(Expression expression, IObservedChange<object, object> sourceChange, bool beforeChange)

I would add a parameter bool suppressMessage = false, and pass it into NotifyForProperty

ReactiveNotifyPropertyChangedMixin.SubscribeToExpressionChain

public static IObservable<IObservedChange<TSender, TValue>> SubscribeToExpressionChain<TSender, TValue>(

I would add the parameter bool suppressMessage = false, and pass into NestedObservedChanges

RxApp

I would add a public property called SuppressViewCommandBindingMessage

ReactiveUI.Wpf.Registrations

Set RxApp.SuppressViewCommandBindingMessage = true

CommandBinderImplementation.BindCommandInternal

view.WhenAnyDynamic(controlExpression, x => x.Value),

This is where you will pass in the "suppressWarnings" as true for WPF.

You will need to change the line

from:

view.WhenAnyDynamic(controlExpression, x => x.Value),

to:

view.SubscribeToExpressionChain(controlExpression, false, false, RxApp.SuppressViewCommandBindingMessage).Select(x => x.Value),

Summary

The changes will pass down a optional suppress message chain downwards. We use RxApp to indicate if we suppress or not.

mgnslndh added a commit to mgnslndh/ReactiveUI that referenced this issue May 19, 2019

mgnslndh added a commit to mgnslndh/ReactiveUI that referenced this issue May 19, 2019

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.