Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings
This repository was archived by the owner on Feb 26, 2023. It is now read-only.

Clean injected View fields in onDestroyView()#1165

Closed
ghost wants to merge 1 commit into
developandroidannotations/androidannotations:developfrom
unknown repositoryCopy head branch name to clipboard
Closed

Clean injected View fields in onDestroyView()#1165
ghost wants to merge 1 commit into
developandroidannotations/androidannotations:developfrom
unknown repositoryCopy head branch name to clipboard

Conversation

@ghost

@ghost ghost commented Sep 26, 2014

Copy link
Copy Markdown

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
Copy Markdown
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

ghost commented Sep 26, 2014

Copy link
Copy Markdown
Author

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
Copy Markdown
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

ghost commented Sep 27, 2014

Copy link
Copy Markdown
Author

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

@ghost

ghost commented Sep 27, 2014

Copy link
Copy Markdown
Author

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

@WonderCsabo

Copy link
Copy Markdown
Member

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

@ghost

ghost commented Sep 28, 2014

Copy link
Copy Markdown
Author

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

@WonderCsabo

Copy link
Copy Markdown
Member

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

@yDelouis

Copy link
Copy Markdown
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
Copy Markdown
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

ghost commented Sep 29, 2014

Copy link
Copy Markdown
Author

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

@WonderCsabo

Copy link
Copy Markdown
Member

Yes, please.

@ghost

ghost commented Sep 29, 2014

Copy link
Copy Markdown
Author

Here it is: #1171

@WonderCsabo

Copy link
Copy Markdown
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

ghost commented Sep 29, 2014

Copy link
Copy Markdown
Author

Rebased onto wrong branch, should be fine now.

@WonderCsabo

Copy link
Copy Markdown
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
Copy Markdown
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

Morty Proxy This is a proxified and sanitized view of the page, visit original site.