Simpler, safer ViewEnvironment and ViewRegistry management.

[rjrjr]

ViewEnvironment(Map<ViewEnvironmentKey<*>, Any>) makes it easy to build a
ViewEnvironment whose with values that don't match the type of the
ViewEnvironmentKeys. We deprecate it with an eye toward making it
non-public down the road. In its place we introduce ViewEnvironment.EMPTY,
and guide people to use it and the type safe plus operators.

We introduce a pattern of providing more plus operators with new environment
value types.

For ViewRegistry, in addition to the plus operator we provide a set of
merge methods, which finish the long overdue job of making it simpler to
mess with ViewEnvironment without stomping on ViewRegistry entries, and
to override them. EnvironmentScreen.withRegistry and .withEnvironment use
these.

Fixes #395. Replaces #630, reverted due to accidental merge.

[steve-the-edwards]

Have not finished reviewing because paused thinking about why we can't have the + operator come in via an interface. Just fiddling with the variance to see if I can get it to work a la

interface ViewEnvironmental

operator fun <T: ViewEnvironmental> ViewEnvironment.plus(value: T): ViewEnvironment =
  this + ViewEnvironment(mapOf(value::class to value))

which currently doesn't compile due to the variance of value::class

[steve-the-edwards]

Which lint warning is this from and why do we have to suppress now?

[rjrjr]

It's complaining about the capitalized function name. We don't have to suppress it, the build was green. I just hate IDE warnings.

[rjrjr]

Oops, this is a dup of the merge() method above. Should call that from here.

[rjrjr]

Have not finished reviewing because paused thinking about why we can't have the + operator come in via an interface. Just fiddling with the variance to see if I can get it to work a la

interface ViewEnvironmental

operator fun <T: ViewEnvironmental> ViewEnvironment.plus(value: T): ViewEnvironment =
  this + ViewEnvironment(mapOf(value::class to value))

which currently doesn't compile due to the variance of value::class

It's a fun problem space, isn't it? Really seems like ViewEnvironment could be spun out into its own microlibrary -- com.block.polymap.

I keep resisting the urge to create a general purpose mechanism for things to declare themselves mergable, b/c I strongly suspect that ViewRegistry will be the only use case forever.

[steve-the-edwards]

Have not finished reviewing because paused thinking about why we can't have the + operator come in via an interface. ...

Blarg we'd be sacrificing one convenience/legibility helper for another. I think i'm rocking the boat too much here, but the idea would be to take your pattern of making the companion object the ViewEnvironmentKey (nice because it has only one initialization at class init time) and instead move it to an interface where we could then enforce the operations etc.

interface ViewEnvironmental<T: Any> {
  val envKey: ViewEnvironmentKey<T>
}

operator fun <T: ViewEnvironmental<T>> ViewEnvironment.plus(value: T): ViewEnvironment =
  this + ViewEnvironment(mapOf(value.envKey to value))

public enum class BackStackConfig: ViewEnvironmental<BackStackConfig> {
  None,
  First,
  Other;

  override val envKey: ViewEnvironmentKey<BackStackConfig>
   get() = object : ViewEnvironmentKey<BackStackConfig>(BackStackConfig::class) {
    override val default: BackStackConfig = None
  }
}

I think i'm trying to get back to the ViewWrapper that I (unhelpfully) nerd-sniped you into with the failed #606 .

Ok will stop trying to make this harder than it is 😊

[rjrjr]

Latest force-push makes the tests green.

TicTacToeEspressoTest#showRenderingTagStaysFresh was failing because it inadvertently exposed newly wasteful allocations -- when we started calling the new merge functions on every render pass, we were creating new instances of CompositeViewRegistry for no reason. I've added / restored some checks to return the existing ViewEnvironment or ViewRegistry when merging with an empty one.

I'll add some explicit coverage for that, seems worth being formal about it.

[rjrjr]

@steve-the-edwards PTAL, I've added a commit that adds the missing test coverage for that optimization, and adds a few more such if (this.isEmpty()) return that cases.

[steve-the-edwards]

Beware I think once we start having mixed typed operators things can sometimes get confusing - this is the case for CoroutineContext which maps scope, dispatcher, name etc. in a very similar way to this. 90% of the time things just 'work' and work in a quick and satisfying way, but sometimes when reasoning about it it can get confusing how things overlap.

[rjrjr]

Yeah, I was a bit concerned about that. But when I applied it across our app I saw a lot of boilerplate melt away, and didn't run into any problems.

I really wish the IDE was better about offering to import the .plus method, though. Always have to paste that import in myself.