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

Fix double refresh issue with RefreshView #7840

Closed
wants to merge 5 commits into from

Conversation

adrianknight89
Copy link
Contributor

@adrianknight89 adrianknight89 commented Oct 6, 2019

Description of Change

If you refresh a RefreshView and add a time delay beforehand (through Task.Delay or network latency), then previously we ended up calling the refresh command twice. This PR fixes the issue. Also added a UI test.

Issues Resolved

Testing Procedure

Pull to refresh and make sure that the count shows 20 instead of 30.

PR Checklist

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

Copy link
Contributor

@jsuarezruiz jsuarezruiz left a comment

Choose a reason for hiding this comment

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

LGTM.

Captura de pantalla 2019-10-07 a las 13 14 57

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.

Looking good!

What are your thoughts on moving the CanExecute up to the top and setting the value with it?

if(value && RefreshView?.Command != null)
	value = RefreshView.Command.CanExecute(RefreshView?.CommandParameter);

// Allow RefreshView.IsRefreshing to sync up with Refreshing
if (RefreshView != null && RefreshView.IsRefreshing != value)
{
	RefreshView.SetValueFromRenderer(RefreshView.IsRefreshingProperty, value);
	return;
}

_refreshing = value;

base.Refreshing = _refreshing;

if (base.Refreshing && RefreshView != null && RefreshView.Command != null)
	RefreshView.Command.Execute(RefreshView?.CommandParameter);

On a side note there is also a bug with RefreshView where the Command bound to it isn't correctly propagating to IsEnabled when the control is created (I'm creating a PR for this now)

So in theory IsRefresh should never be set to true if in fact the Commands can execute is false but it still feels odd to let the Refresh trigger if CanExecute is false.

// Allow refreshView.IsRefreshing to sync up with Refreshing
if (refreshView != null && refreshView.IsRefreshing != value)
{
refreshView.IsRefreshing = value;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
refreshView.IsRefreshing = value;
refreshView.SetValueFromRenderer(RefreshView.IsRefreshingProperty, value);

// Allow RefreshView.IsRefreshing to sync up with Refreshing
if (RefreshView != null && RefreshView.IsRefreshing != value)
{
RefreshView.IsRefreshing = value;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
RefreshView.IsRefreshing = value;
RefreshView.SetValueFromRenderer(RefreshView.IsRefreshingProperty, value);

Sprint 160 automation moved this from Ready for Review (PRs) to In progress Oct 8, 2019
@samhouts samhouts moved this from In Review to In Progress in v4.3.0 Oct 8, 2019
@adrianknight89
Copy link
Contributor Author

@PureWeen Can you review again? I made some changes to the property setter since it was bloated with several lines of code.

So in theory IsRefresh should never be set to true if in fact the Commands can execute is false but it still feels odd to let the Refresh trigger if CanExecute is false.

I'd agree with you, but the user can manually set IsRefreshing to true when the command is not executable. So why not allow this to be the same? I don't see any validation on IsRefreshingProperty based on the command:

public static readonly BindableProperty IsRefreshingProperty =
BindableProperty.Create(nameof(IsRefreshing), typeof(bool), typeof(RefreshView), false, BindingMode.TwoWay);

@samhouts samhouts added this to In Review in CollectionView Oct 8, 2019
@PureWeen PureWeen added the DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. label Oct 8, 2019
v4.3.0 automation moved this from In Progress to Closed Oct 8, 2019
CollectionView automation moved this from In Review to Done Oct 8, 2019
Sprint 160 automation moved this from In progress to Done Oct 8, 2019
@samhouts samhouts moved this from Closed to In Progress in v4.3.0 Oct 8, 2019
@samhouts samhouts moved this from Done to In progress in Sprint 160 Oct 8, 2019
@samhouts samhouts moved this from In progress to Done in Sprint 160 Oct 10, 2019
@samhouts samhouts added this to the 4.3.0 milestone Oct 17, 2019
@samhouts samhouts added this to In progress in Sprint 161 Oct 28, 2019
@samhouts samhouts moved this from In progress to Done in Sprint 161 Oct 28, 2019
@samhouts samhouts moved this from In Progress to Done in v4.3.0 Nov 5, 2019
@samhouts samhouts removed this from Done in CollectionView May 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Sprint 160
  
Done
Sprint 161
  
Done
v4.3.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants