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

Drag and Drop made available for a label that has an Image view. #713

Merged
merged 1 commit into from
Apr 5, 2024
Merged

Drag and Drop made available for a label that has an Image view. #713

merged 1 commit into from
Apr 5, 2024

Conversation

elsazac
Copy link
Member

@elsazac elsazac commented Jun 27, 2023 •
edited
Loading

Bringing the required changes to DropTarget.java

In the method static long dropTargetProc(long id, long sel, long arg0), the label widget that doesn't actaully have a drop target key was being checked to see if it has a drop target key or not instead of the widget that actually has the drop Target key. So the code eventually failed to recognise label as an area where drop is possible. As soon as we get a label as a widget, the modified code checks for the label's parent to see if drop target key is present or not. If present the area is recognised for drop.

Also a new function called private void handleLabels(Control c) is also added to bring the feature of dropping on to the top of a label that has an image view. This functions takes a control as a parameter to go up the hierarchy till its parent to find the label which has an image view and add drag handlers for that.

Fixes: #649

cc/ @lshanmug

@elsazac elsazac force-pushed the DragAndDropChanges branch 3 times, most recently from 7aa565f to dcbd786 Compare July 3, 2023 18:00
@elsazac elsazac marked this pull request as ready for review December 14, 2023 04:32
@BeckerWdf
Copy link
Contributor

what's the status of this?

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 20, 2023 •
edited
Loading

Test Results

   299 files  ±0     299 suites  ±0   7m 32s ⏱️ + 1m 24s
 4 101 tests ±0   4 093 ✅ ±0   8 💤 ±0  0 ❌ ±0 
12 215 runs  ±0  12 140 ✅ ±0  75 💤 ±0  0 ❌ ±0 

Results for commit 988fac9. ± Comparison against base commit 648549b.

♻️ This comment has been updated with latest results.

merks
merks requested changes Jan 10, 2024
View reviewed changes
Copy link
Contributor

@merks merks left a comment

Choose a reason for hiding this comment

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

I have two concerns that I think need attention.

bundles/org.eclipse.swt/Eclipse SWT Drag and Drop/cocoa/org/eclipse/swt/dnd/DropTarget.java Outdated Show resolved Hide resolved
bundles/org.eclipse.swt/Eclipse SWT Drag and Drop/cocoa/org/eclipse/swt/dnd/DropTarget.java Outdated Show resolved Hide resolved
@merks
Copy link
Contributor

merks commented Jan 11, 2024

FYI, I think most people prefer that you amend commits so that there is a single commit in the PR. I know that multiple commits tend to confuse me personally.

@elsazac
Copy link
Member Author

elsazac commented Jan 17, 2024

I have tested :

  • Snippets related to drag and drop
  • dnd Examples where dropping is performed on top of label
  • dropping on Editor window's image.

@BeckerWdf
Copy link
Contributor

@merks: Are your concerns addressed?

merks
merks approved these changes Feb 1, 2024
View reviewed changes
Copy link
Contributor

@merks merks left a comment

Choose a reason for hiding this comment

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

I can't say I understand all the details, but it okay.

@elsazac elsazac force-pushed the DragAndDropChanges branch 2 times, most recently from 82a7570 to 57162a1 Compare February 12, 2024 06:08
@elsazac elsazac requested review from lshanmug and merks and removed request for merks February 22, 2024 10:27
@elsazac
Copy link
Member Author

elsazac commented Mar 6, 2024

@lshanmug Is anything else required on this PR ? If yes, please let me know.

@elsazac
Copy link
Member Author

elsazac commented Mar 14, 2024

@lshanmug Were you able to check on this?

@lshanmug
Copy link
Member

@lshanmug Were you able to check on this?

@elsazac Sorry missed this, will check it today.

lshanmug
lshanmug reviewed Mar 22, 2024
View reviewed changes
Copy link
Member

@lshanmug lshanmug left a comment

Choose a reason for hiding this comment

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

@elsazac Added comments as discussed. Please fix formatting as well.

bundles/org.eclipse.swt/Eclipse SWT Drag and Drop/cocoa/org/eclipse/swt/dnd/DropTarget.java Outdated Show resolved Hide resolved
bundles/org.eclipse.swt/Eclipse SWT Drag and Drop/cocoa/org/eclipse/swt/dnd/DropTarget.java Outdated Show resolved Hide resolved
bundles/org.eclipse.swt/Eclipse SWT Drag and Drop/cocoa/org/eclipse/swt/dnd/DropTarget.java Outdated Show resolved Hide resolved
@elsazac
Copy link
Member Author

elsazac commented Mar 25, 2024

@elsazac Added comments as discussed. Please fix formatting as well.

@lshanmug I have addressed the changes. Please have a look.

lshanmug
lshanmug reviewed Apr 2, 2024
View reviewed changes
Copy link
Member

@lshanmug lshanmug left a comment

Choose a reason for hiding this comment

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

Looks good. @elsazac Please rebase.

bundles/org.eclipse.swt/Eclipse SWT Drag and Drop/cocoa/org/eclipse/swt/dnd/DropTarget.java Outdated Show resolved Hide resolved
@elsazac elsazac force-pushed the DragAndDropChanges branch 2 times, most recently from a4a8e90 to 25faa47 Compare April 3, 2024 06:33
@elsazac
Copy link
Member Author

elsazac commented Apr 3, 2024

Looks good. @elsazac Please rebase.

@lshanmug I have made the changes and rebased too. Please have a look.

@lshanmug
Copy link
Member

lshanmug commented Apr 5, 2024

@elsazac Let's merge once M1 is out.

@akurtakov
Copy link
Member

M1 has been pushed to simrel already eclipse-simrel/simrel.build@afe289a

@elsazac
Bringing the required changes to DropTarget.java
11830be 
In the method dropTargetProc(long id, long sel, long arg0) the widget
was being recognised as label instead of the widget that actually has
the drop Target key. As soon as the first drop target key is acquired
the area is recognised as a Drop Target area and the Drag handlers are
added.

Also a new function called handleLabels( Control c, long cls) is
also added to bring the feature of droping on to the top of a label that
has an image view.This functions takes a control as a parameter to go up
till its parent to find the label which has an image view and add drag
handlers for that.
Fixes: eclipse-platform#649
@lshanmug lshanmug merged commit 5b437ee into eclipse-platform:master Apr 5, 2024
1 check passed
@lshanmug
Copy link
Member

lshanmug commented Apr 5, 2024

Thanks @elsazac for the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iloveeclipse iloveeclipse iloveeclipse left review comments

@lshanmug lshanmug lshanmug approved these changes

@merks merks merks approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Drag and Drop Not Working When Label Contains Image
6 participants
@elsazac @BeckerWdf @merks @lshanmug @akurtakov @iloveeclipse