[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

        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.

[PureWeen]

I'm poking the team to see what they think the best path would be going forward.

The path that led to these APIs basically boils down to matching UWP and going for an approach that's bindable. Having to dip down into the View to call a method in order to trigger the visualization felt annoying but perhaps once we add a visualization property that will make more sense

1) On UWP all you have is a method to call on the RefreshControl.RequestRefresh 2) Calling this method triggers an event that gives you a deferral token and that you have to complete once you are done loading the data 3) When the user pulls down on the Refresh control it triggers an event that you have to complete once you are done loading the data

This flow is the same regardless if the user initiates or if you do it programmatically (which is actually in contrast to how iOS and Android work. On iOS and Android if you put the control into a refresh state programmatically the attached events don't fire)

So, if we were to match UWP exactly we would get rid of the flag "IsRefreshing" (or make it readonly) and just have a method on the control that triggers the event and the command. You'd still have to differentiate between a refresh initiated by the user vs one initiated from code behind. Unless we stray from UWP even further and make it so a refresh trigged by the user doesn't fire any events and only causes the visualization to appear.

I liked the use of "IsRefreshing" over a method only because it's bindable though I realize there are various laws of software programming this might violate based on side effects.

And that's how we got here :-)

[DennisWelu]

I appreciate the continued discussion on this API. My previous comments on this kind of thread still apply: https://github.com/xamarin/Xamarin.Forms/issues/11470#issuecomment-665150160. The invoking of a refresh feels much like the invoking of a Command from a Button. The state of "refreshing" is much like wanting to know if a Command is executing. ("IsExecuting" is a frequently added property in ICommand implementations for this reason) I would be in favor of a more Button-like approach to separate command invocation from "current execution status".