On Fri, 12 May 2023 15:05:01 GMT, Lukasz Kostyra <lkost...@openjdk.org> wrote:
>> Crashes started happening due to macOS DnD API change from macOS 10.14 >> onwards. 10.14 incrodues some [DnD >> constrains](https://developer.apple.com/documentation/macos-release-notes/appkit-release-notes-for-macos-10_14#Drag-and-Drop) >> which our DnD code happened to trigger on some occasions. >> >> Our code used deprecated `dragImage` API which since 10.14 had new >> requirement - each NSPasteboard item had to have a corresponding drag image. >> Not meeting this constraint raised an exception, which crashed the app. >> Since there was no possibility to add more drag images to `dragImage` API >> (it only took one NSImage as parameter) the code had to be rewritten - as >> such I upgraded it to new DnD API utilizing NSDraggingSession and related >> protocols. >> >> One side-effect of new DnD API is that it now modifies NSPasteboard for us - >> previous behavior was more "separated" from user code perspective (write >> items to pasteboard -> initiate a drag via `dragImage`). Manually updating >> NSPasteboard before calling `beginDraggingSessionWithItems` raised another >> exception related to NSPasteboard already having DnD-ed elements inside it. >> Some system tests, however, relied on that behavior and writing to >> NSPasteboard (`MacPasteboardShim.java` used in some tests creates a >> `Clipboard.DND` for test purposes). Since this path is in tests I assumed >> this behavior should stay and tried to make it as close to working as >> possible. Tests (including those using `MacPasteboardShim`) pass after my >> changes. >> >> Additionally, added a new manual test based on `DndTest.java` test which >> creates two temporary files and allows for testing faulty behavior. > > Lukasz Kostyra has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 10 additional > commits since the last revision: > > - GlassViewDelegate: Return to deprecated *PboardType symbols > > New NSPasteboardType* symbols were introduced starting 10.13, and we're > targetting 10.12. This deprecation should probably be resolved once we > target > macOS 10.13+. > > Old NS*PboardType symbols do not have an equivalent of > NSPasteboardTypeFileURL, > so this branch was removed. > - GlassPasteboard: Correct macOS version information in comments > > New DnD API was introduced starting macOS 10.7, not 10.14 > - GlassView: Remove deprecated draggingSourceOperationMask > - DndTest: Update instructions > > On Mac to enable moving you must press Cmd, not Shift. > - Restore logic responsible for Cmd key support > - Merge branch 'master' into dnd_multiple_items-JDK-8233955 > - Add DnD Multiple File manual test > - Fix preview position; cleanup code > - Add image previews and finish DnD code > > Fixes issues with tests caused by first commit. > > Removes old code used as placeholder. > - Migrate DnD native code to DraggingSession API > > Previous implementation used dragImage call which is deprecated since > macOS 10.14. Additionally, > 10.14 introduced a restriction not allowing for more than one drag item in > the Pasteboard. This > change fixes crashes caused by old API use when DnD-ing more than one item. modules/javafx.graphics/src/main/native-glass/mac/GlassViewDelegate.h line 71: > 69: > 70: NSDragOperation dragOperation; > 71: NSDraggingSession *draggingSession; I don't think the dragOperation is used anymore. You also don't need draggingSession since you're not assigning anything to it. I think you can just remove it, the NSView should retain the draggingSession while the drag is going on and release it afterward. modules/javafx.graphics/src/main/native-glass/mac/GlassViewDelegate.m line 962: > 960: for (NSDraggingItem* item in items) > 961: { > 962: NSPoint dragPoint = [self->nsView convertPoint:[self->lastEvent > locationInWindow] fromView:nil];//[self->lastEvent locationInWindow]; Minor: left over comment at end of line modules/javafx.graphics/src/main/native-glass/mac/GlassViewDelegate.m line 1061: > 1059: // main thread blocked here until drag completes > 1060: > 1061: [GlassDragSource setDelegate:nil]; I don't see any place where the delegate is set back to `nil` in this PR so `isDelegateSet` will always return `true` after a mouse drag. And it could turn into a dangling pointer if the GlassViewDelegate goes away. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1089#discussion_r1194126454 PR Review Comment: https://git.openjdk.org/jfx/pull/1089#discussion_r1194079640 PR Review Comment: https://git.openjdk.org/jfx/pull/1089#discussion_r1194152034
