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

Mediator.initialize() called out of order #161

Closed
bholloway opened this issue Mar 6, 2014 · 7 comments
Closed

Mediator.initialize() called out of order #161

bholloway opened this issue Mar 6, 2014 · 7 comments

Comments

@bholloway
Copy link

Please refer to file:

robotlegs.bender.extensions.mediatorMap.impl.MedaitorManager.as:93

Where there are re-parenting issues in a flex module it seems to be possible for the mediator to be removed from stage before creation complete occurs. This causes Mediator.destroy() to come before Medaitor.initialize().

On line 93 we see that the creation complete handler is added which will trigger mediator initialization. This handler is an anonymous function and is only removed when fired. I'm not against anonymous functions however in this particular case I would suggest changing this handler to a member function which is then removing during MedaitorManager.removeMediator().

This would then cause Medaitor.destroy() to be fired without Medaitor.initialize() in the case in question. I myself do not see this as a problem - I like to null my injected properties when destroy() occurs.

Please forgive me if this request is in any way impolite. I'm a big fan of this project but have never contributed to an open source project and do not know the correct etiquette.

Best regards,

Ben H.

@bholloway
Copy link
Author

Sorry I have been premature in this post. I now see why the function is anonymous and that there is indeed a check for this condition. I will investigate my code further to see what is going on.

@bholloway
Copy link
Author

So it turns out that the problem is line 97:

if (_factory.getMediator(displayObject, mapping))

In that case that re-parenting occurs then there will be a creation complete handler for the old (destroyed) mediator as well as for the new mediator. Both will fire, but the old one will fire first. When the above condition is evaluated there will indeed exist a mapping for the new mediator. However it will allow the old mediator to initialize().

The condition needs to be slightly stronger, namely:

if (_factory.getMediator(displayObject, mapping) == mediator)

This means that the initialize() will only be called for the mediator that is currently mapped. Which i believe is what was originally intended.

Hope this helps.

Ben H.

@darscan
Copy link
Member

darscan commented Mar 6, 2014

Hell Ben,

Please forgive me if this request is in any way impolite.

Not at all! This is exactly how Open Source works. Anyone is free to dig in, suggest fixes, write patches etc.

Your investigation sounds good. Have you tried to patch it locally and see if it fixes the problem?

@bholloway
Copy link
Author

Yes. I am running this fix locally and it has the desired effect. However I do not want to orphan my local source code so if you have a different solution then I will align with that.

It strikes me that the proposed fix is more 'correct' if you know what I mean - we are wanting the specific case that the mediator hasn't "been removed in the meantime". As it was we are testing the corresponding view for the existence of any mediator. This is not equivalent.

Having a quick look at the unit test it seems that reparenting is an edge case that is not covered. The exact case I am experiencing is that the component is mediated twice, due to being re-added to stage, and then creation complete follows that.

The offender seems to be an mx:ViewStack with clipContent=true (default) or more generally the mx:Container. It reparents its children when it creates its content pane.

mx_internal function createContentPane():void

Unfortunately I cannot remove these mx components in our legacy code. To make matters worse there is a race condition somewhere and the problem does not always occur. As such I am not sure it requires another unit test. I think it depends on how easily one can make a test that fails the current implementation.

Cheers,

Ben H.

@darscan
Copy link
Member

darscan commented Mar 7, 2014

Hiya, I'll take a look into writing up a unit test for this later today (or over the weekend).

@darscan darscan closed this as completed in 35bfcf0 Mar 8, 2014
@darscan
Copy link
Member

darscan commented Mar 8, 2014

Hi Ben,

I've rolled this in and released a new version of RL (2.2.0) with the fix. Specifically:

  1. Destroy should not fire out of order during reparenting
  2. Initialize should not fire for removed mediator (as per your fix)

Let me know how it goes.

@bholloway
Copy link
Author

Nice work!

Updated our code base to RL 2.2.0: My specific problem (above) is no longer an issue with this release. Will live with it for a while and let you know if there are any issues.

Ben H.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants