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

Fixed drag-and-drop in the case when there are multiple compositions attached to the same root (AWT) container. #1493

Merged
merged 7 commits into from
Aug 27, 2024

Conversation

m-sasha
Copy link
Member

@m-sasha m-sasha commented Aug 12, 2024

There is currently a problem with the DnD implementation because on the one hand the API implies a DragAndDropManager instance for each composition (RootNodeOwner) and on other hand, AwtDragAndDropManager needs to install its own AWT TransferHandler and DropTarget into the root container.

In this PR the first attached AwtDragAndDropManager installs a TransferHandler and a DropTarget into the root container, and the following AwtDragAndDropManagers reuse them. The DropTarget forwards its events to the AwtDragAndDropManager of the top-most "focusable" RootNodeOwner.

Fixes https://youtrack.jetbrains.com/issue/CMP-5855/Popup-breaks-drag-and-drop-functionality-on-Desktop

Testing

Tested manually with

@OptIn(ExperimentalFoundationApi::class, ExperimentalComposeUiApi::class)
fun main() = singleWindowApplication {
    val exportedText = "Hello, DnD!"
    Row(
        horizontalArrangement = Arrangement.SpaceEvenly,
        verticalAlignment = Alignment.CenterVertically,
        modifier = Modifier.fillMaxSize()
    ) {
        val textMeasurer = rememberTextMeasurer()
        Box(Modifier
            .size(200.dp)
            .background(Color.LightGray)
            .dragAndDropSource(
                drawDragDecoration = {
                    drawRect(
                        color = Color.White,
                        topLeft = Offset(x = 0f, y = size.height/4),
                        size = Size(size.width, size.height/2)
                    )
                    val textLayoutResult = textMeasurer.measure(
                        text = AnnotatedString(exportedText),
                        layoutDirection = layoutDirection,
                        density = this
                    )
                    drawText(
                        textLayoutResult = textLayoutResult,
                        topLeft = Offset(
                            x = (size.width - textLayoutResult.size.width) / 2,
                            y = (size.height - textLayoutResult.size.height) / 2,
                        )
                    )
                }
            ) {
                detectDragGestures(
                    onDragStart = { offset ->
                        startTransfer(
                            DragAndDropTransferData(
                                transferable = DragAndDropTransferable(
                                    StringSelection(exportedText)
                                ),
                                supportedActions = listOf(
                                    DragAndDropTransferAction.Copy,
                                    DragAndDropTransferAction.Move,
                                    DragAndDropTransferAction.Link,
                                ),
                                dragDecorationOffset = offset,
                                onTransferCompleted = { action ->
                                    println("Action at source: $action")
                                }
                            )
                        )
                    },
                    onDrag = { _, _ -> },
                )
            }
        ) {
            Text("Drag Me", Modifier.align(Alignment.Center))
        }

        var showTargetBorder by remember { mutableStateOf(false)}
        var targetText by remember { mutableStateOf("Drop Here") }
        val coroutineScope = rememberCoroutineScope()
        val dragAndDropTarget = remember {
            object: DragAndDropTarget {

                override fun onStarted(event: DragAndDropEvent) {
                    showTargetBorder = true
                }

                override fun onEnded(event: DragAndDropEvent) {
                    showTargetBorder = false
                }

                override fun onDrop(event: DragAndDropEvent): Boolean {
                    println("Action at target: ${event.action}")
                    val result = (targetText == "Drop Here")// && (event.action == DragAndDropTransferAction.Link)
                    targetText = event.awtTransferable.let {
                        if (it.isDataFlavorSupported(DataFlavor.stringFlavor))
                            it.getTransferData(DataFlavor.stringFlavor) as String
                        else
                            it.transferDataFlavors.first().humanPresentableName
                    }
                    coroutineScope.launch {
                        delay(2000)
                        targetText = "Drop Here"
                    }
                    return result
                }
            }
        }

        Box(Modifier
            .size(200.dp)
            .background(Color.LightGray)
            .then(
                if (showTargetBorder)
                    Modifier.border(BorderStroke(3.dp, Color.Black))
                else
                    Modifier
            )
           .dragAndDropTarget(
                shouldStartDragAndDrop = { true },
                target = dragAndDropTarget
            )
        ) {
            Text(targetText, Modifier.align(Alignment.Center))
        }
    }

    var showPopup by remember { mutableStateOf(false) }
    if (showPopup) {
        Popup {
            Text("Popup is shown")
        }
    }
   LaunchedEffect(Unit) {
        delay(5000)
        showPopup = true
        delay(5000)
        showPopup = false
    }
}

