https://kotlinlang.org logo
Join Slack
Powered by
Searching how to optimize code duplication when I ...
# compose
p
Searching how to optimize code duplication when I need to use different screen structure in portrait/landscape, I found that you can embed a composable function inside a composable function. So you can call that function many times in your different structures (row/column) depending on orientation. Doing this, you don't have to duplicate the code for passing again all the parameters of the composables... but it seems to be very strange to make a composable doing this. Is this a good practice? sample in the thread...
Copy code
@Composable
fun ExampleScreen(isPortrait: Boolean) {
    @Composable
    fun LocationFields() {
        Column(
            modifier = Modifier
                .fillMaxWidth()
                .padding(8.dp)
        ) {
            LocationField(
                label = "Origin",
                value = "originValue",
                updateValue = { /* Update origin logic */ },
                acceptValue = { /* Search origin logic */ }
            )

            LocationField(
                label = "Destination",
                value = "destinationValue",
                updateValue = { /* Update destination logic */ },
                acceptValue = { /* Search destination logic */ }
            )
        }
    }

    @Composable
    fun MapPanel() {
        RouteMapPanel(
            centerLocation = "centerLocation",
            onUpdateCenter = { /* Update map center logic */ },
            onSelectLocation = { /* Select location logic */ },
            onNavigate = { /* Navigation logic */ },
            onSwitch = { /* Switch locations logic */ }
        )
    }

    if (isPortrait) {
        Column(
            modifier = Modifier.fillMaxSize()
        ) {
            LocationFields()
            MapPanel()
        }
    } else {
        Row(
            modifier = Modifier.fillMaxSize()
        ) {
            Column(
                modifier = Modifier
                    .fillMaxWidth(0.4f)
                    .padding(8.dp)
            ) {
                LocationFields()
            }
            MapPanel()
        }
    }
}
the alternative is to duplicate the calls to all the parameters on RouteMapPanel and LocationField having a very long composable
z
You might want to use 
movableContentOf
to keep composable state between the two calls to 
MapPanel
when the orientation changes
p
thanks, but what about my question?
z
it's fine
p
I like 
movableContentOf
it can substitute the functions, for both, the map and the fields
is it possible to pass a named parameter to it?
having this:
Copy code
val movableFieldsContent = remember {
    movableContentOf { singleLineFields: Boolean ->
and this:
Copy code
if (portrait) {
    Column(modifier = Modifier.fillMaxSize()) {
        movableFieldsContent(singleLineFields = true)
        movableMapContent()
    }
} else {
    Row(modifier = Modifier.fillMaxSize()) {
        Column(modifier = Modifier.fillMaxWidth(0.4f).padding(6.dp)) {
            movableFieldsContent(singleLineFields = false)
        }
        movableMapContent()
    }
}
z
the docs will answer your question
p
can you believe I didn't found the docs?
z
yea that's annoying
p
and I learned using it throught other persons samples
z
also 
ctrl+j
on the function in Studio
p
ctrl + j? that doesn't work for me
on the other hand, I can't see how to pass named parameters on the docs you passed me
I found that I can do this: 
movableFieldsContent(false)
but I can't name it like this: 
movableFieldsContent(singleLineFields = false)
z
or F1
you can't call lambdas with named params, that's a kotlin limitation
p
well, I think it's ok to use that for the map, and a @composable internal function for the locationfields
but it feels very strange to do this
to call internal functions inside functions
XD
maybe this will cause issues with recompositions?
z
not as far as i can tell, it's fine to define nested composables inside composables, i've seen it all over
it does allocate more than static functions, but that probably doesn't matter
p
thank you, this is a pretty good way to avoid duplicating calls with huge parameters on them
z
make sure you know what 
movableContentOf
actually does though before just using it everywhere. It does a very specific thing
p
docs says "_can be used to produce a composable that moves its content between a row and a column based on a parameter_"
in this case I need to reuse the map when it's landscape or portrait
👍🏻 1
in landscape is in a row, in protrait in a column
but there is a differente in how I use it and how it's used in the docs you passed me
I do this:
Copy code
val movableMapContent = remember {
            movableContentOf {
                RouteMapPanel(
docs says this:
Copy code
val movableContent = remember(content) { movableContentOf(content) }
I'm not sure if I'm doing wrong
z
i think what you're doing is ok, the example just assumes the movable composable is coming in as an argument
p
ok
a
I’d be a bit suspicious of an unkeyed 
remember
with 
remember { movableContentOf { } }
- because the lambda passed to 
remember
will only run once, you may end up capturing things inside the 
movableContentOf
call that are arguments to 
RouteMapPanel
A trick I’ve found helpful with 
movableContentOf
is something like this:
Copy code
val currentContent by rememberUpdatedState @Composable {
                RouteMapPanel()
            }
            val movableContent = remember { movableContentOf { currentContent() } }
It allows having a single movable slot of content, but the 
rememberUpdatedState
means that you can arbitrarily change what is rendered within that movable slot without worrying about capturing things accidentally since the only thing that is captured is the delegate that is being updated in composition.
👍🏻 1
And we should definitely have more docs about when and why to use 
movableContentOf
yes black 1
Unrelated to your initial question about structuring and 
movableContentOf
- I’d confirm if the decision point between portrait and landscape is the UX you really want to have for changing the screen layout, or if a certain amount of available window width or height would be more appropriate. Using portrait vs landscape can lead to some surprising behavior: making a window taller by increasing the height can switch from a landscape window to a portrait window - if that results in a different layout, you can end up in the confusing situation where giving the app more space to render resulted in less information being displayed. Window size classes are based on available width and available height instead of orientation, so decisions based on those don’t run into that issue.
p
Alex, thank you, I explored the option of using expanded Window size class for determining if I need to display the fields on the left of the map, but finally I ended doing this:
Copy code
val portrait = LocalConfiguration.current.screenWidthDp < LocalConfiguration.current.screenHeightDp
And if the result of portrait is false, I produced my "landscape" screen structure with the fields on the left of the map. Why I did this? because I didn't found which aspect ratio determines that expanded window size, and checking if 
LocalConfiguration.current.screenWidthDp < LocalConfiguration.current.screenHeightDp
worked in my tests with a tablet and a resizable dual window with two apps test. When I resized the app until that if changed to true, the content moved to the portrait structure. What do you think about doing that?
Also, @Alex Vanyo I don't completly understand this "you may end up capturing things inside the 
movableContentOf
call that are arguments to `RouteMapPanel`"
this is my full declaration of movableContentOf:
Copy code
// I store the map call in a movableContentOf to remember it between orientation changes
val movableMapContent = remember {
    movableContentOf {
        RouteMapPanel(
            simpleMapCenterLocation = uiState.simpleMapCenterLocation,
            selectedLocation = uiState.selectedLocation,
            selectedLocationAddress = uiState.selectedLocationAddress,
            originLocation = uiState.originLocation,
            destinationLocation = uiState.destinationLocation,
            hasLocationPermission = uiState.hasLocationPermission,
            updateSimpleMapCenterLocation = { vm.updateSimpleMapCenterLocation(it) },
            updateSelectedLocation = { vm.updateSelectedLocation(it) },
            onOriginSelected = {
                vm.updateSelectedLocation(null)
                vm.transformLatLngIntoAddress(it, AddressType.ORIGIN)
            },
            onDestinationSelected = {
                vm.updateSelectedLocation(null)
                vm.transformLatLngIntoAddress(it, AddressType.DESTINATION)
            },
            onNavigate = { vm.navigate(context) },
            onSwitchAddresses = { vm.switchAddresses() }
        )
    }
}
Do you see any issue there? what do you mean with that some arguments will be captured?
a
Something related to 
uiState
and 
vm
will be captured in that setup - it might be work fine depending on exactly how 
uiState
and 
vm
are declared and if they can change. A simpler example where you can see the difference in behavior is this:
Copy code
@Composable
@Preview
fun Repro() {
    var count by rememberSaveable { mutableStateOf(0) }
    MyButton(count, onClick = { count++ })
}

@Composable
fun MyButton(
    count: Int,
    onClick: () -> Unit,
) {
    val movableContent = remember {
        movableContentOf {
            Button(
                onClick = onClick,
            ) {
                Text("count: $count")
            }
        }
    }
    movableContent()
}
This won’t update the text in the button, because 
count
is being captured in the 
remember
from the first composition of 
MyButton
(
onClick
is also being captured) But this one will work:
Copy code
@Composable
@Preview
fun Repro() {
    var count by rememberSaveable {
        mutableStateOf(0)
    }

    val movableContent = remember {
        movableContentOf {
            Button(
                onClick = { count++ }
            ) {
                Text("count: $count")
            }
        }
    }
    movableContent()
}
Something is still being captured here, but now it’s the delegate for 
count
that is being captured, not value of 
count
itself. That allows fixing the setup for the first one by using 
rememberUpdatedState
:
Copy code
@Composable
@Preview
fun Repro() {
    var count by rememberSaveable { mutableStateOf(0) }
    MyButton(count, onClick = { count++ })
}

@Composable
fun MyButton(
    count: Int,
    onClick: () -> Unit,
) {
    val currentContent by rememberUpdatedState @Composable {
        Button(
            onClick = onClick,
        ) {
            Text("count: $count")
        }
    }
    val movableContent = remember {
        movableContentOf { currentContent() }
    }
    movableContent()
}
So your example may work fine as-is if 
uiState
is already defined with a delegate, and if 
vm
doesn’t change. But if you later add a parameter that is being passed through like 
MyButton
, or 
uiState
isn’t a delegate, you can get into a lot of headaches with capturing that are really hard to debug
movableContentOf
is especially tricky because the normal rule of adding keys to 
remember
for anything used inside of it often defeats the purpose of 
movableContentOf
- the whole point is to try to keep that internal state around without throwing it away, and recreating the 
movableContentOf
will throw away that internal state
p
@Alex Vanyo thanks, but this sounds very very complex, and probably I will not understand/remember it in a year if I go back to the source code. The benefits it offers are far exceeded by the complexity added by the implementation. MovableContentOf must be removed from my source code, as it is too much complex
What do you think if I do the same with the map that what I'm doing with the fields? I mean, transforming the map into a inner local function, so using it in portrait/landscape conditions will not require to duplicate the call the to map composable with all their parameters and function parameters? will that also have any possible issues?
I mean doing this:
Copy code
@Composable
fun ExampleScreen(isPortrait: Boolean) {
    @Composable
    fun LocationFields() {
        Column(modifier = Modifier.fillMaxWidth().padding(8.dp)) {
            LocationField(
                label = "Origin",
                value = "originValue",
                updateValue = { /* Update origin logic */ },
                acceptValue = { /* Search origin logic */ }
            )

            LocationField(
                label = "Destination",
                value = "destinationValue",
                updateValue = { /* Update destination logic */ },
                acceptValue = { /* Search destination logic */ }
            )
        }
    }

    @Composable
    fun MapPanel() {
        RouteMapPanel(
            centerLocation = "centerLocation",
            onUpdateCenter = { /* Update map center logic */ },
            onSelectLocation = { /* Select location logic */ },
            onNavigate = { /* Navigation logic */ },
            onSwitch = { /* Switch locations logic */ }
        )
    }

    if (isPortrait) {
        Column(modifier = Modifier.fillMaxSize()) {
            LocationFields()
            MapPanel()
        }
    } else {
        Row(modifier = Modifier.fillMaxSize()) {
            Column(modifier = Modifier.fillMaxWidth(0.4f).padding(8.dp)) {
                LocationFields()
            }
            MapPanel()
        }
    }
}
My idea was always to not duplicate long calls to composables with a lot of parameters and function parameters
what do you think @Alex Vanyo ?
a
The local 
@Composable
function calls are totally fine themselves, the downside of not using 
movableContentOf
would be loss of state of content within 
MapPanel
or 
LocationFields
When the call hierarchy changes for where 
MapPanel
is invoked, all of the state within 
MapPanel
will be lost as the new 
MapPanel
call is effectively a fresh "instance". So if 
MapPanel
has internal state like the current lat-long of where the user is looking at, that will be lost if the orientation changes - which is definitely a bug from the user perspective if they rotate their device and the state of where they were looking at is gone.
p
I understand, but seems to be simpler to put the map camera state into the uistate of the screen and remember it on the viewmodel, than to dive in that movableContentOf complexity
on the other hand, I disscovered that with my simple implementation of movableContentOf without passing any parameter to remember, did not show any problem apparently, when changing orientation, the camera state and everything seems to work and to be remembered, but I'm confused as you told me that it was necessary to add that other complexity
2 Views
Open in Slack
PreviousNext
Powered by