Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

Don't run Command CanExecute on incorrect inherited binding context type #572

Merged
merged 6 commits into from
Dec 1, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
using System;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.ComponentModel;
using System.Runtime.CompilerServices;
using Xamarin.Forms.CustomAttributes;
using Xamarin.Forms.Internals;

namespace Xamarin.Forms.Controls.Issues
{
[Preserve(AllMembers = true)]
[Issue(IssueTracker.Bugzilla, 47971, "UWP doesn't display list items when binding a CommandParameter to BindingContext in a DataTemplate and including a CanExecute method", PlatformAffected.WinRT)]
public class Bugzilla47971 : TestContentPage
{
protected override void Init()
{
var viewModel = new _47971ViewModel();

var lv = new ListView { BindingContext = viewModel };

lv.SetBinding(ListView.ItemsSourceProperty, new Binding("Models"));
lv.SetBinding(ListView.SelectedItemProperty, new Binding("SelectedModel"));

lv.ItemTemplate = new DataTemplate(() =>
{
var tc = new TextCell();

tc.SetBinding(TextCell.TextProperty, new Binding("Name"));
tc.SetBinding(TextCell.CommandParameterProperty, new Binding("."));
tc.SetBinding(TextCell.CommandProperty, new Binding("BindingContext.ModelSelectedCommand", source: lv));

return tc;
});

var layout = new StackLayout { Spacing = 10 };
var instructions = new Label {Text = "The ListView below should display three items (Item1, Item2, and Item3). If it does not, this test has failed." };

layout.Children.Add(instructions);
layout.Children.Add(lv);

Content = layout;
}

[Preserve(AllMembers = true)]
internal class _47971ViewModel : INotifyPropertyChanged
{
_47971ItemModel _selectedModel;
Command<_47971ItemModel> _modelSelectedCommand;
ObservableCollection<_47971ItemModel> _models;

public ObservableCollection<_47971ItemModel> Models
{
get { return _models; }
set
{
_models = value;
OnPropertyChanged();
}
}

public _47971ItemModel SelectedModel
{
get { return _selectedModel; }
set
{
_selectedModel = value;
OnPropertyChanged();
}
}

public Command<_47971ItemModel> ModelSelectedCommand => _modelSelectedCommand ??
(_modelSelectedCommand = new Command<_47971ItemModel>(ModelSelectedCommandExecute, CanExecute));

bool CanExecute(_47971ItemModel itemModel)
{
return true;
}

void ModelSelectedCommandExecute(_47971ItemModel model)
{
System.Diagnostics.Debug.WriteLine(model.Name);
}

public _47971ViewModel()
{
_models = new ObservableCollection<_47971ItemModel>(
new List<_47971ItemModel>()
{
new _47971ItemModel() { Name = "Item 1"},
new _47971ItemModel() { Name = "Item 2"},
new _47971ItemModel() { Name = "Item 3"}
});
}

public event PropertyChangedEventHandler PropertyChanged;

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

[Preserve(AllMembers = true)]
internal class _47971ItemModel
{
public string Name { get; set; }
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@
<Compile Include="$(MSBuildThisFileDirectory)Bugzilla46494.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Bugzilla44476.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Bugzilla46630.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Bugzilla47971.cs" />
<Compile Include="$(MSBuildThisFileDirectory)CarouselAsync.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Bugzilla34561.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Bugzilla34727.cs" />
Expand Down
100 changes: 99 additions & 1 deletion Xamarin.Forms.Core.UnitTests/CommandTests.cs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
using System;
using NUnit.Framework;


namespace Xamarin.Forms.Core.UnitTests
{
[TestFixture]
Expand Down Expand Up @@ -151,5 +150,104 @@ public void GenericCanExecute ([Values (true, false)] bool expected)
Assert.AreEqual (expected, cmd.CanExecute ("Foo"));
Assert.AreEqual ("Foo", result);
}

class FakeParentContext
{
}

// ReSharper disable once ClassNeverInstantiated.Local
class FakeChildContext
{
}

[Test]
public void CanExecuteReturnsFalseIfParameterIsWrongReferenceType()
{
var command = new Command<FakeChildContext>(context => { }, context => true);

Assert.IsFalse(command.CanExecute(new FakeParentContext()), "the parameter is of the wrong type");
}

[Test]
public void CanExecuteReturnsFalseIfParameterIsWrongValueType()
{
var command = new Command<int>(context => { }, context => true);

Assert.IsFalse(command.CanExecute(10.5), "the parameter is of the wrong type");
}

[Test]
public void CanExecuteUsesParameterIfReferenceTypeAndSetToNull()
{
var command = new Command<FakeChildContext>(context => { }, context => true);

Assert.IsTrue(command.CanExecute(null), "null is a valid value for a reference type");
}

[Test]
public void CanExecuteUsesParameterIfNullableAndSetToNull()
{
var command = new Command<int?>(context => { }, context => true);

Assert.IsTrue(command.CanExecute(null), "null is a valid value for a Nullable<int> type");
}

[Test]
public void CanExecuteIgnoresParameterIfValueTypeAndSetToNull()
{
var command = new Command<int>(context => { }, context => true);

Assert.IsFalse(command.CanExecute(null), "null is not a valid valid for int");
}

[Test]
public void ExecuteDoesNotRunIfParameterIsWrongReferenceType()
{
int executions = 0;
var command = new Command<FakeChildContext>(context => executions += 1);

Assert.DoesNotThrow(() => command.Execute(new FakeParentContext()), "the command should not execute, so no exception should be thrown");
Assert.IsTrue(executions == 0, "the command should not have executed");
}

[Test]
public void ExecuteDoesNotRunIfParameterIsWrongValueType()
{
int executions = 0;
var command = new Command<int>(context => executions += 1);

Assert.DoesNotThrow(() => command.Execute(10.5), "the command should not execute, so no exception should be thrown");
Assert.IsTrue(executions == 0, "the command should not have executed");
}

[Test]
public void ExecuteRunsIfReferenceTypeAndSetToNull()
{
int executions = 0;
var command = new Command<FakeChildContext>(context => executions += 1);

Assert.DoesNotThrow(() => command.Execute(null), "null is a valid value for a reference type");
Assert.IsTrue(executions == 1, "the command should have executed");
}

[Test]
public void ExecuteRunsIfNullableAndSetToNull()
{
int executions = 0;
var command = new Command<int?>(context => executions += 1);

Assert.DoesNotThrow(() => command.Execute(null), "null is a valid value for a Nullable<int> type");
Assert.IsTrue(executions == 1, "the command should have executed");
}

[Test]
public void ExecuteDoesNotRunIfValueTypeAndSetToNull()
{
int executions = 0;
var command = new Command<int>(context => executions += 1);

Assert.DoesNotThrow(() => command.Execute(null), "null is not a valid value for int");
Assert.IsTrue(executions == 0, "the command should not have executed");
}
}
}
62 changes: 49 additions & 13 deletions Xamarin.Forms.Core/Command.cs
Original file line number Diff line number Diff line change
@@ -1,22 +1,59 @@
using System;
using System.Reflection;
using System.Windows.Input;

namespace Xamarin.Forms
{
public sealed class Command<T> : Command
public sealed class Command<T> : Command
{
public Command(Action<T> execute) : base(o => execute((T)o))
public Command(Action<T> execute)
: base(o =>
{
if (IsValidParameter(o))
{
execute((T)o);
}
Copy link
Member

Choose a reason for hiding this comment

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

what if o is null ?

})
{
if (execute == null)
throw new ArgumentNullException("execute");
{
throw new ArgumentNullException(nameof(execute));
}
}

public Command(Action<T> execute, Func<T, bool> canExecute) : base(o => execute((T)o), o => canExecute((T)o))
public Command(Action<T> execute, Func<T, bool> canExecute)
: base(o =>
{
if (IsValidParameter(o))
{
execute((T)o);
}
}, o => IsValidParameter(o) && canExecute((T)o))
{
if (execute == null)
throw new ArgumentNullException("execute");
throw new ArgumentNullException(nameof(execute));
if (canExecute == null)
throw new ArgumentNullException("canExecute");
throw new ArgumentNullException(nameof(canExecute));
}

static bool IsValidParameter(object o)
{
if (o != null)
{
// The parameter isn't null, so we don't have to worry whether null is a valid option
return o is T;
}

var t = typeof(T);

// The parameter is null. Is T Nullable?
if (Nullable.GetUnderlyingType(t) != null)
{
return true;
}

// Not a Nullable, if it's a value type then null is not valid
return !t.GetTypeInfo().IsValueType;
Copy link
Member

Choose a reason for hiding this comment

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

I'd swap the 2 checks. first check if it's a reference type, and if it's not, start to wonder about Nullable...

}
}

Expand All @@ -28,31 +65,31 @@ public class Command : ICommand
public Command(Action<object> execute)
{
if (execute == null)
throw new ArgumentNullException("execute");
throw new ArgumentNullException(nameof(execute));

_execute = execute;
}

public Command(Action execute) : this(o => execute())
{
if (execute == null)
throw new ArgumentNullException("execute");
throw new ArgumentNullException(nameof(execute));
}

public Command(Action<object> execute, Func<object, bool> canExecute) : this(execute)
{
if (canExecute == null)
throw new ArgumentNullException("canExecute");
throw new ArgumentNullException(nameof(canExecute));

_canExecute = canExecute;
}

public Command(Action execute, Func<bool> canExecute) : this(o => execute(), o => canExecute())
{
if (execute == null)
throw new ArgumentNullException("execute");
throw new ArgumentNullException(nameof(execute));
if (canExecute == null)
throw new ArgumentNullException("canExecute");
throw new ArgumentNullException(nameof(canExecute));
}

public bool CanExecute(object parameter)
Expand All @@ -73,8 +110,7 @@ public void Execute(object parameter)
public void ChangeCanExecute()
{
EventHandler changed = CanExecuteChanged;
if (changed != null)
changed(this, EventArgs.Empty);
changed?.Invoke(this, EventArgs.Empty);
}
}
}