This should be tested by QA

Release Notes

Fixes - Desktop

  • (prerelease fix) Fixed drag-and-drop not working after a popup is displayed in the window.

@m-sasha m-sasha requested review from MatkovIvan and igordmn August 12, 2024 14:22
@m-sasha m-sasha requested a review from MatkovIvan August 12, 2024 15:06
/**
* Returns the currently active [PlatformDragAndDropManager].
*/
fun activeDragAndDropManager(): PlatformDragAndDropManager
Copy link
Member

Choose a reason for hiding this comment

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

What is the reasoning behind having a function instead of a property? Users of this API don't need to know that there is an implementation that contains multiple owners/managers

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fun activeDragAndDropManager(): PlatformDragAndDropManager
val dragAndDropManager: PlatformDragAndDropManager

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 property has the connotation of being simple value to calculate. A function is more generic with no such connotations, and so the user is more likely to avoid calling it twice in a row instead of reusing the already obtained value.

Additionally, a property typically returns the same value over invocations.

https://kotlinlang.org/docs/coding-conventions.html#functions-vs-properties

In this case we have implementations that iterate over an array, and return different values at different times. So a function is more appropriate.

Copy link
Member

Choose a reason for hiding this comment

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

While I partially agree with it for the generic case, I don't think that it fits here. Even in general, the property doesn't have a semantic guarantee of a stable, non-calculated value.

In this particular case you want to "expose" some implementation details, but the semantic of it is clearly a property

Even from the link you posted, we have 3/3 cases why it should be a property

  • does not throw
  • is cheap to calculate (non-calculated in one implementation and for other in most of the cases we have just 1-2 layers)
  • returns the same result over invocations if the object state hasn't changed (different value ONLY if state changed)

Copy link
Member Author

Choose a reason for hiding this comment

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

is cheap to calculate (non-calculated in one implementation and for other in most of the cases we have just 1-2 layers)

  1. That's an implementation detail.
  2. The number of layers isn't really known.
  • This is an interface, and this isn't an obvious property, so we should default to a function.
  • Using val can give the caller the wrong impression about this, possibly encouraging two mistakes:
  1. Assuming it's a cheap call, calling it several times unnecessarily.
  2. Assuming it can't change at any time, writing code like this:
    val dndManager = scene.dragAndDropManager
    // long running task here
    dndManager.doSomething()
    

Making it a function does not bring these mistaken connotations.

Some examples from our code:

  • PointerIconService.getIcon()
  • Selectable.getText()
  • ComposeScene.hasInvalidations().

Copy link
Member

Choose a reason for hiding this comment

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

That's an implementation detail

Exactly. This is why it should be a property

Assuming it can't change at any time

It's not the case. The case is "if the object state hasn't changed"

It's the predictable value of the current state, I don't see a reason to make this as a function

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly. This is why it should be a property

It's an implementation detail whether it's cheap. It could be non-cheap. Hence it should not be a val.

It's not the case. The case is "if the object state hasn't changed"

This is not an important distinction. Someone using ComposeScene via its interface isn't aware of all the possible state changes it could have. val brings with it connotations of a stable/unchanging property, which is wrong here and can encourage wrong assumptions.

Copy link
Member

Choose a reason for hiding this comment

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

This interface is hiding implementation details. Including the fact that it can have multiple owners/layers inside. Exposing this and forcing users to think about it seems wrong to me.

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 function is the most generic thing, that doesn't carry any extra connotations. A property does carry some.

I'm not exposing here the fact that there are multiple owners/layers inside. I'm exposing the fact that there could be (something; not specifically owners/layers), and the caller should not assume there aren't.

Copy link
Member

Choose a reason for hiding this comment

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

