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

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

Closed
rjrjr opened this issue Dec 6, 2021 · 4 comments
Closed

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

rjrjr opened this issue Dec 6, 2021 · 4 comments
Labels
ui Related to UI integration
Milestone

Comments

@rjrjr
Copy link
Contributor

rjrjr commented Dec 6, 2021

WorkflowLayout and now AndroidDialogBounds.kt hard code use of Dispatchers.Main.immediate. Make them look for that in the ViewEnvironment. Maybe also give them a shared implementation of View.takeWhileAttached. Maybe.

@rjrjr rjrjr added this to the ui-1.0 milestone Dec 6, 2021
@rjrjr rjrjr added the ui Related to UI integration label Dec 6, 2021
@rjrjr
Copy link
Contributor Author

rjrjr commented Dec 7, 2021

Also consider adding a useful name to the scope, e.g.:

      val scopeName = window.peekDecorView()?.getTag(R.id.workflow_modal_dialog_content)
        ?.let { contentView ->
          contentView as View
          contentView.getRendering<Any>()?.toString()
        }
        ?: this@maintainBounds.toString()

      scope = CoroutineScope(Dispatchers.Main.immediate + CoroutineName(scopeName)).also {

@rjrjr
Copy link
Contributor Author

rjrjr commented Jan 20, 2022

@steve-the-edwards @RBusarow @vRallev Thinking about this hard coded Main.immediate issue and wondering if we actually care.

  • We've been shipping this way for nearly two years and haven't noticed
  • In the spot where it happens
  • both of those things happen in our Espresso tests before any of our custom injected dispatchers are cleaned up

tl;dr: no harm, no foul, let's not introduce complexity that won't solve any actual problems.

@RBusarow
Copy link
Member

It's not a big deal.

Espresso handles syncing with the main thread automatically. Hard-coded dispatchers only become a problem if you leave the main thread, and there's basically zero risk of that here.

@rjrjr
Copy link
Contributor Author

rjrjr commented Apr 15, 2022

The only remaining non-test, non-deprecated use of this is in AndroidDialogBounds, and it looks like it would be pretty silly to make it configurable:

    override fun onAttachedToWindow() {
      scope = CoroutineScope(Dispatchers.Main.immediate).also {
        bounds.onEach { b -> onBoundsChange(this@maintainBounds, b) }
          .launchIn(it)
      }
    }

@rjrjr rjrjr closed this as completed Apr 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ui Related to UI integration
Projects
None yet
Development

No branches or pull requests

2 participants