[Bug] RefreshView to completes instantly when using AsyncCommand or Command with CanExecute functionality

[beeradmoore]

Description

If you have a RefreshView to load some data asyncronsly on a pull to refresh the pull to refresh actions (IsRefreshing = false, animation goes away) will complete differently depending if you are using the normal Command vs Command with CanExecute functionality (Xamarin Community Toolkits AsyncCommand included).

This was discovered when I thought it was a problem with AsyncCommand (with allowsMultipleExecutions set to false) in Xamarin Community Toolkit (original issue is filed here), but the last changes to RefreshView by @PureWeen actually added this CanExecute functionality. So this may be by design.

IMO if I am using AsyncCommand (with allowsMultipleExecutions) I would expect the RefreshView to finish refreshing once the Task has finished executing. When using a regular command (with canExecute functionality setup) I would expect RefreshView to not finish refreshing early but instead wait for the Command to finish executing. It makes sense to not allow refreshing to start if the command CanExecute = false, but it shouldn't end it prematurely.

Steps to Reproduce

1. Add RefreshView and wire it up as per most tutorials
2. Implement CanExecute functionality in your command (or alternatively use AsyncCommand with allowsMultipleExecutions = false)

Expected Behavior

When the user pulls to refresh the RefreshView will await for the specific method (ExecuteRefreshCommand in my sample) to finish before setting IsRefreshing to false and making the refresh animation go away.

Actual Behavior

RefreshView completes refresh instantly when RefeshCommand has CanExecute functionality (or is AsyncCommand with allowsMultipleExecutions = false) .

Basic Information

Xamarin.Forms: 5.0.0.2012
Xamarin Community Toolkit: 1.1.0

Environment

Show/Hide Visual Studio info

=== Visual Studio Community 2019 for Mac ===

Version 8.10.1 (build 71)
Installation UUID: 0bad3465-09eb-41af-8e6c-ba3571637739
	GTK+ 2.24.23 (Raleigh theme)
	Xamarin.Mac 6.18.0.23 (d16-6 / 088c73638)

	Package version: 612000140

=== Mono Framework MDK ===

Runtime:
	Mono 6.12.0.140 (2020-02/51d876a041e) (64-bit)
	Package version: 612000140

=== Roslyn (Language Service) ===

3.10.0-4.21269.26+029847714208ebe49668667c60ea5b0a294e0fcb

=== NuGet ===

Version: 5.9.0.7134

=== .NET Core SDK ===

SDK: /usr/local/share/dotnet/sdk/5.0.301/Sdks
SDK Versions:
	5.0.301
	5.0.203
	5.0.202
	5.0.201
	5.0.103
	5.0.102
	5.0.101
	3.1.410
	3.1.409
	3.1.408
	3.1.407
	3.1.406
	3.1.405
	3.1.404
MSBuild SDKs: /Applications/Visual Studio.app/Contents/Resources/lib/monodevelop/bin/MSBuild/Current/bin/Sdks

=== .NET Core Runtime ===

Runtime: /usr/local/share/dotnet/dotnet
Runtime Versions:
	5.0.7
	5.0.6
	5.0.5
	5.0.4
	5.0.3
	5.0.2
	5.0.1
	3.1.16
	3.1.15
	3.1.14
	3.1.13
	3.1.12
	3.1.11
	3.1.10

=== .NET Core 3.1 SDK ===

SDK: 3.1.410

=== Xamarin.Profiler ===

Version: 1.6.15.68
Location: /Applications/Xamarin Profiler.app/Contents/MacOS/Xamarin Profiler

=== Updater ===

Version: 11

=== Apple Developer Tools ===

Xcode 12.5 (18205)
Build 12E262

=== Xamarin.Mac ===

Version: 7.14.0.3 (Visual Studio Community)
Hash: 17fdcf569
Branch: d16-10
Build date: 2021-05-20 15:46:14-0400

=== Xamarin.iOS ===

Version: 14.20.0.3 (Visual Studio Community)
Hash: 17fdcf569
Branch: d16-10
Build date: 2021-05-20 15:46:15-0400

=== Xamarin Designer ===

Version: 16.10.0.119
Hash: 36a2d986f
Branch: remotes/origin/d16-10
Build date: 2021-06-02 19:41:34 UTC

=== Xamarin.Android ===

Version: 11.3.0.1 (Visual Studio Community)
Commit: xamarin-android/d16-10/22fc2b3
Android SDK: /Users/bradmoore/Library/Developer/Xamarin/android-sdk-macosx
	Supported Android versions:
		None installed

SDK Tools Version: 26.1.1
SDK Platform Tools Version: 30.0.4
SDK Build Tools Version: 30.0.2

