Fix bounds of ComposePanel in IntelliJ on macOs #988

igordmn · 2024-09-16T17:46:02Z

Fixes https://youtrack.jetbrains.com/issue/CMP-5856/Desktop-ComposePanel-size-breaks-with-.fillMax-modifiers#focus=Comments-27-10632441.0-0

Fixes https://youtrack.jetbrains.com/issue/CMP-5968/Compose-content-is-rendered-in-the-wrong-place-in-IJ-when-using-AWT-compositing

Regression after https://github.com/JetBrains/skiko/pull/661/files#diff-910a6e28fda20a00bc98c6a8a04f74ab701d79e841b9baddf146b810e610572fR363 (setBounds is called more often, but not enough as ancestorMoved)

When a panel changes its position without changing its size, doLayout isn't called because the content itself wasn't changed. But we still need to update the bounds of the underlying layer.

Testing

https://youtrack.jetbrains.com/issue/CMP-5856/Desktop-ComposePanel-size-breaks-with-.fillMax-modifiers#focus=Comments-27-10632441.0-0 isn't reproducible after the fix
there are no resize glitches

Release Notes

Fixes - Desktop

Fix bounds of ComposePanel in IntelliJ on macOs

Fixes https://youtrack.jetbrains.com/issue/CMP-5856/Desktop-ComposePanel-size-breaks-with-.fillMax-modifiers#focus=Comments-27-10632441.0-0 Fixes https://youtrack.jetbrains.com/issue/CMP-5968/Compose-content-is-rendered-in-the-wrong-place-in-IJ-when-using-AWT-compositing Regression after https://github.com/JetBrains/skiko/pull/661/files#diff-910a6e28fda20a00bc98c6a8a04f74ab701d79e841b9baddf146b810e610572fR363 When a panel changes its bounds, `doLayout` isn't called because the content itself wasn't changed. But we still need to update the bounds of the underlying layer. Docs: ``` > doLayout Causes this container to lay out its components > ``` ## Testing https://youtrack.jetbrains.com/issue/CMP-5856/Desktop-ComposePanel-size-breaks-with-.fillMax-modifiers#focus=Comments-27-10632441.0-0 isn't reproducible after the fix ## Release Notes ### Fixes - Fix bounds of ComposePanel in IntelliJ

… a deprecated version of `actions/upload-artifact: v2` Learn more: https://github.blog/changelog/2024-02-13-deprecation-notice-v1-and-v2-of-the-artifact-actions/

This reverts commit cf1cc55.

This reverts commit a0d2b16.

igordmn · 2024-09-16T19:58:20Z

skiko/src/awtMain/kotlin/org/jetbrains/skiko/SkiaLayer.awt.kt

