Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changed Cocoa activation to trigger on WillAppear instead of DidAppear. #779

Closed
wants to merge 1 commit into from
Closed

Changed Cocoa activation to trigger on WillAppear instead of DidAppear. #779

wants to merge 1 commit into from

Conversation

thedillonb
Copy link
Contributor

My original thought was that activation/deactivation should trigger off of the Will events instead of the Did. However, I think the deactivation makes sense to keep on the Did since you don't technically want to deactivate a component that may be animating offscreen - It has the potential to look weird if deactivation is tied to some visual component.

Fixes #778

Fixes #778

WillAppear allows activation to take place before the view is displayed. This means that handlers or visual components can get into place and transition into view. Otherwise, DidAppear creates a window of inactivity where an animated view may appear void of activation until it's fully in view.
@thedillonb
Copy link
Contributor Author

Hey @paulcbetts, I think this guy is good to go to fix #778. It also would seem to coincide with #783 which has already been merged.

@anaisbetts
Copy link
Member

I need to go through all of the controllers to see if one was missed

@thedillonb
Copy link
Contributor Author

Hey Paul, any update on whether you think this is a viable change?

@petergledhill
Copy link

+1

@thedillonb
Copy link
Contributor Author

Any update on this?

@anaisbetts
Copy link
Member

Still on my list, sorry I'm stalling forever

@thedillonb
Copy link
Contributor Author

Just checking in. Any chance this might see a review before the next ReactuveUI release?

@tgjones
Copy link
Contributor

tgjones commented Oct 15, 2015

Any update on this @paulcbetts? I'm just getting started with ReactiveUI on iOS, but if I create bindings in WhenActivated - which I believe is the best practice, for garbage collection reasons - then they don't execute until after the view is visible on the screen. It's really not a good user experience at the moment - is there a workaround that everyone's using in the meantime?

@tgjones
Copy link
Contributor

tgjones commented Oct 15, 2015

I think there's a related issue with ReactiveView.WillMoveToSuperview - as far as I can tell, it asynchronously does the activation after calling base.WillMoveToSuperview, which at least in my testing, means activation doesn't occur until after the view is visible. Is that a known issue?

This is my first day using ReactiveUI, so apologies if I'm doing something stupid :)

@thedillonb
Copy link
Contributor Author

@paulcbetts, any update on this? In a few more weeks it'll be this PR's one year aniversary

@tgjones
Copy link
Contributor

tgjones commented Dec 16, 2015

And we're now one day away from the one year anniversary... @paulcbetts Any reason this hasn't been merged yet (apart from time needed to do it)? It's quite broken as it is, and this PR fixes it, without any obvious downsides.

@tgjones
Copy link
Contributor

tgjones commented Dec 16, 2015

I've got an alternate version of the changes here: https://github.com/tgjones/ReactiveUI/commit/e5b7ff3dca3ba9890f9c334a2c2eff54a21cae6a. This includes the fix for ReactiveView.ViewWillMoveToSuperview.

@ghuntley ghuntley added this to the ReactiveUI 7.0 milestone Dec 21, 2015
@ghuntley
Copy link
Member

Spoke with @paulcbetts, this PR makes the change to a few of the classes but in order to merge it needs to change all of them. If you could add a few commits then we can go about merging.

ps. right now we can't produce a build until #993 is resolved.

@thedillonb
Copy link
Contributor Author

@ghuntley, thanks for following up with this PR. Any idea what classes this PR is missing? I'm pretty sure this PR has been 100% ready to roll since it was posted. Even today, I cannot find any additional classes that require modification. To the contrary, it looks like newer classes, such as ReactiveNavigationController.cs, added some time after this PR, actually implements it's activation in the ViewWillAppear methods - like this PR is trying to fix - meaning that Cocoa functionality around activation is already not consistent!

I know this is not on you since you just got involved in this, but, the fact that newer classes have been added which implicitly fix this issue while the older classes have been pending on this PR for a year now is less than ideal.

@tgjones
Copy link
Contributor

tgjones commented Dec 23, 2015

@ghuntley @thedillonb As I mentioned above, to be complete, this PR should include the ReactiveView.ViewWillMoveToSuperview change as well. But it is somewhat separate, so you may wish to handle that in a separate PR.

@kentcb
Copy link
Contributor

kentcb commented Oct 29, 2016

Superseded by #1182, which I plan to merge shortly. So sorry this has taken so long, all.

@kentcb kentcb closed this Oct 29, 2016
@ghuntley ghuntley modified the milestone: 7.0.0 Nov 6, 2016
@lock lock bot locked and limited conversation to collaborators Jun 26, 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.

Any reason that cocoa activation takes place after view transition rather than before?
6 participants