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

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

Merged
merged 6 commits into from Dec 1, 2016

Conversation

@hartez
Copy link
Member

commented Nov 29, 2016

Description of Change

On WinRT/UWP during the process of binding, items in a ListView initially inherit the binding context of the ListView before getting a more specific binding context. If the DataTemplate of the ListView includes an item which has both

  • a bound Command which includes a CanExecute method and
  • a bound CommandParameter

the initial binding of the item will trigger a check on the CanExecute method (to determine if IsEnabled should be true). The CanExecute method attempts to cast the binding context to the correct type and throws an InvalidCastException. The InvalidCastException is not fatal, but it causes the rest of the DataTemplate binding to fail, and the ListView items will appear blank (though in the most recent version of XF they are still tappable).

This change checks the type before executing the command and does not execute the command if the type is incorrect; it also checks the type before checking CanExecute and returns false if the type is incorrect.

Bugs Fixed

API Changes

List all API changes here (or just put None), example:

None

Behavioral Changes

None

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense
@StephaneDelcroix
Copy link
Member

left a comment

if canExecute<T> can fail the cast, so could execute<T>

I was thinking about a fix like this, and leave Command untouched:

public sealed class Command<T> : Command
	{
		public Command(Action<T> execute) : base((o) => {
			if (!(o is T))
				return;
			execute((T)o);
		})
		{
			if (execute == null)
				throw new ArgumentNullException(nameof(execute));
		}

		public Command(Action<T> execute, Func<T, bool> canExecute) : base(
			execute: o => {
				if (!(o is T))
					return;
				execute((T)o);
			}, canExecute: o => {
				if (!(o is T))
					return false;
				return canExecute((T)o);
			})
		{
			if (execute == null)
				throw new ArgumentNullException(nameof(execute));
			if (canExecute == null)
				throw new ArgumentNullException(nameof(canExecute));
		}
	}

Before pushing this, it needs some unit tests, and we have to think about what's going on when the parameter is null.

Also, even if this fix is required, I'm wondering why LV works differently in UWP and if that shouldn't be fixed as well.

{
try
{
return _canExecute(parameter);

This comment has been minimized.

Copy link
@StephaneDelcroix

StephaneDelcroix Nov 29, 2016

Member

_canExecute can not throw, unless it's in user code

@StephaneDelcroix

This comment has been minimized.

Copy link
Member

commented Nov 29, 2016

[...] and we have to think about what's going on when the parameter is null.

by that, I mean testing if T is a value or reference type, and only continuing with a null value in case of a reference type.

Also, to make things super easy, Nullable<> are reference types that can be null...

@hartez

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2016

Taking the suggestion from @StephaneDelcroix and just avoiding the InvalidCastException in the first place.

@@ -3,15 +3,22 @@

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))

This comment has been minimized.

Copy link
@campersau

campersau Nov 29, 2016

Contributor

What about this?

This comment has been minimized.

Copy link
@hartez

hartez Nov 29, 2016

Author Member

Yep. Missed that one. Fixing it now.

@StephaneDelcroix
Copy link
Member

left a comment

I like this better. but it still lack unit tests, handling of null values for reference types and Nullable<>

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

This comment has been minimized.

Copy link
@StephaneDelcroix

StephaneDelcroix Nov 29, 2016

Member

while you're at it, change that line as well, and use nameof()

if (o is T)
{
execute((T)o);
}

This comment has been minimized.

Copy link
@StephaneDelcroix

StephaneDelcroix Nov 29, 2016

Member

what if o is null ?

{
execute((T)o);
}
}, o => o is T && canExecute((T)o))

This comment has been minimized.

Copy link
@StephaneDelcroix

StephaneDelcroix Nov 29, 2016

Member

what if o is null ?

@hartez

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2016

@StephaneDelcroix Added some unit tests and handling for value types and Nullable. Let me know if I missed any combinations or cases.

}

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

This comment has been minimized.

Copy link
@StephaneDelcroix

StephaneDelcroix Nov 29, 2016

Member

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

@hartez hartez force-pushed the fix-bugzilla47971 branch from af95f48 to aa88b09 Nov 30, 2016

@rmarinho rmarinho force-pushed the fix-bugzilla47971 branch from aa88b09 to e7ef612 Nov 30, 2016

@hartez hartez changed the title Allow Command CanExecute to recover when run on inherited bindingcontext Don't run Command CanExecute on incorrect inherited binding context type Nov 30, 2016

@StephaneDelcroix StephaneDelcroix merged commit 46c25a2 into master Dec 1, 2016

4 of 6 checks passed

iOS8-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified IOS8 : Canceled (Exit co…
Details
iOS9-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS9 : Tests failed: 1, …
Details
Android-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run Android 6.0.1 : Tests passed: 349, i…
Details
OSX-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: OSX Debug : Running
Details
Windows-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: Windows Debug : Tests passed: 3595, ignored: 10
Details
iOS10-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS10 : Tests passed: 34…
Details

@StephaneDelcroix StephaneDelcroix deleted the fix-bugzilla47971 branch Dec 1, 2016

StephaneDelcroix added a commit that referenced this pull request Dec 1, 2016
Don't run Command CanExecute on incorrect inherited binding context t…
…ype (#572)

* Allow Command CanExecute to recover when run on inherited bindingcontext

* Make exception handler more generic

* Checking types in Command delegates to avoid exception in the first place

* Adding type chekc to other Command constructor

* Use nameof for ArgumentNullExceptions

* Add unit tests for null parameters, handle value types and Nullable<T>

@samhouts samhouts modified the milestones: 2.3.4, 2.3.3 Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.