Provide a way to calculate ComposeScene preferredSize that is needed for Swing #884

Walingar · 2023-10-19T16:41:25Z

Proposed Changes

Previous solution with preferred size didn't work well because constraints with maxSize taken from component.size were used, but component.size is not set (0 by default).

See issue: JetBrains/compose-multiplatform#3834

By Swing design preferred size should be calculated without incoming constraints. It is not possible in Compose, so I artificially set max constraints taking sum of displays dimensions here. Not the best solution, but I couldn't imagine better one..

Testing

Test: run ComposePanelTest.kt

Issues Fixed

Fixes: JetBrains/compose-multiplatform#3834

Walingar · 2023-10-19T16:42:15Z

compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/SkiaBasedOwner.skiko.kt

+    internal fun measureInConstraints(constraints: Constraints): IntSize {
+        val oldConstraints = this.constraints
+        try {
+            // TODO: is it possible to measure without reassigning root constraints?


I couldn't find a better way to measure in given constraints...

Also, as I understand it will break layout caches...

But it should be called on parent relayout which should happen a lot

I checked, and Swing doesn't call getPreferredSize on window resize in the initial panel size of LazyColumn with border layout test. So, we are safe, and we only have an additional measure at startup?

It depends on LayoutManager, in case of window resize everything should be fine, since PreferredSize is used only for initial size. But other LayoutManagers may call preferredSize any time :(

compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/awt/ComposeBridge.desktop.kt

m-sasha · 2023-10-20T14:13:52Z

compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/ComposeScene.skiko.kt

            val mainOwner = mainOwner ?: return IntSize.Zero
-            mainOwner.measureAndLayout()
-            return mainOwner.contentSize
+            return contentSizeInConstraints(mainOwner.constraints)


We should cache this or turn it into a function. Code that previously assumed this was a quick operation will need to be rewritten. For example in SemanticsNode.isInScreenBounds():

return nodeBoundsInWindow.top >= 0 && nodeBoundsInWindow.left >= 0 && nodeBoundsInWindow.right <= composeView.contentSize.width && nodeBoundsInWindow.bottom <= composeView.contentSize.height

Actually, are you sure this isn't breaking anything? Before, contentSize was the actual size of the scene. Now it's the result of some computation - why is it guaranteed to match the real size of the scene?

Ah, I missed this particular case, indeed this one can break, thank you

Mentioned code was recently changed by Ivan. Also, this API always was not that cheap, since it did measure and layout.
So, nothing is changed here, code still works the same

igordmn · 2023-11-07T10:40:39Z

compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/SkiaBasedOwner.skiko.kt

+     * Provides a way to measure Owner's content in given [constraints]
+     * Draw/pointer and other callbacks won't be called here like in [measureAndLayout] functions
+     */
+    internal fun measureInConstraints(constraints: Constraints): IntSize {


Let's make it public and experimental. The policy for ComposeScene - if something is needed for one platform, it can be needed for other platforms, implemented by users.

Also, let's add TODO for contentSize:

TODO: investigate all internal usages, and deprecated it, if it isn't actually needed

I found one usage in ui-test, and I am not sure that is correct usage of contentSize there.

It seems that this is not used anymore. Anyway, contentSize api is not changed. It still returns the size in mainOwner constraints

@igordmn contentSize is public API, and calculateContentSize is not a replacement, as it does something else entirely.

compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/awt/ComposeBridge.desktop.kt

compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/SkiaBasedOwner.skiko.kt

igordmn · 2023-11-07T13:34:15Z

compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/SkiaBasedOwner.skiko.kt

+    internal fun measureInConstraints(constraints: Constraints): IntSize {
+        val oldConstraints = this.constraints
+        try {
+            // TODO: is it possible to measure without reassigning root constraints?


I checked, and Swing doesn't call getPreferredSize on window resize in the initial panel size of LazyColumn with border layout test. So, we are safe, and we only have an additional measure at startup?

…for Swing

…aBasedOwner should be public not internal

Walingar · 2023-11-13T14:55:10Z

compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/ComposeScene.skiko.kt

            val mainOwner = mainOwner ?: return IntSize.Zero
-            mainOwner.measureAndLayout()
-            return mainOwner.contentSize
+            return contentSizeInConstraints(mainOwner.constraints)


Just note, that after my changes this code doesn't perform layout anymore

Walingar · 2023-11-13T14:58:08Z

@igordmn @m-sasha Folks, sorry for the delay. I've answered on comments and pushed recent changes, please take a look.

I've changed max constraints to infinity, so exception will be thrown if user doesn't provide maxSize for lazy layouts.
Also, I provided experimental API in ComposeScene since it is needed by convention of ComposeScene.

It seems that ComposeScene.contentSize is not used anymore in our codebase. But since it is public API, I keep it here. The only change is that it doesn't perform layout

compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/ComposeScene.skiko.kt

…Scene.skiko.kt Co-authored-by: Igor Demin <igordmn@users.noreply.github.com>

igordmn

LGTM. Let's wait for the second approval from @m-sasha

m-sasha · 2023-11-16T06:41:27Z

Don't know how important this is, but some of the javadocs exceed 100 columns.

m-sasha · 2023-11-16T06:46:30Z

compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/ComposeScene.skiko.kt

+     */
+    // TODO: make stable and deprecate contentSize in 1.7
+    @ExperimentalComposeUiApi
+    fun calculateContentSize(): IntSize {


Maybe call it preferredSize()?

m-sasha · 2023-11-16T06:47:45Z

compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/ComposeScene.skiko.kt

@@ -477,14 +477,36 @@ class ComposeScene internal constructor(
    /**
     * Returns the current content size
     */
+    // TODO: make calculateContentSize stable in 1.7 and mark it as
+    //  @Deprecated("Use calculateContentSize() instead", replaceWith = ReplaceWith("calculateContentSize()"))


It seems to me that "current content size" and "preferred size" (in infinite constraints) are not at all the same thing. If so, why do we want to deprecate getting the current size?

@igordmn told that by design contentSize was expected to provide size in infinity constraints, so there is no usecase to get current size and should be received in another way

Yes, there wasn't an intention to provide current content size, and I am not aware of use cases where we need it. It was added to determine the size of the window/panel before its construction.

m-sasha · 2023-11-16T06:50:27Z

compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/SkiaBasedOwner.skiko.kt

+     * Provides a way to measure Owner's content in given [constraints]
+     * Draw/pointer and other callbacks won't be called here like in [measureAndLayout] functions
+     */
+    internal fun measureInConstraints(constraints: Constraints): IntSize {


@igordmn contentSize is public API, and calculateContentSize is not a replacement, as it does something else entirely.

compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/ImageComposeScene.skikoMain.kt

…for Swing (#884) --------- Co-authored-by: Igor Demin <igordmn@users.noreply.github.com>

Walingar requested review from igordmn and m-sasha October 19, 2023 16:41

Walingar commented Oct 19, 2023

View reviewed changes

compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/awt/ComposeBridge.desktop.kt Outdated Show resolved Hide resolved

m-sasha reviewed Oct 20, 2023

View reviewed changes

igordmn reviewed Nov 7, 2023

View reviewed changes

Walingar added 7 commits November 13, 2023 15:30

Provide a way to calculate ComposeScene preferredSize that is needed …

23e3986

…for Swing

Use max display dimension instead of sum to not exceed half of Int

d68f624

Use infinity constraints for measuring preferredSize of the scene

a54eb28

Use measureOnly instead of measureAndLayout

83f5a85

Make measureInConstraints public by convention that everything in Ski…

f694afa

…aBasedOwner should be public not internal

Move check back

4d86008

Make ComposeScene API public instead of SkiaBasedOwner

28345c5

Walingar force-pushed the nr/preferred-size branch from 69a182e to 28345c5 Compare November 13, 2023 14:54

Walingar commented Nov 13, 2023

View reviewed changes

Update desktop api

f8997df

igordmn approved these changes Nov 13, 2023

View reviewed changes

igordmn reviewed Nov 13, 2023

View reviewed changes

compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/ComposeScene.skiko.kt Outdated Show resolved Hide resolved

Deprecate contentSize and introduce calculateContentSize api

a43b821

igordmn reviewed Nov 14, 2023

View reviewed changes

compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/ComposeScene.skiko.kt Outdated Show resolved Hide resolved

Walingar and others added 2 commits November 14, 2023 13:42

Update compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/Compose…

0a2d22e

…Scene.skiko.kt Co-authored-by: Igor Demin <igordmn@users.noreply.github.com>

Mark as experimental and remove deprecation until 1.7

8b14d3e

Walingar requested review from igordmn and m-sasha November 14, 2023 12:48

igordmn approved these changes Nov 14, 2023

View reviewed changes

Walingar added the changes in API label Nov 14, 2023

m-sasha reviewed Nov 16, 2023

View reviewed changes

m-sasha mentioned this pull request Nov 16, 2023

Add the ability to "pack" a window while allowing its content to match the window size JetBrains/compose-multiplatform#2923

Open

m-sasha self-requested a review November 16, 2023 15:18

m-sasha approved these changes Nov 16, 2023

View reviewed changes

Walingar merged commit f01d58e into jb-main Nov 16, 2023
3 of 4 checks passed

Walingar deleted the nr/preferred-size branch November 16, 2023 15:24

MatkovIvan reviewed Nov 17, 2023

View reviewed changes

compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/ImageComposeScene.skikoMain.kt Show resolved Hide resolved

mazunin-v-jb pushed a commit that referenced this pull request Dec 7, 2023

Provide a way to calculate ComposeScene preferredSize that is needed …

f4fbbfe

…for Swing (#884) --------- Co-authored-by: Igor Demin <igordmn@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide a way to calculate ComposeScene preferredSize that is needed for Swing #884

Provide a way to calculate ComposeScene preferredSize that is needed for Swing #884

Walingar commented Oct 19, 2023

Walingar Oct 19, 2023

Walingar Oct 20, 2023

Walingar Oct 20, 2023

igordmn Nov 7, 2023

Walingar Nov 8, 2023

m-sasha Oct 20, 2023

m-sasha Oct 20, 2023

Walingar Oct 20, 2023

Walingar Nov 13, 2023 •

edited

Loading

igordmn Nov 7, 2023

igordmn Nov 7, 2023

Walingar Nov 13, 2023

m-sasha Nov 16, 2023

igordmn Nov 7, 2023

Walingar Nov 13, 2023

Walingar commented Nov 13, 2023

igordmn left a comment

m-sasha commented Nov 16, 2023

m-sasha Nov 16, 2023

m-sasha Nov 16, 2023

Walingar Nov 16, 2023

igordmn Nov 16, 2023

m-sasha Nov 16, 2023

Provide a way to calculate ComposeScene preferredSize that is needed for Swing #884

Provide a way to calculate ComposeScene preferredSize that is needed for Swing #884

Conversation

Walingar commented Oct 19, 2023

Proposed Changes

Testing

Issues Fixed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Walingar Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Walingar commented Nov 13, 2023

igordmn left a comment

Choose a reason for hiding this comment

m-sasha commented Nov 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Walingar Nov 13, 2023 •

edited

Loading