Skip to content

Fix RecordDrawRectRenderDecorator closing #2864

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.

Sign up for GitHub

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

Open
MatkovIvan wants to merge 3 commits into
base: jb-main
Choose a base branch
from
Open

Fix RecordDrawRectRenderDecorator closing #2864

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a151fd5
Fix `RecordDrawRectRenderDecorator` closing
MatkovIvan Mar 12, 2026
6b1f152
Close all `drawBoundsRecorder`s in `close` call
MatkovIvan Mar 12, 2026
e96d198
Require calling recordDrawBounds once
MatkovIvan Mar 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions ...i/ui/src/desktopMain/kotlin/androidx/compose/ui/scene/DesktopComposeSceneLayer.desktop.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ internal abstract class DesktopComposeSceneLayer(
eventType: PointerEventType,
button: PointerButton?
) -> Unit)? = null
private var drawBoundsRecorder: RecordDrawRectRenderDecorator? = null
private var isClosed = false

final override var density: Density = density
Expand Down Expand Up @@ -109,6 +110,8 @@ internal abstract class DesktopComposeSceneLayer(

@CallSuper
override fun close() {
drawBoundsRecorder?.close()
drawBoundsRecorder = null
isClosed = true
}

Expand Down Expand Up @@ -147,6 +150,9 @@ internal abstract class DesktopComposeSceneLayer(
bottom = boundsInWindow.bottom + maxDrawInflate.bottom
)
onDrawBoundsChanged()
}.also {
drawBoundsRecorder?.close()
drawBoundsRecorder = it
Copy link

@m-sasha m-sasha Mar 12, 2026

Choose a reason for hiding this comment

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

This looks strange. We're returning an object but continue owning it, and its lifecycle is the same as the DesktopComposeSceneLayer, unless someone asks for a new one.

Maybe something like this instead?

    protected var targetRenderDelegate: SkikoRenderDelegate? = null
    protected val drawBoundsRecorder = RecordDrawRectRenderDecorator(
        decorated = { canvas, width, height, nanoTime ->
            targetRenderDelegate?.onRender(canvas, width, height, nanoTime)
        }
    ) { canvasBoundsInPx ->
        val currentCanvasOffset = drawBounds.topLeft
        val drawBoundsInWindow = canvasBoundsInPx.roundToIntRect().translate(currentCanvasOffset)
        maxDrawInflate = maxInflate(boundsInWindow, drawBoundsInWindow, maxDrawInflate)
        drawBounds = IntRect(
            left = boundsInWindow.left - maxDrawInflate.left,
            top = boundsInWindow.top - maxDrawInflate.top,
            right = boundsInWindow.right + maxDrawInflate.right,
            bottom = boundsInWindow.bottom + maxDrawInflate.bottom
        )
        onDrawBoundsChanged()
    }

Copy link
Member Author

@MatkovIvan MatkovIvan Mar 12, 2026

Choose a reason for hiding this comment

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

This assumes that the function is called only once and we're having single targetRenderDelegate. This is kinda true for now, but seems more dangerous than "closing during close".

I'll think about different ways...

Copy link
Member Author

@MatkovIvan MatkovIvan Mar 12, 2026

Choose a reason for hiding this comment

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

Made it more generic, ptal

Copy link

@m-sasha m-sasha Mar 13, 2026

Choose a reason for hiding this comment

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

This assumes that the function is called only once and we're having single targetRenderDelegate. This is kinda true for now, but seems more dangerous than "closing during close".

I think it's better for the code to reflect the current state rather than some future possible state.

With the latest changes it creates the illusion that recordDrawBounds can be called multiple times to wrap different, concurrently "alive" SkikoRenderDelegates , but this is not actually the case, as it will cause race conditions (the decorator writes to "global" state).

}

/**
Expand Down
21 changes: 10 additions & 11 deletions .../ui/src/skikoMain/kotlin/androidx/compose/ui/skiko/RecordDrawRectRenderDecorator.skiko.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,10 @@ import org.jetbrains.skiko.SkikoRenderDelegate
internal class RecordDrawRectRenderDecorator(
private val decorated: SkikoRenderDelegate,
private val onDrawRectChange: (Rect) -> Unit
) : SkikoRenderDelegate by decorated {
) : SkikoRenderDelegate by decorated, AutoCloseable {
private val pictureRecorder = PictureRecorder()
private val bbhFactory = RTreeFactory()
private var isClosed = false
private var drawRect = Rect.Zero
set(value) {
if (value != field) {
Expand All @@ -39,13 +40,19 @@ internal class RecordDrawRectRenderDecorator(
}
}

// TODO(@MatkovIvan): nobody calls it
fun close() {
override fun close() {
if (isClosed) {
return
}
isClosed = true
pictureRecorder.close()
bbhFactory.close()
}

override fun onRender(canvas: Canvas, width: Int, height: Int, nanoTime: Long) {
if (isClosed) {
return
}
drawRect = canvas.recordCullRect {
decorated.onRender(it, width, height, nanoTime)
}?.toComposeRect() ?: Rect.Zero
Expand Down Expand Up @@ -87,11 +94,3 @@ internal class RecordDrawRectRenderDecorator(
* so temporary applying some offset is required to get right measurement in negative area.
*/
private const val MeasureOffset = (1 shl 14).toFloat()

private val SkRect.Companion.Unconstrained: SkRect
get() = makeLTRB(
l = Float.MIN_VALUE,
t = Float.MIN_VALUE,
r = Float.MAX_VALUE,
b = Float.MAX_VALUE
)
Loading