Introduces ViewEnvironmentKey.combine, makes ViewRegistry use it. #737
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
0legg
approved these changes
Apr 24, 2022
| override fun hashCode(): Int = map.hashCode() | ||
|
|
||
| @Suppress("UNCHECKED_CAST") | ||
| private fun <T : Any> getOrNull(key: ViewEnvironmentKey<T>): T? = map[key] as? T |
There was a problem hiding this comment.
Worth to make it getOrDefault, as all callers are using some sort of fallback.
There was a problem hiding this comment.
But the main call wants to run a lambda only in the non-null case.
Comment on lines
37
to
44
| other.map.entries.forEach { (key, value) -> | ||
| @Suppress("UNCHECKED_CAST") | ||
| newMap[key] = | ||
| getOrNull(key as ViewEnvironmentKey<Any>)?.let { key.combine(it, value) } ?: value | ||
| } |
There was a problem hiding this comment.
We could try to use Java8 merge method
Suggested change
| other.map.entries.forEach { (key, value) -> | |
| @Suppress("UNCHECKED_CAST") | |
| newMap[key] = | |
| getOrNull(key as ViewEnvironmentKey<Any>)?.let { key.combine(it, value) } ?: value | |
| } | |
| other.map.forEach { key, value -> | |
| newMap.merge(key, value) { valueLeft, valueRight -> key.combine(valueLeft, valueRight) } | |
| } |
There was a problem hiding this comment.
This is non-Android, though, and there's a chance of it someday becoming KMP.
steve-the-edwards
approved these changes
Apr 25, 2022
| ViewEnvironment(map + pair) | ||
| public operator fun <T : Any> plus(pair: Pair<ViewEnvironmentKey<T>, T>): ViewEnvironment { | ||
| val (newKey, newValue) = pair | ||
| val newPair = getOrNull(newKey)?.let { newKey to newKey.combine(it, newValue) } ?: pair |
There was a problem hiding this comment.
nit on naming:
Suggested change
| val newPair = getOrNull(newKey)?.let { newKey to newKey.combine(it, newValue) } ?: pair | |
| val newPair = getOrNull(newKey)?.let { oldValue to oldValue.combine(it, newValue) } ?: pair |
There was a problem hiding this comment.
I think you meant this:
val newPair = getOrNull(newKey)
?.let { oldValue -> newKey to newKey.combine(oldValue, newValue) }
?: pairThere was a problem hiding this comment.
yes - sorry got lambda confused with the pair vs named argument syntax
| other.map.entries.forEach { (key, value) -> | ||
| @Suppress("UNCHECKED_CAST") | ||
| newMap[key] = | ||
| getOrNull(key as ViewEnvironmentKey<Any>)?.let { key.combine(it, value) } ?: value |
There was a problem hiding this comment.
nit or naming
Suggested change
| getOrNull(key as ViewEnvironmentKey<Any>)?.let { key.combine(it, value) } ?: value | |
| getOrNull(key as ViewEnvironmentKey<Any>)?.let { existingValue.combine(it, value) } ?: value |
There was a problem hiding this comment.
newMap[key] = getOrNull(key as ViewEnvironmentKey<Any>)
?.let { oldValue -> key.combine(oldValue, value) }
?: value
|
Updated with better naming in the two lambdas calling |
`ViewEnvironmentKey.combine` is called from `ViewEnvironment.plus` when both operands have keys of the same type. `ViewRegistry.Companion` is already a `ViewEnvironmentKey`, and now it implements `combine`, calling `ViewRegistry.merge`. This changes `ViewEnvironment.plus` to merge the values of `ViewRegistry` entries instead of replacing the registry on the left with the one on the right, which is the only thing we've ever actually wanted to do. This allows us to eliminate `ViewEnvironment.merge`. We have never seen a use case for completely stomping the `ViewRegistry` on the left, but have done it by accident a lot. If the need really does arise, we can add a `ViewEnvironment.minus` operator. Also eliminates the `ViewEnvironmentKey()` factory function, which was just silly.
|
Update deletes a redundant |
RBusarow
approved these changes
Apr 25, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ViewEnvironmentKey.combineis called fromViewEnvironment.pluswhen both operands have keys of the same type.ViewRegistry.Companionis already aViewEnvironmentKey, and now it implementscombine, callingViewRegistry.merge.This changes
ViewEnvironment.plusto merge the values ofViewRegistryentries instead of replacing the registry on the left with the one on the right, which is the only thing we've ever actually wanted to do. This allows us to eliminateViewEnvironment.merge.We have never seen a use case for completely stomping the
ViewRegistryon the left, but have done it by accident a lot. If the need really does arise, we can add aViewEnvironment.minusoperator.Also eliminates the
ViewEnvironmentKey()factory function, which was just silly.