Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bounds of ComposePanel in IntelliJ on macOs #988

Merged
merged 16 commits into from
Sep 17, 2024
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ jobs:
./gradlew --stacktrace --info -Pskiko.native.enabled=true -Pskiko.test.onci=true :skiko:tvosX64Test
# tvosSimulatorArm64Test will build the binary but the tests will be skipped due to X64 host machine
./gradlew --stacktrace --info -Pskiko.native.enabled=true -Pskiko.test.onci=true :skiko:tvosSimulatorArm64Test
- uses: actions/upload-artifact@v2
- uses: actions/upload-artifact@v4
if: always()
with:
name: test-reports-macos
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/web.yml
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ jobs:
cd ./skiko
source ./emsdk/emsdk_env.sh
./gradlew --stacktrace --info -Pskiko.wasm.enabled=true -Pskiko.js.enabled=true -Pskiko.test.onci=true jsTest
- uses: actions/upload-artifact@v2
- uses: actions/upload-artifact@v4
if: always()
with:
name: test-reports-wasm
Expand Down
38 changes: 24 additions & 14 deletions skiko/src/awtMain/kotlin/org/jetbrains/skiko/SkiaLayer.awt.kt
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import javax.swing.JPanel
import javax.swing.SwingUtilities
import javax.swing.SwingUtilities.isEventDispatchThread
import javax.swing.UIManager
import javax.swing.event.AncestorEvent
import javax.swing.event.AncestorListener

actual open class SkiaLayer internal constructor(
externalAccessibleFactory: ((Component) -> Accessible)? = null,
Expand Down Expand Up @@ -133,6 +135,16 @@ actual open class SkiaLayer internal constructor(
@Suppress("LeakingThis")
add(backedLayer)

addAncestorListener(object : AncestorListener {
override fun ancestorAdded(event: AncestorEvent?) = Unit

override fun ancestorRemoved(event: AncestorEvent?) = Unit

override fun ancestorMoved(event: AncestorEvent?) {
revalidate()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not redrawer?.syncBounds() here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revalidate sounds more expensive...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() } ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
})

backedLayer.addHierarchyListener {
if (it.changeFlags and HierarchyEvent.SHOWING_CHANGED.toLong() != 0L) {
checkShowing()
Expand All @@ -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()
Copy link
Collaborator Author

@igordmn igordmn Sep 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed because layout depends on the scale inside adjustSizeToContentScale

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

@igordmn igordmn Sep 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()
        }
    }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

notifyChange(PropertyKind.ContentScale)

// Workaround for JBR-5259
Expand Down Expand Up @@ -191,7 +203,7 @@ actual open class SkiaLayer internal constructor(
redrawer?.setVisible(isShowing)
}
if (isShowing) {
redrawer?.syncSize()
redrawer?.syncBounds()
repaint()
}
}
Expand Down Expand Up @@ -252,7 +264,7 @@ actual open class SkiaLayer internal constructor(
private val redrawerManager = RedrawerManager<Redrawer>(properties.renderApi) { renderApi, oldRedrawer ->
oldRedrawer?.dispose()
val newRedrawer = renderFactory.createRedrawer(this, renderApi, analytics, properties)
newRedrawer.syncSize()
newRedrawer.syncBounds()
newRedrawer
}

Expand Down Expand Up @@ -316,12 +328,11 @@ actual open class SkiaLayer internal constructor(

override fun doLayout() {
Logger.debug { "doLayout on $this" }
backedLayer.setBounds(0, 0, roundSize(width), roundSize(height))
backedLayer.setBounds(0, 0, adjustSizeToContentScale(width), adjustSizeToContentScale(height))
backedLayer.validate()
redrawer?.syncSize()
redrawer?.syncBounds()
}


override fun paint(g: java.awt.Graphics) {
Logger.debug { "Paint called on: $this" }
checkContentScale()
Expand All @@ -338,7 +349,7 @@ actual open class SkiaLayer internal constructor(
// Please note that calling redraw during layout might break software renderers,
// so applying this fix only for Direct3D case.
if (renderApi == GraphicsApi.DIRECT3D && isShowing) {
redrawer?.syncSize()
redrawer?.syncBounds()
tryRedrawImmediately()
}
}
Expand Down Expand Up @@ -579,16 +590,15 @@ actual open class SkiaLayer internal constructor(
}
}

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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Use floor(scaled) rather than scaled.toInt().
  2. Pass contentScale as an argument rather than access it inside, to avoid accessing it twice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}

fun requestNativeFocusOnAccessible(accessible: Accessible?) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ internal class MetalRedrawer(
}
}

override fun syncSize() = synchronized(drawLock) {
override fun syncBounds() = synchronized(drawLock) {
check(isEventDispatchThread()) { "Method should be called from AWT event dispatch thread" }
val rootPane = getRootPane(layer)
val globalPosition = convertPoint(layer.backedLayer, 0, 0, rootPane)
Expand Down
43 changes: 43 additions & 0 deletions skiko/src/awtTest/kotlin/org/jetbrains/skiko/SkiaLayerTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import java.awt.BorderLayout
import java.awt.Color
import java.awt.Dimension
import java.awt.event.*
import javax.swing.Box
import javax.swing.JFrame
import javax.swing.JLayeredPane
import javax.swing.JPanel
Expand Down Expand Up @@ -261,6 +262,48 @@ class SkiaLayerTest {
}
}

@Test
fun `move without redrawing`() = uiTest {
val window = JFrame()
val layer = SkiaLayer(
properties = SkiaLayerProperties(renderApi = renderApi)
)

layer.renderDelegate = object : SkikoRenderDelegate {
override fun onRender(canvas: Canvas, width: Int, height: Int, nanoTime: Long) {
canvas.drawRect(Rect(0f, 0f, width.toFloat(), height.toFloat()), Paint().apply {
color = Color.RED.rgb
})
}
}
layer.size = Dimension(100, 100)
val box = Box.createVerticalBox().apply {
add(layer)
}
box.setBounds(0, 0, 100, 100)

try {
val panel = JLayeredPane()
panel.add(box)
window.contentPane.add(panel)
window.setLocation(200, 200)
window.size = Dimension(200, 200)
window.defaultCloseOperation = WindowConstants.DISPOSE_ON_CLOSE
window.isUndecorated = true
window.isVisible = true

delay(1000)
screenshots.assert(window.bounds, "frame1")

box.setBounds(100, 0, 100, 100)
delay(1000)
screenshots.assert(window.bounds, "frame2")
} finally {
layer.dispose()
window.close()
}
}

@Test
fun `resize window`() = uiTest {
val window = UiTestWindow()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ internal interface Redrawer {
fun dispose()
fun needRedraw()
fun redrawImmediately()
fun syncSize() = Unit
fun syncBounds() = Unit
fun setVisible(isVisible: Boolean) = Unit
val renderInfo: String
}
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
Expand Up @@ -84,13 +84,13 @@ actual open class SkiaLayer {
private val nsViewObserver = object : NSObject() {
@ObjCAction
fun frameDidChange(notification: NSNotification) {
redrawer?.syncSize()
redrawer?.syncBounds()
redrawer?.redrawImmediately()
}

@ObjCAction
fun windowDidChangeBackingProperties(notification: NSNotification) {
redrawer?.syncSize()
redrawer?.syncBounds()
redrawer?.redrawImmediately()
}

Expand Down Expand Up @@ -126,7 +126,7 @@ actual open class SkiaLayer {
nsView.postsFrameChangedNotifications = true
nsViewObserver.addObserver()
redrawer = createNativeRedrawer(this, renderApi).apply {
syncSize()
syncBounds()
needRedraw()
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ internal class MacOsMetalRedrawer(
/**
* Synchronizes the [metalLayer] size with the size of underlying nsView
*/
override fun syncSize() {
override fun syncBounds() {
syncContentScale()
val osFrame = skiaLayer.nsView.frame
val (w, h) = osFrame.useContents {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ internal class MacOsOpenGLRedrawer(
glLayer.dispose()
}

override fun syncSize() {
override fun syncBounds() {
syncContentScale()
skiaLayer.nsView.frame.useContents {
glLayer.setFrame(
Expand Down
Loading