search

sockeqwe / mosby

A Model-View-Presenter / Model-View-Intent library for modern Android apps
http://hannesdorfmann.com/mosby/
Apache License 2.0
5.49k stars 841 forks source link

Use isFinishing instead of isChangingConfigurations #304

Open johnwilde opened 6 years ago

johnwilde commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

I looked through the older issues and came across your comment https://github.com/sockeqwe/mosby/issues/256#issuecomment-359770008 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.