-
Notifications
You must be signed in to change notification settings - Fork 80
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
Conversation
…attached to the same root (AWT) container.
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/PlatformContext.skiko.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/node/RootNodeOwner.skiko.kt
Outdated
Show resolved
Hide resolved
…her than pass it to RootNodeOwner.
...e/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/platform/AwtDragAndDropManager.desktop.kt
Outdated
Show resolved
Hide resolved
/** | ||
* Returns the currently active [PlatformDragAndDropManager]. | ||
*/ | ||
fun activeDragAndDropManager(): PlatformDragAndDropManager |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fun activeDragAndDropManager(): PlatformDragAndDropManager | |
val dragAndDropManager: PlatformDragAndDropManager |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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)
- That's an implementation detail.
- 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:
- Assuming it's a cheap call, calling it several times unnecessarily.
- 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()
.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/scene/CanvasLayersComposeScene.skiko.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/node/RootNodeOwner.skiko.kt
Outdated
Show resolved
Hide resolved
/** | ||
* Receives and processes events from the [ComposeDropTarget] installed in the root component. | ||
*/ | ||
val dropTargetListener = object : DropTargetListener { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
…seSceneImpl.activeDragAndDropManager()`
Removed myself from the reviewers, because the API is |
@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:
|
...e/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/platform/AwtDragAndDropManager.desktop.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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?
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 edit: Maybe there is a way; I'll try |
Ok, added tests. |
container.dropTarget = ComposeDropTarget( | ||
rootContainer = container, | ||
dropTargetListener = { | ||
(scene.activeDragAndDropManager() as? AwtDragAndDropManager)?.dropTargetListener |
There was a problem hiding this comment.
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
:
-
When the active
activeDragAndDropManager
changes, we can't longer senddragExit
to the previous manager. But we might need it (maybe not now for simplicity), and it should be responsibility ofCanvasLayersComposeScene
, as we do with pointer/keyboard events. -
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. -
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
:
- 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.
- Using AWT in
ui
module.
We have 2 options here:
- create an
expect
and use AWT directly - create an
interface
and inject it intoComposeScene
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.
...e/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/platform/AwtDragAndDropManager.desktop.kt
Outdated
Show resolved
Hide resolved
ff50436
to
12f07d8
Compare
…`OwnerDragAndDropManager` instead of the platform-specific drag-and-drop manager.
12f07d8
to
b01a9ef
Compare
There was a problem hiding this 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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this 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
- 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)
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 AWTTransferHandler
andDropTarget
into the root container.In this PR the first attached
AwtDragAndDropManager
installs aTransferHandler
and aDropTarget
into the root container, and the followingAwtDragAndDropManager
s reuse them. TheDropTarget
forwards its events to theAwtDragAndDropManager
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
This should be tested by QA
Release Notes
Fixes - Desktop