@@ -143,7 +155,7 @@ actual open class SkiaLayer internal constructor(
 addPropertyChangeListener("graphicsContextScaleTransform") {
 Logger.debug { "graphicsContextScaleTransform changed for $this" }
 latestReceivedGraphicsContextScaleTransform = it.newValue as AffineTransform
- redrawer?.syncSize()
+ revalidate()


Changed because layout depends on the scale inside adjustSizeToContentScale

Probably it can fix https://youtrack.jetbrains.com/issue/CMP-5811/Moving-Desktop-window-across-monitors-causes-change-in-layout-since-1.7.0-alpha01-when-running-from-the-IDE

Sounds like it could bring back https://youtrack.jetbrains.com/issue/CMP-6550/Resizing-the-window-using-WindowState-breaks-the-UI

Is there a pure Compose reproducer I can try out?

A pure Compose reproducer is:

import androidx.compose.foundation.background import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.fillMaxSize import androidx.compose.ui.Modifier import androidx.compose.ui.awt.ComposePanel import androidx.compose.ui.graphics.Color import kotlinx.coroutines.delay import kotlinx.coroutines.runBlocking import org.jetbrains.skiko.MainUIDispatcher import java.awt.Dimension import javax.swing.JFrame import javax.swing.JLayeredPane import javax.swing.WindowConstants fun main() { runBlocking(MainUIDispatcher) { val window = JFrame() val panel = ComposePanel() panel.setContent { Box(Modifier.fillMaxSize().background(Color.Red)) } panel.size = Dimension(100, 100) val box = javax.swing.Box.createVerticalBox().apply { add(panel) } box.setBounds(0, 0, 100, 100) try { val container = JLayeredPane() container.add(box) window.contentPane.add(container) window.setLocation(200, 200) window.size = Dimension(200, 200) window.defaultCloseOperation = WindowConstants.DISPOSE_ON_CLOSE window.isUndecorated = true window.isVisible = true delay(1000) box.setBounds(100, 0, 100, 100) delay(10000) } finally { window.dispose() } } }

I checked https://youtrack.jetbrains.com/issue/CMP-6550/Resizing-the-window-using-WindowState-breaks-the-UI with this Skiko, it doesn't reproduce (it reproduces in 1.7.0-beta02 on my machine)

m-sasha · 2024-09-17T10:54:54Z

skiko/src/awtMain/kotlin/org/jetbrains/skiko/SkiaLayer.awt.kt

- private fun roundSize(value: Int): Int {
- var rounded = value * contentScale
- val diff = rounded - rounded.toInt()
+ private fun adjustSizeToContentScale(value: Int): Int {
+ val scaled = value * contentScale
+ val diff = scaled - scaled.toInt()
 // We check values close to 0.5 and edit the size to avoid white lines glitch
- if (diff > 0.4f && diff < 0.6f) {
- rounded = value + 1f
+ return if (diff > 0.4f && diff < 0.6f) {
+ (value + 1f).toInt()
 } else {
- rounded = value.toFloat()
+ value.toFloat().toInt()
 }
- return rounded.toInt()


The change looks valid (i.e. doesn't change semantics), but this is a really weird function. I understand neither what it does nor why it does it.

Just purely mechanical, minor, suggestions:

Use floor(scaled) rather than scaled.toInt().

Pass contentScale as an argument rather than access it inside, to avoid accessing it twice.

m-sasha · 2024-09-17T11:00:30Z

skiko/src/awtMain/kotlin/org/jetbrains/skiko/SkiaLayer.awt.kt

+ override fun ancestorRemoved(event: AncestorEvent?) = Unit
+
+ override fun ancestorMoved(event: AncestorEvent?) {
+ revalidate()


Why not redrawer?.syncBounds() here?

revalidate sounds more expensive...

I tried, but we have glitches on resize. It seems we need to schedule it to the layout phase, to keep layouts in sync.

I checked the performance when we move the window, it doesn't decrease. When we call revalidate, it schedules relayout of only SkiaLayer + one child java.awt.Canvas. They don't have other children (a SwingPanel in Compose is added to a sibling container)

I guess I'm just not sure I understand why revalidation is needed here... Only the (absolute) position of the layer changes, not the size, so layout should not change.

Maybe SwingUtilities.invokeLater { redrawer?.syncBounds() } ?

We can't say that invokeLater is in sync with layout.

Actually I figured why there were resize glitches. addAncestorListener receives events asynchronously. I replaced it by addHierarchyBoundsListener which is called synchronously on the parent layout. We don't need revalidate then.

Also, I looked at the revalidate doc, and you are right, it is in't performant, as it actually revalidates the whole tree back to the root.

So, I removed it from the other place, I am not confident now to fix another very small issue (recalling adjustSizeToContentScale on content scale change).

Sorry, wait a minute. I tested it with a wrong version. Glitches are back. Investigating

I returned to revalidate.

I looked at the documentation:
https://docs.oracle.com/javase/7/docs/api/javax/swing/JComponent.html#revalidate()

it notify the validateRoot, but it doesn't validate all children, just what was invalidated

it seems that only validate is expensive, but it is deferred, and actually is already called on every parent size change or when a component is added/removed

I don't want to user invokeLater because:

we need to handle a proper cancelation during dispose

we need to handle double invokeLater

we are not sure that it will be called the same time as the layout. I investigated resize glitches, and it appeared that we called syncBounds too often - we called it even if it haven't performed doLayout yet, and so syncBounds was based on incorrect size.

m-sasha · 2024-09-17T13:09:21Z

skiko/src/awtMain/kotlin/org/jetbrains/skiko/SkiaLayer.awt.kt

+// TODO Recheck this method validity in 2 cases - full Window content, and a Panel content
+// https://youtrack.jetbrains.com/issue/CMP-5447/Window-white-line-on-the-bottom-before-resizing
+/**
+ * Change [value] size to avoid rounding errors, when converting AWT Int coordinates to pixels.


What does it mean "avoid rounding errors"? What rounding errors? The link to the ticket does help though.

But why 0.4 and 0.6 specifically?

Should we maybe instead use the same function that determines the real content size and compare it to the window/container size? If it's smaller, then increase the unscaled size.

e.g.

return if ((value * contentScale).roundToInt() < windowSize) value + 1 else value

(assuming the scaled/real content size is (value * contentScale).roundToInt()).

Anyway, this is not a blocker for this PR in any way.

Rounding errors when converting Int points (10) to pixels (12) with float scale (1.25)

I will change the comment, but I will not change the logic in this PR, as it isn't relevant to it, and I also don't know the whole context of the fix itself, just knowledge of the issue.

The link to the issue is in the comment above, this is directly related.

As far as I understand, with this code there was an attempt to increase the size of the component, to avoid white line (but instead overlapped one meaningful content line)

Changed the comment

This reverts commit b036927.

igordmn added 2 commits September 16, 2024 20:02

Fix Error: This request has been automatically failed because it uses…

547ac25

… a deprecated version of `actions/upload-artifact: v2` Learn more: https://github.blog/changelog/2024-02-13-deprecation-notice-v1-and-v2-of-the-artifact-actions/

igordmn force-pushed the igor.demin/fix-macos-syncSize branch from 38c22fb to 547ac25 Compare September 16, 2024 18:02

igordmn marked this pull request as draft September 16, 2024 18:24

igordmn added 10 commits September 16, 2024 20:37

Revert "Fix bounds of ComposePanel in IntelliJ on macOs"

a0d2b16

This reverts commit cf1cc55.

Test

56200f3

Reapply "Fix bounds of ComposePanel in IntelliJ on macOs"

6d4dcd5

This reverts commit a0d2b16.

Fix test

b0aa9f5

Test

b6fffaf

Fix

3d9124f

Fix test

62394a4

Remove redundancy with doLayout

ee5bcba

Fix test

e97db0b

Fix test

7e0277b

igordmn commented Sep 16, 2024

View reviewed changes

igordmn marked this pull request as ready for review September 16, 2024 20:08

igordmn requested a review from m-sasha September 16, 2024 20:51

m-sasha approved these changes Sep 17, 2024

View reviewed changes

Extract adjustSizeToContentScale

5386b5a

igordmn mentioned this pull request Sep 17, 2024

Replace half leading to top ratio #989

Merged

m-sasha reviewed Sep 17, 2024

View reviewed changes

igordmn added 3 commits September 17, 2024 15:23

Remove revalidate

b036927

Revert "Remove revalidate"

f54ed82

This reverts commit b036927.

adjustSizeToContentScale comment

e1c1c6b

igordmn requested a review from m-sasha September 17, 2024 17:00

m-sasha approved these changes Sep 17, 2024

View reviewed changes

igordmn merged commit 1ca83c1 into master Sep 17, 2024
6 checks passed

igordmn deleted the igor.demin/fix-macos-syncSize branch September 17, 2024 22:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bounds of ComposePanel in IntelliJ on macOs #988

Fix bounds of ComposePanel in IntelliJ on macOs #988

igordmn commented Sep 16, 2024 •

edited

Loading

igordmn Sep 16, 2024 •

edited

Loading

igordmn Sep 16, 2024

m-sasha Sep 17, 2024

igordmn Sep 17, 2024 •

edited

Loading

igordmn Sep 17, 2024

m-sasha Sep 17, 2024

igordmn Sep 17, 2024

m-sasha Sep 17, 2024

m-sasha Sep 17, 2024

igordmn Sep 17, 2024

m-sasha Sep 17, 2024

igordmn Sep 17, 2024

igordmn Sep 17, 2024

igordmn Sep 17, 2024

m-sasha Sep 17, 2024

igordmn Sep 17, 2024 •

edited

Loading

igordmn Sep 17, 2024

igordmn Sep 17, 2024

Fix bounds of ComposePanel in IntelliJ on macOs #988

Fix bounds of ComposePanel in IntelliJ on macOs #988

Conversation

igordmn commented Sep 16, 2024 • edited Loading

Testing

Release Notes

Fixes - Desktop

igordmn Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igordmn Sep 17, 2024 • 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

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

igordmn Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igordmn commented Sep 16, 2024 •

edited

Loading

igordmn Sep 16, 2024 •

edited

Loading

igordmn Sep 17, 2024 •

edited

Loading

igordmn Sep 17, 2024 •

edited

Loading