JetBrains / compose-multiplatform

Compose Multiplatform, a modern UI framework for Kotlin that makes building performant and beautiful user interfaces easy and enjoyable.
https://jetbrains.com/lp/compose-multiplatform
Apache License 2.0
15.86k stars 1.15k forks source link

Simplify todo app example #1201

Closed erikhuizinga closed 2 years ago

erikhuizinga commented 2 years ago

The todoapp example has a readme section that explains that it uses several libraries. While some of them are quite well-known or might be self-explanatory, others are far from trivial. This makes the todoapp example difficult to understand for someone who's interested in the compose-jb parts of the source, that are somewhere between all other kinds of code that these libraries need.

The suggestion in this issue is to remove all libraries that this example uses, except for the essential ones. The example is described as:

TODO items tracker with persistence and multiple screens

Since compose isn't about persistence, I understand that a library is used for that. But why does the example need libraries like decompose (a Kotlin MP BLoC library), MVIKotlin (a Kotlin MP MVI lib) and Reaktive (a Kotlin MP reactive extensions lib)? Can't the Kotlin stdlib and other standard libraries like coroutines provide a much more accessible, understandable and readable project?

A lengthy and opinionated example of how a user (I) might look at this example. TL;DR at the end.


Because I'm an Android developer, I started by looking at the common and the android projects. I naturally navigate to what seems to be an important entry point for an Android compose-jb app: App.kt. Indeed, it uses the Android Application super class. But then I see this:

https://github.com/JetBrains/compose-jb/blob/5dc4f1d9d9d19505836cbe0250f1b7752aae6851/examples/todoapp/android/src/main/java/example/todo/android/App.kt#L11

That is confusing. What is it? Where does it come from? Oh, some kind of library, so it seems. Not sure why we need that on Android, but oh well. I proceed to look at some other files, because the Android application class doesn't tell me anything specifically related to compose-jb.

The MainActivity file. That's something I know, let's have a look:

https://github.com/JetBrains/compose-jb/blob/5dc4f1d9d9d19505836cbe0250f1b7752aae6851/examples/todoapp/android/src/main/java/example/todo/android/MainActivity.kt#L20-L39

So we create a root and set some content. That seems like something from compose-jb, right?

Nope.

defaultComponentContext is used on line 23, which is from a library called decompose. So are we not using compose, but decompose now? What is this library and its context and why do we need it instead of something more default? Then we use that context to create a TodoRootComponent on line 35, apparently from a common source set. So that tells me that the common source set also is coupled to that library. I don't like where this is going... Furthermore, a chain of all kinds of store factories is constructed on line 37. What is all this and why do I need it in this project?

At this point I just think: this example is too complicated for its purpose. Or compose-jb is so immature that it needs substantial knowledge of existing Kotlin MP libraries and how to use them for a developer to be able build something simple like a TODO app (albeit on desktop, web, Android and iOS).

TL;DR: It's confusing for someone who doesn't know what all this is about. Someone who just want to take a look at an application that uses Kotlin, Kotlin multiplatform and a nice example of a 'TODO items tracker with persistence and multiple screens'.

igordmn commented 2 years ago

I think, we can write a simplified version of Todo, just using pure Compose, writing to File instead of database, and using a single state variable instead of a navigation framework.

But the current version should stay too, as it shows how to integrate Compose with another libraries, and shows one of the approaches how to write a scalable application. Probably, we should mention that explicitly in Readme. cc @arkivanov

igordmn commented 2 years ago

Also, it would help users if we will graduate our examples by complexity.

For example, falling-balls is a very simple desktop-only example.

codeviewer, imageviewer, issues are multiplatform examples, and more complex.

todoapp example probably is the most detailed and most complex at the same time. It introduces new concepts like MVI architecture, time-travelling, database handling (+migrations), complex project structure (common buildSrc), ios support, navigation handling, etc.

erikhuizinga commented 2 years ago

Thanks for chiming in, @igordmn! I like the suggestions that there are other projects who might be simpler to start with.

Personally, I was looking for an example that targeted various platforms. Looking at the list of examples:

https://github.com/JetBrains/compose-jb/blob/5dc4f1d9d9d19505836cbe0250f1b7752aae6851/README.md#L19-L29

I think that I found the todoapp example being the closest match to what I was trying to build: a web and mobile app.

So, naturally I chose to start looking at the todoapp example, but I found it way too complicated.

That being said, the todoapp could still be a good example (like you suggested) of how to integrate compose-jb with various good Kotlin multiplatform libs! Maybe there could be a 'vanilla' todoapp example and a more rich version (the current version)?

arkivanov commented 2 years ago

@igordmn thanks for mentioning me. I would like to contribute with a simple version of the Todo example.

Also if we are going to use "single state variable instead of a navigation framework", then what should we use for UI state preservation? Currently it is managed by Decompose. E.g. when you navigate to the details screen and then back to the main screen, then the list scrolling state remains unchanged. We can either reimplement it (e.g. copy-paste from Decompose), or continue using Decompose here.

And the same question about back stack preservation. When the Android app is killed by the system due to memory pressure and later restored, the back stack is restored as well. We can reimplement this behaviour as well, or use Decompose, or drop the Android target (desktop only example). If dropping the Android target, then the UI state preservation is still relevant.

erikhuizinga commented 2 years ago

@igordmn thanks for mentioning me. I would like to contribute with a simple version of the Todo example.

Thanks for offering help!

Also if we are going to use "single state variable instead of a navigation framework", then what should we use for UI state preservation? Currently it is managed by Decompose. E.g. when you navigate to the details screen and then back to the main screen, then the list scrolling state remains unchanged. We can either reimplement it (e.g. copy-paste from Decompose), or continue using Decompose here.

Restoring scrolling state is, IMHO, too complicated and unnecessary for a todo list app example. It's a good reason to maybe add libraries like decompose (IDK, because IDK decompose, but that's what I understand from your comment. Correct me if I'm wrong!); it would be a reason for an interested user to dig through a second todoapp example that provides more complicated state, state management and UI features like restoring list scroll state.

Besides, shouldn't a UI lib (compose-jb) provide a way to scroll a list to a certain position (is there a simple way?), or should a third party lib do that?

And the same question about back stack preservation. When the Android app is killed by the system due to memory pressure and later restored, the back stack is restored as well. We can reimplement this behaviour as well, or use Decompose, or drop the Android target (desktop only example). If dropping the Android target, then the UI state preservation is still relevant.

The same as above applies: this might be a good candidate to simply ignore in the simpler 'vanilla' example todoapp. It's not essential for an example project, unless you think that Android app lifecycle behaviour is essential for a compose-jb example.

arkivanov commented 2 years ago

From my point of view the UI state preservation is essential when there is navigation involved (regardless of the platform). And the back stack preservation is essential in Android. Compose is a multiplatform framework, all supported platforms should be treated equally.

But perhaps we can skip it indeed, and put a comment in the code and in the readme, explicitly mentioning the limitation and ways to improve.

Wondering what maintainers think about it.

igordmn commented 2 years ago

I would like to contribute with a simple version of the Todo example.

That would be awesome :)

then the list scrolling state remains unchanged

We can use rememberSaveableStateHolder:

import androidx.compose.foundation.clickable
import androidx.compose.foundation.lazy.LazyColumn
import androidx.compose.material.Text
import androidx.compose.runtime.Composable
import androidx.compose.runtime.getValue
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.saveable.rememberSaveableStateHolder
import androidx.compose.runtime.setValue
import androidx.compose.ui.Modifier
import androidx.compose.ui.window.singleWindowApplication

private var place: Place by mutableStateOf(Place.TodoList)

fun main() = singleWindowApplication {
    val stateHolder = rememberSaveableStateHolder()
    stateHolder.SaveableStateProvider(place) {
        when (val place = place) {
            Place.TodoList -> TodoList()
            is Place.TodoItem -> TodoItem(place.text)
        }
    }
}

@Composable
fun TodoList() {
    LazyColumn {
        items(100) {
            val text = "TODO $it"
            Text(
                text,
                modifier = Modifier.clickable {
                    place = Place.TodoItem(text)
                }
            )
        }
    }
}

@Composable
fun TodoItem(text: String) {
    Text(
        text,
        modifier = Modifier.clickable {
            place = Place.TodoList
        }
    )
}

sealed class Place {
    object TodoList : Place()
    class TodoItem(val text: String) : Place()
}
erikhuizinga commented 2 years ago

[...] all supported platforms should be treated equally.

Agreed, but how far does a mp framework go to do that? If you really make it equal on all platforms, then you wouldn't need anything but a common sourceset that compiles to all platforms and behaves appropriately on all. But that's not the case and not the goal.

But it's also beyond the scope of the current issue.

I like what was suggested before: make a different example of an app (possibly the same todo app) that shows what can be achieved with 'pure' Kotlin stdlib, compose-jb and maybe a few other well-known APIs. Then clearly document that it may have limitations in general, or on specific platforms. That is where the example can lead to another example, e.g. the current todoapp, that solves the issues in a clever way. That could be by using smart Kotlin mp libs, but the example should be clear about why they are used, where and how, and what they (don't) solve.

arkivanov commented 2 years ago

@igordmn

We can use rememberSaveableStateHolder

This is what I meant by "reimplement". Also it seems we should remove keys from the SaveableStateHolder which are no longer in the back stack? At least this is what Decompose does. When we move from TodoList to TodoItem, the former leaves the composition and goes to the back stack. It should be kept in SaveableStateHolder. But when we move back to TodoList, then TodoItem should be removed from SaveableStateHolder. So there should be a stack of screens, not only the current one. Here is the related code.

igordmn commented 2 years ago

Also it seems we should remove keys from the SaveableStateHolder which are no longer in the back stack

Fair point 🤔. But maybe it is enough for sample, if implementing a proper state saving will overcomplicate it.

But I have tried to write a tiny navigation, providing it via composition local:

import androidx.compose.runtime.Composable
import androidx.compose.runtime.CompositionLocalProvider
import androidx.compose.runtime.compositionLocalOf
import androidx.compose.runtime.mutableStateListOf
import androidx.compose.runtime.remember
import androidx.compose.runtime.saveable.rememberSaveableStateHolder

interface Navigation {
    fun go(place: Place)
    fun back()
}

val LocalNavigation = compositionLocalOf<Navigation> { error("There is no Navigation") }

@Composable
fun NavigationProvider(
    initialPlace: Place,
    content: @Composable (Place) -> Unit
) {
    val stack = remember { mutableStateListOf(initialPlace) }
    val stateHolder = rememberSaveableStateHolder()
    val navigation = remember(stateHolder) {
        object : Navigation {
            override fun go(place: Place) {
                stack.add(place)
            }

            override fun back() {
                val place = stack.removeLast()
                stateHolder.removeState(place)
            }
        }
    }

    val place = stack.last()
    stateHolder.SaveableStateProvider(place) {
        CompositionLocalProvider(
            LocalNavigation provides navigation
        ) {
            content(place)
        }
    }
}

What do you think, will it be enough for Todo application?

arkivanov commented 2 years ago

@igordmn yeah, something like this should work. I will start working on this soon-ish.

My point about incomplete solutions is that people usually tend to build real projects using the code from samples. If something is not working properly, it may confuse them. Like if you access files from the main thread for the sake of simplicity, then absolutely many people will do the same. So yeah, if we are going to use a simplified version of navigation, then I will put descriptive comments.

okushnikov commented 1 month ago

Please check the following ticket on YouTrack for follow-ups to this issue. GitHub issues will be closed in the coming weeks.