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

CommandParameter is null when call CanExecute method at first time #316

Closed
chenyj796 opened this issue Feb 1, 2019 · 29 comments
Closed
Labels
Design Discussion Ongoing discussion about design without consensus Enhancement Requested Product code improvement that does NOT require public API changes/additions

Comments

@chenyj796
Copy link

  • .NET Core Version: (3.0.0-preview-27122-01)
  • Windows version: (windows 10)
  • Does the bug reproduce also in WPF for .NET Framework 4.6.1?: Yes

Problem description:

I have a TestCommand as below:

public class TestCommand : ICommand
{
    public event EventHandler CanExecuteChanged;

    public bool CanExecute(object parameter)
    {
        return true;
    }

    public void Execute(object parameter)
    {
    }
}

and then I use it in a xaml file:

<Button Content="Test" Command="{StaticResource testCommand}" CommandParameter="abcdef"/>

Actual behavior:
At first time the CanExecute method is called, the parameter's value is null。
If I change the XAML property's order as below, the parameter's value is abcdef (that's ok):

<Button Content="Test" CommandParameter="abcdef" Command="{StaticResource testCommand}"/>

Expected behavior:
The parameter's value is not null at each time, regardless of the XAML order

Minimal repro:

@bhood-zorus
Copy link

I'm having uncomfortable flashbacks to when I first learned of this ☹️

I'm not sure anything can be done about this. XAML properties are assigned sequentially, and I doubt that's negotiable. This would probably require some kind of hack in the XAML reader.

@brian-reichle
Copy link

@brandonhood, or the button could just defer the call between BeginInit and EndInit?

@chenyj796
Copy link
Author

In my opinion, the XAML equals to the codes:

var button = new Button
{
    Content = "Test",
    Command = ...
    CommandParameter = ...
};

and the CanExecute method will be called by some methods such as Load ...

@brian-reichle
Copy link

The .NET framework version of the xaml parser honours the ISupportInitialize interface (which is implemented by FrameworkElement), so its actually more like this:

var button = new Button();
((ISupportInitialize)button).BeginInit();
button.Content = "Test";
button.Command = ...
button.CommandParameter = ...
((ISupportInitialize)button).EndInit();

@weltkante

This comment has been minimized.

@stevenbrix stevenbrix added the Enhancement Requested Product code improvement that does NOT require public API changes/additions label Apr 4, 2019
@stevenbrix stevenbrix added this to the Future milestone Apr 4, 2019
@stevenbrix stevenbrix added the Good First Issue metadata: Issue should be easy to implement, good for first-time contributors label Apr 4, 2019
@stevenbrix
Copy link
Contributor

I agree that this behavior is incorrect. I think something like this should be pretty straightforward to do. FrameworkElement has an OnInitialized override that Button should be able to invoke the command from. However, we don't know who is currently depending on the current behavior, so this is something that we can look into fixing after the initial release of .NET Core 3.

@vflame
Copy link

vflame commented May 13, 2019

This issue been around for a while. The easiest fix for now is to change the order so that CommandParameter appears before Command but order should not matter.

Related: https://stackoverflow.com/questions/335849/wpf-commandparameter-is-null-first-time-canexecute-is-called

@grubioe grubioe added this to Incoming / New in Future Enhancements / Bugs May 15, 2019
MichaelVach added a commit to MichaelVach/wpf that referenced this issue Jul 5, 2019
…after initialization. Issue dotnet#316

Revert changes to Hyperlink.cs
@MichaelVach
Copy link

I would like to propose a solution for the Button and MenuItem cases mentioned in the linked Stack Overflow question above.

Looking at stack trace that leads to the bug for the Button case

StackTrace leading to ICommand.CanExecute Bug.txt

we see that the error occurs in the
PresentationFramework.dll!MS.Internal.Commands.CommandHelpers.CanExecuteCommandSource(System.Windows.Input.ICommandSource commandSource)
method

The idea is to delay the execution of this method until the WPF elements are initialized. See the above linked commit 0c6b967 for the details.

@MichaelVach
Copy link

The attached test project can be used to verify the proposed solution.

CommandCanExecuteProject.zip

@vatsan-madhavan vatsan-madhavan added Design Discussion Ongoing discussion about design without consensus and removed Good First Issue metadata: Issue should be easy to implement, good for first-time contributors labels Jul 16, 2019
@weltkante
Copy link

weltkante commented Jul 17, 2019

@vatsan-madhavan not sure whats left to discuss, I think everyone agreed that its a bug. My concern about having to duplicate the code has been addressed by placing the code in a central helper method so it doesn't have to be placed in every site that has commands, I don't think the change is unreasonable.

As far as behavioral changes is happening, this just suppresses a call with incomplete arguments, the next query of CanExecute will have the correct parameters, most callers are going to ignore the first incomplete call anyways.

I suggest to reconsider the PR for post 3.0 RTM when behavioral changes are allowed again.

@Daniel-Svensson
Copy link

I think a better approach would be to first fix the behaviour of CommandParameter so that CanExecute is called whenever the parameter is changed. That should also solve the problems described here. Even if CanExecute would be called twice if parameters are in "wrong" order the end result would be the expected behaviour.

The behavior was fixed in silverlight but did never make it back to wpf (for backwards compatibility reasons i guess) so would really like to see it fixed for 3.1 so that CommanManager needs not be used for simple commands

@miloush
Copy link
Contributor

miloush commented Jul 18, 2019

I agree with @Daniel-Svensson.

There is no guarantee a caller will use the ISupportInitialize anyway, so it's not really solving the real problem. As a consumer, you should not only check whether the argument is null but also whether it's the type you expect.

@vflame I believe the XAML standard specifies the order actually matters.

@weltkante
Copy link

@miloush this issue is about XAML attribute order and the WPF implementation of the XAML loader does use ISupportInitialize (the interface is implemented on FrameworkElement base class and will always be used when loading from XAML). So I don't think your argument against using ISupportInitialize is valid.

That said I don't mind the alternative solution suggested by @Daniel-Svensson

@miloush
Copy link
Contributor

miloush commented Jul 18, 2019

@weltkante XAML loader is not the only way how an element can be constructed. The problem with moving to ISupportInitialize is that then you can file similar bug for everything that does not use that interface, rather than fixing it once at the receiving side.

Either way, I think the compatibility favours Daniel's solution (I actually consider it a bug that changing command parameter does not update the CanExecute state). You will still get the first call with null parameter, and the handler is expected to be called multiple times (not deterministically), so adding more calls shouldn't be breaking.

@ThomasBaechlerConti
Copy link

This is truely ridiculous.

As @Daniel-Svensson said, the actual issue here is that ICommand.CanExecute is not reevaluated when the value bound to CommandParameter changes. Doing so would obviously be the correct behavior, since the command parameter is passed to CanExecute, so everyone expects this behavior intuitively.

This causes issues beyond this one, for example when using Button in ItemsControl-like containers that use virtualization internally (which is currently impossible, since the enabled/disabled state of the button may refer to an entirely different item than the one the user sees).

This affects WPF for .NET Framework as well, not only the new .NET Core variant.

@creamershaq
Copy link

Hopefully this link will help
'https://stackoverflow.com/questions/335849/wpf-commandparameter-is-null-first-time-canexecute-is-called'

Thanks "Ed Downs"

public static class ButtonHelper
{
    public static DependencyProperty CommandParameterProperty = DependencyProperty.RegisterAttached(
        "CommandParameter",
        typeof(object),
        typeof(ButtonHelper),
        new PropertyMetadata(CommandParameter_Changed));

    private static void CommandParameter_Changed(DependencyObject d, DependencyPropertyChangedEventArgs e)
    {
        var target = d as ButtonBase;
        if (target == null)
            return;

        target.CommandParameter = e.NewValue;
        var temp = target.Command;
        // Have to set it to null first or CanExecute won't be called.
        target.Command = null;
        target.Command = temp;
    }

    public static object GetCommandParameter(ButtonBase target)
    {
        return target.GetValue(CommandParameterProperty);
    }

    public static void SetCommandParameter(ButtonBase target, object value)
    {
        target.SetValue(CommandParameterProperty, value);
    }
}

@sungaila
Copy link

Can we have a fix for this broken behavior?

I remember running into this bug for over 8 years now and having to use a workaround. Putting Command before CommandParameter (due to alphabetic ordering) gives me headaches every time.

There should be a switch or something to apply this bug fix. Surely there will be workarounds from the last 14 years that will break.

@GF-Huang
Copy link

GF-Huang commented Nov 18, 2021

Microsoft has shifted its focus to WinUI 3, give it up, rubbish WPF.

@sungaila
Copy link

Microsoft has shifted its focus to WinUI 3, give it up, rubbish WPF.

Abandoned UI frameworks I am aware of since the introduction of WPF:

  • Silverlight
  • WinRT
  • WinPRT
  • Windows Universal Apps (for Windows 8)
  • UWP (WinUI 2)

If you believe that WinUI 3 will stay alive for more than 2-3 years, then you haven't been paying attention.

@GF-Huang
Copy link

If WinUI 3 focus on Win32 App, that will be difference.

@heku
Copy link

heku commented Sep 9, 2022

Got same issue today, .NET 6 WPF.

@GF-Huang
Copy link

GF-Huang commented Sep 9, 2022

Got same issue today, .NET 6 WPF.

Stop expecting any functional updates, improvements, and fixes in WPF. The vast majority of updates are xaml-related, which is likely just incidental to Microsoft updating other UI frameworks that use xaml.

@pchaurasia14
Copy link
Contributor

@heku - This has been fixed in RC1 release. You may try it out.

@pchaurasia14
Copy link
Contributor

pchaurasia14 commented Sep 9, 2022

@DaveInCaz - We haven't yet. But we'll be publishing them soon.

@DaveInCaz
Copy link

@pchaurasia14 it looks like this was the change you're referring to, is that right?

#4217

I could see that in the list of commits for the RC1 release.

@pchaurasia14
Copy link
Contributor

pchaurasia14 commented Sep 9, 2022

@DaveInCaz - Yup, that's the one I think it is.

@DaveInCaz
Copy link

DaveInCaz commented Sep 9, 2022

@pchaurasia14 what does it mean that .NET 6.0 RC1 was released in 2021, but this commit was done only in July 2022?

@pchaurasia14
Copy link
Contributor

@DaveInCaz - I meant, .NET 7 RC1.

@singhashish-wpf
Copy link
Member

Closing as the PR is already merged for this.

@ghost ghost removed this from the Future milestone Jan 6, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Design Discussion Ongoing discussion about design without consensus Enhancement Requested Product code improvement that does NOT require public API changes/additions
Projects
No open projects
Development

No branches or pull requests