Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

RefreshView: add new internal property IsRefreshAllowed to fix #14350 #14837

Closed
wants to merge 8 commits into from

Conversation

cpraehaus
Copy link
Contributor

Description of Change

Add new internal property IsRefreshAllowed to RefreshView to avoid that change in Command.CanExecute cancels refresh animation.

Implementations of ICommand - like AsyncCommand - usually set CanExecute() based on command state. If command execution starts 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.

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 (through IsEnabled = false) also 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 need to consider
IsRefreshAllowed and disable swipe to refresh AFTER finishing current refresh activity/indication.

Issues Resolved

API Changes

Added:

  • bool RefreshView.IsRefreshAllowed { get; set; } // INTERNAL bindable Property

Platforms Affected

  • iOS
  • Android
  • UWP

Behavioral/Visual Changes

None

Before/After Screenshots

Not applicable

Testing Procedure

Launch Xamarin.Forms.ControlGallery.Android and navigate to issue 14350. Follow the steps there to test the issue.

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)

…n#14350

Platform renderers can use IsRefreshAllowed to disable swipe to refresh in a
controlled fashion - letting any pending refresh/command finish first to
avoid that ongoing refresh indication is stopped prematurely (even if
the refresh command is still executing).
This is necessary since UIRefreshControl should no be disabled while refresh is in progress - therefore this is deferred until refreshing is done/disabled.
@jfversluis
Copy link
Member

jfversluis commented Nov 11, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jfversluis
Copy link
Member

@cpraehaus could you have a look at the failing tests please? :)

@cpraehaus
Copy link
Contributor Author

@jfversluis Thanks for the hint - fixed unit tests for RefreshView.

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PureWeen PureWeen removed the request for review from StephaneDelcroix November 13, 2021 15:55
@cpraehaus
Copy link
Contributor Author

@PureWeen Wasn't aware the this is being worked in in MAUI already. I also had the feeling that my PR is bigger than needed, but more in the direction that I wanted the change to be localized in RefreshView core class without affecting platform renderers.

Your approach is simpler, which is good :-). However, I see two disadvantages (just took an overview, so maybe I overlooked something):

  1. If Command remains disabled after refresh is done the RefreshControls' Pull-to-Refresh function will not be disabled. Although command will not be executed if user attempts to refresh its still strange for the user (s:he sees the usual PTR animation but is confused since nothing happens).
  2. Its questionable if RefreshControl.IsDisabled is the right property for this task. It disables the entire View, potentially including child elements (there I'm not 100 % sure), see [Bug] RefreshView "disabling" does not work on UWP #13749. Don't know if this is by design or not. but I would assume the latter. In my opinion a new property IsRefreshAllowed make sense, since its clearer for the XF user, has less unwanted side-effects. and is consistent with the ListView control.

@jfversluis
Copy link
Member

Now that we're so close to the sunsetting of Xamarin.Forms unfortunately we won't be able to take this in anymore, we're really sorry about that. Nevertheless, thank you so much for your time and effort that you have put into this PR.

Please have a look at the evolution of Xamarin.Forms, .NET MAUI. A lot of development has been going on there. Hopefully this issue was already fixed in that codebase. If not, feel free to port this over to there.

Again, thank you so much for being a contributor and Xamarin.Forms user!

@jfversluis jfversluis closed this Apr 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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