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 render order of interop views #1145

Merged
merged 12 commits into from
Feb 29, 2024
Merged

Conversation

MatkovIvan
Copy link
Member

Proposed Changes

Currently on both Desktop and iOS interop views are added to the view hierarchy in order to add nodes to Compose. It works only if all intersecting interop views were added at the same time (frame). So it's basically last-added - above-displayed. This PR changes this behavior in a way that it will respect the order inside Compose like regular compose elements.

It does NOT make any changes in the ability to display Compose content above interop view on Desktop, this fix was made in #915

Main changes:

  • Unify a way to work with interop on Desktop (SwingPanel) and iOS (UIKitView)
  • LocalInteropContainer -> LocalUIKitInteropContainer on iOS
  • LocalLayerContainer -> LocalSwingInteropContainer on Desktop
  • Reduce copy-pasting by moving OverlayLayout and EmptyLayout
  • Remove overriding add method on ComposePanel and ComposeWindowPanel - it was required to redirect interop, but now it's not required and it's better to avoid changing default AWT hierarchy behaviour
  • Do not use JLayeredPane's layers anymore - it brings a lot of transparency issues (faced with it on Windows too after unrelated change). Sorting via indexes is used instead
  • Add InteropOrder page to mpp demo

How it works

It utilizes TraversableNode to traverse the tree in the right order and calculate the index based on interop views count that placed before the current node in the hierarchy. All interop nodes are marked via Modifier.trackSwingInterop/Modifier.trackUIKitInterop modifier to filter them from the LayoutNodes tree.

Testing

Test: run reproducers from the issues or look at "InteropOrder" page in mpp demo

Desktop iOS
Screenshot 2024-02-27 at 12 51 06 Simulator Screenshot - iPhone 15 Pro - 2024-02-27 at 12 49 50

Issues Fixed

Desktop

Fixes JetBrains/compose-multiplatform#2926
Fixes JetBrains/compose-multiplatform#1521 (comment)

iOS

Fixes JetBrains/compose-multiplatform#4004
Fixes JetBrains/compose-multiplatform#3848

@MatkovIvan MatkovIvan force-pushed the ivan.matkov/interop-render-order branch from 0c61457 to e1e6886 Compare February 28, 2024 12:28
# Conflicts:
#	compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/awt/SwingPanel.desktop.kt
# Conflicts:
#	compose/mpp/demo/src/uikitMain/kotlin/androidx/compose/mpp/demo/main.uikit.kt
Copy link
Collaborator

@igordmn igordmn left a comment

Choose a reason for hiding this comment

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

Cool!

With interopBlending=true it works with Compose in the middle:

        Box {
            if (red) {
                TestInteropView(Modifier.size(150.dp).offset(0.dp, 0.dp), Color.Red)
            }
            if (green) {
                Box(Modifier.size(150.dp).offset(75.dp, 75.dp).background(Color.Green))
            }
            if (blue) {
                TestInteropView(Modifier.size(150.dp).offset(150.dp, 150.dp), Color.Blue)
            }
        }

The code looks good to me.

import androidx.compose.ui.unit.dp

@Composable
fun InteropOrder() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I tried this strange example with useInteropBlending = true on Windows:

        Box {
            if (red) {
                Box(Modifier.size(150.dp).offset(0.dp, 0.dp).background(Color.Red))
                TestInteropView(Modifier.size(150.dp).offset(0.dp, 0.dp), Color.Red)
            }
            if (green) {
                TestInteropView(Modifier.size(150.dp).offset(75.dp, 75.dp), Color.Green)
            }
            if (blue) {
                Box(Modifier.size(150.dp).offset(150.dp, 150.dp).background(Color.Blue))
            }
        }

And sometimes it has a wrong ordering:
image

Sometimes correct:
image

Is it expected or is it a bug?

Copy link
Member Author

Choose a reason for hiding this comment

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

A bug. Fixed by adding validate/repaint after adding of interop (this PR) + Fix from #1147

@MatkovIvan MatkovIvan merged commit 339dbd3 into jb-main Feb 29, 2024
9 checks passed
@MatkovIvan MatkovIvan deleted the ivan.matkov/interop-render-order branch February 29, 2024 23:01
MatkovIvan added a commit that referenced this pull request Feb 29, 2024
## Proposed Changes

- Fix setting bounds - currently it relys on clipped size
- Use clipped bounds for actual clipping, but not clipped for interop
layout
- Based on #1145

## Testing

Test: try to use `SwingPanel` inside scroll

```kt
Column(Modifier.size(250.dp).verticalScroll(rememberScrollState())) {
    repeat(10) {
        SwingPanel(
            factory = { JButton() },
            modifier = Modifier.size(100.dp)
        )
    }
}
````

Before:


https://github.com/JetBrains/compose-multiplatform-core/assets/1836384/362198c2-f8fa-4004-881b-5a7a6867f0c0

After:


https://github.com/JetBrains/compose-multiplatform-core/assets/1836384/546968e6-dcff-4f70-9935-0465dcb2a6e4




## Issues Fixed

Fixes JetBrains/compose-multiplatform#3207
MatkovIvan added a commit that referenced this pull request Mar 1, 2024
## Proposed Changes

Additional fix for #1145 and #1147

- Re-validate interop container after resizing component
- Fix incorrect calls - `componentResized` -> `onChangeWindowSize` and
`componentMoved` -> `onChangeWindowPosition`

## Testing

Test: Resize a window on "InteropOrder" mpp page (reproduced on macOS)

## Issues Fixed

Fixes
#1145 (comment)
MatkovIvan added a commit that referenced this pull request Mar 14, 2024
## Proposed Changes

Regression after #1145 (more specific is -
156b2ee)
- it was replaced by a zero-size layout that prevented drawing anything
and might lead to crashes in native views due to a zero size.
Please note that before we had incorrect/inconsistent behavior - iOS
interop occupied max available space. This PR aligns behaviour with
[`AndroidView`](https://github.com/JetBrains/compose-multiplatform-core/blob/v1.6.0/compose/ui/ui/src/androidMain/kotlin/androidx/compose/ui/viewinterop/AndroidViewHolder.android.kt#L387)
and just
[`Box`](https://github.com/JetBrains/compose-multiplatform-core/blob/v1.6.0/compose/foundation/foundation-layout/src/commonMain/kotlin/androidx/compose/foundation/layout/Box.kt#L212-L214)

## Testing

Test: mpp demo, pages with interop

## Issues Fixed

Fixes JetBrains/compose-multiplatform#4447
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants