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.

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

Jump to bottom

Refactor ComposeScene #908

Merged
merged 67 commits into from Nov 24, 2023
Merged

Refactor ComposeScene #908

merged 67 commits into from Nov 24, 2023

Conversation

MatkovIvan
Copy link
Member

@MatkovIvan MatkovIvan commented Nov 20, 2023 •
edited

Proposed Changes

It's the evolution of #891

Compose scene:

  • Deprecate the existing API and introduce an internal interface instead
  • Remove key listeners from setContent
  • constraints -> size
  • Add lastKnownPointerPosition to public interface
  • CombinedComposeScene as a compatible version of scene
  • Remove recently added semanticsOwner
  • ComposeScene.Pointer -> ComposeScenePointer

Other changes:

  • Rename SkiaBasedOwner to RootNodeOwner. Also, it uses aggregation instead of implementing a few interfaces.
  • SkikoComposeUiTest overrides PlatformContext.RootForTestListener now. It solves TODOs about WindowInfo in tests.
  • SkiaRootForTest is renamed to PlatformRootForTest
  • onRootCreatedCallback/onRootDisposedCallback is now part of PlatformContext
  • PlatformRootForTest exposes real visibleBounds instead of just window size information.
  • PlatformRootForTest exposes send pointer input methods instead of scene reference.
  • InternalComposeUiApi is opted in inside the same module.
  • Platform -> PlatfromContext
  • dialogScrimBlendMode -> isWindowTransparent
  • AccessibilityController: Remove interface from skikoMain source set
  • AccessibilityController: syncLoop triggered right from desktop-only part
  • Replaced Platform.accessibilityController(SemanticsOwner) to semanticsOwnerListener
  • Fixes an invalid test case: out of screen Popup cannot receive input now
  • Removed unnecessary expect/actual createSkiaLayer
  • Moves "Deprecated" DefaultViewConfiguration to test where there is a single usage of it.

API Changes

See compose/ui/ui/api/desktop/ui.api file

Testing

Test: run existing tests.

@MatkovIvan
Move input logic from scene into CombinedRootNodeOwner
3cfd52e
@MatkovIvan
Fix attachedOwners list cache in case of recursive calls
da3597a
@MatkovIvan
Fix missing setting constrains to attached owners
9e2dec2
@MatkovIvan
Make RootNodeOwner skia independent
f9353e4
@MatkovIvan
Fix root owner initialization
2d4149c
@MatkovIvan
Fix missing measuring pass in tests
8dd1575
@MatkovIvan
BaseComposeScene, ComposeSceneInputHandler, SnapshotInvalidationTracker
914eb54
@MatkovIvan
Hide RootNodeOwnerImpl
850a8aa
@MatkovIvan
Remove unused expect/actual createSkiaLayer
438c5dd
@MatkovIvan
ComposeSceneRecomposer
cf6c20d
@MatkovIvan
PlatformContext, ComposeSceneContext
b804aad
@MatkovIvan
New ComposeScene
561dafb
@MatkovIvan
CombinedComposeScene
c4333f5
@MatkovIvan
AttachedComposeSceneLayer
21afbf9
@MatkovIvan
Focus TODOs
403382d
@MatkovIvan
Another moving round
919968f
@MatkovIvan
Update usage in ImageComposeScene
4909ef1
@MatkovIvan
Dialog scrim
e81eadd
@MatkovIvan
DesktopAccessibilityListener
7ddf9f4
@MatkovIvan
Fix closing order
5bfd70f
@MatkovIvan
Revert rememberCursorPosition
703ebe1
@MatkovIvan
Bounds instead of constrains, AttachedComposeSceneLayer callbacks
a490d87
@MatkovIvan
Update API usage in desktop tests
d062317
@MatkovIvan
Fix window onKeyEvent, Move isWindowTransparent
1e13361
@MatkovIvan
Split RootNodeOwner and Owner
7f5b6cd
@MatkovIvan
Fix Popup/Dialog keyboard handling
5d7a857
@MatkovIvan
Fix scene invalidation in tests
8bcc410
@MatkovIvan
Adopt DepthSortedSetTest
a51e724
@MatkovIvan
Initialize focusable before invalidation
9ec9abc
@MatkovIvan
Fix incorrect expectations in Popup test
17b9353
@MatkovIvan MatkovIvan marked this pull request as ready for review November 22, 2023 09:07
m-sasha
m-sasha reviewed Nov 22, 2023
View reviewed changes
compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/platform/ComposeSceneAccessible.kt Outdated Show resolved Hide resolved
compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/awt/ComposeBridge.desktop.kt Outdated Show resolved Hide resolved
compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/awt/ComposeBridge.desktop.kt Outdated Show resolved Hide resolved
compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/awt/ComposeBridge.desktop.kt Outdated Show resolved Hide resolved
compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/awt/ComposeBridge.desktop.kt Show resolved Hide resolved
compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/awt/ComposeBridge.desktop.kt
Comment on lines +395 to +413
FocusDirection.Next -> {
val toFocus = component.focusCycleRootAncestor?.let { root ->
val policy = root.focusTraversalPolicy
policy.getComponentAfter(root, component)
?: policy.getDefaultComponent(root)
}
val hasFocus = toFocus?.hasFocus() == true
!hasFocus && toFocus?.requestFocusInWindow(FocusEvent.Cause.TRAVERSAL_FORWARD) == true
}

override val textInputService = PlatformInput(platformComponent)
FocusDirection.Previous -> {
val toFocus = component.focusCycleRootAncestor?.let { root ->
val policy = root.focusTraversalPolicy
policy.getComponentBefore(root, component)
?: policy.getDefaultComponent(root)
}
val hasFocus = toFocus?.hasFocus() == true
!hasFocus && toFocus?.requestFocusInWindow(FocusEvent.Cause.TRAVERSAL_BACKWARD) == true
}
Copy link

Choose a reason for hiding this comment

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

Was this copy/pasted from somewhere else? Can we simplify it? Maybe

        override fun moveFocus(focusDirection: FocusDirection): Boolean {
            val (componentGetter, cause) = when(focusDirection) {
                FocusDirection.Next ->
                    FocusTraversalPolicy::getComponentAfter to FocusEvent.Cause.TRAVERSAL_FORWARD
                FocusDirection.Previous ->
                    FocusTraversalPolicy::getComponentAfter to FocusEvent.Cause.TRAVERSAL_BACKWARD
                else -> return false
            }

            val toFocus = component.focusCycleRootAncestor?.let { root ->
                val policy = root.focusTraversalPolicy
                componentGetter(policy, root, component)
                    ?: policy.getDefaultComponent(root)
            } ?: return false


            return !toFocus.hasFocus() && toFocus.requestFocusInWindow(cause)
        }

Copy link
Member Author

Choose a reason for hiding this comment

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

Was this copy/pasted from somewhere else?

Yes, it's just moved from another line without change.

compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/scene/ComposeScene.kt Outdated Show resolved Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/scene/ComposeScene.kt Outdated Show resolved Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/scene/SimpleComposeScene.kt Outdated Show resolved Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/scene/CombinedComposeScene.kt Outdated Show resolved Hide resolved
igordmn
igordmn approved these changes Nov 22, 2023
View reviewed changes
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.

Overall LGTM with the old/new naming approach. Great job 🎉.

@MatkovIvan MatkovIvan merged commit 602e66d into jb-main Nov 24, 2023
4 checks passed
@MatkovIvan MatkovIvan deleted the ivan.matkov/scene-refactor-2 branch November 24, 2023 10:51
@MatkovIvan MatkovIvan mentioned this pull request Dec 6, 2023
Merged
MatkovIvan added a commit that referenced this pull request Dec 6, 2023
@MatkovIvan
Avoid unnecessary composition recreation in Dialog (#925)
8f4f4cb 
## Proposed Changes

- Fixes regression after #908
- Repeated call of `setContent` recreates recomposition that breaks the
state.

## Testing

```kt
fun main() = singleWindowApplication {
    var text by remember { mutableStateOf("") }
    Dialog(
        onDismissRequest = {  },
    ) {
        Box(modifier = Modifier.background(Color.White).size(400.dp, 300.dp)) {
            OutlinedTextField(
                value = text,
                onValueChange = { text = it },
                modifier = Modifier.padding(32.dp)
            )
        }
    }
}
```
mazunin-v-jb pushed a commit that referenced this pull request Dec 7, 2023
@MatkovIvan @mazunin-v-jb
Refactor ComposeScene (#908)
bb7044f 
## Proposed Changes

It's the evolution of #891

Compose scene:
- Deprecate the existing API and introduce an internal interface instead
- Remove key listeners from `setContent`
- `constraints` -> `size`
- Add `lastKnownPointerPosition` to public interface 
- `CombinedComposeScene` as a compatible version of scene
- Remove recently added `semanticsOwner`
- `ComposeScene.Pointer` -> `ComposeScenePointer`

Other changes:
- Rename `SkiaBasedOwner` to `RootNodeOwner`. Also, it uses aggregation
instead of implementing a few interfaces.
- `SkikoComposeUiTest` overrides `PlatformContext.RootForTestListener`
now. It solves TODOs about `WindowInfo` in tests.
- `SkiaRootForTest` is renamed to `PlatformRootForTest`
- `onRootCreatedCallback`/`onRootDisposedCallback` is now part of
`PlatformContext`
- `PlatformRootForTest` exposes real `visibleBounds` instead of just
window size information.
- `PlatformRootForTest` exposes send pointer input methods instead of
`scene` reference.
- `InternalComposeUiApi` is opted in inside the same module.
- `Platform` -> `PlatfromContext`
- `dialogScrimBlendMode` -> `isWindowTransparent`
- `AccessibilityController`: Remove interface from `skikoMain` source
set
- `AccessibilityController`: `syncLoop` triggered right from
desktop-only part
- Replaced `Platform.accessibilityController(SemanticsOwner)` to
`semanticsOwnerListener`
- Fixes an invalid test case: out of screen Popup cannot receive input
now
- Removed unnecessary expect/actual `createSkiaLayer`
- Moves "Deprecated" `DefaultViewConfiguration` to test where there is a
single usage of it.

## API Changes

See `compose/ui/ui/api/desktop/ui.api` file

## Testing

Test: run existing tests.
mazunin-v-jb pushed a commit that referenced this pull request Dec 7, 2023
@MatkovIvan @mazunin-v-jb
Avoid unnecessary composition recreation in Dialog (#925)
bdda943 
## Proposed Changes

- Fixes regression after #908
- Repeated call of `setContent` recreates recomposition that breaks the
state.

## Testing

```kt
fun main() = singleWindowApplication {
    var text by remember { mutableStateOf("") }
    Dialog(
        onDismissRequest = {  },
    ) {
        Box(modifier = Modifier.background(Color.White).size(400.dp, 300.dp)) {
            OutlinedTextField(
                value = text,
                onValueChange = { text = it },
                modifier = Modifier.padding(32.dp)
            )
        }
    }
}
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m-sasha m-sasha m-sasha approved these changes

@igordmn igordmn igordmn approved these changes

@dima-avdeev-jb dima-avdeev-jb dima-avdeev-jb approved these changes

Assignees
No one assigned
Labels
changes in API
Projects
None yet
Milestone
No milestone
4 participants
@MatkovIvan @m-sasha @igordmn @dima-avdeev-jb