Build Information: 
Mono: b4a3858
Java.Interop: xamarin/java.interop/d16-10@f39db25
ProGuard: Guardsquare/proguard/v7.0.1@912d149
SQLite: xamarin/sqlite/3.35.4@85460d3
Xamarin.Android Tools: xamarin/xamarin-android-tools/d16-10@c5732a0

=== Microsoft OpenJDK for Mobile ===

Java SDK: /Users/bradmoore/Library/Developer/Xamarin/jdk/microsoft_dist_openjdk_1.8.0.25
1.8.0-25
Android Designer EPL code available here:
https://github.com/xamarin/AndroidDesigner.EPL

=== Android SDK Manager ===

Version: 16.10.0.12
Hash: e240b8c
Branch: remotes/origin/d16-10
Build date: 2021-06-01 18:26:34 UTC

=== Android Device Manager ===

Version: 16.10.0.14
Hash: e340248
Branch: remotes/origin/d16-10
Build date: 2021-06-01 18:26:52 UTC

=== Build Information ===

Release ID: 810010071
Git revision: 3664adb3ec17b7a29476110bb1c6f7083d43b1ab
Build date: 2021-06-08 13:22:15-04
Build branch: release-8.10

=== Operating System ===

Mac OS X 10.16.0
Darwin 20.5.0 Darwin Kernel Version 20.5.0
    Sat May  8 05:10:33 PDT 2021
    root:xnu-7195.121.3~9/RELEASE_X86_64 x86_64

=== Enabled user installed extensions ===

MFractor 4.4.8

Screenshots

Command (without canExecute):

Command (with canExecute) or AsyncCommand

Reproduction Link

AsyncCommandRefreshView.zip

Workaround

Now that I understand the underlying issue I will go back to using a regular Command, not implement CanExecute, instead inside my method that is being called add extra checks to avoid it being called multiple times.

This workaround is fine, but IMO it feels the current implementation adds a nice got-ya that people may not be aware of.

[josephlbailey]

Can confirm this issue as well.

[AmrAlSayed0]

I noticed several weird things about the RefreshView:
1. The CanExecute is not respected when IsRefreshing is changed.
https://github.com/xamarin/Xamarin.Forms/blob/5.0.0/Xamarin.Forms.Core/RefreshView.cs#L27-L38
Nevermind, it is respected here
https://github.com/xamarin/Xamarin.Forms/blob/5.0.0/Xamarin.Forms.Core/RefreshView.cs#L55-L56
2. When the RefreshCommand's CanExecute is changed to false the IsEnabled is set to false
https://github.com/xamarin/Xamarin.Forms/blob/5.0.0/Xamarin.Forms.Core/RefreshView.cs#L101-L107
which in turn sets the IsRefreshing to false
https://github.com/xamarin/Xamarin.Forms/blob/5.0.0/Xamarin.Forms.Core/RefreshView.cs#L131-L137
which is not always true. A RefreshView that is disabled can't call RefreshCommand can't be re-refreshed, doesn't mean it should stop the currently refreshing (which it doesn't, actually, the RefreshCommand still completes, but the spinner stops).

[jfversluis]

@cpraehaus is this related to what you did for the ListView in #8384 ?

[Hackmodford]

I can also confirm this. It bit me when trying to use MvvmCross' MvxAsyncCommand. :(

[jfversluis]

@Hackmodford do you still have a project where you could try this on?

[Hackmodford]

@jfversluis I have what I am currently working on. But I can't share that source if that's what you're asking.

[jfversluis]

That is definitely not what I'm asking :)

This issue has similarities with something we just fixed. So I was wondering if maybe fixed this as well, but it seems the fix was only for ListView so that's not going to work here. I was going to ask if you could try out the fix, but seems that is not necessary

[cpraehaus]

Your are right @jfversluis, this issue is about the RefreshView not the ListView. I already looked into the problem with RefreshView and it looks like the same root cause as in #8384. I will prepare a PR for fixing it for RefreshView as well.

[jfversluis]

That would be amazing, thank you!

[cpraehaus]

Implementations of ICommand - like AsyncCommand - usually set CanExecute() based on command state. If command execution
starts then CanExecute() is often set to false to avoid that command is executed multiple times in parallel. If such a command is used for RefreshView.RefreshCommand then the refresh indication is stopped immediately - even if refresh command is still running.

Reason: RefreshView sets its IsEnabled property depending on RefreshCommand.CanExecute(). Two things happen regarding refresh indicator when IsEnabled is set to false:

1. RefreshView sets IsRefreshing to false, thereby stopping the refresh indicator
2. On Android, disabling the RefreshView immediately stops the refresh indicator since the Android SwipeRefreshLayout is also disabled (this happens through standard XF view/renderer interactions).

