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

the previous implemention has some assumption which is not always … #3

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

Conversation

whmbuaa
Copy link

@whmbuaa whmbuaa commented Apr 27, 2016

…true.

the previous implemention assumes below is true:

  1. ActivityA->ActivityB
    1. press back
      2.1 ActivityA.onStart
      2.1 ActivityB.onStop
      otherwize, the behavior will be wrong.

unfornautely, this assumption is not always true.
So I make a new version.
Please review and give your comments.
Thanks!

…true.

the previous implemention has a pre-condition:
  1. ActivityA->ActivityB
   2. press back
       2.1   ActivityA.onStart
       2.1   ActivityB.onStop
otherwize, the behavior will be wrong.

unfornautely, this assumption is not always true. 
So I make a new version.   
Please review and give your comments.
Thanks!
@steveliles
Copy link
Owner

First of all, thanks for taking the time to submit a pull request! - I'm fully prepared to believe it doesn't work in all cases - unfortunately I never had time to test it thoroughly. I've enjoyed reading your version and thinking about the problem again :)

I like the use of a stack to track the activity backstack, and your version is certainly simpler and cleaner without the non-deterministic handler.postDelayed nonsense.

I can't spot anything in the existing implementation that makes the assumption you described, since it only ever compares against the most recently started activity ... am I missing something? I guess you have encountered situations where the current implementation fails and your version works ... can you provide a quick description of those? Is this related to issue #4, or is it just a coincidence that that issue was opened within minutes of your pull request?

The change of API (removing Binding in favour of removeListener) probably needs a major version bump. Might be safer to keep the Binding but mark its usage as deprecated.

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