For me it still looks quite unnatural to have the function instead of a property here. Also, I believe that property fits all criteria from Kotlin conventions.
But all of it I already wrote before. Since we have no compatibility guarantees here, I won't block the change because of it. But I consider it as a subject of future adjustments.

/**
* Receives and processes events from the [ComposeDropTarget] installed in the root component.
*/
val dropTargetListener = object : DropTargetListener {
Copy link
Member

Choose a reason for hiding this comment

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

Might be private in case if ComposeDropTarget will receive it via lambda

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but then I'd need to create an unnecessary lambda, and hiding it here isn't important as it's all part of one file/implementation.

@igordmn igordmn removed their request for review August 13, 2024 10:27
@igordmn
Copy link
Collaborator

igordmn commented Aug 13, 2024

Removed myself from the reviewers, because the API is public @Internal, which doesn't require 2 reviewers (I have no strong opinion here; if someone thinks otherwise, let's review by 2 persons)

@m-sasha
Copy link
Member Author

m-sasha commented Aug 13, 2024

Removed myself from the reviewers, because the API is public @Internal, which doesn't require 2 reviewers (I have no strong opinion here; if someone thinks otherwise, let's review by 2 persons)

@igordmn I prefer you review this also. Not because of the API change, but because it involves two decisions which I'm not sure about:

  1. The hack where the firstAwtDragAndDropManager installs a TransferHandler and DropTarget, which are then reused by all following AwtDragAndDropManagers.
  2. The decision that DropTarget sends events to the topmost focusable layer, when CanvasLayersComposeScene is used.

@igordmn igordmn self-requested a review August 13, 2024 13:24
@m-sasha m-sasha requested review from MatkovIvan and igordmn August 16, 2024 08:49
Copy link
Member

@MatkovIvan MatkovIvan left a comment

Choose a reason for hiding this comment

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

Is it possible to add some auto test for the fixed case?

@m-sasha
Copy link
Member Author

m-sasha commented Aug 16, 2024

Is it possible to add some auto test for the fixed case?

It's hard to make a good test here because there doesn't seem to be a way to create AWT drag-and-drop events. I can check that showing a popup doesn't change the TransferHandler or DropTarget, but it's not such a good test because it's tied to an implementation detail...

edit: Maybe there is a way; I'll try

@m-sasha
Copy link
Member Author

m-sasha commented Aug 16, 2024

Ok, added tests.

@m-sasha m-sasha requested a review from MatkovIvan August 16, 2024 09:57
container.dropTarget = ComposeDropTarget(
rootContainer = container,
dropTargetListener = {
(scene.activeDragAndDropManager() as? AwtDragAndDropManager)?.dropTargetListener
Copy link
Collaborator

@igordmn igordmn Aug 16, 2024

Choose a reason for hiding this comment

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

The decision that DropTarget sends events to the topmost focusable layer

The feature itself seems reasonable, as we stop keyboard/pointer events as well.

Though, there can be issues with the implementation via activeDragAndDropManager:

  1. When the active activeDragAndDropManager changes, we can't longer send dragExit to the previous manager. But we might need it (maybe not now for simplicity), and it should be responsibility of CanvasLayersComposeScene, as we do with pointer/keyboard events.

  2. Casting to AwtDragAndDropManager may indicate that our data flow is not direct (so, more difficult to understand), or that external users can't implement something.

  3. It seems that other targets has this issue as well? I.e. they create one drag listener per native view, but native view contain multiple owners/dragAndDropManagers.

All that indicate that maybe we need to move logic of choosing the right owner to ComposeScene:
1.patch

I didn't check it, but it shows the idea, and the tests pass. Also, we need to change the other targets.

Also I moved creation of transferHandler/dropTarget back to the manager, as we no longer need to cast and set the container properties there (these were the main issues)

Copy link
Member Author

Choose a reason for hiding this comment

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

I polished and pushed the patch, but I really think we should discuss the necessity of decoupling our desktop code from the AWT implementation.

It significantly complicates our code and data flow and sometimes even negatively affects the public API we present. The benefits are mostly illusory in my opinion (but maybe I'm wrong). Anyone needing to provide an alternative implementation will be better served by a simpler codebase, even if they need to recompile the project.

Copy link
Collaborator

@igordmn igordmn Aug 27, 2024

Choose a reason for hiding this comment

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

necessity of decoupling our desktop code from the AWT implementation.

There are 2 cases of desktopMain:

  1. Using AWT in foundation and higher level modules.

In this case we are trying to eliminate desktopMain sourceSet at all, an move all code to skikoMain (or better to commonMain). It is beneficial for usual users of Compose Multiplatform when they write a cross-paltform application.

Let's remember a real situation with AlertDialog that had own implementations for each platform. AlertDialog is usually very opinionated thing, and if we need to customize it, we have to copy paste code. But because it had completely different implementations on all platforms, we had to copy-paste it on each platform.

Now we commonized it, extracting a public API Dialog in ui. We solved 2 issues - users only need to copy paste commonMain, and they have a public platform abstraction for Dialog, they can build on top of it very customised dialogs. Just extracting this abstraction into a public expect won't be enough, as it will be in material only, we need it in ui (or a hypothetical future ui-platform), so other design system can access it.

androidMain currently don't follow this principle as it uses Android SDK directly, and has this copy-paste issue. To follow this principle, we need to move all the code to commonMain and design a lower-level platform facade in ui. This is the price of having platform abstraction over a lower level.

  1. Using AWT in ui module.

We have 2 options here:

  • create an expect and use AWT directly
  • create an interface and inject it into ComposeScene

In my opinion these are just 2 different ways to provide an implementation, and the second one is not more difficult than the first one.

But the second one has advantages:

  • allowing constructing a custom platform bindings. I remember real user cases where it was useful:

    • Compose rendering on a server
    • Virtual testing
    • Replacing AWT by JWM, which behave better in window management things.
    • Minecraft integration 🙃
  • it separates Compose and a platform, which are on different layers of abstraction. Mixing different layers usually leads to over-complicated code if people aren't careful. Having separation by design helps to avoid this, and helps to think about new features in a certain way (is it a Compose feature, or is it a platform feature?)

See also the recent presentation, it partially covers why we doing it this way.

@m-sasha m-sasha requested a review from igordmn August 18, 2024 10:54
@m-sasha m-sasha force-pushed the m-sasha/fix-multi-scene-dnd branch 2 times, most recently from ff50436 to 12f07d8 Compare August 19, 2024 09:56
…`OwnerDragAndDropManager` instead of the platform-specific drag-and-drop manager.
@m-sasha m-sasha force-pushed the m-sasha/fix-multi-scene-dnd branch from 12f07d8 to b01a9ef Compare August 19, 2024 10:00
Copy link
Member

@MatkovIvan MatkovIvan left a comment

Choose a reason for hiding this comment

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

Let's merge it as-is to unblock beta, but ComposeScene/PlatformContext must be reworked (I'll do it separately)


private val interestedNodes = ArraySet<DragAndDropModifierNode>()
private val rootContainer: JComponent,
private val getScene: () -> ComposeScene
Copy link
Member

Choose a reason for hiding this comment

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

It should be dropTarget, not scene

transferData: DragAndDropTransferData,
decorationSize: Size,
drawDragDecoration: DrawScope.() -> Unit
): Boolean = false
Copy link
Member

Choose a reason for hiding this comment

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

This is not compatible with our common design and other platforms. Subject to change.

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.

I added (prerelease fix) to the Release Notes

@MatkovIvan MatkovIvan merged commit 70d6caa into jb-main Aug 27, 2024
6 checks passed
@MatkovIvan MatkovIvan deleted the m-sasha/fix-multi-scene-dnd branch August 27, 2024 11:39
igordmn pushed a commit that referenced this pull request Aug 30, 2024
- Fix followups after #1493
- Sync internals with https://r.android.com/3197432 (unlock merging iOS
implementation)
- Restore `PlatformDragAndDropManager` and add related APIs
- Use `hasEligibleDropTarget` for dynamic drop target indication


https://github.com/user-attachments/assets/8b7a65af-97d2-4033-8d88-33445256f8cb

## Testing
Test sample from #1493
This should be tested by QA

## Release Notes
### Features - Desktop
- Add dynamic Drag&Drop target indication (🚫 icon under cursor if
currently there is no valid drop target under it)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants