Xamarin.forms: [Bug] iOS: RefreshView + CollectionView with Thresholds = RefreshView Command trigger

[InquisitorJax]

Description

When using the RefreshView in conjunction with the CollectionView - the RefreshView Command is triggering without the user interaction.
This seems to only happen when items are loaded async.

In our production app - the refresh command is triggered repeatedly in an infinite loop :(

Steps to Reproduce

1. Run the attached sample
2. RefreshView.Command is immediately triggered

Expected Behavior

The RefreshView.Command should only be triggered on user interaction pull to refresh

Actual Behavior

The RefreshView.Command is triggered without the user doing a pull to refresh

Basic Information

• Version with issue:
• Last known good version:
• IDE:
• Platform Target Frameworks:
  
  
  
  • iOS: 14.1
  
  

XF_Bug_RefreshView_IncrementalLoad.zip

[PureWeen]

@InquisitorJax is this your issue?

https://github.com/xamarin/Xamarin.Forms/issues/11632

I tested your sample but I wasn't quite able to reproduce the scenario you describe

Your OnAppearing code calls LoadItems

C# protected override async void OnAppearing() { base.OnAppearing(); await _viewModel.LoadItems(); }

Which is triggering the refresh code.

The discussion here might also help highlight the behavior of the RefreshView
https://github.com/xamarin/Xamarin.Forms/issues/12506

[InquisitorJax]

yeah it looks like the Binding to the IsRefreshing may be throwing things into unexpected behavior there - I'll confirm....

[InquisitorJax]

@PureWeen I guess the expectation is that setting IsRefreshing in the ViewModel shouldn't trigger the Command - imo that's working cross purposes with the Command itself. (It should only toggle the visibility of the loading indicator - as in other implementations of the functionality)
This is a fairly common pattern to have a "busy" flag while records are fetching.

• imo you guys are going to experience a lot of noise and confusion on this (as in the ticket you linked), because of this non-explicit behavior,

Our production app issue looks very similar to #11632 , except where were getting infinite repeat calls to the refresh command - remove the threshold property bindings stopped the bug - so there was some sort of infinite call loop happening between IsRefreshing triggering the RefreshCommand, and the ThresholdCommand - unfortunately I couldn't repro that here, but my feeling it's connected to the booelan flag triggering the Command when set.

[PureWeen]

The behavior is similar to the UWP RefreshView (https://docs.microsoft.com/en-us/windows/uwp/design/controls-and-patterns/pull-to-refresh#request-a-refresh) which was another motivation. On UWP when you trigger the Refresh method it calls the event which is then where you have to cancel the refresh. Android/iOS don't work like this though and do separate out user invoked vs programmatically invoked

I think some of this will be helped by adding some additional APIs to customize whether the visualization is showing or not. I like the way it works but I'm probably in the minority :-) Like, I want my commands and the methods of those commands to fire from an action indicated by the view. The Command tied to a RefreshView should fire based on the state of the RefreshView. If I want to indicate to the user that the RefreshView is refreshing then I'll put the RefreshView into a state of refreshing which will fire my RefreshCode that now populates the view.

It feels odd to me to

• Call VM code from my page that triggers a refresh
• now manually switch the RefreshView into a visual state of refreshing

If you're inside your refresh code you've now initiated a refresh. What happens if multiple threads call that same RefreshCode? Now you'll have multiple calls on that refresh and nothing is blocking that from happening. You could check IsRefreshing but that still isn't thread safe so now you have to make sure that code always runs on the UIThread or lock the check on IsRefreshing. I prefer to just set IsRefreshing to true because that's a deterministic thread safe loop. It's a reliable cause and effect.

I feel like overloading "RefreshItems" so that it's called under two different conditions is more of a confusing place to be. The Refresh method should have localized context (Command) and not have to influence behavior outside of itself. Once I hit refresh I should only be concerned about processing the refresh. Having to worry about the visualization at this point feels problematic to me.

[PureWeen]

@InquisitorJax

except where were getting infinite repeat calls to the refresh command - remove the threshold property bindings stopped the bug

Can you pause the app and give me a stack trace?

[InquisitorJax]

The behavior is similar to the UWP RefreshView was another another motivation. On UWP when you trigger the Refresh method it calls the event which is then where you have to cancel the refresh. Android/iOS don't work like this though and do separate out user invoked vs programmatically invoked

I think some of this will be helped by adding some additional APIs to customize whether the visualization is showing or not. I like the way it works but I'm probably in the minority :-) Like, I want my commands and the methods of those commands to fire from an action indicated by the view. The Command tied to a RefreshView should fire based on the state of the RefreshView. If I want to indicate to the user that the RefreshView is refreshing then I'll put the RefreshView into a state of refreshing which will fire my RefreshCode that now populates the view.

It feels odd to me to

• Call VM code from my page that triggers a refresh
• now manually switch the RefreshView into a visual state of refreshing

If you're inside your refresh code you've now initiated a refresh. What happens if multiple threads call that same RefreshCode? Now you'll have multiple calls on that refresh and nothing is blocking that from happening. You could check IsRefreshing but that still isn't thread safe so now you have to make sure that code always runs on the UIThread or lock the check on IsRefreshing. I prefer to just set IsRefreshing to true because that's a deterministic thread safe loop. It's a reliable cause and effect.

I feel like overloading "RefreshItems" so that it's called under two different conditions is more of a confusing place to be. The Refresh method should have localized context (Command) and not have to influence behavior outside of itself. Once I hit refresh I should only be concerned about processing the refresh. Having to worry about the visualization at this point feels problematic to me.

all fair points.

[chaoyebugao]

Same problem here, I also binded IsRefreshing and Command. Then iOS would refresh on pulling down，not after finger lifting up. Android all is fine with it.

[Elashi]

I am facing the same problem. I discovered that IsRefreshing triggers the Command.
Look at the attached Call Stack.
Inside my command handler (1) I set IsBusy to true which is bound to IsRefreshing (2) which triggers my command handler (3) to be called again.

[jsuarezruiz]

Reviewing the example, invoking LoadItems in OnAppearing is setting IsRefreshing to true, binding to RefreshView and causing the associated Command to be executed. It is expected behavior at the moment, but I am concerned that it is causing some confusion.

[GuidoNeele]

Reviewing the example, invoking LoadItems in OnAppearing is setting IsRefreshing to true, binding to RefreshView and causing the associated Command to be executed. It is expected behavior at the moment, but I am concerned that it is causing some confusion.

I've looked at this issue several times, thought about it, and there's something to say for both views. The problem is that it's customary to bind the IsBusy property on the ViewModel to a refresh indicator, in this case the RefreshView (you don't want two indicators on the page), to indicate data is loading. Normally the IsBusy property is set when things are starting off, and set to false when loading has stopped. This flow is now disrupted because IsBusy is triggering a refresh on itself.
It's strange to start a refresh by only setting IsBusy to true. Likewise I think it's strange to trigger a command by setting this.RefreshView.IsRefreshing to true. I would expect this.RefreshView.Refresh() or something like it to initiate this action. That way you can have both. Triggering refresh from code behind and indicating refresh from the ViewModel.

Just my 2 cents.

[Elashi]

I moved from other Pull To Refresh controls and I find this is really weird; to have one flag that serves two purposes.

Please accept my criticism: to use one flag to indicate the state of a refresh process AND the very same flag is used to trigger refresh process is the very opposite of software engineering. It starts with "Is" then it is a flag. And more important is that it has been customary for IsBusy is to be a flag that indicates the start of a process and end of it, I even have it in my view model base class. And in some cases (which I do not recommend) is used to prevent code from entering refresh twice.

Anyway, revealing this disturbing fact resolves my crash. thank you.

[Elashi]

@InquisitorJax
You have issues here that was perfect when we used other Pull-To-Refresh controls but in RefreshView , it is not.
To fix your problem, you have to do the following:
1- Stop setting IsRefreshing (IsBusy in my case) to true at the beginning of refresh process. Just set it to false at the end.
2- Don't call LoadItems , just set IsRefreshing to true
3- during binding the IsRefreshing property, make sure to remove Mode=OneWay
4- in case you have the following condition in LoadItems , make sure you remove it

if (IsRefreshing)
    return true;

[InquisitorJax]

@PureWeen @jsuarezruiz It seems that there's general confusion as to the usage of the RefreshView.
ito this issue - my crash is solved when using it correctly.
Recommended action? I can close this guy, but my concern is you guys are going to receive a lot of noise with the behavior the way that it is currently.