Fix crash and improve MvxRecyclerViewAdapter and MvxRecyclerViewHolder

[softlion]

✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)

Bug fix + refactor of MvxRecyclerViewAdapter and MvxRecyclerViewHolder

⤵️ What is the current behavior?

• Potential crash when updating ItemsSource from worker thread.
• Inconsistencies when changing Adapter, making ItemClick and ItemLongClick not work until items are recycled and shown again

🆕 What is the new behavior (if this is a feature change)?

Fixes the above crash and inconsistencies
Allows extensibility through new virtual methods:

• OnItemViewClick
• OnItemViewLongClick
• ExecuteCommandOnItem

Also improved cleaning up of resources when Dispose is called from the Java side

💥 Does this PR introduce a breaking change?

No

🐛 Recommendations for testing

📝 Links to relevant issues/docs

Fixes #3295

🤔 Checklist before submitting

• All projects build
• Follows style guide lines (code style guide)
• Relevant documentation was updated (docs style guide)
• Rebased onto current develop

[martijn00]

@softlion can you address the comments?

[martijn00]

@softlion we use rebase instead of merge to get changes from develop into the branch you make changes in. I did one on your branch, but that means that you have to delete your local branch and check it out again because the history is overwritten.

If you want i can do it again to get rid of the merge commit. Or you commit your changes to my comments first, and then i do a rebase.

[Cheesebaron]

I've rebased it for you. Can you please check the File Changed tab and verify that those are the correct changes? @softlion

[Cheesebaron]

@softlion actually I do have access to it. As soon as you make a Pull Request, I have access to push to that branch. Unless you remove the check mark in the checkbox on the PR, which allows/disallows others to push to it.

[Cheesebaron]

This looks good to me. Also tested it on a sample app I made, which tested both, ItemClick, ItemLongClick, adding, removing items, adding items in bulk. Everything seems OK.

Was rebased

[fadaEntrepidus]

If I had my ViewHolder like this:

public override RecyclerView.ViewHolder OnCreateViewHolder(ViewGroup parent, int viewType){ var bc = new MvxAndroidBindingContext(parent.Context, BindingContext.LayoutInflaterHolder); var v = InflateViewForHolder(parent, viewType, bc); return new ViewHolder(v, bc) { Click = ItemClick, LongClick = ItemLongClick }; return new ViewHolder(v, bc); }

public override void OnAttachedToWindow() { try { base.OnAttachedToWindow(); ... } catch (Exception ex) { Mvx.IoCProvider.Resolve<ILogger>().LogException("TAG", ex, nameof(View.Click)); } }

How should I be with the current changes? Because it gives me this error:

MonoDroid: UNHANDLED EXCEPTION:

MonoDroid: System.ArgumentNullException: missing source event info in MvxWeakEventSubscription

MonoDroid: Parameter name: sourceEventInfo

MonoDroid: at MvvmCross.WeakSubscription.MvxWeakEventSubscription2[TSource,TEventArgs]..ctor (Android.Views.View source, System.Reflection.EventInfo sourceEventInfo, System.EventHandler1[TEventArgs] targetEventHandler) [0x0001d] in :0

[tbalcom]

You may be running into the issue described in #3411. Check there for a solution.

[fadaEntrepidus]

You may be running into the issue described in #3411. Check there for a solution.

Yesss!! Work. Thanks @tbalcom

[sdebruyn]

This did introduce a breaking change by adding events to the IMvxRecyclerViewHolder. Maybe some documentation should be added.

[anne-k]

This change removes public and protected items from the MvxRecyclerViewHolder, which does in fact make it breaking imo. Sure, none of them were in the documentation, but if they're accessible you can expect they will be used.

To be more specific, what this breaks in my app is some of my custom ViewHolders/Adapters. That is a usage not covered at all in the documentation but I don't consider it a strange thing to do. The MvxRecyclerView can cover a lot of use cases with axml and TemplateSelectors only, but not all of them :)

Edit: I'm only noticing this now because the issue it caused was a very subtle one, but: what is the purpose of moving the call to ItemTemplateSelector.GetItemLayoutId to GetItemViewType? Because I'm only seeing downsides:

• The name of the Adapter method is GetItemViewType which sets the reasonable expectation that what it returns is the value of the ItemTemplateSelector's GetItemViewType method. Instead, it returns the value of GetItemLayoutId.
• We now have two non-private, virtual methods with a parameter called viewType (OnCreateViewHolder and InflateViewForHolder) where what is passed as this parameter is not some symbolic viewType int, but an int that represents an Android layout ID. It no longer corresponds to what the ItemTemplateSelector calls viewType. Nothing in the method descriptions or the parameter names suggests this, only seeing how it is used in the source code tells you what this int really is.
• Regardless of the names of methods and parameters and opinions on the current implementation, it is a change from how it was implemented before. The interfaces of these three methods were changed, which means any code using or overriding any of these methods potentially needs to be changed. And unlike the properties that were removed from the ViewHolder, this change is a very subtle one. If you're unlucky, your code will still compile, but do something different than what it did before.

All of this makes me feel like I'm not supposed to subclass the MvxRecyclerAdapter. Is that your official view on these classes, that they are to be used as-is only? I don't really want to have to choose between having the Mvx features and being able to add my own features.

[anne-k]

@softlion Ah, I didn't know that! That's interesting, that makes it make more sense. It's a bit unfortunate that it doesn't match with the TemplateSelector's viewtype anymore but changing that would break stuff for a lot more people, I get that you don't want to do that.

Thank you for replying!