Skip to content

Conversation

@tgjones
Copy link
Contributor

@tgjones tgjones commented Nov 22, 2017

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

This is either a bug fix or enhancement, depending on how you look at it. It applies the equivalent activation fix for UIView subclasses, that #1182 already did for UIViewController subclasses.

#1182 changed UIViewController subclasses to use ViewWillAppear instead of ViewDidAppear for activation, such that activation occurs before the view becomes visible.

This PR does the equivalent thing for UIView subclasses.

What is the current behavior? (You can also link to an open issue here)

UIView subclasses (correctly) override WillMoveToSuperview, which is called before the view becomes visible. However, within those overrides, they schedule an async callback on the UI thread like this:

public override void WillMoveToSuperview(UIView newsuper)
{
    base.WillMoveToSuperview(newsuper);
    RxApp.MainThreadScheduler.Schedule(() => (newsuper != null ? activated : deactivated).OnNext(Unit.Default));
}

Because Schedule schedules an async callback on the UI thread, the callback may (and in my testing, will) be called after the view becomes visible. This means that there's a visible flicker where you first see uninitialized views, and then they are populated with data.

What is the new behavior (if this is a feature change)?

The activation callbacks are now called synchronously from UIView subclasses, such that activation occurs before the view becomes visible. Instead of the code above, the WillMoveToSuperview overrides become simpler:

public override void WillMoveToSuperview(UIView newsuper)
{
    base.WillMoveToSuperview(newsuper);
    (newsuper != null ? activated : deactivated).OnNext(Unit.Default);
}

This matches the existing behaviour for ReactiveCollectionReusableView and ReactiveCollectionView, which already called the activation callback synchronously.

What might this PR break?

If anybody relies on the current behaviour of activation occurring after views become visible, this would be a breaking change. But that was almost certainly undesirable behaviour in the first place.

Please check if the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

Other information:

Fixes current (undesirable) behaviour where bindings are evaluated after views become visible.

After this change, bindings are evaluated prior to views being shown.
@dnfclas
Copy link

dnfclas commented Nov 22, 2017

@tgjones,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla2.dotnetfoundation.org.

It will cover your contributions to all .NET Foundation-managed open source projects.
Thanks,
.NET Foundation Pull Request Bot

@dnfclas
Copy link

dnfclas commented Nov 22, 2017

@tgjones, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, .NET Foundation Pull Request Bot

@tgjones
Copy link
Contributor Author

tgjones commented Nov 22, 2017

The AppVeyor build failure looks unrelated to my changes, but please let me know if I need to fix something.

@Qonstrukt
Copy link
Member

Qonstrukt commented Nov 22, 2017

I actually ran into this (again) today, so I'm all for it!

I've also noticed breaking of AutoLayout in cells because of the unnecessary scheduling.

@kentcb
Copy link
Contributor

kentcb commented Nov 24, 2017

Great - thanks @tgjones! And yeah, the build error is definitely unrelated :/

@kentcb
Copy link
Contributor

kentcb commented Nov 24, 2017

Build fixed in #1542, just waiting for merge before I can merge this.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9196f30 on tgjones:cocoa-activation into ** on reactiveui:develop**.

@kentcb kentcb added this to the vNext milestone Nov 29, 2017
@kentcb kentcb changed the title Use synchronous activation in iOS view classes fix: activate iOS views synchronously Nov 29, 2017
@kentcb kentcb merged commit a20157d into reactiveui:develop Nov 29, 2017
@tgjones
Copy link
Contributor Author

tgjones commented Nov 29, 2017

Great, thanks for merging!

@tgjones tgjones deleted the cocoa-activation branch November 29, 2017 07:58
@lock lock bot locked and limited conversation to collaborators Jun 25, 2019
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.

5 participants