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
16.3k stars 1.18k forks source link

Secondary `ComposeWindow` does not respond to data changes in its composables #777

Closed kirill-grouchnikov closed 3 years ago

kirill-grouchnikov commented 3 years ago

This is the smallest reproducible code that I could find. The idea here is to use ComposeWindow in undecorated mode for displaying popup content (let's say combobox or a complex menu). The content in this secondary ComposeWindow is interactive. Clicking on content in that window changes the underlying data and these changes should be reflected on the screen.

Here's the code first:

import androidx.compose.desktop.AppManager
import androidx.compose.desktop.ComposeWindow
import androidx.compose.desktop.Window
import androidx.compose.foundation.background
import androidx.compose.foundation.clickable
import androidx.compose.foundation.layout.*
import androidx.compose.foundation.text.BasicText
import androidx.compose.runtime.Composable
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.remember
import androidx.compose.runtime.rememberCompositionContext
import androidx.compose.ui.ExperimentalComposeUiApi
import androidx.compose.ui.Modifier
import androidx.compose.ui.graphics.Color
import androidx.compose.ui.unit.IntSize
import androidx.compose.ui.unit.dp

@ExperimentalComposeUiApi
fun main() {
    Window(
        title = "Main window",
        size = IntSize(400, 300),
        centered = true
    ) {
        val topCountA = remember { mutableStateOf(0) }
        val topCountB = remember { mutableStateOf(0) }
        val secondaryCount = remember { mutableStateOf(0) }

        val topDataA = Data("Text top A", topCountA.value) { topCountA.value++ }
        val topDataB = Data("Text top B", topCountB.value) { topCountB.value++ }
        val secondaryData = Data("Text secondary", secondaryCount.value) { secondaryCount.value++ }

        Column(modifier = Modifier.fillMaxSize()) {
            BoxContent(topDataA)
            Spacer(modifier = Modifier.height(12.dp))
            BoxContent(topDataB)
            Spacer(modifier = Modifier.height(12.dp))

            val parentComposition = rememberCompositionContext()

            Box(
                modifier = Modifier.size(100.dp, 50.dp).background(color = Color.Yellow)
                    .clickable(onClick = {
                        val secondWindow = ComposeWindow()
                        secondWindow.focusableWindowState = false
                        secondWindow.type = java.awt.Window.Type.POPUP
                        secondWindow.isAlwaysOnTop = true
                        secondWindow.isUndecorated = true

                        val locationOnScreen =
                            AppManager.focusedWindow!!.window.locationOnScreen

                        secondWindow.setBounds(
                            locationOnScreen.x,
                            locationOnScreen.y + 300,
                            500,
                            200
                        )

                        secondWindow.setContent(
                            parentComposition = parentComposition
                        ) {
                            BoxContent(secondaryData)
                        }

                        secondWindow.invalidate()
                        secondWindow.validate()
                        secondWindow.isVisible = true
                    })
            ) {
                BasicText(text = "Open second window!")
            }
        }
    }
}

data class Data(val text: String, val counter: Int, val onClick: () -> Unit)

@Composable
fun BoxContent(data: Data) {
    Box(
        modifier = Modifier.size(100.dp, 50.dp)
            .background(color = Color(255, 0, 0, 92))
            .clickable(onClick = {
                data.onClick.invoke()
            })
    ) {
        BasicText(text = "${data.text} ${data.counter}")
    }
}

The top-level window shows two red boxes. Clicking on each box increments the counter which then updates the screen. Clicking on the yellow box creates a ComposeWindow, configures it to be an undecorated "popup", and then places the same "red" composable in it.

Click on the yellow box to see this secondary window. Now click in its red box - nothing changes in the display. Click a couple more times. Now click on the yellow box in the main window again to open another secondary window. Note that in this new window, the counter is displaying correct (updated) count. So the underlying data has changed, but those changes are not being reflected in the secondary window.

This is with build 0.5.0-build224.

The bigger example is in https://github.com/kirill-grouchnikov/aurora where popup content can be interactive - such as toggle menu buttons in popup menus that do not automatically close the popup. In this case, clicking on a toggle menu button should visually update its state, but it doesn't - even though the underlying data is changed as the result of a click.

kirill-grouchnikov commented 3 years ago

The line val parentComposition = rememberCompositionContext() comes from my attempt to track the implementation from https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/window/v1/DesktopDialog.desktop.kt;l=86 but it doesn't work

igordmn commented 3 years ago

secondaryData by itself is not observable state, so when it is changed in the root composition - it is not visible to the child composition. To make it visible you can use rememberUpdatedState. It wraps the value into mutableStateOf:

val secondaryData by rememberUpdatedState(Data("Text secondary", secondaryCount.value) { secondaryCount.value++ })
kirill-grouchnikov commented 3 years ago

So in a more real-world example I'd need to wrap parts of the data with rememberUpdatedState?

Let's say I have a User object from the database. Some user fields are displayed in the main window, and some are displayed in the secondary window. Do I need to wrap each field that is displayed in the secondary window in rememberUpdatedState?

Or in a case of a split button that has the main action and a popup menu with let's say 5 actions. Do I need to wrap the part of the data object that is used to compose the popup menu content in rememberUpdatedState?

igordmn commented 3 years ago

I asked the similar question in Compose Runtime issues (recompose the child composition when we use CompositionLocal) and answer was it works as expected (there was another issue though).

Alternative to using rememberUpdatedState is to define setContent's lambda in the Composable function, so compiler will have enough information, when to recompose this lambda:

val secondaryData = Data("Text secondary", secondaryCount.value) { secondaryCount.value++ }

val childContent = @Composable {
   BoxContent(secondaryData)
}

You can also extract the second window to the separate function, that will also work:

val secondaryData = Data("Text secondary", secondaryCount.value) { secondaryCount.value++ }

BoxWithPopup {
   BoxContent(secondaryData)
}
igordmn commented 3 years ago

@chuckjaz, is it possible to recompose the content of the child composition (println(data)) when we change data in the parent composition?

import androidx.compose.desktop.ComposeWindow
import androidx.compose.desktop.Window
import androidx.compose.foundation.background
import androidx.compose.foundation.clickable
import androidx.compose.foundation.layout.*
import androidx.compose.runtime.LaunchedEffect
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.remember
import androidx.compose.runtime.rememberCompositionContext
import androidx.compose.ui.ExperimentalComposeUiApi
import androidx.compose.ui.Modifier
import androidx.compose.ui.graphics.Color
import androidx.compose.ui.unit.dp
import kotlinx.coroutines.delay
import java.awt.Dimension

@ExperimentalComposeUiApi
fun main() = Window {
    val count = remember { mutableStateOf(0) }
    val data = Data(count.value)
    val parentComposition = rememberCompositionContext()

    Box(
        modifier = Modifier.size(100.dp).background(color = Color.Yellow)
            .clickable(onClick = {
                val window = ComposeWindow()
                window.size = Dimension(200, 200)

                window.setContent(parentComposition = parentComposition) {
                    println(data)

                    LaunchedEffect(Unit) {
                        delay(1000)
                        println("change")
                        count.value++
                    }
                }

                window.isVisible = true
            })
    )
}

data class Data(val counter: Int)
chuckjaz commented 3 years ago

Change Data to,

class Data(counter: Int) {
    var counter by mutableStateOf(counter)
}
kirill-grouchnikov commented 3 years ago

Is the expectation that every single data object in the Compose app that has a non-trivial structure - including nested data objects - needs to shadow its attributes by these mutable variables?

I guess at some point somebody will write a generator plugin if this is indeed the recommended approach. Just trying to understand how this is "recommended" / "expected" to work for complex data objects that are driving multiple windows on a large desktop-sized screen.

igordmn commented 3 years ago

Change Data to,

Yes, it is one of the ways to solve the issue, but it is not always convinient.

If we don't use a child composition explicitly, just call another Composable, everything works:

import androidx.compose.desktop.Window
import androidx.compose.foundation.layout.Box
import androidx.compose.material.Text
import androidx.compose.runtime.LaunchedEffect
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.remember
import androidx.compose.ui.ExperimentalComposeUiApi
import kotlinx.coroutines.delay

@ExperimentalComposeUiApi
fun main() = Window {
    val count = remember { mutableStateOf(0) }
    val data = Data(count.value)

    LaunchedEffect(Unit) {
        delay(1000)
        println("change")
        count.value++
    }

    Box {
        Text(data.counter.toString())
    }
}

data class Data(val counter: Int)

Can we theoretically implement automatic recomposition in case above, where we define a child composition via setContent, or it is impossible?

chuckjaz commented 3 years ago

In general, yes. Usually the direction is the opposite however. The data source is a stream of immutable data (such as a Flow) that is sync'ed to a single mutable state which is the latest with asState() or similar API.

We started with @Model that would transform a class as you described but it was too magical and had very difficult to handle edge conditions around initialization so we removed it.

Reading the above more closely this can be performed with derivedStateOf instead. For example,

val data = derivedStateOf { Data(count.value) }

data.value will always produce a new Data instance whenever the value of count.value changes.

Chuck.

On Tue, Jun 15, 2021 at 9:33 AM Kirill Grouchnikov @.***> wrote:

Is the expectation that every single data object in the Compose app that has a non-trivial structure - including nested data objects - needs to shadow its attributes by these mutable variables?

I guess at some point somebody will write a generator plugin if this is indeed the recommended approach. Just trying to understand how this is "recommended" / "expected" to work for complex data objects that are driving multiple windows on a large desktop-sized screen.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JetBrains/compose-jb/issues/777#issuecomment-861652139, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC22LMUIKMM5DYUP6KCNMTTS56G5ANCNFSM46UBHGGQ .

chuckjaz commented 3 years ago

There needs to be something to track that data.counter is invalid when count.value changes. The dervivedStateOf() technique above will do that. It ensure first that a new version of Data will be produced for every new value of count and will also ensure that only one is created per snapshot and typically on one per value of count.value thought uniqueness is not guaranteed.

Chuck.

On Tue, Jun 15, 2021 at 9:38 AM Igor Demin @.***> wrote:

Change Data to,

Yes, it is one of the ways to solve the issue, but it is not always convinient.

If we don't use a child composition explicitly, just call another Composable, everything works:

import androidx.compose.desktop.Window import androidx.compose.foundation.layout.Box import androidx.compose.material.Text import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember import androidx.compose.ui.ExperimentalComposeUiApi import kotlinx.coroutines.delay

@ExperimentalComposeUiApi fun main() = Window { val count = remember { mutableStateOf(0) } val data = Data(count.value)

LaunchedEffect(Unit) {
    delay(1000)
    println("change")
    count.value++
}

Box {
    Text(data.counter.toString())
}

}

data class Data(val counter: Int)

Can we theoretically implement automatic recomposition in case above, where we define a child composition via setContent, or it is impossible?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JetBrains/compose-jb/issues/777#issuecomment-861655418, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC22LJJIE5SQEJLBMLJPOLTS56WPANCNFSM46UBHGGQ .

kirill-grouchnikov commented 3 years ago

derivedStateOf is combined here with a full copy of the data part needed for the secondary window, right? That can get pretty heavy for big data objects, or a need to create data "slice" objects to pass around.

chuckjaz commented 3 years ago

If the value is immutable it doesn't need to be copied, a reference is fine.

In general, for Compose, a model must be either immutable or observable, we call objects with this property stable. If it is mutable it must also be observable to be stable. If the model is a flow of immutable objects, that is fine, we observe the flow. If the model is mutable we provide mutableStateOf() to allow compose to observe the changes. deriviedStateOf allows observing function results that are produced from values of observable objects.

chuckjaz commented 3 years ago

Reading back to the origin of this issue, the use of setContent to imperatively create a window requires a observable object bridge to track when the data captured by the lambda passed to setContent changes. Since the call to setContent is in the clickable() callback handler, all the changes in the lambda capture variables must be explicitly observable. If the code was more declarative, then this observation would be handled automatically. For example,

Window {
  val count = remember { mutableStateOf(0) }
  val data = Data(count.value)
  val showWindow by remember { mutableStateOf(false) }
  Button(click = { showWindow = !showWindow) }) { Text("Show window") }
  Button(click = { count.value++ }) { Text("Add to count") }
  if (showWindow) {
      Window {
          Text("Data = $data")
      }
  }
}

Changes to count produces a new lambda instance if showWindow is true causing the nested Window to receive an updated captured data value. It is the imperative nature of the code above that requires the explicit use of observable objects to communicate the changes.

kirill-grouchnikov commented 3 years ago

Using showWindow to keep track of whether the Window composable needs to be shown is going to be a lot more complicated for popup windows. A popup window can be opened from any number of comboboxes, split buttons, popup buttons and just plain right-click with the mouse. And a popup window can be closed by clicking an item in it, or clicking outside of the menu, or hitting the Escape key, or the window losing focus, or others.

So it might not be straightforward to flip the showWindow back to false.

chuckjaz commented 3 years ago

In this case create a hoisted model that can be created by the caller but defaults to a local model such as,

fun Popup(popupModel: PopupModel = remember { PopupModel() }, popupContent: @Composable () -> Unit) {
  ...
  if (popupModel.showPopup) {
      Window(content = popupContent)
  }
}

where the popupModel will control whether the sub-window is visible. Close request would go through the model provided.

kirill-grouchnikov commented 3 years ago

Thanks Chuck. I'll need to think of how this more declarative way of working with secondary windows can be squared with the complex world of handling multi-level popups.

I think this can be closed now as I have a working solution as suggested by Igor, and something to think of as well from Chuck.

okushnikov commented 2 months ago

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