Hello, so, I'm curious about architecture. We are...

# compose

[Cicero]

Hello, so, I'm curious about architecture.

We are now wrapping our UI with something that handles our viewmodel and states and then providing the UI with information rather then passing in the viewmodel. Right?
So, starting here how do we handle when there are too many parameters being passed to the UI, like 20?

In my mind, when I read trough this case I would leave it as is because it was pretty much explanatory.

I also had an option to break it in several smaller UI components but it looked off and ugly (like, let's say, header body and footer)

How are you handling your viewmodel and UI when your UI needs to receive too many parameters?

[Cicero]

Example of something I found in our source code:

exerciseName: String?,
    exerciseNameHasError: Boolean,
    durationTimeState: State<String?>,
    StepDescriptionItems: List<StepDescriptionItem>?,
    bitmapState: State<Bitmap?>,
    onMediaDeleteButtonClick: () -> Unit,
    onOpenGalleryClick: () -> Unit,
    onTakePhotoClicked: () -> Unit,
    onCaptureVideoClick: () -> Unit,
    onSaveButtonClick: () -> Unit,
    onExerciseNameChange: (String) -> Unit,
    onStepDescriptionValueChange: (index: Int, text: String) -> Unit,
    onStepDescriptionFocusChange: (index: Int, focusState: FocusState) -> Unit,
    onIncreaseDurationButtonClick: () -> Unit,
    onDecreaseDurationButtonClick: () -> Unit,
    onAddNewStepDescriptionClick: () -> Unit

with this implementation

CreateOrEditExerciseUI(
        exerciseName = exerciseName,
        exerciseNameHasError=exerciseNameHasError,
        durationTimeState = timeFormatted,
        bitmapState = previewBitmapState,
        onMediaDeleteButtonClick = { viewModel.deleteVideoFile() },
        onOpenGalleryClick = {
            viewModel.getVideoUri()
                ?.let { openGalleryLauncher.launch(arrayOf("video/*", "image/*")) }
        },
        onTakePhotoClicked = { viewModel.getPictureUri()?.let { takePictureLauncher.launch(it) } },
        onCaptureVideoClick = {
            viewModel.getVideoUri()?.let { captureVideoLauncher.launch(it) }
        },
        onExerciseNameChange = viewModel.exerciseNameChange,
        onSaveButtonClick =viewModel.validateAndSaveExercise,
        StepDescriptionItems = stepDescriptions,
        onStepDescriptionValueChange = { index, text ->
            viewModel.modifyStepDescription(index, text, false)
        },
        onStepDescriptionFocusChange = { index, focusState ->
            stepDescriptions?.let {
                if (focusState.isFocused) {
                    val stepDescription = it[index]
                    viewModel.modifyStepDescription(index, stepDescription.string, false)
                } else {
                    if (it[index].string.isEmpty() && it[index].needFocus == false) {
                        viewModel.removeStepDescriptionAt(index)
                    }
                }
            }
        },
        onIncreaseDurationButtonClick = viewModel.increaseDuration,
        onDecreaseDurationButtonClick = viewModel.decreaseDuration,
        onAddNewStepDescriptionClick = viewModel.addEmptyStepDescription
    )

For my eye this reads clearly what it does. It's just passing the parameters that the UI require.
What could be done that I don't feel necessary is breaking it down into smaller widgets but in the end this UI do require all this parameters.


I know that there are some obvious improvements here to the way we pass in this parameters but besides this. Is there an architecture better than the classic:

@Composable
fun TestView(
    action: MainActions,
    viewModel: OnboardViewModel = getViewModel()
) {
    TestUI(onClick = viewModel.clickMethod())
}

@Composable
fun TestUI(onClick: () -> Unit) {}

[Damian Zawadzki]

We don't have that much parameters but i can imagine that you can use one big object state that holds many parts. At top lvl you got this one big state and then you pass partial states to each sub component / composable.
This matches our mvi aproach with android view model providing single state on view model. Of corse this state could be more complicated and holds many information (if to many - try to break it to separate view model)

[Cicero]

Also, inside of this UI block everything is broken down like:

@Composable
fun FitnessItemDetailUI(
    fitnessDetailItem: FitnessItemDetail?,
    onClickBack: () -> Unit = {},
    onAddFavorite: (FitnessItemDetail?) -> Unit = {},
    exoPlayer: SimpleExoPlayer?,
    isPlaying: Boolean,
    onPlayPause: () -> Unit,
    currentTime: Float = 50f,
    totalTime: Float = 100f
) {

    FitnessItemDetailScaffold(
        fitnessDetailItem = fitnessDetailItem,
        onClickBack = onClickBack,
        onAddFavorite = onAddFavorite,
    ) {
        AppLazyColumn {
            item {
                Column(
                    modifier = Modifier
                        .fillMaxSize()
                        .padding(gu())
                ) {
                    VideoPlayer(exoPlayer = exoPlayer)
                    PlayerSliderWithSoundController(currentTime, totalTime)
                    ExerciseTimeController()
                    PlayerControllerButtons(
                        isPlaying = isPlaying,
                        onPlayPauseClick = onPlayPause,
                        onFinishTrainingClick = {}
                    )
                    TrainingExercises(
                        currentTime = currentTime,
                        exercises = fitnessDetailItem.exercises
                    )

                    Instructions()
                }
            }
        }
    }
}

So the insides are not much of a problem

[Damian Zawadzki]

as i sadi earlier, if there i to many parameters maybe there is a place for improvment by grouping some information with state objects:
https://github.com/androidx/androidx/blob/androidx-main/compose/docs/compose-api-guidelines.md#default-policies-through-hoisted-state-objects

[Cicero]

Yea, but then how this would look?

we are using this format:

@Composable
fun TestView(
    action: MainActions,
    viewModel: OnboardViewModel = getViewModel()
) {
    TestUI(onClick = viewModel.clickMethod())
}

@Composable
fun TestUI(onClick: () -> Unit) {}

[Cicero]

Ok, so I could use the same model I described above but break it down in many viewmodels and UI composables

[Cicero]

Like:

@Composable
fun HeaderView(
    action: MainActions,
    viewModel: HeaderViewModel = getViewModel()
) {
    HeaderUI(onClick = viewModel.clickMethod())
}

@Composable
fun HeaderUI(onClick: () -> Unit) {}

@Composable
fun BodyView(
    action: MainActions,
    viewModel: BodyViewModel = getViewModel()
) {
    BodyUI(onClick = viewModel.clickMethod())
}

@Composable
fun BodyUI(onClick: () -> Unit) {}

[Cicero]

I don't mean the body header example explicitly, just for visualization purposes

[Cicero]

I also understood the idea behind state objects. I will keep this in mind as different tools anyway.

This is not for me. There is a question in my company and no one can come up with an answer that everyone agrees.

Does everyone agree with this?

[Csaba Szugyiczki]

Definitely not! 🙂 We in our company have the same opinion as you, that we do not want to create huge State objects just to make it easier to pass data down Composables

[Damian Zawadzki]

I think that if you are able to separate some logic from components then you should do that. But this is my personal opinion and im also curious on other points of view 🙂

[Csaba Szugyiczki]

By separating using separate ViewModels, sharing state within the same screen will become harder, so I am not sure if that would work well with more complicated UIs

[Cicero]

But isn't this already separating logic from components?

@Composable
fun HeaderView(
    action: MainActions,
    viewModel: HeaderViewModel = getViewModel()
) {
    HeaderUI(onClick = viewModel.clickMethod())
}

@Composable
fun HeaderUI(onClick: () -> Unit) {}

[Cicero]

And yes, depending on the structure multiple viewmodels could be a limited option

[Cicero]

Sharing view models using multiple views is also an option.

[Cicero]

I personally don't mind monster UI Composables. If their insides are organized.

[Damian Zawadzki]

well you cannot always skip the complexity

[Csaba Kozák]

Huge state object could also negatively effect recompositions, see here.

[adjpd]

The docs say you shouldn't include viewmodels as composition locals because a) it's hard to test and b) the composable isn't so reusable. But if you can mock out the viewmodel and your component isn't suppose to be reusable then I don't see why you shoudn't include a viewmodel as a composition local instead of making a composable with 10s of callbacks that are filtered through to other composables.

