canGoBack logic fixed according to documentation for Flow::goBack.

[novachevskyi]

Fix for the issue #195.

[novachevskyi]

ReentranceTest::reentrantGoThenBack test has failed because there was attempt to call Flow::goBack when pending traversal state was equal to TraversalState.DISPATCHED. Back navigation shouldn't be performed when there is traversal in a progress.

[Zhuinden]

If flow.goBack() is changed in the test to flow.setHistory(flow.getHistory().buildUpon().pop(1).build(), Direction.BACKWARD), then it'll pass.

[maxxx]

bump for this one. .goBack() will return flow to first key, if called on second while third is adding (pending traversal, dispatched state)

[edenman]

I just ran into this same problem, and my issue is that the approach in this PR is just that sometimes you do want a goBack to happen, just not based on the History during the transition. So in this scenario:
The history looks like [RootScreen, MapScreen]. The MapScreen detects that the user doesn't have a location fix, so it calls set(NoLocationDialogScreen). During that transition, NoLocationDialogScreenView gets a location fix, and calls goBack().

Crashing in this scenario is obviously bad, but I'm not sure the right fix is for that goBack() call to just be a no-op. It's entirely possible that NoLocationDialogScreenView won't get another location fix in a timely manner after the transition finishes, and then the user will be stuck on NoLocationDialogScreen while actually having a valid location fix.

I'd much rather have the goBack be enqueued as a PendingTraversal, but instead of nextHistory being calculated when it's enqueued, PendingTraversal should take a function that could be applied to the History when the pending traversal is about to actually be run. That way, NoLocationDialogScreenView would call goBack, but it would correctly calculate the new backstack based on [RootScreen, MapScreen, NoLocationDialogScreen].

Thoughts?

[rjrjr]

@edenman I agree. There's still a corner case, though: what should happen if the history size is 1 by the time the function fires? We can't return false, there's no one watching.

My inclination is that calling goBack() during a transition:

• Always returns true
• Enqueues a function like you said
• The function no-ops if going back is not possible.

If no one shouts me down I'll implement that.

cc @Zhuinden

[rjrjr]

@edenman your scenario will still be kind of racy though. You're relying on no one else making a surprise call to setHistory(History) or something before the goBack() happens.

I think the real solution to that is something like the Transforms that @ChrisRenke sketched in #191, but that's another yak to shave another day.

[edenman]

@rjrjr even better: expose the "enqueue a function" as a first-class API, so then I can write something like:

flow.setHistory { currentHistory ->
  if (currentHistory.top() is NoLocationDialogScreen) {
    return currentHistory.buildUpon().pop().build()
  } else {
    return null // the enqueued function could return a 'History?' to allow no-op?  Or we could just always return the current History if it's supposed to be a no-op.
  }
}

[rjrjr]

@edenman Heh. I already built exactly that into our dispatcher. It would have been a lot easier if it was built into flow.

@loganj Stop me before I scope creep.

[rjrjr]

@edenman So presuming I succumb to temptation and write the fun thing, which would be in character, that brings us back to the correct behavior for goBack(). With the functional capability in hand I'd be inclined to go conservative on goBack() and use @Zhuinden 's fix -- during a transition return true and ignore the request.

[edenman]

@rjrjr I guess I'm ok with that? I'll probably stop using goBack altogether in favor of something like what I posted above.

[rjrjr]

@edenman @loganj WDYT? #239

[rjrjr]

The more I think about it the more I prefer @Zhuinden's fix for the original goBack() problem, presuming #239 lands. Our app already puts a lot of effort into ignoring UI events during transitions, including the back button, and his fix is in that spirit. Queuing up UI events to happen later, which is effectively what we're talking about with the @edenman alternative, strikes me as being a source of pain a lot more often than not.

[rjrjr]

Aaaand after sleeping on it, I'm 180° (360°?) back to thinking @edenman's original notion is the right one. It will lead to fewer cases where goBack() is a surprising no-op. And the only place where it will no-op is the case where a user would accidentally back all the way out of an unresponsive app.

Sorry for all the noise, but it's my nature.

[rjrjr]

#240

[Zhuinden]

Welp, I regularly check my notifications but apparently I skipped this one? Even though I was cc'd too... 😞

Flow#updateHistory(HistoryUpdater) was interesting because it's technically the ability to write custom operators if set, setHistory and goBack just aren't enough.

I ended up here because of #264 but I'm taking the comment over there.