Idea is to introduce a new property RefreshView.IsRefreshAllowed instead of setting RefreshView.IsEnabled - similar to ListView.RefreshView.IsRefreshAllowed is set depending on RefreshCommand.CanExecute(). Platform renderers can now consider IsRefreshAllowed and disable swipe to refresh AFTER finishing current refresh activity/indication.

Prepared a fix but since this change affects multiple platforms I want to perform some further tests before submitting PR.

[jfversluis]

@beeradmoore, a PR (#14837) for this is open now, would you be able to grab the NuGet as described here and let us know if this fixes this issue? That will greatly speed up the review process.

Besides verifying if this particular issue is fixed also be sure to check other scenarios in the same area to make sure that this fix doesn't accidentally has side-effects 🙂

Thanks!

[Hackmodford]

Your link to the PR links to this issue.

[jfversluis]

Ha whoops! Sorry about that. Fixed it!

[Hackmodford]

Also, I don't see the check for the build.

[jfversluis]

Wow I really need more coffee. Seems a build has not been triggered for the PR. Doing that now. As soon as it's finished you should be able to try :) thanks!

[Hackmodford]

@jfversluis looks like the tests failed.

[jfversluis]

@Hackmodford it's finally there now! :D

[beeradmoore]

@jfversluis , I put it in my main work project and did not notice my CollectionView inside RefreshView from behaving oddly. The RefreshView seemed to work as expected.

[jfversluis]

@beeradmoore in other words, this seems to fix it?

[Hackmodford]

I just tried the PR. On iOS my refresh view is shown at page launch (as I expected). But when I do a pull to refresh it goes away immediately.

I will test Android next.

[Hackmodford]

same behavior on Android.

<RefreshView x:Name="DevicesList" IsRefreshing="{Binding IsRefreshing, Mode=OneWay}"
                        Command="{Binding ScanCommand}">
                
                <CollectionView
                    ItemsSource="{Binding Devices}"
                    Margin="0,0,0,0"
                    VerticalOptions="FillAndExpand"
                    ItemTemplate="{StaticResource GroupingTemplateSelector}" />

</RefreshView>

This is how I originally had my command.

public MvxAsyncCommand ScanCommand => new MvxAsyncCommand(async () =>
{
    if (IsRefreshing) return;
            
    await _kneeBluetoothService.ScanForDevicesAsync();
});

I also tried this.

public MvxAsyncCommand ScanCommand => new MvxAsyncCommand(async () =>
{
    await _kneeBluetoothService.ScanForDevicesAsync();
}, () => !IsRefreshing);

All scanning is performed by running this command (either via viewAppeared or pulling to refresh).

[cpraehaus]

@Hackmodford RefreshView works the following way: when user pulls to refresh then RefreshView.IsRefreshing will be set to true and RefreshView.ScanCommand is executed (ScanCommand in your example). When the refresh is done you have to indicate that by setting RefreshView.IsRefreshing to false. I fear using OneWay binding mode - like in your example - won't work properly in this case based on the mentioned interaction.

Try setting default binding mode for IsRefreshing:

<RefreshView x:Name="DevicesList" IsRefreshing="{Binding IsRefreshing}"
                        Command="{Binding ScanCommand}">

And change your command implementation as follows:

public MvxAsyncCommand ScanCommand => new MvxAsyncCommand(async () =>
{
    if (IsRefreshing) return;
    IsRefreshing = true; // Indicate we are refreshing in case ScanCommand is invoked programmatically (not from RefreshView)
     
    await _kneeBluetoothService.ScanForDevicesAsync();

    IsRefreshing = false; // Indicate that refresh is done
});

Hope this helps.

[cpraehaus]

@Hackmodford Ah sorry, just realized what I suggested is not going to work since you want to use IsRefreshing to control executability of ScanCommand. What about this:

<RefreshView x:Name="DevicesList" IsRefreshing="{Binding IsRefreshing, Mode=OneWay}"
                        Command="{Binding ScanCommand}">

and

public MvxAsyncCommand ScanCommand => new MvxAsyncCommand(async () =>
{
    IsRefreshing = true; // Indicate we are refreshing since we are using IsRefreshing in OneWay binding mode
     
    await _kneeBluetoothService.ScanForDevicesAsync();

    IsRefreshing = false; // Indicate that refresh is done
}, () => !IsRefreshing);

[Hackmodford]

what I didn't mention is that IsRefreshing is being tied to an observable. So the ScanForDeviecAsync() command would already have that effect.

[beeradmoore]

@jfversluis , yep fixes it.

Although discussion on the PR you linked looks like this isn't the solution that will go out and maybe it needs to be done a different way.