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

Don't thrown when tap/selected is null, return null. For deselect. #950

Merged
merged 1 commit into from
Feb 22, 2021

Conversation

jamesmontemagno
Copy link
Contributor

@jamesmontemagno jamesmontemagno commented Feb 20, 2021

Description of Change

For example I want to write this in my VM:

        Coffee selectedCoffee;
        public Coffee SelectedCoffee
        {
            get => selectedCoffee;
            set => SetProperty(ref selectedCoffee, value);
        }

        async Task Selected(Coffee coffee)
        {
            if (coffee == null)
                return;

            await Application.Current.MainPage.DisplayAlert("Selected", coffee.Name, "OK");
            SelectedCoffee = null;
        }

And this in my XAML:

    <ListView
        SelectedItem="{Binding SelectedCoffee, Mode=TwoWay}"
>
        <ListView.Behaviors>
            <xct:EventToCommandBehavior
                Command="{Binding SelectedCommand}"
                EventArgsConverter="{StaticResource ItemSelectedEventArgsConverter}"
                EventName="ItemSelected" />
        </ListView.Behaviors>

Today, this will throw an exception because it it null and is not of the type it is expecting, but that should be allowed.

The only work around is to do something like AsyncCommand<object> instead of AsyncCommand<Coffee>

Bugs Fixed

  • Fixes #

API Changes

Behavioral Changes

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Has samples (if omitted, state reason in description)
  • Rebased on top of main at time of PR
  • Changes adhere to coding standard

@pictos
Copy link
Contributor

pictos commented Feb 20, 2021

@jamesmontemagno thanks for this PR, can you rephrase this part, I'm a little lost in there

The only work around is to do something like AsyncCommand instead of AsyncCommand

@jamesmontemagno
Copy link
Contributor Author

Whoops -> AsyncCommand<object> instead of AsyncCommand<Coffee>

@jamesmontemagno
Copy link
Contributor Author

The problem is when you set "SelectedCoffee = null" in the Command it throws an exception because this code is false:
value is SelectedItemChangedEventArgs selectedItemChangedEventArgs

so you need to check to see if value is null first.

@jamesmontemagno
Copy link
Contributor Author

I wasn't able to run tests locally, so someone probably needs to pull this PR down.

@pictos
Copy link
Contributor

pictos commented Feb 21, 2021

I can do it!

@pictos pictos added a/converters This issue/PR is related to converters bug Something isn't working. Breaky break. labels Feb 21, 2021
@jamesmontemagno
Copy link
Contributor Author

Fixed up my example

@jamesmontemagno
Copy link
Contributor Author

Take a look here -> https://github.com/jamesmontemagno/MyCoffeeApp/blob/master/MyCoffeeApp/MyCoffeeApp/ViewModels/CoffeeEquipmentViewModel.cs#L61

I should be able to do AsyncCommand<Coffee> and also async Task Selected(Coffee coffee). This works fine until I do SelectedCoffee = null

@pictos
Copy link
Contributor

pictos commented Feb 21, 2021

Yeah, I can confirm that the problem is the null return in the converter, to fix that we need to think in a way to know what is the type of SelectedItemChangedEventArgs.SelectedItem and ItemTappedEventArgs.Item, to cast that null to them.

image

Obs.: XCT.AsyncCommand also throws the same exception.

@pictos
Copy link
Contributor

pictos commented Feb 21, 2021

@jamesmontemagno I was able to fix it on XCT but is a very ugly fix. So I would say that is a known bug and the workaround is using the object as a parameter. The null check that you added on converters is enough, I believe. Can we merge this?

@jfversluis
Copy link
Member

Thanks James! <3

@jfversluis jfversluis merged commit 4ddcf4e into main Feb 22, 2021
@jfversluis jfversluis deleted the selected-converter-null branch February 22, 2021 09:37
@jamesmontemagno
Copy link
Contributor Author

Did this actually fix the full bug or does AsyncCommand also need to be patched?

@pictos
Copy link
Contributor

pictos commented Feb 22, 2021

@jamesmontemagno the fix will be on EventToCommandBehavior, you can see my spec here #953, if you have time, please let me know what you think about it (:

@jamesmontemagno
Copy link
Contributor Author

Ahhh gotcha! cool! Did my code do anything? I saw @jfversluis pulled it in.. :)

@pictos
Copy link
Contributor

pictos commented Feb 22, 2021

@jamesmontemagno I tested yesterday using your code and works like a charm. Now is just a matter to see if my idea is good and safe to implement (:

@jamesmontemagno
Copy link
Contributor Author

Gotcha, so it will be a combination of your fix and my fix that fixes it up fully for me. Got it

@thecuong064
Copy link

thecuong064 commented Jul 28, 2021

I'm using XCT version 1.2.0 and still facing this issue. Is there anything else I'm missing?

image

This is my code.

<ListView
SelectedItem="{Binding SelectedItem, Mode=TwoWay}">

<ListView.Behaviors>
    <xct:EventToCommandBehavior
        Command="{Binding SelectItemCommand}"
        EventArgsConverter="{StaticResource ItemSelectedEventArgsConverter}"
        EventName="ItemSelected" />
</ListView.Behaviors>
private Item selectedItem;
public Item SelectedItem 
{ 
    get => selectedItem;
    set => SetProperty(ref selectedItem, value);
}

public AsyncCommand<Item> SelectItemCommand { get; }
private async Task ExecuteSelectItemCommand(Item item)
{
    if (item == null)
        return;

    SelectedItem = null;

    await Application.Current.MainPage.DisplayAlert("Selected", item.Text, "OK");
}

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a/converters This issue/PR is related to converters bug Something isn't working. Breaky break.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants