Add discard-changes dialog when back is pressed when editing a credential. #1062
Conversation
| data class SaveChanges(val item: ServerPassword) : | ||
| ItemDetailAction(TelemetryEventMethod.tap, TelemetryEventObject.update_credential) | ||
| data class DiscardChanges(val itemId: String) : | ||
| ItemDetailAction(TelemetryEventMethod.tap, TelemetryEventObject.back) |
| .distinctUntilChanged() | ||
| .filter { !it } | ||
| .flatMapIterable { | ||
| edited = null |
There was a problem hiding this comment.
Why are we setting this to null on edit?
| ) | ||
| .map { | ||
| edited?.let { ItemDetailAction.SaveChanges(it) } | ||
| ?: ItemDetailAction.DiscardChanges(itemId) |
There was a problem hiding this comment.
If we can't save we want it to fail, not ask them if they want to discard their changes. I don't think that's a logical flow for the user to click ✅ and then be prompted to discard.
There was a problem hiding this comment.
If there are no changes, then we should just exit. DiscardChanges simply exits.
| if (it != credentials) { | ||
| DialogAction.DiscardChangesDialog(itemId) | ||
| } else { | ||
| null |
There was a problem hiding this comment.
Should we fail here too? (if the edited credential is somehow inconsistent with credentials)
There was a problem hiding this comment.
This could be written as
// if the user has changed the credentials
if (edited != null && edited != credentials) {
// display a dialog checking if they want to discard
DialogAction.DiscardChangesDialog(itemId)
} else {
// just go ahead and discard/exit.
ItemDetailAction.DiscardChanges(itemId)
}| } else { | ||
| null | ||
| } | ||
| } ?: ItemDetailAction.DiscardChanges(itemId) |
There was a problem hiding this comment.
Same thought about failing here
There was a problem hiding this comment.
There seems to be a confusion on what ItemDetailAction.DiscardChanges means.
I should rename to FinishEditing or comment it.
| override fun onBackPressed(): Boolean { | ||
| val itemId = credentials?.id ?: return false | ||
| dispatcher.dispatch(checkDismissChanges(itemId)) | ||
| return true | ||
| } |
| @@ -30,6 +30,7 @@ import mozilla.lockbox.flux.Presenter | |||
| import mozilla.lockbox.log | |||
| import mozilla.lockbox.store.RouteStore | |||
| import mozilla.lockbox.view.DialogFragment | |||
| import mozilla.lockbox.view.Fragment as SpecializedFragment | |||
There was a problem hiding this comment.
Interesting 🤔 Why do we want an alias here?
There was a problem hiding this comment.
Hmm. I thought it might be clearer to use an obviously-not-android-fragment name, here. Since we're not using androidx.*.Fragment, it seems it just adds confusion.
| if (!presenter.onBackPressed()) { | ||
| super.onBackPressed() |
There was a problem hiding this comment.
(if I'm understanding this correctly:) if the presenter doesn't have a specific implementation, use super's?
There was a problem hiding this comment.
Yes. The activity's implementation will trigger the back button behaviour.
…tore. This allows us to intercept the back button, discard changes, and save changes coherently and consistently.
jhugman
force-pushed
the
jhugman/994-dismiss-changes-back-button
branch
from
34a4266 to
485d864
Compare
…tial. (#1062) * Suppress dismiss changes dialog if no editing has taken place * Intercept back button pressed, and feed it through to the presenters. * Tease out stopping editing and saving changes in to the item detail store. This allows us to intercept the back button, discard changes, and save changes coherently and consistently. * Address reviewer comments
Fixes #994.
This PR introduces: