Skip to content

8333919: [macOS] dragViewOffsetX/dragViewOffsetY are ignored for the dragView image #1532

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

Closed
wants to merge 4 commits into from
Closed

8333919: [macOS] dragViewOffsetX/dragViewOffsetY are ignored for the dragView image #1532

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
92c190e
GlassViewDelegate: Restore offset calculation on startDrag
lukostyra Aug 8, 2024
c922de8
GlassViewDelegate: Correctly set origin and offset in dragImage
lukostyra Aug 9, 2024
41cd675
Adjust clamping values, remove old TODO
lukostyra Aug 12, 2024
f6d065c
Merge two if image != nil statements together
lukostyra Aug 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 14 additions & 17 deletions modules/javafx.graphics/src/main/native-glass/mac/GlassViewDelegate.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1021,7 +1021,7 @@ - (void)startDrag:(NSDragOperation)operation withItems:(NSArray<NSDraggingItem*>

if (image != nil)
{
DNDLOG("created an image with dim %dx%d", [image size].width, [image size].height);
DNDLOG("created an image with dim %fx%f", [image size].width, [image size].height);
// check the drag image size and scale it down as needed using Safari behavior (sizes) as reference
rect.size.width = [image size].width;
rect.size.height = [image size].height;
Expand All @@ -1042,30 +1042,27 @@ - (void)startDrag:(NSDragOperation)operation withItems:(NSArray<NSDraggingItem*>
[image setSize:NSMakeSize(rect.size.width, rect.size.height)];
}
}
}

if (image != nil)
{
// select the center of the image as the drag origin
Copy link
Collaborator

Choose a reason for hiding this comment

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

The old comment about RT-17629 (https://bugs.openjdk.org/browse/JDK-8091965) does not apply anymore: after this PR, the offset, which comes from the Java layer, does adjust the drag image origin.

So I believe we can remove the whole comment as well (and possibly close RT-17629).

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are two consecutive if (image != nil) blocks, so you can remove the 4 lines above this one.

// TODO http://javafx-jira.kenai.com/browse/RT-17629
// would be nice to get this info from the Java layer,
// so that we could adjust the drag image origin based on where in the src it was clicked on
dragPoint.x -= ([image size].width/2.0f);
dragPoint.y -= ([image size].height/2.0f);

NSString *offsetString = [pbItem stringForType:DRAG_IMAGE_OFFSET];
if (offsetString != nil) {
NSPoint offset = NSPointFromString(offsetString);
//Adjust offset to the image size
float imageHalfX = [image size].width/2.0f;
float imageHalfY = [image size].height/2.0f;
float imageW = [image size].width;
float imageH = [image size].height;

if (offset.x > imageHalfX || offset.x < -imageHalfX) {
offset.x = imageHalfX * (offset.x > 0 ? 1 : -1);
if (offset.x < 0.0f) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am puzzled a bit: a NaN input seem to produce NaN offsets, yet the testing shows that it behaves as if 0 offsets (possibly in the native code)?

Should the ifs be restructured so as not to pass NaN downstream?

Copy link
Member

Choose a reason for hiding this comment

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

That seems out of scope for this PR. We typically don't do any special processing to deal with NaN unless there is a problem. If needed, maybe it could be done as part of the follow-up bug JDK-8338204.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, especially since it seems to treat NaNs as 0's.

offset.x = 0.0f;
} else if (offset.x > imageW) {
offset.x = imageW;
}
if (offset.y > imageHalfY || offset.y < -imageHalfY) {
offset.y = imageHalfY * (offset.y > 0 ? 1 : -1);
if (offset.y < 0.0f) {
offset.y = 0.0f;
} else if (offset.y > imageH) {
offset.y = imageH;
}

dragPoint.x -= offset.x;
dragPoint.y -= offset.y;
Comment on lines +1063 to +1065
Copy link
Collaborator

Choose a reason for hiding this comment

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

This works fine now, if the offset values are smaller than the imageHalf X,Y values, which doesn't make sense anymore:

  • If you drag from {0,0} (top-left), it works from that position
  • But if you drag from {imageW, imageH} (bottom-right), the offset is clamped to the image centre, which it is not expected.

So now we need to change the above lines to clamp the offset between 0,0 and imageW,H (though Windows doesn't clamp the offset at all).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Out of curiosity I removed offset clamping (macOS snaps the drag image regardless) and technically it still works, but when you cancel the drag the image snaps back to the offset position. With high enough (or negative) values drag image can animate back even outside the application bounds, which makes little sense and looks weird. I'll clamp it between 0,0 and imageW,H as you mentioned.

I also didn't find a way to disable above drag image "snapping". However, if it makes sense to clamp it on macOS, maybe it would also make sense to introduce similar limits to offsets on other platforms (probably as a separate issue)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, there is a built-in native animation from the offset point to the mouse cursor (in and out).

When the drag event is cancelled, the image returns to the offset location with this animation. We still want that, so I wouldn't remove the animation (if we could), but of course, with the original position of the source, no far away from it, so it makes sense to clamp between the image bounds.

And doing the same (clamping) for Windows would be a separate issue, indeed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

By the way, this is how you can disable the cancel animation:

diff --git a/modules/javafx.graphics/src/main/native-glass/mac/GlassDraggingSource.m b/modules/javafx.graphics/src/main/native-glass/mac/GlassDraggingSource.m
index 50a79cabf6..373537f24e 100644
--- a/modules/javafx.graphics/src/main/native-glass/mac/GlassDraggingSource.m
+++ b/modules/javafx.graphics/src/main/native-glass/mac/GlassDraggingSource.m
@@ -40,6 +40,7 @@
 
 - (NSDragOperation)draggingSession:(NSDraggingSession *)session sourceOperationMaskForDraggingContext:(NSDraggingContext)context
 {
+     session.animatesToStartingPositionsOnCancelOrFail = NO;
     // The Command key masks out every operation other than NSDragOperationGeneric. We want
     // internal Move events to get through this filtering so we copy the Move bit into the
     // Generic bit and treat Generic as a synonym for Move.

I'm still in favour of this native animation, as opposed to the Windows case that doesn't have it. This can be a separated issue as well (unifying the animation in all platforms).

Copy link
Contributor

Choose a reason for hiding this comment

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

I wanted to chime in, thought it might be unrelated:

In the context of a docking framework, it is impossible to disable the cancel animation. We should have a way to disable it. The use case is when the user drags and drops a tab/pane outside of the dockable windows. In this case a new window should appear at the drop location, without the drag image animated back to the start position.

Another thing that is impossible is to control the drag image: sometimes one needs a scaled icon, sometimes one wants the full content.

These two issues basically prevent implementing a docking framework via DnD, one needs to use other solution, for example
https://github.com/andy-goryachev/FxDock

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if I follow, but about the cancel animation, do you mean that is not possible to disable it on macOS? Have you tested session.animatesToStartingPositionsOnCancelOrFail?
As for the image, DragBoard::setDragView allows you setting any image that you need for the drag gesture, or am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

it's been a while - java8 time frame - back then I was unable to find a way to disable animation on cancel.

as for image - I was setting large image and fx tried to scale it to be smaller, I was unable to force it to show the unscaled full size image.

also, why does the drag image look fuzzy on mac retina?

}
}
else
Expand Down