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

Fix bug #192: crash when home button pressed during quick add task sa… #255

Closed
wants to merge 3 commits into from
Closed

Fix bug #192: crash when home button pressed during quick add task sa… #255

wants to merge 3 commits into from

Conversation

lemonboston
Copy link
Contributor

…ved message displayed to user. (Solution from http://stackoverflow.com/a/21701607/4247460)

@dmfs
Copy link
Owner

dmfs commented Aug 18, 2016

Sorry, that doesn't look like a solution, but more like a hack.
From the stack trace I'd say that the issue caused by a fragment transaction that's started after the activity has been stopped or suspended. A fix makes sure there is no further transaction started after onSaveInstanceState was called.

@lemonboston
Copy link
Contributor Author

Yes, I debugged it and it happens when QuickAddDialogFragment is dismissed automatically with delay after the 'Saved' notification was shown to user, and when in the meantime the app was put to background (home button pressed), so the activity had stopped.

(Using dismissAllowingStateLoss() (which uses FragmentTransaction.commitAllowingStateLoss()) for this case is suggested here as well:
http://stackoverflow.com/a/10275071/4247460)

At first I thought the possible state loss with commitAllowingStateLoss() is only about the state of the fragment, which would be okay for a removed fragment, because it won’t be restored anyway. But I see in the docs now that it might loose the actual commit itself as well..
So you are probably right that its usage should be avoided then, I will look into this further.

@lemonboston
Copy link
Contributor Author

lemonboston commented Aug 19, 2016

After looking into this more, I think this bug may tell that a different approach might be needed for this delayed dialog updates:
It could be argued that a Fragment transaction (as a DialogFragment dismiss) is a major UI state update which shouldn't happen while the app is in background, so any delayed transaction should be avoided because user can put it to background any time.
This is a very good post on the topic, the section "Avoid performing transactions inside asynchronous callback methods" refers to this generally:
http://www.androiddesignpatterns.com/2013/08/fragment-transaction-commit-state-loss.html
There is also a link inside from Dianne Hackborn on a similar issue:
https://groups.google.com/forum/#!msg/android-developers/dXZZjhRjkMk/QybqCW5ukDwJ

I see the following options now:

  1. When "SAVE" pressed, instantly dismiss and use just a Toast for the message.
    Con: There is a subtle bug with "SAVE AND CONTINUE" as well, if app is put to background after that, the keyboard comes up after the delay on the home screen. It wouldn't solve that (although for that we may only need to remove mEditText.requestFocus(); Also the 2 kind of saves would have different UX.
    Pro: Quick save is quicker
  2. Introduce boolean state flags and either dismiss dialog in onPause() if it is in 'closing' state, or dismiss after onResume() if it was in 'closing' state (with delay or not).
    Cons:
    Maybe not guaranteed to work pre-Honeycomb (see the table at the post).
    More complexity, feels hacky, can be error prone if this approach is continued.
  3. Use a bit more general solution for "pausing" fragment transaction while app is in background and resuming after resume: http://stackoverflow.com/questions/8040280/how-to-handle-handler-messages-when-activity-fragment-is-paused
    (again, not sure about pre-Honeycomb)
  4. Handle this "Task created" UI message somehow differently. Not sure how.
    If it was a Dialog, not a DialogFragment, there would be no problem, but we need the Fragment here.
    A separate Dialog for this message? Not sure..

@lemonboston
Copy link
Contributor Author

lemonboston commented Aug 19, 2016

In the last commit I used Option 2., so dismiss() in onPause() if the delayed dismiss has been scheduled already.
I suppose this would be the solution with the least change in code and UI to fix this bug.

According to the post there is still a chance in pre-Honeycomb to loose state but it won't throw exception.
I suppose that's fine.

@lemonboston
Copy link
Contributor Author

@dmfs Please review.
The solution is to dismiss() the DialogFragment in onPause() if the delayed dismiss() was already scheduled. (A new boolean state flag is used for that.)

@dmfs
Copy link
Owner

dmfs commented Aug 31, 2016

Looks good. Merged into staging.

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