Skip to content

Commit

Permalink
Fix bounds of ComposePanel in IntelliJ on macOs (#988)
Browse files Browse the repository at this point in the history
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
- Fix bounds of ComposePanel in IntelliJ on macOs
  • Loading branch information
igordmn committed Sep 17, 2024
1 parent e2bf12b commit 1ca83c1
Show file tree
Hide file tree
Showing 11 changed files with 102 additions and 28 deletions.
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
69 changes: 50 additions & 19 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,10 @@ 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
import kotlin.math.ceil
import kotlin.math.floor

actual open class SkiaLayer internal constructor(
externalAccessibleFactory: ((Component) -> Accessible)? = null,
Expand Down Expand Up @@ -133,6 +137,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()
}
})

backedLayer.addHierarchyListener {
if (it.changeFlags and HierarchyEvent.SHOWING_CHANGED.toLong() != 0L) {
checkShowing()
Expand All @@ -143,7 +157,7 @@ actual open class SkiaLayer internal constructor(
addPropertyChangeListener("graphicsContextScaleTransform") {
Logger.debug { "graphicsContextScaleTransform changed for $this" }
latestReceivedGraphicsContextScaleTransform = it.newValue as AffineTransform
redrawer?.syncSize()
revalidate()
notifyChange(PropertyKind.ContentScale)

// Workaround for JBR-5259
Expand Down Expand Up @@ -191,7 +205,7 @@ actual open class SkiaLayer internal constructor(
redrawer?.setVisible(isShowing)
}
if (isShowing) {
redrawer?.syncSize()
redrawer?.syncBounds()
repaint()
}
}
Expand Down Expand Up @@ -252,7 +266,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 +330,16 @@ 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(contentScale, width),
adjustSizeToContentScale(contentScale, 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 +356,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,18 +597,6 @@ actual open class SkiaLayer internal constructor(
}
}

private fun roundSize(value: Int): Int {
var rounded = value * contentScale
val diff = rounded - rounded.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
} else {
rounded = value.toFloat()
}
return rounded.toInt()
}

fun requestNativeFocusOnAccessible(accessible: Accessible?) {
backedLayer.requestNativeFocusOnAccessible(accessible)
}
Expand Down Expand Up @@ -637,3 +643,28 @@ internal fun Canvas.clipRectBy(rectangle: ClipRectangle, scale: Float) {
true
)
}

// TODO Recheck this method validity in 2 cases - full Window content, and a Panel content
// issue: https://youtrack.jetbrains.com/issue/CMP-5447/Window-white-line-on-the-bottom-before-resizing
// suggestions: https://github.com/JetBrains/skiko/pull/988#discussion_r1763219300
// possible issues:
// - isn't obvious why 0.4/0.6 is used
// - increasing it by one, we avoid 1px white line, but we cut the content by 1px
// - it probably doesn't work correctly in a Panel content case - we don't need to adjust in this case
/**
* Increases the value of width/height by one if necessary,
* to avoid 1px white line between Canvas and the bounding window.
* 1px white line appears when users resizes the window:
* - it is resized in Px (125, 126, 127,...)
* - the canvas is resized in Points (with 1.25 scale, it will be 100, 100, 101)
* during converting Int AWT points to actual screen pixels.
*/
private fun adjustSizeToContentScale(contentScale: Float, value: Int): Int {
val scaled = value * contentScale
val diff = scaled - floor(scaled)
return if (diff > 0.4f && diff < 0.6f) {
value + 1
} else {
value
}
}
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

0 comments on commit 1ca83c1

Please sign in to comment.