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.

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

Launching Molecule with Dispatchers.Main.immediate breaks state invalidations in some Compose code #465

Open
benkay opened this issue Jun 21, 2024 · 1 comment

Comments

@benkay
Copy link

benkay commented Jun 21, 2024

The issue is demonstrated by the below code, which reproduces the issue without actually depending on Molecule. The original
problem manifested as images loaded by Coil not fading in correctly - Coil's CrossfadePainter functions in much the same way as the Painter in this code.

class MainActivity : ComponentActivity() {
  override fun onCreate(savedInstanceState: Bundle?) {
    super.onCreate(savedInstanceState)
    enableEdgeToEdge()
    setContent {
      MaterialTheme {
        Scaffold(modifier = Modifier.fillMaxSize()) { innerPadding ->
          TestImage(modifier = Modifier.padding(innerPadding))
        }
      }
    }

    // Replacing the below block of code with this also causes the same issue
    // lifecycle.coroutineScope.launchMolecule(RecompositionMode.Immediate) {}

    lifecycle.coroutineScope.launch {
      var applyScheduled = false
      Snapshot.registerGlobalWriteObserver {
        if (!applyScheduled) {
          applyScheduled = true
          launch {
            applyScheduled = false
            Snapshot.sendApplyNotifications()
          }
        }
      }

      awaitCancellation()
    }
  }
}

@Composable
fun TestImage(modifier: Modifier = Modifier) {
  Image(
    painter = remember { InvalidatingPainter() },
    modifier = modifier,
    contentDescription = ""
  )
}

class InvalidatingPainter : Painter() {

  private var invalidateTick by mutableIntStateOf(0)
  private var lastStartTime: TimeSource.Monotonic.ValueTimeMark? = null
  private var duration = 1.seconds
  private var reverse = false

  override val intrinsicSize: Size = Size(200f, 200f)

  override fun DrawScope.onDraw() {
    val startTime = lastStartTime ?: TimeSource.Monotonic.markNow().also { lastStartTime = it }
    val percent = (startTime.elapsedNow() / duration).toFloat().coerceIn(0f, 1f)
    val alpha = if (reverse) 1 - percent else percent
    drawRect(Color.Blue, alpha = alpha)

    if (percent >= 1f) {
      lastStartTime = null
      reverse = !reverse
    }
    invalidateTick++
  }
}

The cause seems to be

  • The default coroutine scopes exposed by AndroidX use Dispatchers.Main.immediate
  • Because of this, the launch within the global write observer doesn't actually dispatch, and apply notifications are sent immediately instead of in the next event loop
  • The invalidateTick state write is happening within the draw phase. This is where my Compose internals knowledge gets a bit fuzzy, but I'm guessing Snapshot.sendApplyNotifications() needs to happen after read SnapshotStateObserver.observeReads finishes for any updates to the read state to actually cause a change notification.

The answer here seems to be "never use the immediate dispatcher", but it's very easy to do so without knowing at the moment.
e.g. viewModelScope.launchMolecule(..) will use the immediate dispatcher unless otherwise specified.

Perhaps Molecule could detect this and do the right thing?

@JakeWharton
Copy link
Collaborator

So there's a few paths forward here, and I'll probably do all of them:

  1. We should provide the option to disable starting our own snapshot application callback. If you are in an Android app with Compose UI there's no reason to run a second snapshot applier.
  2. We should provide the ability to pass a parent Composition (which likely implies 1). If you are hosting Molecule beneath Compose UI to drive the UI, we can make our composition a child of it.
  3. We should detect this case and force dispatch. I assume this implies a CoroutineDispatcher is available as a key in the context, but if not perhaps we just hard fail?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants