Skip to content
This repository was archived by the owner on Feb 26, 2023. It is now read-only.

Conversation

@ghost
Copy link

@ghost ghost commented Sep 26, 2014

If setRetainInstance(true) has been set on the fragment or
addToBackStack() was applied to FragmentTransaction, the fragment won't be
destroyed and will keep the reference to its parent view in contentView_
and could lead to memory leak.

This change implements onDestroyView() which cleans the reference.
However, the developer is still responsible for cleaning up the injected
views, because otherwise a (badly implemented) background task may get a
NullPointerException.

@WonderCsabo
Copy link
Member

I think we really should clean up the injected View fields. Touching the View fields in a background task is a very bad idea, also i think it is very rare. If somebody's app will break, he/she can examine the generated code. For clarification, i suggest to add some javadocs to the generated onDestroyView to explain the reason.

@ghost
Copy link
Author

ghost commented Sep 26, 2014

The background thread does not need to access views directly, it may just call a @UiThread-decorated method that will. I'll try to make the change you propose, but since I don't yet have deep understanding of how does AndroidAnnotations work, this may take a while.

@WonderCsabo
Copy link
Member

Yes, you are right. To almost properly fix that issue, the developer can add @IgnoreWhenDetached annotation to the @UiThread decorated method, so it won't be called if the Fragment is detached. It will be still a problem if the method is called between onDestroyView and onDetach.

@ghost
Copy link
Author

ghost commented Sep 27, 2014

What would be the safe way then? A new annotation like @IgnoreWhenViewDestroyed?

@ghost
Copy link
Author

ghost commented Sep 27, 2014

Please review this commit carefully, as I may have missed something important.

@WonderCsabo
Copy link
Member

I think this is good. But i would not merge without @yDelouis taking a look at this.

@ghost
Copy link
Author

ghost commented Sep 28, 2014

Wait, I think I did indeed miss something. Will push a commit in few minutes.

@WonderCsabo
Copy link
Member

Good point, actually i was thinking about, too, but forgot to mention.
Can you squash the last two commits?

@yDelouis
Copy link
Contributor

By cleaning injected views, we will make some existing app crash.
I'm ok with cleaning contentView_. But, shouldn't we wait for version 4.0 to be released to clean the injected views ?

@WonderCsabo
Copy link
Member

@yDelouis, i guess you are right.

But we have to release 4.0.0 with the new plugin system. When do you have time to continue work on that?

@ghost
Copy link
Author

ghost commented Sep 29, 2014

Shall I make another PR only with the first (safe) commit then?

@WonderCsabo
Copy link
Member

Yes, please.

@ghost
Copy link
Author

ghost commented Sep 29, 2014

Here it is: #1171

@WonderCsabo
Copy link
Member

Great. Could you rebase this branch (since the first PR got merged)?

@WonderCsabo WonderCsabo changed the title Clean contentView_ reference in onDestroyView() Clean injected View fields in onDestroyView() Sep 29, 2014
@ghost
Copy link
Author

ghost commented Sep 29, 2014

Rebased onto wrong branch, should be fine now.

@WonderCsabo
Copy link
Member

Can you rebase this?

WonderCsabo added a commit to WonderCsabo/androidannotations that referenced this pull request Jun 10, 2015
WonderCsabo added a commit to WonderCsabo/androidannotations that referenced this pull request Jun 10, 2015
Clean refs to injected views in onDestroyView()
@WonderCsabo
Copy link
Member

Rebased and merged as of faaaae5.

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.

2 participants