Is there a chance to remove the InvalidCastException from DelegateCommand<T>?

[weitzhandler]

Hi,

An InvalidCastException is thrown here.

It looks like a justified exception, but it causes me problems. Please see my code (some things removed for brevity):

Code in VM:

public class UsersPageViewModel {
...
DelegateCommand<Cert> _UploadCertScanCommand;
public DelegateCommand<Cert> UploadCertScanCommand =>
  _UploadCertScanCommand ?? (_UploadCertScanCommand = new DelegateCommand<Cert>(UploadCertScan));
async void UploadCertScan(Cert arg)
{
//...
}

Code in View:

<MasterDetailPage x:Name="usersPage" prism:ViewModelLocator.AutowireViewModel="True">
  <MasterDetailPage.Detail>
    <TabbedPage BindingContext="{Binding SelectedUser}">
      <ContentPage Title="Roles">
        <ListView ItemsSource="{Binding Certs}"><!--SelectedUser.Certs-->
          <ListView.ItemTemplate>
            <DataTemplate>
              <ViewCell x:Name="certContainer">
                <StackLayout Orientation="Horizontal">
                  <!-- some controls removed for brevity-->
                  <Grid>
                    <Image Source="{Binding Scan,
                            Converter={StaticResource BytesToImageConverter}}"
                          BackgroundColor="Transparent" IsOpaque="True"/>

                    <!--I want CommandParameter to be the current DataItem -->
                    <Button Text="Upload scan" CommandParameter="{Binding}" 
                       Command="{Binding BindingContext.UploadCertScanCommand,
                          Source={Reference usersPage}}"
                         BackgroundColor="Transparent"/>
                    </......>

When rendering, nothing shows up, and the command line reports about Prism's InvalidCastException.
I tried converting the DelegateCommand generic's type to object, and placed a breakpoint on the command execution method, and it oddly bumps in there with one of the three values: null, instance of User (I guess from property SelectedUser), and the Cert I'm looking for.
So the exception is thrown when the current binding is the SelectedUser, so 1) why is the SelectedUser becoming the current BindingContext inside the list? 2) I know I can use a converter and return Cert only if it's indeed a Cert type, but am I not missing something? I can also subclass DelegateCommand and make it safe.

Maybe Prism shouldn't throw an exception but instead just ignore the execution in case of an invalid cast?

[brianlagunas]

Sorry, no chance that is going to be removed. That is there due to the possibility of null being passed when bindings are initialized in WPF. If it that wasn't there then apps would crash, and they wouldn't know why. No value types allowed. Make your object nullable, or use a converter to pass a valid parameter. Can you just define DelegateCommand<Cert?>?

[weitzhandler]

Hi @brianlagunas and thanks for your quick response. Cert is a reference type. But I thought the problem is not when it's null but rather when it's User, no?

I don't understand why it's even trying to pass null or User, if the ItemsList is bound to Certs (an ObservableCollection<Cert>), and the current item in the DataTemplate is Cert, when does the ListView item's {Binding} become User/null? Why is this behavior happening?

Could it be because SelectedUser is initially null and only loaded on demand? I reckon the exception I get is thrown in this line when it's unable to convert User to Cert.

[weitzhandler]

Until I figure out why CurrentUser is passed in as the ListView.Items[n]'s Binding , I'm using this class:

public class SafeDelegateCommand<T> : DelegateCommand<T>
{
  public SafeDelegateCommand(Action<T> executeMethod)
    : base(executeMethod) { }

  public SafeDelegateCommand(Action<T> executeMethod, Func<T, bool> canExecuteMethod)
    : base(executeMethod, canExecuteMethod) { }

  protected override bool CanExecute(object parameter) =>
    (parameter is null || parameter is T) && CanExecute((T)parameter);
}

That does the job for me. If it goes wrong, I should see the disabled command button (which I don't).

[brianlagunas]

Yup, you definitely need to figure out why a different type than what you are expecting is being passed. That is the root of your issue.

[weitzhandler]

Just for the reference, I am suspecting it's caused for the theory I mentioned before, that is the SelectedUser is initially null and is loaded at a later stage on demand after rendering completed, so all the inner properties (i.e. SelectedUser.Certs) would be null, thus causing the bound contexts to be null, so when the parent's BindingContext changes to non-null, all the subsequent BindingContext properties are set to the upper BindingContext and only then to the subsequent property. Could be I'm right?

[brianlagunas]

Anything is possible :)

