-
Notifications
You must be signed in to change notification settings - Fork 1.9k
RefreshView: add new internal property IsRefreshAllowed to fix #14350 #14837
Conversation
…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.
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@cpraehaus could you have a look at the failing tests please? :) |
@jfversluis Thanks for the hint - fixed unit tests for |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jfversluis I don't know if this fix needs so many parts. This is fixed on MAUI so we could just back port that code. Unless my fix is being naive.
@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 Your approach is simpler, which is good :-). However, I see two disadvantages (just took an overview, so maybe I overlooked something):
|
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! |
Description of Change
Add new internal property
IsRefreshAllowed
toRefreshView
to avoid that change inCommand.CanExecute
cancels refresh animation.Implementations of
ICommand
- likeAsyncCommand
- 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 forRefreshView.RefreshCommand
then the refresh indication is stopped immediately - even if refresh command is still running.RefreshView
sets itsIsEnabled
property depending onRefreshCommand.CanExecute()
. Two things happen regarding refresh indicator whenIsEnabled
is set to false:RefreshView
setsIsRefreshing
to false, thereby stopping the refresh indicatorIsEnabled = false
) also immediately stops the refresh indicatorsince 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 settingRefreshView.IsEnabled
- similar toListView
.RefreshView.IsRefreshAllowed
is set depending onRefreshCommand.CanExecute()
. Platform renderers need to considerIsRefreshAllowed
and disable swipe to refresh AFTER finishing current refresh activity/indication.Issues Resolved
API Changes
Added:
Platforms Affected
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