MvxEnumerableExtensions.GetPosition - The operator '==' is not having the expected result (IMHO)

[zleao]

In an Android App, I have a IDictionary binded to the ItemsSource of MvxSpinner and I also have a property binded to the SelectedItem of the spinner.
If I try to set the Selecteditem through the ViewModel, the code reaches the GetPosition method in the MvxEnumerableExtensions.
As the ItemsSource is not a IList, it reaches the following code (Line 58)

if (enumerator.Current == item) { return i; }

As the comparison is beeing made by two object, the operator '==' will use System.Object.ReferenceEquals. This will only return true, if the objects are the same instance. In my case apparently they are not the same instance but they are equal objects( if I use enumerator.Current.Equals(item) it returns true)

So that makes me think that we should use the Equals instead of the '==' operator.
My change sugestion is as follows:

            if (enumerator.Current == null)
            {
                if (enumerator.Current == item)
                {
                    return i;
                }
            }
            else
            {
                if (enumerator.Current.Equals(item))
                {
                    return i;
                }
            }

[slodge]

Thanks. This sounds familiar...

Linked to #135

I know at some point we changed https://github.com/slodge/MvvmCross/blob/v3/Cirrious/Cirrious.MvvmCross.Binding.Droid/Target/MvxSpinnerSelectedItemBinding.cs

While in the area we'd also need to check other files and double check our null and ValueType cases:

• https://github.com/slodge/MvvmCross/blob/v3/Cirrious/Cirrious.MvvmCross.Binding.Droid/Target/MvxListViewSelectedItemTargetBinding.cs
• https://github.com/slodge/MvvmCross/blob/v3/Cirrious/Cirrious.MvvmCross.Binding.Droid/Target/MvxAdapterViewSelectedItemPositionTargetBinding.cs
• and maybe a few others

I believe last time this came up I looked at the Android and the underlying Java default implementations will use things like .indexOf which internally should use .equals - so the aim was to match that...

[slodge]

As a further note, GetPosition is used in two target bindings via code like:

             var index = listView.Adapter.GetPosition(value);

I'm not 100% sure that we should actually be using IAdapter.GetPosition here - need to think carefully here as this GetPosition should really be used for Java objects... This is tied to http://stackoverflow.com/questions/13842864/why-does-the-gref-go-too-high-when-i-put-a-mvxbindablespinner-in-a-mvxbindableli/13995199#comment19319057_13995199 which is not a place I want to touch too closely. It may be better to add our own GetRawPosition method here and to just return -1 for the Java method - need to think and test on this.

[slodge]

Phew.... scratch that last comment - looks like GetPosition is our method. In which case maybe it should just be renamed GetRawPosition to prevent people like me worrying about it

phew...

[zleao]

Mini heart attack there... :)
Indeed the #135 issue is very similar. That is exactly the problem I'm having inside GetPosition (soon to be GetRawPosition).

[slodge]

I believe this is working in 3.0.9 - plus I think it should be covered by the ApiExample - https://github.com/slodge/MvvmCross-Tutorials/blob/master/ApiExamples/ApiExamples.Droid/Resources/Layout/Test_Spinner.axml (this binds to Thing)

Thanks again

Closing

Stuart