[weitzhandler]

I am pretty certain it happens because of Xamarin updating the BindingContext gradually until it gets to the proper inner property.
I'm unsure this is the desired design by Xamarin but this is how it works and we have to deal with it. Question is doesn't it make our InvalidCastException redundant and troublesome?

[brianlagunas]

I'm curious, have you tried using XF Command to see if it happens?

[weitzhandler]

@brianlagunas Apparently XF treats wrong parameters in a similar but a bit more efficient way to what I suggested. It calls this method to verify the argument is the proper type, and is ignore if not. But I was also wrong. The InvalidCastException thrown by Prism.dll, isn't coming from this line as I initially suspected. It's an uncaught exception that occurs here, when trying to convert the argument to T, when the binding has not yet became T.

[dvorn]

@weitzhandler Basically, you have incorrect bindings which results that wrong parameter type is passed to the command. And instead of fixing your binding you want that Prism silently ignore that. That would be a wrong way to go: if the errors are detected, the programmer has a chance to know about them early and fix them. Otherwise the errors go unnoticed which may result in unstable programs.

Otherwise, what was the purpose of using Execute(T parameter) instead of Execute(object parameter) than type checking which you suggest to remove?

The solution taken by Xamarin has performance implications: they insert some reflection into each and every Execute and CanExecute call.

[weitzhandler]

@dvorn No as I have already explained, I don't have incorrect bindings. It's just the BindingContext is populated with a sub-property (e.g. BindingContext.SomeProperty) at a later stage, so until the main context is populate along its properties and children, the underlying BindingContext (i.e. the BindingContext of all the descendants in the tree) refers to the parent BindingContext, not the property path.
I beg you to read my previous comments. I wish the problem is elsewhere easier to tackle.

[dvorn]

@weitzhandler Try looking at the problem from a different angle: if you have a piece of code and it does not work as you expected, it does not necessary imply that Xamarin's binding system is wrong or Prism's Commands are wrong. May be it is a problem with your code which does not take into account how the binding system really works and you need to accommodate your bindings for that?

[dvorn]

And this is not the right place to discuss bindings. Maybe StackOverflow or Xamarin forums is a better place.

[weitzhandler]

And this is not the right place to discuss bindings. Maybe StackOverflow or Xamarin forums is a better place.

whatever

[brianlagunas]

I am with @dvorn on this one. Not only would we introduce a perf hit on every CanExecuteChanged notification, but we would essentially introduce a breaking change for no real benefit. What happens if the types aren't the same, and it is a legitimate bug? The developer will have no idea what's going on or why the CanExecute is not being called. This would be an extremely frustrating and impossible scenario to debug. Not to mention, this may produce a larger number of issues being submitted that we would have to support. If the wrong object is being passed to the Command, this tells me that there is either an issues in the platform, or in the developers code/approach. I do not think that Prism should swallow these types of exceptions, as that would be a very poor practice for a framework follow.

[weitzhandler]

Just for the reference I now debugged a custom control nested in several layers with BindingContext set to some inner properties:

<StackLayout BindingContext="{Binding Person}">
  <StackLayout BindingContext="{Binding Address}">
    <my:CustomControl BindingContext="{Binding Zip}">
  </StackLayout>
</StackLayout>

The custom control's OnBindingContextChanged was raised several times each having the element's BindingContext a deeper level, but first has been set with the outer BindingContext.
Whether Xamarin should have worked this way or not, is a different question, but it does currently work this way, and Prism's DelegateCommand<T> can't deal with it.
I already gave up with this issue and changed all the DelegateCommand in my project to Command, but I'm just saying now, after debugging and confirming this, that I thought Prism is a means to work against XF, not the other way around.

To remind you, that:

1. Xamarin itself designed its Command to deal with this appropriately.
2. We're not allowing the command to run, we're just pre-filtering before even asking the CanExecute method.
3. Prism should swallow this exception, if it's supposed to support the way XF was designed.
4. Maybe there is a way to determine whether the BindingContext has reached its final destination, only then should the exception be legit, but since the command doesn't have a way to check that, indeed a type filter must be applied before the CanExecute is called.
5. If the user insists in getting lower data-level he can lower the command parameter to object or any lower type.

If you happen to know someone from Xamarin (which I believe you do), I'd love to hear from you. Maybe just reconsider checking this deeper.

[brianlagunas]

FYI, they didn't design command to overcome the issue, they accepted a PR that implemented a workaround on Dec 1st, 2016. So the same behavior was in Xamarin since it's first release. Instead of doing a proper API review to check the functionality of the inherited binding context, they made a bad decision to patch the behavior in the command and hid potential legitimate issues from developers. The issue here is that DelegateCommand is used on more platforms than just Xamarin.Forms, and there are many more Prism apps in the wild than there are Xamarin.Forms apps.

[weitzhandler]

I thought the code for XF's DelegateCommand isn't shared. Now it is you're absolutely right. I hope they fix this. This is annoying in plenty other aspects.

And there is not even a way to determine if the bindable has reached its final BindingContext value!

[brianlagunas]

This is definitely problematic and annoying as hell.

[weitzhandler]

@dansiegel Reading your post (thank you so much for the great effort and awesome work!), I just discovered about the ObserveProperty and ObserveCanExecute, never new about these amazing functions! Anyway, the reason I stick to Xamarin's Command is because of this closed issue. I hate using anything Xamarin-related in my VMs but I have no choice. Can you please take a look at it?

[brianlagunas]

Honestly, if you want to use DelegateCommad, your best bet is to just have it be DelegateCommand<object> and check the parameter to make sure it is the correct type in the Execute and CanExecute methods.

void Execute(object item)
{
    //if item is Cert then do something
}

[weitzhandler]

@brianlagunas (read below, please)

Or use this patch. And I keep wondering, if Prism has put so much effort into being Xamarin friendly why this patch can't become part of the system. Maybe we can determine if current environment is Xamarin base using a processor directive, and only safe-checking for the argument type?

namespace Prism.Commands
{
  public class SafeDelegateCommand<T> : DelegateCommand<T>
  {
    public SafeDelegateCommand(Action<T> executeMethod)
      : base(executeMethod) { }

    public SafeDelegateCommand(Action<T> executeMethod, Func<T, bool> canExecuteMethod)
      : base(executeMethod, canExecuteMethod) { }

    protected override void Execute(object parameter) => base.Execute(parameter);

    protected override bool CanExecute(object parameter) =>
      parameter is T arg && CanExecute(arg);
  }
}

But then, it's impossible to use the observe functions smoothly because you can't declare the member and property assigned by the command as SafeDelegateCommand but only DelegateCommand, so the function returns the matching type. And even worse, the InvalidCastException provides no stack trace at all, but a single line saying it's related to a Prism command! One of the most time-consuming and frustrating exception any developer using prism is going to encounter.

I'm begging you to please address this issue, or at least make this obsolete or include some remarks etc.

[brianlagunas]

@weitzhandler the reason this will not be built into the DelegateCommand has already been discussed in this thread.

please address this issue, or at least make this obsolete or include some remarks etc

Not sure what you mean here. Nothing is obsolete, and the DelegateCommand functions exactly like it should.

[weitzhandler]

DelegateCommand is unusable with nested properties. Please warn about this in the remarks, so people don't pull out their hair of frustration trying to figure out this annoying issue.

[brianlagunas]

Can you explain? What do you mean unusable with nested properties?

[weitzhandler]

As I explained above, when you set a BindingContext to a nested property, or when nesting two controls with an inner depending BindingContext (i.e. the inner control's BindingContext is bound to a property in the parent's BindingContext), Xamarin will first set up all the children's BindingContext to the parent context, then gradually walk in and update to the appropriate context. This behavior leads to a failure in the command, and as said earlier, even XF themselves have their Command checking the parameter like that to avoid this frustrating exception. If one of the target technologies in Prism's buffet is XF, Prism should respect that quirk in Xamarin in order to make its API properly useful in XF.

I'm curious what @dansiegel has to say about this...

[brianlagunas]

As I already explained above. DelegateCommand works exactly how is it supposed to and is used on more platforms than just XF. It does work just fine with "nested properties". You just have to define it as DelegateCommand<object> instead to workaround the quirk in XF when required.

[weitzhandler]

Even if there is a way to determine that the current environment is XF, and even nor as an solution until we find a better way?

[brianlagunas]

We cannot use a precompiler directive to determine the current environment.

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.