square/flow

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.

rjrjr commented

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.

rjrjr commented

Ah, I read through this too quickly. I think I'm agreeing with @Zhuinden 's fix.

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.

rjrjr commented

I think he's right, responding on the PR.