DialogCollator destroys/rebuild instead of dismiss/show

[rjrjr]

When DialogCollator has to reorder, it did so by calling Dialog.dismiss() and Dialog.show(). #1213 fixed a problem that caused for ComposeView, but that fix is undone by Compose 1.6.8. @vumngoc dug in to see why and discovered:

The onClick in Modifier.clickable is passed all the way down to pointerInputHandler in SuspendingPointerInputModifierNodeImpl (when the screen is responsive, you should see pointerInputHandler() being called in SuspendingPointerInputModifierNodeImpl#onPointerEvent).

After shuffling the overlays, even though the ClickableNode is found and SuspendingPointerInputModifierNodeImpl#onPointerEvent is called, pointerInputHandler() is never called because the pointerInputJob in SuspendingPointerInputModifierNodeImpl was launched from a canceled coroutineScope. Tracking down that coroutineScope shows that it was canceled when DialogCollator calls Dialog.dismiss -> onViewDetachedFromWindow -> Recomposer.cancel(). So it makes sense why destroying/rebuilding instead of dismissing and reusing the dialog fixes this issue.

Still need to put together a non-workflow repro sample and submit a bug to the Compose team, but this unblocks our move to Compose 1.6.8 in the meantime.

[rjrjr]

So this is now very, very green, but I'm really torn about whether it's a good idea. On the one hand, I'm tired of obscure bug hunts that boil down to "oh this view is bad about handling being attached to a window more than once." On the other hand, relying on view code to use view state correctly in order to smoothly be reordered doesn't seem great either -- although at least they already have to do that for config change.

Going to try harder to understand why ComposeView is being a jerk. Best case our glue is dumb and can be fixed. Worst case we file a bug against the Compose team and mere this.

Once again, thank you @zach-klippenstein for all these wonderful unit tests!

cc @wardellbagby

[rjrjr]

Initial testing reveals:

• Default ViewCompositionStrategy.Default does not handle re-showing a dialog
• Both DisposeOnViewTreeLifecycleDestroyed and DisposeOnDetachedFromWindow fix that problem, which is a pleasant surprise because they didn't do so when I changed ScreenComposableFactory.asViewFactory() to use them

[rjrjr]

Initial testing reveals:

• Default ViewCompositionStrategy.Default does not handle re-showing a dialog
• Both DisposeOnViewTreeLifecycleDestroyed and DisposeOnDetachedFromWindow fix that problem, which is a pleasant surprise because they didn't do so when I changed ScreenComposableFactory.asViewFactory() to use them

Actually, I wasn't re-showing all the dialogs in that first test. Fixing that reveals that DisposeOnDetachedFromWindow does not work, which makes a lot of sense. Only DisposeOnViewTreeLifecycleDestroyed tames ComposeView in this situation, which is what I expected.

Testing with ScreenComposableFactory.asViewFactory() as-is on today's main branch confirms the problem. Changing that to use DisposeOnViewTreeLifecycleDestroyed fixes it!

So: why didn't that fix do the job for us in production?

[rjrjr]

#1212 (improved testing), #1213 (change ComposeView strategy) spin off of this

[rjrjr]

Updated the commit message to explain why we're going to go ahead and merge this after all. tl;dr: Compose 1.6.8 breaks what is fixed by #1213

[wardellbagby]

I understand it at a high-level but I'm trusting you with the ViewState stuff 'cause that's outta my wheelhouse!

[rjrjr]

I understand it at a high-level but I'm trusting you with the ViewState stuff 'cause that's outta my wheelhouse!

Fair. The good news is that I realized I need to fix these because various tests broke. I'm pretty confident.