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

Use isFinishing instead of isChangingConfigurations #304

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

johnwilde
Copy link

@johnwilde johnwilde commented Feb 1, 2018

When the activity is destroyed by the system (e.g. testing with "Don't keep activities" enabled) presenter.destroy() is not being called because isFinishing is false. However PresenterManager just checks isChangingConfigurations() so the cache is reset and the presenter is not retrieved when the activity is recreated. So a new presenter is created and now we have two presenter instances.

This changes the logic to only rely on isFinishing to destroy the presenter and reset the cache.

Alternatively, maybe the isFinishing check should just be removed?

  static boolean retainPresenterInstance(boolean keepPresenterInstance, Activity activity) {
    return keepPresenterInstance && activity.isChangingConfigurations();
  }

@sockeqwe
Copy link
Owner

sockeqwe commented Feb 6, 2018

Thanks, I will take a look this week.

Howerver, from some older issues / comments in this issue tracker it seems that people are expexting to recreate the presenter with "Don't keep Activities". Any opinion?

@johnwilde
Copy link
Author

johnwilde commented Feb 6, 2018

I looked through the older issues and came across your comment #256 (comment) which was very helpful because I didn't realize that "real world" behavior will kill the app process (and we won't end up with multiple presenters). It seems you're aware of the issue and it will only happen when testing with "don't keep activities."

I don't think I found the issue/comments in which people are expecting to recreate the presenter. It's unfortunate that "don't keep activities" doesn't match what happens in reality. I'm not sure this change is a good idea after all because this now treats it essentially the same as a configuration change. However, not destroying the presenter is a potential source of confusion for developers trying to test with that option enabled - maybe removing the isFinishing check would make it clearer that retaining the presenter is only relevant for configuration changes.

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

Successfully merging this pull request may close these issues.

2 participants