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

External drag and drop support for compose application #391

Merged
merged 31 commits into from
Mar 13, 2023

Conversation

Walingar
Copy link
Collaborator

Proposed Changes

Introduce drag and drop support from external resources to compose application.
For now, for desktop only.

Testing

It can be tested by introduced androidx.compose.ui.dnd.ExternalDragTest or manually by androidx.compose.desktop.examples.dnd.Main_jvmKt#main

Issues Fixed

Fixes: JetBrains/compose-multiplatform#222

@igordmn igordmn self-requested a review February 1, 2023 08:09
@Walingar Walingar force-pushed the nr/dnd-support-222 branch from 048229e to b57d9bd Compare March 2, 2023 10:23
@Walingar Walingar force-pushed the nr/dnd-support-222 branch from b57d9bd to 6614da1 Compare March 2, 2023 10:25
@Walingar
Copy link
Collaborator Author

Walingar commented Mar 2, 2023

Made few force-pushes with rebasing on up-to-date main branch and removed links to the issues in commit messages, since it spams there :(

@dima-avdeev-jb dima-avdeev-jb requested a review from m-sasha March 7, 2023 14:59
Copy link
Member

@m-sasha m-sasha left a comment

Choose a reason for hiding this comment

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

Mostly nitpicks. A great PR overall.

* @param onDragStart will be called when the pointer with external content entered the component.
* @param onDrag will be called for all drag events inside the component.
* @param onDrop is called when the pointer is released with [DropData] the pointer held.
* @param onDragCancel is called if the pointer exited the component bounds or unknown data was dropped.
Copy link
Member

Choose a reason for hiding this comment

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

Style guide says we're supposed to fit into 100 columns.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm, weird that IDEA doesn't move it to the new line. Where do you see this into styleguide?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure; @igordmn told me.

Copy link
Collaborator

@igordmn igordmn Mar 9, 2023

Choose a reason for hiding this comment

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

This was worked perfectly in Android Studio in AOSP, but we have bugs in our fork, and this limit doesn't always apply.

There is an idea to increase it for our fork. It will complicate upstreaming into AOSP, but that is another discussion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should I manually controll this length and change it here or it is ok?

Copy link
Collaborator

@igordmn igordmn Mar 10, 2023

Choose a reason for hiding this comment

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

Check your settings in IDEA, if you will find that you don't use Project, switch to it, and then comply to it:
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I use the project one, weird

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.

Thanks!

I reviewed only API and behavior. Let's m-sasha do a full review.

@Walingar
Copy link
Collaborator Author

Walingar commented Mar 8, 2023

@m-sasha thank you for your review! I've fixed most of your comments, most importantly, I've rewritten the code that handles component bounds changes, please take a look at it.
Igor send review about API, I'll handle it a bit later, it won't affect implementation a lot (only API + AwtWindowDragTargetListener)

@Walingar Walingar requested a review from m-sasha March 8, 2023 15:33
@Walingar
Copy link
Collaborator Author

Walingar commented Mar 9, 2023

@m-sasha thank you for your comments, fixed most of them (except code style, since don't get what to do better)
Also, I continue investigation of better DropData API regarding Igor's comments, looking at other UI frameworks as well.

@Walingar Walingar requested a review from m-sasha March 9, 2023 10:29
* don't use sealed interface and data classes
* provide mimeTypes for all drop data
* provide methods to read drop data content instead of having properties
@Walingar
Copy link
Collaborator Author

@igordmn I've updated the pull request with changes in API, please take a look at it. I'd keep it experimental, since there are a few things that can be improved, but we can decide how to do that only according to the feedback.

@Walingar Walingar requested a review from igordmn March 10, 2023 14:36
@Walingar Walingar requested a review from igordmn March 13, 2023 10:08
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.

Thanks! API looks good now.

Left one small comment, and we good to merge

…nalDrag.desktop.kt

Co-authored-by: Igor Demin <igordmn@users.noreply.github.com>
@Walingar Walingar merged commit 5869a7f into JetBrains:jb-main Mar 13, 2023
@Walingar Walingar deleted the nr/dnd-support-222 branch March 13, 2023 16:16
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.

Will support drag and drop file into widget?
4 participants