WorkflowLayout.start needs a Lifecycle, BackStackScreen is supported. #632

rjrjr · 2022-01-18T20:41:07Z

On a lot of Samsung devices running Anroid 12 / API 30, the sketchy WorkflowLayout.takeWhileAttached method at the heart of WorkflowLayout.start was having surprising effects -- we'd see redundant calls to OnAttachStateChangeListener.onViewAttachedToWindow. The problem goes away if we get more conventional and use the new repeatOnLifecycle function as described in this blog post.

The old methods are deprecated and will be deleted soon.

Making this change required making our BackPressHandler mechanism more robust, as it too was pretending that OnAttachStateChangeListener was an adequate lifecycle. Specifically, we were relying on onViewAttachedToWindow to be called in a predicable order, which stopped being the case with the move away from WorkflowLayout.takeWhileAttached. This PR changes things so that…

We register handlers immediately, so that we can count on them being stacked in the expected order
We lean on OnAttachStateChangeListener solely to enable and disable the BackPressHandler associated with a view
And take advantage of our recently added support for ViewTreeLifecycleOwner for clean up

ModalContainer's contribution to coping with the back button also had to be beefed up, for similar reasons. It needs to ensure that back button events are blocked by modal windows, just like any other event, which is tricky with the singleton OnBackPressedDispatcher that androidx provides. It does this by putting a no-op handler in place as a backstop for any set by modal renderings. Its bespoke code for doing this was also inadvertently relying on the order of onViewAttachedToWindow calls. We now achieve this by wrapping modal renderings with a reliable BackStackScreen, which is promoted from the sample container set.

We make the cored update method for `WorkflowLayout` public to give apps complete control over how exactly they collect renderings. Doing this because the `takeWhileAttached` method used by `start` is causing problems. Opening this up lets those impacted by these problems avoid them. In a downstream release, we will upgrade `androidx.lifecycle:lifecycle-viewmodel` and update the convenient `start` to use its improved API. Details in #632.

We make the core update method for `WorkflowLayout` public to give apps complete control over how exactly they collect renderings. Doing this because the `takeWhileAttached` method used by `start` is causing problems. Opening this up lets those impacted by these problems avoid them. In a downstream release, we will upgrade `androidx.lifecycle:lifecycle-viewmodel` and update the convenient `start` to use its improved API. Details in #632.

We make the core `update` method for `WorkflowLayout` public to give apps complete control over how exactly they collect renderings. Doing this because the `takeWhileAttached` method used by `start` is causing problems. Opening this up lets those impacted by these problems avoid them. In a downstream release, we will upgrade `androidx.lifecycle:lifecycle-viewmodel` and update the convenient `start` to use its improved API. Details in #632.

rjrjr · 2022-01-25T18:54:26Z

Cool, should have left this as a draft:

Starting 7 tests on Pixel_4_API_30(AVD) - 11

com.squareup.sample.TicTacToeEspressoTest > configChangePreservesBackStackViewStateCache[Pixel_4_API_30(AVD) - 11] FAILED 
        androidx.test.espresso.NoMatchingViewException: No views in hierarchy found matching: with id is <com.squareup.sample.tictacworkflow:id/login_email>
        

com.squareup.sample.TicTacToeEspressoTest > showRenderingTagStaysFresh[Pixel_4_API_30(AVD) - 11] FAILED 
        leakcanary.NoLeakAssertionFailedError: Application memory leaks were detected:
        ====================================

Thanks @pyricau!

rjrjr · 2022-01-25T19:04:50Z

Meh, I think the LeakCanary bit is noise: seems like the NoMatchingViewException exception from the previous test is what is leaking, nothing in the library code:

├─ androidx.test.espresso.NoMatchingViewException instance
│    Leaking: UNKNOWN
│    Retaining 219.1 kB in 3819 objects
│    ↓ NoMatchingViewException.rootView
│                              ~~~~~~~~

rjrjr · 2022-01-25T19:37:36Z

The real broken test is because the change somehow breaks the device back button after config changes in (at least) the Tic Tac Toe sample.

rjrjr · 2022-01-25T19:40:24Z

Maybe it's about breaking the back button on modals after config change. Good times.

...amples/src/main/java/com/squareup/sample/compose/hellocomposebinding/HelloBindingActivity.kt

...c/main/java/com/squareup/sample/compose/hellocomposeworkflow/HelloComposeWorkflowActivity.kt

...low-ui/container-android/src/main/java/com/squareup/workflow1/ui/modal/ModalViewContainer.kt

steve-the-edwards · 2022-01-31T15:49:42Z

workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/BackButtonScreen.kt

+@WorkflowUiExperimentalApi
+public class BackButtonScreen<W : Any>(
+  public val wrapped: W,
+  public val override: Boolean = false,


nit: now that this is promoted can we not use a reserved keyword? Maybe overrideWrappedBackHandling? Too verbose?

How about shadow?

Hmm shadow seems vague. Maybe with good kdoc we'd be fine :)

* @param shadow If `true`, [onBackPressed] is set as the * [backPressedHandler][android.view.View.backPressedHandler] after * the [wrapped] rendering's view is built / updated, effectively overriding it. * If false (the default), [onBackPressed] is set afterward, to allow the wrapped rendering to * take precedence if it sets a `backPressedHandler` of its own -- the handler provided * here serves as a default.

workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/BackPressHandler.kt

workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/WorkflowLayout.kt

steve-the-edwards · 2022-01-31T16:00:13Z

workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/WorkflowLayout.kt

+    lifecycle.coroutineScope.launch {
+      lifecycle.repeatOnLifecycle(Lifecycle.State.STARTED) {
+        renderings.collect { update(it, environment) }
+      }
+    }


This is quite tidy and makes very clear sense!

I'm really grateful they cleaned this up. The androidx Lifecycle stuff is flawed, but its sooo much better than everything that came before it.

workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/WorkflowLayout.kt

On a lot of Samsung devices running Anroid 12 / API 30, the sketchy `WorkflowLayout.takeWhileAttached` method at the heart of `WorkflowLayout.start` was having surprising effects -- we'd see redundant calls to `OnAttachStateChangeListener.onViewAttachedToWindow`. The problem goes away if we get more conventional and use the new `repeatOnLifecycle` function as described in [this blog post](https://medium.com/androiddevelopers/a-safer-way-to-collect-flows-from-android-uis-23080b1f8bda). The old methods are deprecated and will be deleted soon. Making this change required making our `BackPressHandler` mechanism more robust, as it too was pretending that `OnAttachStateChangeListener` was an adequate lifecycle. Specifically, we were relying on `onViewAttachedToWindow` to be called in a predicable order, which stopped being the case with the move away from `WorkflowLayout.takeWhileAttached`. This PR changes things so that… - We register handlers immediately, so that we can count on them being stacked in the expected order - We lean on `OnAttachStateChangeListener` solely to enable and disable the `BackPressHandler` associated with a view - And take advantage of our recently added support for `ViewTreeLifecycleOwner` for clean up `ModalContainer`'s contribution to coping with the back button also had to be beefed up, for similar reasons. It needs to ensure that back button events are blocked by modal windows, just like any other event, which is tricky with the singleton `OnBackPressedDispatcher` that androidx provides. It does this by putting a no-op handler in place as a backstop for any set by modal renderings. Its bespoke code for doing this was also inadvertently relying on the order of `onViewAttachedToWindow` calls. We now achieve this by wrapping modal renderings with a reliable `BackStackScreen`, which is promoted from the sample container set.

rjrjr · 2022-01-31T17:17:12Z

Update addresses all of @steve-the-edwards nits but one -- named params on the various start() calls seem soooo redundant.

steve-the-edwards

Thanks! LGTM

rjrjr force-pushed the ray/embrace-the-viewmodel branch 3 times, most recently from 7bf2fd6 to 2d789d1 Compare January 19, 2022 22:50

rjrjr changed the title ~~WorkflowLayout now relies on Lifecycle.~~ WorkflowLayout.start now relies on Lifecycle. Jan 19, 2022

rjrjr force-pushed the ray/embrace-the-viewmodel branch from 2d789d1 to bedf43a Compare January 19, 2022 22:56

rjrjr changed the base branch from main to ray/upgrades January 19, 2022 23:04

rjrjr force-pushed the ray/upgrades branch 2 times, most recently from 1c65e56 to 1a9569b Compare January 20, 2022 00:33

rjrjr force-pushed the ray/embrace-the-viewmodel branch from bedf43a to 3c12d95 Compare January 20, 2022 00:35

rjrjr mentioned this pull request Jan 20, 2022

Don't hard code Dispatchers.Main.immediate #600

Closed

rjrjr force-pushed the ray/upgrades branch from 1a9569b to df884e6 Compare January 20, 2022 01:10

rjrjr force-pushed the ray/embrace-the-viewmodel branch from 3c12d95 to 579230a Compare January 20, 2022 01:11

rjrjr force-pushed the ray/upgrades branch from df884e6 to e597b33 Compare January 20, 2022 01:32

rjrjr force-pushed the ray/embrace-the-viewmodel branch from 579230a to 417bd53 Compare January 20, 2022 01:32

rjrjr mentioned this pull request Jan 20, 2022

Introduces WorkflowLayout.update. #638

Merged

rjrjr force-pushed the ray/upgrades branch 2 times, most recently from aec5713 to 03ab545 Compare January 25, 2022 17:44

Base automatically changed from ray/upgrades to main January 25, 2022 18:28

rjrjr force-pushed the ray/embrace-the-viewmodel branch from 417bd53 to 1ffc126 Compare January 25, 2022 18:34

rjrjr changed the title ~~WorkflowLayout.start now relies on Lifecycle.~~ WorkflowLayout.start now requires a Lifecycle parameter. Jan 25, 2022

rjrjr force-pushed the ray/embrace-the-viewmodel branch from 1ffc126 to 4eb2c07 Compare January 25, 2022 18:34

rjrjr marked this pull request as ready for review January 25, 2022 18:35

rjrjr requested review from zach-klippenstein and a team as code owners January 25, 2022 18:35

rjrjr force-pushed the ray/embrace-the-viewmodel branch from 4eb2c07 to 0785914 Compare January 25, 2022 18:36

rjrjr force-pushed the ray/embrace-the-viewmodel branch from 0785914 to 705d753 Compare January 25, 2022 18:49

rjrjr marked this pull request as draft January 25, 2022 18:54

pyricau mentioned this pull request Jan 28, 2022

Reset weak refs in between tests square/leakcanary#2297

Closed

rjrjr force-pushed the ray/embrace-the-viewmodel branch 3 times, most recently from 3e83946 to c2434a4 Compare January 29, 2022 01:15

rjrjr changed the title ~~WorkflowLayout.start now requires a Lifecycle parameter.~~ WorkflowLayout.start needs a Lifecycle, BackStackScreen is supported. Jan 29, 2022

rjrjr force-pushed the ray/embrace-the-viewmodel branch 2 times, most recently from 8b51bba to 0176bb4 Compare January 29, 2022 17:56

rjrjr marked this pull request as ready for review January 29, 2022 17:56

rjrjr assigned steve-the-edwards and RBusarow Jan 29, 2022

steve-the-edwards reviewed Jan 31, 2022

View reviewed changes

rjrjr force-pushed the ray/embrace-the-viewmodel branch from 0176bb4 to 01eb576 Compare January 31, 2022 17:16

steve-the-edwards approved these changes Jan 31, 2022

View reviewed changes

rjrjr merged commit 61b94d1 into main Jan 31, 2022

rjrjr deleted the ray/embrace-the-viewmodel branch January 31, 2022 19:31

rjrjr mentioned this pull request Oct 27, 2022

BackPressHandler leaks the view after its detached #889

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WorkflowLayout.start needs a Lifecycle, BackStackScreen is supported. #632

WorkflowLayout.start needs a Lifecycle, BackStackScreen is supported. #632

rjrjr commented Jan 18, 2022 •

edited

rjrjr commented Jan 25, 2022

rjrjr commented Jan 25, 2022

rjrjr commented Jan 25, 2022

rjrjr commented Jan 25, 2022

steve-the-edwards Jan 31, 2022

rjrjr Jan 31, 2022

steve-the-edwards Jan 31, 2022

rjrjr Jan 31, 2022

steve-the-edwards Jan 31, 2022

rjrjr Jan 31, 2022

rjrjr commented Jan 31, 2022

steve-the-edwards left a comment

WorkflowLayout.start needs a Lifecycle, BackStackScreen is supported. #632

WorkflowLayout.start needs a Lifecycle, BackStackScreen is supported. #632

Conversation

rjrjr commented Jan 18, 2022 • edited

rjrjr commented Jan 25, 2022

rjrjr commented Jan 25, 2022

rjrjr commented Jan 25, 2022

rjrjr commented Jan 25, 2022

steve-the-edwards Jan 31, 2022

Choose a reason for hiding this comment

rjrjr Jan 31, 2022

Choose a reason for hiding this comment

steve-the-edwards Jan 31, 2022

Choose a reason for hiding this comment

rjrjr Jan 31, 2022

Choose a reason for hiding this comment

steve-the-edwards Jan 31, 2022

Choose a reason for hiding this comment

rjrjr Jan 31, 2022

Choose a reason for hiding this comment

rjrjr commented Jan 31, 2022

steve-the-edwards left a comment

Choose a reason for hiding this comment

rjrjr commented Jan 18, 2022 •

edited