On Thu, 11 May 2023 15:48:39 GMT, Martin Fox <d...@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. > > modules/javafx.graphics/src/main/native-glass/mac/GlassDraggingSource.m line > 44: > >> 42: return self->dragOperation; >> 43: } >> 44: > > `sourceOperationMaskForDraggingContext` replaces the deprecated > `draggingSourceOperationMaskForLocal:` over in GlassViewDelegate.m. There's > some logic there that needs to be copied over here. Basically the Apple > documentation on how the Cmd key filters the set of available operations > doesn't match reality and we have to compensate for that. This sounds like it could be the reason for the functional regression in handling the CMD modifier while dragging. > modules/javafx.graphics/src/main/native-glass/mac/GlassViewDelegate.m line > 988: > >> 986: } >> 987: >> 988: if (image == nil && [pbItemTypes >> containsObject:NSPasteboardTypeFileURL]) > > `NSPasteboardTypeFileURL` was introduced in MacOS 10.13 and JavaFX is > currently targeted at 10.12. The compiler caught this but the warning was > lost in the sea of other warnings the Mac build generates. Good catch (which reminds me that I need to get back to your warnings PR). This might misbehave on macOS 10.12 then, but I haven't seen a machine that old in a few years, so it would be difficult to test. We really should bump the minimum at some point (to at _least_ 10.14, but ideally to 11 to match what Apple currently supports, and to match what the minimum has always been for ARM-based Mac systems), so this seems like the right time to broach that subject. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1089#discussion_r1191426568 PR Review Comment: https://git.openjdk.org/jfx/pull/1089#discussion_r1191431942
