On Wed, 17 May 2023 08:50:23 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 incrementally with two additional > commits since the last revision: > > - GlassViewDelegate: Address review issues > > * Removed draggingSource pointer > * Removed some leftover comments > * Set delegate to nil > - Revert "GlassViewDelegate: Return to deprecated *PboardType symbols" > > This reverts commit fa42b1b13755dec05d77b3865ed657e954854286. > > fa42b1b13 is not needed anymore - JFX will target macOS 11 soon. All my testing looks good. I left a couple comments about possible memory leaks. modules/javafx.graphics/src/main/native-glass/mac/GlassPasteboard.m line 788: > 786: for (NSPasteboardItem* i in objects) > 787: { > 788: [dItems addObject:[[NSDraggingItem alloc] > initWithPasteboardWriter:i]]; I think you need to call `autorelease` on the allocated `NSDraggingItem` (unless I missed seeing a release later on). modules/javafx.graphics/src/main/native-glass/mac/GlassViewDelegate.m line 976: > 974: { > 975: // create an image with contents of URL > 976: image = [[NSImage alloc] initByReferencingFile:[[NSString > alloc] initWithData:[pbItem dataForType:NSPasteboardTypeFileURL] > encoding:NSUTF8StringEncoding]]; I think you need to call `autorelease` on the allocated `NSString`. modules/javafx.graphics/src/main/native-glass/mac/GlassViewDelegate.m line 989: > 987: { > 988: // create an image with contents of URL > 989: image = [[NSImage alloc] initByReferencingURL:[NSURL > URLWithString:[[NSString alloc] initWithData:[pbItem > dataForType:NSPasteboardTypeURL] encoding:NSUTF8StringEncoding]]]; I think you need to call `autorelease` on the allocated `NSString`. ------------- PR Review: https://git.openjdk.org/jfx/pull/1089#pullrequestreview-1431803867 PR Review Comment: https://git.openjdk.org/jfx/pull/1089#discussion_r1197174973 PR Review Comment: https://git.openjdk.org/jfx/pull/1089#discussion_r1197175780 PR Review Comment: https://git.openjdk.org/jfx/pull/1089#discussion_r1197176435
