Hey, i was playing around with the `sharedElement`...

# compose

[Johnny]

Hey, i was playing around with the sharedElement modifier and ended up creating a wrapper modifier for easier usage. It looks like this:

@Composable
fun Modifier.sharedElementModifier(
    key: String,
): Modifier {
    val sharedTransitionScope = LocalSharedTransitionScope.current
    val animatedVisibilityScope = LocalAnimatedContentScope.current
    with(sharedTransitionScope) {
        return animatedVisibilityScope?.let {
            return this@sharedElementModifier then Modifier.sharedElement(
                rememberSharedContentState(key = key),
                animatedVisibilityScope = animatedVisibilityScope,
            )
        } ?: this@sharedElementModifier
    }
}

Is something like that possible with the new Modifier.Node api? And do you see any performance issues by using this wrapper?

[Zach Klippenstein (he/him) [MOD]]

The problem with grabbing composition locals when the modifier is created is that the modifier you return could be used somewhere else. Modifier factories also shouldn’t be composable — historically, this is what composed was for. But now a modifier node can read composition locals so you don’t need to read them ahead of time.

[Stylianos Gakis]

What I have done was just make this modifier

fun Modifier.sharedElement(
  sharedTransitionScope: SharedTransitionScope,
  animatedVisibilityScope: AnimatedVisibilityScope,
  state: SharedContentState,
  boundsTransform: BoundsTransform = DefaultBoundsTransform,
  placeHolderSize: PlaceHolderSize = contentSize,
  renderInOverlayDuringTransition: Boolean = true,
  zIndexInOverlay: Float = 0f,
  clipInOverlayDuringTransition: OverlayClip = ParentClip,
): Modifier = with(sharedTransitionScope) {
  [e-mail](
    state = state,
    animatedVisibilityScope = animatedVisibilityScope,
    boundsTransform = boundsTransform,
    placeHolderSize = placeHolderSize,
    renderInOverlayDuringTransition = renderInOverlayDuringTransition,
    zIndexInOverlay = zIndexInOverlay,
    clipInOverlayDuringTransition = clipInOverlayDuringTransition,
  )
}

where on the call site you do this

.sharedElement(
  LocalSharedTransitionScope.current,
  LocalNavAnimatedVisibilityScope.current,
  rememberSharedContentState(contract.id),
)

Honestly I prefer this one since sometimes you will in fact not want to grab the visibility or transitionscope from your CompositionLocals. You might have a different instance you want to pass in for transitions tied perhaps to something other than your navigation, or to some transitions that are in a smaller scoped transitionScope.

[Johnny]

Thank you! I ended up creating a similar extension like @Stylianos Gakis

[eygraber]

I don't think that call site will work since rememberSharedContentState is a function on SharedTransitionScope

[Stylianos Gakis]

Right, that looks like another wrapper function I made which also defaults to grabbing the scope from LocalSharedTransitionScope

[eygraber]

Wouldn't that have the same issue that Zach was mentioning since it's Composable? Or is it caller beware (which is the approach that I took with my helper function)?

[Stylianos Gakis]

That function internally is just

@Composable
fun rememberSharedContentState(key: Any): SharedContentState {
  return LocalSharedTransitionScope.current.rememberSharedContentState(key)
}

So in the call site when you write

.sharedElement(
  LocalSharedTransitionScope.current,
  LocalNavAnimatedVisibilityScope.current,
  rememberSharedContentState(id),
)

You practically write

.sharedElement(
  LocalSharedTransitionScope.current,
  LocalNavAnimatedVisibilityScope.current,
  LocalSharedTransitionScope.current.rememberSharedContentState(id),
)

So you use the same scope by default for these two.

Of course it's possible to forget about this, and pass a different scope on the first parameter, and use a different scope for the rememberSharedContentState call, if that is what you were worried about.
Otherwise these are both composition local reads next to each other, they should always properly resolve to the current local which must be the same in the same function call. If you hold the result of rememberSharedContentState and pass it further down in some way where the local may have changed, sure that could be a problem too.

[Thomas Hormes]

Sorry to bring this back up.
It looks like your CompositionLocals for the LocalSharedTransitionScope seem to be not nullable.
Looking at https://developer.android.com/develop/ui/compose/animation/shared-elements#understand-scopes however seems to suggest making your composition locals nullable.
How did you define your composition locals?

[Thomas Hormes]

Also how do you go about providing the AnimatedContentScope to your screen composables?
Do you have a top-level AnimatedContent providing the AnimatedContentScope or do you provide it in each composable call of your navgraph?

[Stylianos Gakis]

It's a decision you can make yourself. I made them not null, and I expect all navigation screens to provide the local, otherwise it's a crash at runtime.
It's a choice you can make yourself.
You can make them nullable in order to just fallback to doing nothing instead of actually crashing

[Stylianos Gakis]

You must use the right AnimatedContentScope, which for the screen level transitions will be the one provided for you by your NavHost when you use the composable() function to define a screen. The composable lambda gives you the right scope as the receiver of that lambda.

[Thomas Hormes]

Thank you Stylianos, however I don't quite get how I would define the composition local for the scopes.
When I try to pass an object implementing the corresponding interface with only no-ops, intellij shows me visibility-related errors

[Stylianos Gakis]

Like this https://github.com/HedvigInsurance/android/blob/migration%2Fcompose-typed-simpler-typemaps/app%2Fnavigation%2Fnavigation-compose%2Fsrc%2Fmain%2Fkotlin%2Fcom%2Fhedvig%2Fandroid%2Fnavigation%2Fcompose%2FNavGraphBuilderExt.kt#L26-L51

[Stylianos Gakis]

The default impl for the composition local should just be throwing an exception

https://github.com/HedvigInsurance/android/blob/migration%2Fcompose-typed-simpler-typemaps/app%2Fnavigation%2Fnavigation-compose%2Fsrc%2Fmain%2Fkotlin%2Fcom%2Fhedvig%2Fandroid%2Fnavigation%2Fcompose%2FLocalNavAnimatedVisibilityScope.kt#L12-L14

[Stylianos Gakis]

If you do want to "no-op' instead of crashing, then just make it nullable and default to null instead tbh, that'd be much preferable

[Thomas Hormes]

Ah yeah, of course 🤦 didn't think of just erroring out.
Thank you for your help 😊