Various bugfixes for 1.0 #380
Conversation
| } | ||
|
|
||
| private void finish(int resultCode, Intent data) { | ||
| mActivity.finish(resultCode, data); |
There was a problem hiding this comment.
You'll need to do some more refactoring for this to work. The reason I chose not to pass down the resultCode is because SmartLock used to be an Activity called by other activities (such as RegisterEmailActivity). Those activities didn't care about what the result code was and always returned Activity.RESULT_OK. Say you start the email flow through AuthMethodPickerActivity and end up at RegisterEmailActivity which calls this smart lock code. If saving the credential fails,RegisterEmailActivity will be finished with Activity.RESULT_CANCELED. However, the user is signed in and this line in AuthMethodPickerActivity will prevent the activity from finishing even though the user is singed in. I don't think this is expected behavior.
There was a problem hiding this comment.
I see, but since the resultCode was being ignored finish(RESULT_CANCELLED) like on line 167 would result in finish(RESULT_OK) being called. Maybe it would make sense to change line 167 and line 174 to call finish(RESULT_OK) directly instead?
There was a problem hiding this comment.
There are a few other lines I missed that would need to change to RESULT_OK as well
There was a problem hiding this comment.
Since this is probably going to be a point of confusion in the future, I think we should just remove the option to set a result at all and always pass down RESULT_OK. What do you think?
There was a problem hiding this comment.
Sounds good to me, done!
|
LGTM |
Did a manual run through of a bunch of test cases and uncovered some issues which have been fixed here. As well as addressed some internal feedback we received.
fixed: