0

I have a combo box and a button. I want whenever I choose 1 of the item from it, the button is not disabled

XAML :

<Border Style="{StaticResource borderMain}"
                Grid.Row="7"
                Grid.Column="0">
            <ComboBox   ItemsSource="{Binding Source={StaticResource portNames}}"
                        SelectedItem="{Binding SelectedPort, Mode=TwoWay, UpdateSourceTrigger=PropertyChanged}"
                        x:Name="Port_Selector" Grid.Column="0"
                        Text="Port Selector" Background="White"/>
        </Border>

        <Border Style="{StaticResource borderMain}"
                Grid.Column="1"
                Grid.Row="7">
            <Button Content="Connect"
                    Command="{Binding OpenPortCommand}"
                    CommandParameter="{Binding SelectedPort}"
                    Style="{StaticResource buttonMain}"
                    Margin="5"/>
        </Border>

Command :

public class OpenPortCommand : ICommand
    {
        public OpenPortVM OpenPortVM{ get; set; }

        public OpenPortCommand(OpenPortVM OpenPortVM)
        {
            this.OpenPortVM = OpenPortVM;
        }

        public event EventHandler? CanExecuteChanged
        {
            add { CommandManager.RequerySuggested += value; }
            remove { CommandManager.RequerySuggested -= value; }
        }

        public bool CanExecute(object? parameter)
        {
            string? portCom = parameter as string;

            if (!string.IsNullOrEmpty(portCom))
                return true;
            return false;
        }

        public void Execute(object? parameter)
        {
            OpenPortVM.ConnectPort();
        }
    }

I already debug it and check the value from variable that I using for binding SelectedPort and it has a value on it but somehow, the CommandParameter for my button not detected so CanExecute method is not run properly. Am i missing something?

Update

expected : enter image description here

result: enter image description here

Update 2

breakpoint on Setter: enter image description here

breakpoint on CanExecute enter image description here

Wavesolid
  • 175
  • 11

2 Answers2

1

Your OpenPortCommand is missing the possibility to actually raise the CanExecuteChanged event. I suggest following:

public class OpenPortCommand : ICommand
{
    public OpenPortVM OpenPortVM{ get; set; }

    public OpenPortCommand(OpenPortVM OpenPortVM)
    {
        this.OpenPortVM = OpenPortVM;
    }

    // Leave this event as is, don't do an explicit implementation        
    public event EventHandler? CanExecuteChanged;

    public void RaiseCanExecuteChange() => CanExecuteChanged?.Invoke(this, EventArgs.Empty);
    
    public bool CanExecute(object? parameter) => !string.IsNullOrEmpty(parameter as string);

    // rest ommitted
}

In your ViewModel's SelectedPort property:

public string SelectedPort
{
    get => _selectedPort;
    set
    {
        _selectedPort = value;
        // call your OnPropertyChanged( ... );
        OpenPortCommand?.RaiseCanExecuteChanged();
    }
}

In the XAML I've simplified the binding of the ComboBox.SelectedItem property like this:

<ComboBox ... SelectedItem="{Binding SelectedPort}"/>
Steeeve
  • 824
  • 1
  • 6
  • 14
  • when you say "rest ommited" why I need to remove that? I mean, I need to call `OpenPortVM.ConnectPort();` method to execute another function – Wavesolid Nov 14 '21 at 11:53
  • I have ommitted the rest to keep the code short, you will of course need the rest. – Steeeve Nov 14 '21 at 11:54
  • ah I see, I thought u were removed the rest. Anyway, I tried with ur code but I still confused about `CanExecuteChange` what does that code do? because it doesn't exist in current context – Wavesolid Nov 14 '21 at 11:56
  • It's a typo, it should be CanExecuteChange**d** (the name of the event). I'll correct it in the answer. – Steeeve Nov 14 '21 at 12:02
  • hey, it's still not changing the button state. The button still disabled even I already choose the item from the combo box – Wavesolid Nov 14 '21 at 12:09
  • Was my assumption correct, that `SelectedPort` is a string property? If you put a breakpoint in the setter of this property, get's the breakpoint hint? Also put one in `CanExecute` method of the command. – Steeeve Nov 14 '21 at 12:15
  • yes you right, it's a String property. I updated the post with the result as u requested, please kindly to check it. thanks – Wavesolid Nov 14 '21 at 12:23
  • I've update the CanExecute method of the command, not sure if it solves the problem (I've never used that nullable types in newer .NET releases). Anyways, it is working for me in a .NET 6 project as expected. You have a bug in the setter of the property, it should be `OnPropertyChanged(nameof(SelectedPort));`, but this is not the cause of command not being updated. Your binding to ´SelectedPort` is TwoWay, why do you need that? Are you setting somewhere the property value in your VM? If it still doesn't work for you, you'll need to show more code. – Steeeve Nov 14 '21 at 13:16
  • Oh u right. It should be `OnPropertyChanged(nameof(SelectedPort));` not `OnPropertyChanged(nameof(portSelected));`. It works as expected, thanks Steve for ur assist. – Wavesolid Nov 14 '21 at 13:24
1

Your code has two issues:

  1. The most critical one, which actually prevents your code from executing properly, is the way you raise the INotifyPropertyChanged.PropertyChanged event. Instead of passing in the property name, you currently pass in the name of the backing field.

Instead of

OnPropertyChanged(nameof(portSelected));

it must be

OnPropertyChanged(nameof(SelectedPort));

To simplify the event invocation your invocator should use the CallerMemberName attribute:

protected virtual void OnPropertyChanged([CallerMemberName] string propertyName = "")
  => this.PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName);

Then call the method OnPropertyChanged without any parameter, as the property name is now automatically detected:

OnPropertyChanged();
  1. The method RaiseExecuteChangedCommand is redundant. Your ICommand implementation of the CanExecuteChanged event delegates event handlers to the CommandManager.RequerySuggested event (which is correct). The CommandManager.RequerySuggested event is raised automatically by the WPF framework and because of the event delegation the framework will also raise ICommand.CanExecuteChanged (because you attach all ICommand.CanExecuteChanged handlers to the CommandManager.RequerySuggested event using CommandManager.RequerySuggested += value). If you don't want the framework to raise the CanExecuteChanged event for you you must remove the CommandManager.RequerySuggested += value part i.e. don't attach client handlers to the CommandManager.RequerySuggested event. Because now you will raise it explcitly from your RaiseExecuteChangedCommand method. So you can't have both. Either raise ICommand.CanExecuteChanged explicitly or let the CommandManager do it for you.
BionicCode
  • 1
  • 4
  • 28
  • 44
  • Wow that's a great explanation, gonna take this as my personal note. Also u right I made a mistake on `onPropertyChanged` it should be `SelectedPort` instead `portSelected`. Thanks there – Wavesolid Nov 14 '21 at 13:27
  • A side note regarding 2): I have suggested not to use `CommandManager.RequerySuggested` because it is a blackbox how exactly it works. It can cause some unexpected behavior and is calling the CanExecute methods of the commands too many times unnecessarily (which could also be expensive, depending on what the CanExecute methods actually do). See for example [How does CommandManager.RequerySuggested work?](https://stackoverflow.com/q/2763630/10318835). I personally like the deterministic way, raising the CanExecuteChange event only if it's really needed. – Steeeve Nov 14 '21 at 14:00
  • @Steeeve CommandManager clearly adds convenience to the application. It strips off the complete logic to handle command states. Especially in an MVVM scenario you don't want the view model to participate in view logic. The CanExecuteChanged event only serves for the view (the ICommandSource implementations) to control its state. For example it allows the button to disable itself. Such state is not of interest for the view model. – BionicCode Nov 14 '21 at 14:15
  • @Steeeve It makes sense to off-load the logic to the CommandManager so that the view model must not track the conditions that influence this state. The performance impact is neglectible on todays machines. Usually you don't do expensive computations in the delegate. There are more critical UI related optimizations to consider. It never occurred that we had to change the commanding infrastructure in order to fix UI related performance issues. There is no need to avoid the CommandManager to handle the command states for you. – BionicCode Nov 14 '21 at 14:15
  • @BionicCode I'm still not convinced :) I had to debug some, let's call it 'suboptimal' ViewModel, where every keystroke in a textbox was causing a delay and a CPU usage peek, making fluent typing impossible. It was caused by dosens of commands having some expensive work in CanExecute being called by CommandManager. But as I wrote, it is only my opinion, just a hint to keep in mind when using CommandManager. – Steeeve Nov 14 '21 at 14:39
  • @Steeeve Hard to believe. The problem is not the CommandManager but the irrational *"dosens of commands having some expensive work in CanExecute"*. This is ridiculous. Wonder what expensive work a simple CanExecute predicate can do. If it for example has to communicate to a server than you have a different, a serious design problem here. Calling CanExecuteChanged only once during the lifetime of the application of course cloaks this bad design. CanExecute does not consume any significant resources if implemented properly. But I'm not trying to convince you here. – BionicCode Nov 14 '21 at 15:06
  • @Steeeve I was just replying to your comment that was using performance as the reason to drop the CommandManager. – BionicCode Nov 14 '21 at 15:06