[Cicero]

You just shouldn't 🙂

[Cicero]

There is zero reason to do it, there is no practicality on it that make is an advantage.

It's harder to test, If something makes something else harder to test. Even if you are not testing. Don't do it.

Very good rule of thumb.

Also, forget your preview. I believe you can't mock a viewmodel inside of your preview.

Basically, don't put your VM into your composable, there is no reason for it.

[raenardev]

As you can see - a lot of these are meant to propagate some UI event up to VM:

onMediaDeleteButtonClick: () -> Unit,
    onOpenGalleryClick: () -> Unit,
    onTakePhotoClicked: () -> Unit,
    onCaptureVideoClick: () -> Unit,
    onSaveButtonClick: () -> Unit,
    onExerciseNameChange: (String) -> Unit,
    onStepDescriptionValueChange: (index: Int, text: String) -> Unit,
    onStepDescriptionFocusChange: (index: Int, focusState: FocusState) -> Unit,
    onIncreaseDurationButtonClick: () -> Unit,
    onDecreaseDurationButtonClick: () -> Unit,
    onAddNewStepDescriptionClick: () -> Unit

What if you represent those as Commands/Intents/Actions/Events instead?

sealed class Action {
        object OpenGallery: Action()
        object TakePhoto: Action()
    }

So now you have something that looks like this:

exerciseName: String?,
        exerciseNameHasError: Boolean,
        durationTimeState: State<String?>,
        StepDescriptionItems: List<StepDescriptionItem>?,
        bitmapState: State<Bitmap?>,
        onAction: (Action) -> Unit,

If you also wrap other stuff as your UI state, then it is even better:

state: State
        onAction: (Action) -> Unit,

[raenardev]

Usage of state objects is debatable, yes. I personally think that it is fine to use for most (simple) cases. If it is getting big, or if there are many changes to it - then you can split out something that changes a lot vs something that relatively stable.

But as for action/intent/event handling - i think expressing them as sealed hierarchy is the way to go.

[adjpd]

You can mock a viewmodel in a preview. I do. And so you can test such composables. The advantage and reasons is that you don't spend your time with about 20 callbacks in your composables, or by updating ui state object which forces unneeded compositions.

[Lilly]

@raenardev Do you see a problem in actions that need parameter? Every time you fire such an event you have to instantiate an object, e.g. those events:

onStepDescriptionValueChange: (index: Int, text: String) -> Unit,
    onStepDescriptionFocusChange: (index: Int, focusState: FocusState) -> Unit,

    sealed class Action {
        object OpenGallery: Action()
        object TakePhoto: Action()
        data class ChangeStepDescription(val index: Int, val text: String) : Action()
    }

[raenardev]

@Lilly No, i don’t see a problem with instantiating objects. In this example objects are very lightweight, and they are not intended to be instantiated in thousands per second. I doubt there will be any measurable overhead from this.

I would be more worried about complex state data classes, which have substates and are modified a lot.
But not about object instantiating and copying - that should not be a problem with modern GC - but rather i would be careful with triggering unnecessary recomposition too often.

But as always - measure first and then look for things to optimise 🙂

[Lilly]

Thanks for your feedback 🙂