Flow::goBack returns true, when PendingTraversal::state value equals "DISPATCHED"
Closed this issue · 10 comments
There is inconsistency in Flow::goBack
method documentation and it's actual logic. JavaDoc for this method says that it should return false if going back is not possible or a traversal is in progress.
. Currently, it will return true
when PendingTraversal::state
value would be equal to DISPATCHED
, since boolean canGoBack = history.size() > 1 && (pendingTraversal == null || pendingTraversal.state == TraversalState.FINISHED)
is true
in this case.
To fix this issue canGoBack
value write logic should look like this: boolean canGoBack = history.size() > 1 && (pendingTraversal == null || pendingTraversal.state == TraversalState.FINISHED)
.
Please let me know if I'm missing something here or if some other solution should be applied to fix this issue. I will appreciate for your help.
Honestly, if it returns false
while traversal is in progress, then Activity.onBackPressed()
will handle it and result in history state loss.
It's true that in case if we would have false
when PendingTraversal::state
is DISPATCHED
- Activity.onBackPressed()
should be called. But in case if Flow would try to perform back navigation during traversal it would crash with next trace:
java.lang.IllegalArgumentException: History may not be empty
at flow.Preconditions.checkArgument(Preconditions.java:27)
at flow.History.<init>(History.java:50)
at flow.History.<init>(History.java:36)
at flow.History$Builder.build(History.java:189)
at flow.Flow.goBack(Flow.java:261)
Wrong. This crash happens if the Activity handled the back press during traversal.
In my fork, this is how goBack()
works
@CheckResult
public boolean goBack() {
if(pendingTraversal != null && pendingTraversal.state != TraversalState.FINISHED) { //traversal is in progress
return true;
}
boolean canGoBack = history.size() > 1;
if(!canGoBack) {
return false;
}
setHistory(history.buildUpon().pop(1).build(), Direction.BACKWARD);
return true;
}
This change in Flow 1.0-alpha
would require changing the ReentranceTest to use flow.setHistory(flow.getHistory().buildUpon().pop(1).build());
in place of flow.goBack()
.
It's a good point that we shouldn't navigate back with Activity.onBackPressed()
when we have history.size() > 1
. But I think, assuming fix that I've described in my first comment, we can override onBackPressed
in the way:
if (!flow.goBack() && flow.history.size() <= 1) {
super.onBackPressed();
}
I do not think this should be necessary. Flow should handle not saying it didn't handle it when it's still handling it.
But yes, that would also be an adequate workaround.
I'm not sure that I got what you mean with Flow should handle not saying it didn't handle it when it's still handling it.
. Currently Flow is trying to handle back navigation when it shouldn't, at least according to documentation.
The idea of Flow::goBack
is to handle back navigation and in case if there is pending traversal, then Flow::goBack
should return false
since it can't handle back navigation at exact moment. I just saying that all additional logic should be placed inside of Activity::onBackPressed
and Flow::goBack
body should be consistent with it's description.
I think this is a documentation error rather than an implementation error. Flow is handling it, by ignoring it because we're already in the process of going back. We should just fix the javadoc.
edenman disagreed here #197 though, because he wants to say "goBack" during a traversal. So he thinks maybe you should queue a "BACK" pending traversal instead of ignoring it.
I think he's right, responding on the PR.