Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Workflows started with setContentWorkflow may drop early output emissions. #430

Closed
rjrjr opened this issue Jun 28, 2019 · 5 comments · Fixed by #468
Closed

Workflows started with setContentWorkflow may drop early output emissions. #430

rjrjr opened this issue Jun 28, 2019 · 5 comments · Fixed by #468
Assignees
@rjrjr
Labels
kotlin Affects the Kotlin library.
Milestone
kotlin v0.19.0

Comments

@rjrjr
Copy link
Contributor

rjrjr commented Jun 28, 2019

I'm not thrilled with this extension method any more, but have no change to suggest for this PR. I'll follow up after you merge.

Originally posted by @rjrjr in #426
@rjrjr rjrjr added this to the kotlin v0.18.0 milestone Jun 28, 2019
@rjrjr rjrjr self-assigned this Jun 28, 2019
@zach-klippenstein zach-klippenstein mentioned this issue Jun 28, 2019
Merged
@rjrjr rjrjr modified the milestones: kotlin v0.18.0, kotlin v0.19.0 Jun 28, 2019
@rjrjr
Copy link
Contributor Author

rjrjr commented Jun 28, 2019

More thoughts on this:

  • Worst problem is it's difficult / impossible to receive all output events when you're setting things up from onCreate in an Activity or Fragment
  • Surprising that setContentWorkflow() now returns an object whose start() method must be called. Should just drop the extensions, they're too clever.
  • Confusing that workflow is provided on every call to setContentWorkflow. It's only needed the first time, should make that a factory lambda

@zach-klippenstein
Copy link
Collaborator

This issue name is obsolete: WorkflowRunner no longer has a start method, it starts the workflow host lazily when one of the outputs is subscribed to, like it did before.

As for outputs, we should hook into the lifecycle of whatever is creating/retrieving the workflow runner (activity or fragment) and either buffer or pause (via backpressure) the emission of outputs when the lifecycle owner isn't active. And since that caller will be subscribing to outputs from an activity or component, the subscription will likely close over that object and we should be careful to not invoke it in a bad state.

@zach-klippenstein zach-klippenstein changed the title setContentWorkflow doesn't jibe well with new explicit start. Workflows started with setContentWorkflow may drop early output emissions. Jul 2, 2019
@zach-klippenstein
Copy link
Collaborator

zach-klippenstein commented Jul 2, 2019 •
edited
Loading

I updated the issue name so we can remove from the 0.18 milestone.

@rjrjr
Copy link
Contributor Author

rjrjr commented Jul 3, 2019

As part of the discussion behind #439, we're realizing that this is a pretty easy (and natural) problem for WorkflowRunner to solve. It's job is to ensure that outputs are buffered until someone is subscribed to receive them.

At the same time, we should probably replace its rendering and outputs Rx Observables with LiveData properties.

@rjrjr rjrjr modified the milestones: kotlin v1.0.0, kotlin v0.18.0 Jul 3, 2019
@rjrjr
Copy link
Contributor Author

rjrjr commented Jul 3, 2019

@zach-klippenstein I've added this back to the 0.18.0 milestone. Suddenly it seems like we have the complete pictures, seems like we should implement it all at once and be sure that it works.

rjrjr added a commit that referenced this issue Jul 15, 2019
@rjrjr
Android WorkflowRunner now delivers single result instead of output s…
2d0b7e4 
…tream.

Android's configuration changes make it very difficult to craft an API that
can catch all output values and also avoid double-processing of them, and each
such API we've come up with is very confusing. The cognitive cost  outweighs
the benefit of allowing the Activity or Fragment to consume a stream of
outputs — if that's even a benefit! So far we have no use cases for a stream
v. a single result code that can't be better handled inside the root workflow.

Closes #430.
rjrjr added a commit that referenced this issue Jul 15, 2019
@rjrjr
Android WorkflowRunner now delivers single result instead of output s…
ea4a455 
…tream.

Android's configuration changes make it very difficult to craft an API that
can catch all output values and also avoid double-processing of them, and each
such API we've come up with is very confusing. The cognitive cost  outweighs
the benefit of allowing the Activity or Fragment to consume a stream of
outputs — if that's even a benefit! So far we have no use cases for a stream
v. a single result code that can't be better handled inside the root workflow.

Note that we update the Tic Tac Toe sample to take advantage. We also
(nearly) make it's back button work properly on the initial screen, but
that is blocked by #466.

Closes #430.
rjrjr added a commit that referenced this issue Jul 15, 2019
@rjrjr
WorkflowRunner now delivers single result instead of output stream.
6ca1f19 
Android's configuration changes make it very difficult to craft an API that
can catch all output values and also avoid double-processing of them, and each
such API we've come up with is very confusing. The cognitive cost  outweighs
the benefit of allowing the Activity or Fragment to consume a stream of
outputs — if that's even a benefit! So far we have no use cases for a stream
v. a single result code that can't be better handled inside the root workflow.

Note that we update the Tic Tac Toe sample to take advantage. We also
(nearly) make it's back button work properly on the initial screen, but
that is blocked by #466.

Closes #430.
@rjrjr rjrjr mentioned this issue Jul 15, 2019
Closed
rjrjr added a commit that referenced this issue Jul 16, 2019
@rjrjr
WorkflowRunner now delivers single result instead of output stream.
cb93501 
Android's configuration changes make it very difficult to craft an API that
can catch all output values and also avoid double-processing of them, and each
such API we've come up with is very confusing. The cognitive cost  outweighs
the benefit of allowing the Activity or Fragment to consume a stream of
outputs — if that's even a benefit! So far we have no use cases for a stream
v. a single result code that can't be better handled inside the root workflow.

Note that we update the Tic Tac Toe sample to take advantage. We also
(nearly) make it's back button work properly on the initial screen, but
that is blocked by #466.

Closes #430.
rjrjr added a commit that referenced this issue Jul 16, 2019
@rjrjr
WorkflowRunner now delivers single result instead of output stream.
80ee434 
Android's configuration changes make it very difficult to craft an API that
can catch all output values and also avoid double-processing of them, and each
such API we've come up with is very confusing. The cognitive cost  outweighs
the benefit of allowing the Activity or Fragment to consume a stream of
outputs — if that's even a benefit! So far we have no use cases for a stream
v. a single result code that can't be better handled inside the root workflow.

Note that we update the Tic Tac Toe sample to take advantage. We also
(nearly) make it's back button work properly on the initial screen, but
that is blocked by #466.

Closes #430.
@rjrjr rjrjr mentioned this issue Jul 16, 2019
Merged
rjrjr added a commit that referenced this issue Jul 16, 2019
@rjrjr
WorkflowRunner now delivers single result instead of output stream.
bf18640 
Android's configuration changes make it very difficult to craft an API that
can catch all output values and also avoid double-processing of them, and each
such API we've come up with is very confusing. The cognitive cost  outweighs
the benefit of allowing the Activity or Fragment to consume a stream of
outputs — if that's even a benefit! So far we have no use cases for a stream
v. a single result code that can't be better handled inside the root workflow.

Note that we update the Tic Tac Toe sample to take advantage. We also
(nearly) make it's back button work properly on the initial screen, but
that is blocked by #466.

Closes #430.

Fixed result

f
rjrjr added a commit that referenced this issue Jul 16, 2019
@rjrjr
WorkflowRunner now delivers single result instead of output stream.
5b4cc94 
Android's configuration changes make it very difficult to craft an API that
can catch all output values and also avoid double-processing of them, and each
such API we've come up with is very confusing. The cognitive cost  outweighs
the benefit of allowing the Activity or Fragment to consume a stream of
outputs — if that's even a benefit! So far we have no use cases for a stream
v. a single result code that can't be better handled inside the root workflow.

Note that we update the Tic Tac Toe sample to take advantage. We also
(nearly) make it's back button work properly on the initial screen, but
that is blocked by #466.

Closes #430.
rjrjr added a commit that referenced this issue Jul 16, 2019
@rjrjr
WorkflowRunner now delivers single result instead of output stream.
74707b2 
Android's configuration changes make it very difficult to craft an API that
can catch all output values and also avoid double-processing of them, and each
such API we've come up with is very confusing. The cognitive cost  outweighs
the benefit of allowing the Activity or Fragment to consume a stream of
outputs — if that's even a benefit! So far we have no use cases for a stream
v. a single result code that can't be better handled inside the root workflow.

Note that we update the Tic Tac Toe sample to take advantage. We also
(nearly) make it's back button work properly on the initial screen, but
that is blocked by #466.

Closes #430.
@rjrjr rjrjr modified the milestones: kotlin v0.18.0, kotlin v0.19.0 Jul 16, 2019
@zach-klippenstein zach-klippenstein added the kotlin Affects the Kotlin library. label Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rjrjr rjrjr

Labels
kotlin Affects the Kotlin library.
Projects
None yet
Milestone
kotlin v0.19.0
2 participants
@zach-klippenstein @rjrjr