GNOME Bugzilla – Bug 786509
GDK W32: OLE2 DnD is completely broken
Last modified: 2017-11-30 04:19:45 UTC
When running with GDK_WIN32_USE_EXPERIMENTAL_OLE2_DND=1 the drag and drop functionality simply doesn't work. Looks like the code has been bitrotting for years.
Created attachment 357971 [details] [review] GDK W32: Fix a typo in OLE2 DnD code
Created attachment 357972 [details] [review] Only register application/x-rootwindow-drop on X11 application/x-rootwindow-drop is not useful anywhere else, so put it under #ifdef GDK_WINDOWING_X11 On W32 this prevents toplevels from automatically becoming valid drop targets with a useless drop type.
Created attachment 357973 [details] [review] W32: Massive OLE2-based DnD fix * Keep GdkDragContext and OLE2 objects separate (don't ref/unref them together, don't necessarily create them together). * Keep IDataObject formats in the object itself, not in a global variable. * Create target GdkDragContext on each drag_enter, destroy it on drag_leave, whereas IDropTarget is created when a window becomes a drag destination and is re-used indefinitely. * Query the source IDataObject for its supported types, cache them in the target (!) context. This is how GTK+ works, honestly. * Make sure GDK_DRAG_MOTION is only sent when something changes * Support GTK drag cursors * Work around cursor/focus weirdness by forcibly focusing on the drop target window and making it the window-under-pointer (which will change the mouse cursor as a side-effect). * Extensive format conversion checks for OLE2 DnD. * Put conversion code into separate functions * Ensure that exotic GTK clipboard formats are registered (but try to avoid registering formats that can't be used between applications). * Ensure that DnD indicator window can't accept drags or receive any kind of input (use WS_EX_TRANSPARENT). I know that it's a lot, but since OLE2 DnD is broken anyway, untangling this and separating it into multiple smaller patches is pointless - DnD only works when all these changes are introduced. This code worked reasonably well for me during the testing (didn't crash, DnD worked). Some highlights: you can run WordPad and drag-and-drop text between gtk3-demo and WordPad. You can also run the Clipboard demo and drag an image from that demo into WordPad - it'll work. Some cursor oscillation happens occasionally. Most likely due to the fact that W32 OLE2 DnD doesn't use grabs (X11 DnD uses grabs to change the cursor), so there must be some fighting between DnD and other parts of GTK/GDK. Cursor is controlled by the *source*, so dragging stuff from outside into GTK+ applications will still use cursors that the source application chooses (usually - Windows defaults). Format conversion is now *all over the place*. Ideally, it should be more concentrated. Also, change_property (which, by the way, looks completely nuts) shouldn't just assume that it has to spew CF_DIB and CF_UNICODETEXT - when DnD'ing between GTK+ applications we should have the ability to pass types as-is. I would also like to point out that GTK drops the drag_dest flags too early - these flags could have been used to decide which drag types should or should not be registered (any drag types that have within-same-widget-only or within-same-app-only flags set don't need to be registered, thus saving the limited clipboard type values (0xC000-0xFFFF)).
Review of attachment 357971 [details] [review]: Looks good
Review of attachment 357972 [details] [review]: Patch looks good to me but get an ack from Matthias
Review of attachment 357973 [details] [review]: Just minor nitpicks... I did not make a deep review since anyway the code was broken and it is only enabled by an env var. If you are confident go ahead and push it. A follow up patch btw would be to rename This for self ::: gdk/win32/gdkdnd-win32.c @@ +458,3 @@ + switch (fmt) + { + CASE (CF_TEXT); all these CASE should be with 2 space indentation less @@ +494,3 @@ + IEnumFORMATETC *pfmt = NULL; + FORMATETC fmt; + wchar_t szFmtName[1024]; can we avoid camel case? ::: gdk/win32/gdkselection-win32.c @@ +499,3 @@ + +static int +str_to_wchar (const char *str, do we really need this having g_utf8_to_utf16?
(In reply to Ignacio Casal Quinteiro (nacho) from comment #6) > Review of attachment 357973 [details] [review] [review]: > ::: gdk/win32/gdkselection-win32.c > @@ +499,3 @@ > + > +static int > +str_to_wchar (const char *str, > > do we really need this having g_utf8_to_utf16? This is not UTF8->UTF16, this is CP_ACP->UTF16. I'm not sure how to make gconv do this kind of conversion (most likely i'd have to figure out what CP_ACP currently means, translate it to an encoding that gconv understand, then use gconv to convert the string - too much work when there's an easy to use W32 API available for this). Unrelated: this version of the patch has a few bugs around GdkAtom <-> cfFormat (this is driving me crazy, by the way). Also, Shell ID List support is missing, so OLE2 DnD can't accept files dropped from explorer. And since we're talking, there's this thing: Do we want to allow GTK+ applications to receive data in known W32-specific formats? I.e. this: > static const GtkTargetEntry drop_type[] = { { "CF_UNICODETEXT", 0, 0 } } to receive data in CF_UNICODETEXT format (as opposed to registering for "UTF8_STRING" format and then letting GDK do the conversion). Same for "PNG" (an alias for "image/png"), "JFIF" and "GIF", as well as "CF_DIB". If we won't allow these, the format list extension code (where GDK pretends to provide/accept W32 formats and then quietly converts the data later on) will be a bit simpler (won't have to handle the cases where GDK doesn't have to pretend, because the application actually supports a given format - it will always assume that apps don't accept and don't provide these formats).
Since you are touching win32 dnd anyway, any chance you could look at changing it to "managed" dnd ? that would really help us for gtk4 cleanups
in any case if this is an improvement for dnd on win32, please push it
Created attachment 358157 [details] [review] GDK W32: Don't leak the atom name string
Created attachment 358158 [details] [review] W32: Massive OLE2-based DnD fix v2 v2: * Fix CASE indentation * Remember current_src_object when we initiate a drag, to be able to detect later on that the data object is ours and use a shortcut when querying targets * Fix the mixup between targets (GdkAtom) and formats (UINT) * Multi-tier target->format conversion First we look in up in the table (new hash table - _atom_format_table), then check whether it's convertible then register a new format. * Simplify the format query (only use the format list, no special logic). Format list now holds not only formats, but also targets that GDK should request as a response for getdata() calls for a particular format * Fix getdata() to look up the request target in its format list, not in the global hash table * Pass more data to the conversion routine, do remember the data format that is actually being requested (the conversion routine only gets a target) * Don't enumerate internal formats to external applications (this might require tweaking - only formats that *absolutely* can't be used in DnD between two GTK+ applications have to be "internal"; doing DnD in GTK+-specific formats between two GTK+ applications is a valid and desired feature; i.e. UTF8_STRING is a valid external format, DELETE is not) * Straighten up the compatibility format-adding code, add format registration there * More logging * Remove unneeded indentation in _gdk_win32_dnd_do_dragdrop(), the code itself is almost unchanged * Fix indentation in gdk_win32_drag_context_drop_finish() * Remove obsolete comments in _gdk_win32_window_register_dnd() * Check for DnD in progress when processing WM_KILLFOCUS, don't emit a grab break event in such cases (this allows alt-tabbing while DnD is in progress, though there may be lingering issues with focus after dropping...) * Support Shell ID List -> text/uri-list conversion, now it's possible to drop files (dragged from Explorer or any app that provides files in Shell ID List format) on GTK+ applications (tested on GEdit). * Explicitly use RegisterClipboardFormatA() when we know that the string is not in unicode. Otherwise explicitly use RegisterClipboardFormatW() with a UTF8->UTF16 converted string * Fix _gdk_win32_display_get_selection_owner() to correctly bail when selection owner HWND is NULL (looking up GdkWindow for NULL HWND always succeeds and returns the root window - not the intended effect). Otherwise this prevented some cases of DnD from other processes. * Fix a bug in gdk_win32_selection_add_targets() (a string was freed too early) I still haven't received any comments on the issue of allowing/disallowing GTK+ applications to accept/provide W32-specific DnD formats without using conversion. As for managed DnD, it'll be a separate patch. Also, there's the question of whether i should implement managed DnD for OLE2 DnD only, or for both OLE2 DnD and for LOCAL+DROPFILES DnD. In the former case i'd just attach a separate patch to this bug. In the latter case i'd need to file a new bug for managed DnD.
> As for managed DnD, it'll be a separate patch. > Also, there's the question of whether i should implement managed DnD for > OLE2 DnD only, or for both OLE2 DnD and for LOCAL+DROPFILES DnD. In the > former case i'd just attach a separate patch to this bug. In the latter > case i'd need to file a new bug for managed DnD. I don't know the tradeoffs between these - is OLE2 a superset of LOCAL+DROPFILES? Or can we make it one ? It would be nice to have just one protocol to support. > I still haven't received any comments on the issue of allowing/disallowing GTK+ > applications to accept/provide W32-specific DnD formats without using conversion. It might be interesting to allow that. Do we have any appds that would benefit from this ?
(In reply to Matthias Clasen from comment #12) > > As for managed DnD, it'll be a separate patch. > > Also, there's the question of whether i should implement managed DnD for > > OLE2 DnD only, or for both OLE2 DnD and for LOCAL+DROPFILES DnD. In the > > former case i'd just attach a separate patch to this bug. In the latter > > case i'd need to file a new bug for managed DnD. > > I don't know the tradeoffs between these - is OLE2 a superset of > LOCAL+DROPFILES? Or can we make it one ? It would be nice to have just one > protocol to support. LOCAL is just a hack - in this mode GDK handles the DnD within the same application completely by itself (grabs the cursor, and then follows the pointer motion and does the things it needs to do). This mode actually works well enough (apart from some minor bugs i've heard about over the years, no one ever called this DnD mode dysfunctional). Problem is, LOCAL is completely incapable of doing DnD between applications. DROPFILES is an ages-old DnD mechanism (harking back to the days of Windows 3.1, if documentation is to be believed), which allows an application window to accept files dragged from Explorer (the window receives a message, WM_DROPFILES, hence the name). That was the only kind of external DnD that was possible in LOCAL+DROPFILES combo. OLE2 DnD is a more modern (from 2000s) DnD mode. Its disadvantage is that Windows drives the process (and its semantics don't quite match X/GDK - OLE2 expects tangible results to be returned in response to every callback it makes, whereas in X one process just sends messages, and the other process sends other messages back at its own leisure). The advantage is that this works between processes, in both directions. Obviously, there's the matter of data formats, but at the very least it trivially supports GTK-specific formats (which is exactly what motivated me originally - i wanted to DnD custom data between apps), and there are some conversion routines in place to support W32-native formats (basic ones - text, image, list-of-files). So far OLE2 DnD proved to be able to cover all the DnD needs within the same application (that i could test - currently i'm limited to gtk3-demo, gtk3-widget-factory, testdnd, testdnd2 and gedit), although there are some glitches still. So i'd say that it can do everything LOCAL+DROPFILES can do, and then some. > > > I still haven't received any comments on the issue of allowing/disallowing GTK+ > > applications to accept/provide W32-specific DnD formats without using conversion. > > It might be interesting to allow that. Do we have any appds that would > benefit from this ? 1) W32 applications that somehow implemented OLE2 DnD support independently from GDK (not sure how feasible that is) and, consequentially, had to have code to work with W32 formats directly. They should be able to trivially change that code to work with OLE2 DnD. 2) W32 applications that supported W32 clipboard formats by doing custom clipboard handling. Clipboard and OLE2 DnD use the same underlying data formats, so, again, the code can be adapted for handling data received via OLE2 DnD. I imagine, this list is extremely short. 3) Applications that didn't support this before, but would want to send/receive DnD data without conversion happening behind the scenes. This might be especially useful when dealing with [outdated] 3rd-party applications that have eclectic DnD data format support. For example, i don't think that GDK can send CF_TEXT (text encoded in locale-specific encoding), as CF_UNICODETEXT (UTF-16-encoded) is preferred. Or something like that. Either way, this would be just GDK stepping out of the way and not interfering with the data being sent/received. The complication is that GDK has to keep track of the fact that it should or shouldn't convert a particular format. Nothing non-trivial, just more if/then/else checks.
Created attachment 358789 [details] [review] W32: Massive W32 DnD fix v3 v3: Also adds managed DnD implementation for W32 GDK backend (for both OLE2 and LOCAL protocols). Mostly a copy of the X11 backend code, but there are some minor differences: * doesn't use drag_window field in GdkDragContext, uses the one in GdkWin32DragContext exclusively * subtracts hotspot offset from the window coordinates when showing the dragback animation * tries to consistently support scaling and caches the scale in the context * Some keynav code is removed (places where grabbing/ungrabbing should happen is marked with TODOs), and the rest is probably inert. Also, some OLE2 DnD changes: * More logging * Send DROP_FINISHED event after DnD loop ends * Send STATUS event on feedback * set_cursor() is now works the same way it does on X11, so there shouldn't be any problems with it now (unlike previous iterations of this patch) I think that this iteration is ready enough for testing. Please give it a try. I'm especially interested in feedback on DnD between processes when using OLE2 DnD protocol (obviously, doing that with the LOCAL protocol is impossible).
Review of attachment 358157 [details] [review]: Sure
Still some changes pending. For one, manual re-focusing is no longer needed in managed DnD mode, so that bit of code is going away in the next version of this patch. That's good news. The bad news is that there's a bug related to the GetKeyboardState() API - it might return incorrect (for some definitions of "incorrect") keyboard state initially, when the drag operation begins, and later (at the next invocation) return a different (correct; for some definitions of "correct") state, with different mouse button pressed, thus resulting in immediate cancellation of the drag operation. Currently it's possible to trigger it by clicking the middle mouse button, and then trying to drag anything. I'm still trying to figure out what goes wrong (MSDN says something about thread message queue, but it's not obvious how this applies to GDK).
Created attachment 358988 [details] [review] W32: Massive W32 DnD fix v4 v4: * Move GetKeyboardState() and related code into _gdk_win32_window_drag_begin(), so that it's closer to the point where last_pt and start_pt are set * Use & 0x80 to check for the key being pressed. Windows will set low-order bit to 1 for all mouse buttons to indicate that they are toggled, so simply checking for the value not being 0 is not enough anymore. This is probably a new thing in modern W32 that didn't exist before (OLE2 DnD code is old). * Remove the last_drop_target_window() hack introduced in one of the previous iterations of this patch, it's not needed in managed DnD mode.
Created attachment 359038 [details] [review] W32: Massive W32 DnD fix v5 v5: * Instead of setting _dnd_data_object set only for the duration of dragenter(), keep it around longer, and unset it on dragleave(). Again, this is the split between drag destination and drag source rearing its ugly head. If our application is the destination only, and the source is in another process, we might need to get drag data during dragover(), which does not receive a pointer to the data object, so we need to keep it around for that. Question: maybe i should attach this object to the drag context (i.e. g_object_set_data() or somesuch) instead of using a global variable? Same applies to other global variables that are set and unset over time.
Created attachment 359039 [details] [review] GDK W32: Update layered windows on opacity changes Without this patch layered windows are only updated when they are moved by the user or then their contents changes. This patch adds opacity changes to the list of things that make GDK update a window. Without this windows that don't redraw and are not moved by the used (DnD drag indicator windows, for example) don't change their opacity. This is necessary to allow the drag indicator cancellation animation to change indicator opacity.
Review of attachment 359038 [details] [review]: Hi LRN, Thanks for the great work involved in here, it is indeed quite a feat! However, I think the HiDPI support is a bit off, please see the in-line comments--I might have missed some things here in the comments below. With blessings, and cheers! ::: gdk/win32/gdkdnd-win32.c @@ +146,3 @@ G_DEFINE_TYPE (GdkWin32DragContext, gdk_win32_drag_context, GDK_TYPE_DRAG_CONTEXT) +/* Note: x_root and y_root are in W32 space */ Uh, no... They are in GDK terms, or things need to be done consistently (this is where things can be a bit confusing at points). I am going to mention things as x_root and y_root are in GDK terms... @@ +155,3 @@ + + gdk_window_move (context_win32->drag_window, + (_gdk_offset_x + x_root - context_win32->hot_pt.x) / context_win32->scale, Only divide hot_pt.x by the scale (likewise for hot_pt.y) @@ +1026,3 @@ + e->dnd.context = g_object_ref (ctx->context); + e->dnd.time = GDK_CURRENT_TIME; + e->dnd.x_root = pt.x + _gdk_offset_x; Divide the results of (pt.x + _gdk_offset_x) and (pt.y + _gdk_offset_y) by the scale @@ +2195,2 @@ current_dest_drag_win32 = GDK_WIN32_DRAG_CONTEXT (current_dest_drag); + current_dest_drag_win32->last_pt.x = x_root * current_dest_drag_win32->scale - _gdk_offset_x; Multiply the results of (x_root + _gdk_offset_x) and (y_root + _gdk_offset_y) by the scale @@ +2324,3 @@ } + + context_win32->start_pt.x = x_root * context_win32->scale - _gdk_offset_x; Multiply the results of (x_root + _gdk_offset_x) and (y_root + _gdk_offset_y) by the scale @@ +2595,3 @@ + if (context_win32->drag_window) + move_drag_window (context, (x_root - _gdk_offset_x) * context_win32->scale, y_root - _gdk_offset_y * context_win32->scale); Don't multiply by context_win32->scale for the x and y coordinates--just pass in x_root - _gdk_offset_x and y_root - _gdk_offset_y @@ +2666,3 @@ /* Send a drag-motion event */ + context_win32->last_pt.x = x_root * context_win32->scale - _gdk_offset_x; Multiply the results of (x_root + _gdk_offset_x) and (y_root + _gdk_offset_y) by the scale @@ +3031,3 @@ + + gdk_window_show (context->drag_window); + gdk_window_move (context->drag_window, This seems ok... getting the right multiplication and division can be quite confusing :) @@ +3547,3 @@ + { + /* DnD is managed, update current position */ + move_drag_window (context, win32_context->last_pt.x, win32_context->last_pt.y); Divide win32_context->last_pt.x and win32_context->last_pt.y by the scale
(In reply to Fan, Chun-wei from comment #20) > Review of attachment 359038 [details] [review] [review]: > > Hi LRN, > > Thanks for the great work involved in here, it is indeed quite a feat! > > However, I think the HiDPI support is a bit off I implemented HiDPI support purely by touch&feel, i don't actually have a HiDPI display to test this (well, i can emulate one, but it's a bother, given that i'm still working out the kinks out of non-HiDPI-related parts of this code). > ::: gdk/win32/gdkdnd-win32.c > @@ +146,3 @@ > G_DEFINE_TYPE (GdkWin32DragContext, gdk_win32_drag_context, > GDK_TYPE_DRAG_CONTEXT) > > +/* Note: x_root and y_root are in W32 space */ > > Uh, no... They are in GDK terms, or things need to be done consistently > (this is where things can be a bit confusing at points). I am going to > mention things as x_root and y_root are in GDK terms... The function marked by this comment *does* take coordinates in W32 space. The main reason to keep W32-space coordinates is that we can trivially get them using GetCursorPos() API without any other perturbations. That may or may not have been a sound engineering decision on my part (maybe it would have been better to keep all coordinates in GDK space, and then only do conversion around GetCursorPos() and around the arguments we get directly from W32 in OLE2 DnD callbacks). This is still changeable though, so that's good news. I probably should not have kept the names "root_x" and "root_y" there, as these imply GDK coordinate space. > > @@ +155,3 @@ > + > + gdk_window_move (context_win32->drag_window, > + (_gdk_offset_x + x_root - context_win32->hot_pt.x) / > context_win32->scale, > > Only divide hot_pt.x by the scale (likewise for hot_pt.y) I assume that GDK coordinates are both offset (so that they always start at 0) AND scaled. Am i wrong? (also, as i have said earlier, this function accepts coordinates in W32 space). > > @@ +2195,2 @@ > current_dest_drag_win32 = GDK_WIN32_DRAG_CONTEXT (current_dest_drag); > + current_dest_drag_win32->last_pt.x = x_root * > current_dest_drag_win32->scale - _gdk_offset_x; > > Multiply the results of (x_root + _gdk_offset_x) and (y_root + > _gdk_offset_y) by the scale M-m-m...If x_root *here* is in GDK coordinates, then this should be the right way. De-scale it first, than subtract the offset (offsets are derived from W32 data and shouldn't be scaled in the first place, IIRC - again, am i wrong? Oh...damn, i *am* wrong. Offsets are set from the results of gdk_monitor_get_geometry(), which are set by gdk_monitor_set_size() in W32 backend, which is currently given the scaled size. Bummer). > > @@ +2324,3 @@ > } > + > + context_win32->start_pt.x = x_root * context_win32->scale - _gdk_offset_x; > > Multiply the results of (x_root + _gdk_offset_x) and (y_root + > _gdk_offset_y) by the scale Same as above. > > @@ +2595,3 @@ > > + if (context_win32->drag_window) > + move_drag_window (context, (x_root - _gdk_offset_x) * > context_win32->scale, y_root - _gdk_offset_y * context_win32->scale); > > Don't multiply by context_win32->scale for the x and y coordinates--just > pass in x_root - _gdk_offset_x and y_root - _gdk_offset_y Same as above. > > @@ +2666,3 @@ > /* Send a drag-motion event */ > > + context_win32->last_pt.x = x_root * context_win32->scale - > _gdk_offset_x; > > Multiply the results of (x_root + _gdk_offset_x) and (y_root + > _gdk_offset_y) by the scale Same as above. > @@ +3547,3 @@ > + { > + /* DnD is managed, update current position */ > + move_drag_window (context, win32_context->last_pt.x, > win32_context->last_pt.y); > > Divide win32_context->last_pt.x and win32_context->last_pt.y by the scale Given that move_drag_window() takes W32-space coordinates, this is exactly as it should be. In other (unrelated) news: there's a bug in the DnD code (the core tries to ref a NULL object, throws a critical warning). I already have a fix, will post that later. I'm also thinking about extending the code to use the IDragSourceHelper and IDropTargetHelper interfaces. That will probably be a separate patch.
Hello LRN, (In reply to LRN from comment #21) > > I implemented HiDPI support purely by touch&feel, i don't actually have a > HiDPI display to test this (well, i can emulate one, but it's a bother, > given that i'm still working out the kinks out of non-HiDPI-related parts of > this code). I understand this issue :), thanks though! I happen to own a HiDPI display, so that's where I am basing my review upon, on testing the items. > The function marked by this comment *does* take coordinates in W32 space. > The main reason to keep W32-space coordinates is that we can trivially get > them using GetCursorPos() API without any other perturbations. That may or > may not have been a sound engineering decision on my part (maybe it would > have been better to keep all coordinates in GDK space, and then only do > conversion around GetCursorPos() and around the arguments we get directly > from W32 in OLE2 DnD callbacks). This is still changeable though, so that's > good news. Note though, the main thing is that "things need to be done consistently", so the mention on the comments I had were that x_root and y_root are in terms of GDK coordinates, so that I hope the things sound clearer. That is, we can certainly do it in Windows API coordinates, though, but things need to be done consistently. > I assume that GDK coordinates are both offset (so that they always start at > 0) AND scaled. Am i wrong? You're right on this. > M-m-m...If x_root *here* is in GDK coordinates, then this should be the > right way. De-scale it first, than subtract the offset (offsets are derived > from W32 data and shouldn't be scaled in the first place, IIRC - again, am i > wrong? Oh...damn, i *am* wrong. Offsets are set from the results of > gdk_monitor_get_geometry(), which are set by gdk_monitor_set_size() in W32 > backend, which is currently given the scaled size. Bummer). I fully understand your bummers here (and in the following parts) :). That's the process that was underwent when doing the HiDPI support initially. Basically, the rule of thumb is: GDK coordinates -> * scale -> Windows API Coordinates Windows API coordinates -> / scale -> GDK Coordinates > In other (unrelated) news: there's a bug in the DnD code (the core tries to > ref a NULL object, throws a critical warning). I already have a fix, will > post that later. > > I'm also thinking about extending the code to use the IDragSourceHelper and > IDropTargetHelper interfaces. That will probably be a separate patch. Thanks! With blessings, and cheers!
Created attachment 359729 [details] [review] W32: Massive W32 DnD and selection fix v6 Also significantly changes the way selection (and clipboard) is handled (as MSDN rightly notes, the handling for DnD and Clipboard formats is virtually the same, so it makes sense to handle both with the same code): * Don't spam GDK_OWNER_CHANGE, send them only when owner actually changes * Open clipboard when our process becomes the clipboard owner (we are doing it anyway, to empty the clipboard and *become* the owner), and then don't close it until a scheduled selection request event (with TARGETS target) is received. Process that event by announcing all of our supported formats (by that time add_targets() should have been called up the stack, thus the formats are known; just in case, add_targets() will also schedule a selection request, if one isn't scheduled already, so that late-coming formats can still be announced). * Allow clipboard opening for selection_convert() to be delayed if it fails initially. * The last two points above should fix all the bugs about GTK+ rising too much ruckus over OpenClipboard() failures, as owner change *is allowed* to fail (though not all callers currently handle that case), and selection_convert() is asynchronous to begin with. Still, this is somewhat risky, as there's a possibility that the code will work in unexpected ways and the clipboard will remain open. There's now logging to track the clipboard being opened and closed, and a number of failsafes that try to ensure that it isn't kept open for no reason. * Added copious notes on the way clipboard works on X11, Windows and GDK-W32, also removed old comments in DnD implementation, replaced some of them with the new ones * A lot of crufty module-global variables are stuffed into a singleton object, GdkWin32Selection. It's technically possible to make it a sub-object of the Display object (the way Wayland backend does), but since Display object on W32 is a singleton anyway... why bother? * Fixed (hopefully) and simplified HiDPI parts of the code. * Fixed the send_change_events() a bit (was slightly broken in one of the previous iterations) * Ensure that there's no confusion between selection conversion (an artifact term from X11) and selection transmutation (changing the data to be W32-compatible) * Put all the transmutation code and format-target-matching code into gdkselection-win32.c, now this code isn't spread across multiple files. * Consequently, moved some code away from gdkproperty-win32.c and gdkdnd-win32.c * Ensure that drop target keeps a format->target map for supported formats, this is useful when selection_convert() is called, as it only receives a single target and no hints on the format from which the data should be transmuted into this target. * Use g_set_object() instead of g_ref_object() where it is allowed. * Fix indentation (and convert tabs to spaces), remove unused variables
So, yeah...mission creep. Still, at least there are lots of stuff to nitpick on now! Also, about attachment 359039 [details] [review] - i think that it's unnecessary to invalidate the whole window. We don't really need to redraw it, we only need GDK to *think* that it redrew it, because we change not the pixels themselves, but the metadata (window opacity) that is given to W32 window manager along with the pixels. Or, if we can't con GDK into thinking that the pixels changed, we can at least invalidate the smallest possible region (just one pixel somewhere in the corner)... Anyway, i'm not yet sure how to do either.
Created attachment 359752 [details] [review] W32: Massive W32 DnD and selection fix v7 Sorry, v6 was a bit broken v7: * Remove unneeded _guaded() wrapper around one of the functions * Actually implement clipboard persistence * Relax bad property type check a bit * Pass transmute boolean switch to change_property() * Commit missing changes to gdkevents-win32.c * Commit missing changes to gdkwin32misc.h * Commit missing changes to gtkselection.c * Add clear_targets() on W32, to de called by GTK
Created attachment 360002 [details] [review] GDK W32: Refuse to release mouse grab while in DnD mode Handle WM_CANCELMODE and do nothing in response to it when DnD is active. Otherwise pass it to DefWindowProc, which will call ReleaseCapture() on our behalf. This prevents us from losing mouse capture when alt-tabbing during DnD (this includes the feature of Windows Explorer where dragging stuff over a window button in the taskbar causes that window to receive focus, i.e. keyboardless alt-tabbing). This seems to work fine (i think there was one corner-case where Windows WM changes focus due to dialogs popping up, and they don't pop up because of this patch, or something along these lines; but that's sufficiently rare and not critical). The alternative for this patch is to set up a global mouse hook and use it to detect mouse state (instead of trusting the data that Windows gives to IDataSource_QueryContinueDrag, as that data gets stale once we lose the mouse capture). That would be somewhat more involved, but, on the other hand, won't interfere with the WM.
Created attachment 360003 [details] [review] GDK W32: Ensure that selection request is processed To do that, run the message loop indefinitely until the side-effect of running the selection request handler is achieved. The disavantage of this method is that if the event handling is somehow missed (due to a variety of reasons - after all, it's not a straight path from an event being queued to property_change() being called), this will loop forever. I came up with this patch because i've observed a number of times the strange behaviour of the GDK message processing where the run-message-loop-while-there-are-events-in-the-queue logic does not ensure that the callback for the queued event actually fires. This bug came to fore in my own custom application, and is made evident when the drop site does not accept the drag (due to the source not getting the data from a selection request and thus sending a dud, whereas the drop site is coded to examine the data before accepting it, and it can't accept a dud). This may or may not be related to the fact that the source is queried for the data repeatedly while the mouse cursor moves. Perhaps, something should be cached here? As mentioned above, this patch is somewhat dangerous, and so far i'm not sure whether it is actually possible for this to loop forever. Interestingly, there's a place in gdkevents-win32.c where the code sends the same event, but uses _gdk_event_emit() directly instead of queueing the event and then running the message loop. Would that logic produce better results?
Created attachment 360690 [details] [review] W32: Massive W32 DnD fix v8 v8: * Changed the commit message to correspond better to the actual code (some parts of it were too old and no longer reflected the changes adequately). * Fixed a recently-introduced bug in gdkevents-win32.c (a field that it set was being NULLified elsewhere, now it should keep the data in its own variable to avoid losing it) that resulted in a crash when pasting data from clipboard I'm still looking for feedback on the more controversial attachment 360002 [details] [review], attachment 360003 [details] [review] and attachment 359039 [details] [review] (not to mention testing for the whole thing).
Created attachment 360691 [details] [review] W32: Massive W32 DnD and selection fix v9 Oops, v8 didn't go far enough in making gdkevents-win32.c use property_change_data instead of win32_sel->property_change_data. Sorry about that. And yes, i did check that this time the bug is actually fixed and GTK+ doesn't crash.
Created attachment 360694 [details] [review] W32: Massive W32 DnD and selection fix v10 v10: At some point win32_selection->compatibility_targets was a GHashTable of GArrays of GdkAtom values (targets), keyed by UINTs (formats). However, later on i've decided to switch to GdkSelTargetFormat, since it also contains the information about the use of transmutation for the mapping (otherwise it's ambiguous whether one needs to transmute the data or not), but didn't change things far enough, and compatibility_targets still contained GArrays of GdkAtom, whereas the code that worked with it expected GArrays of GdkSelTargetFormat, which was a gross type error and resulted in bad data being used (specifically, file DnD from Explorer was broken because of it). All fixed now.
Review of attachment 359039 [details] [review]: Is it me or you are only checking the layered_opacity and not the pending flag that you added? From what I can see that boolean is really not used at all...
Review of attachment 360002 [details] [review]: Add the comment and feel free to push it. ::: gdk/win32/gdkevents-win32.c @@ +3280,3 @@ + case WM_CANCELMODE: + if (_modal_operation_in_progress & GDK_WIN32_MODAL_OP_DND) I think this deserves a comment similar to what you wrote on the commit message
Review of attachment 360003 [details] [review]: If we can have an infinite loop here this makes me wonder whether we should put some contingency and loop out after a few tries...
Review of attachment 357972 [details] [review]: As spoken with Matthias on IRC go ahead.
Review of attachment 357972 [details] [review]: Shouldn't that somehow check that it's running on X11 using GDK_IS_X11_DEFAULT(gtk_widget_get_display())? Otherwise we register the drag dest on Wayland, but only if GTK is configured with X support.
(In reply to Ignacio Casal Quinteiro (nacho) from comment #31) > Review of attachment 359039 [details] [review] [review]: > > Is it me or you are only checking the layered_opacity and not the pending > flag that you added? From what I can see that boolean is really not used at > all... Ri-i-i-ght...My bad. I thought that there would be checks for window update being necessary, and added a special field to ensure that these checks can be bypassed (since window contents aren't really changed). But the patch evolved over time and, it turned out, if you invalidate a window, it is just updated, no checks are needed (all the checks in gdk_win32_window_end_paint() test for !layered, which is always false for layered windows). So yeah, that patch will probably need a cleanup. That said, i'm still unsure about the whole invalidate-the-entire-window thing - there should be a way to invalidate it without actually re-drawing its contents. Maybe that field will be useful there. Dunno. (In reply to Ignacio Casal Quinteiro (nacho) from comment #33) > Review of attachment 360003 [details] [review] [review]: > > If we can have an infinite loop here this makes me wonder whether we should > put some contingency and loop out after a few tries... How few is "few"? (In reply to Benjamin Otte (Company) from comment #35) > Review of attachment 357972 [details] [review] [review]: > > Shouldn't that somehow check that it's running on X11 using > GDK_IS_X11_DEFAULT(gtk_widget_get_display())? > > Otherwise we register the drag dest on Wayland, but only if GTK is > configured with X support. I'm open to suggestions. Change both ifdefs to "X11 || Wayland", then only register when GDK_IS_X11_DEFAULT(gtk_widget_get_display()) passes?
(In reply to LRN from comment #36) > (In reply to Ignacio Casal Quinteiro (nacho) from comment #31) > > Review of attachment 359039 [details] [review] [review] [review]: > > > > Is it me or you are only checking the layered_opacity and not the pending > > flag that you added? From what I can see that boolean is really not used at > > all... > > Ri-i-i-ght...My bad. I thought that there would be checks for window update > being necessary, and added a special field to ensure that these checks can > be bypassed (since window contents aren't really changed). > But the patch evolved over time and, it turned out, if you invalidate a > window, it is just updated, no checks are needed (all the checks in > gdk_win32_window_end_paint() test for !layered, which is always false for > layered windows). > > So yeah, that patch will probably need a cleanup. That said, i'm still > unsure about the whole invalidate-the-entire-window thing - there should be > a way to invalidate it without actually re-drawing its contents. Maybe that > field will be useful there. Dunno. > Actually, you know what...why am i bothering with invalidation at all? There's no need to redraw anything. No need to go through the motions of invalidate->redraw->end_paint (update). I can just call UpdateLayeredWindow() with the existing cache surface right there, and be done. Sheesh.
Review of attachment 360003 [details] [review]: I agree with nacho that a (long) timeout as a safety measure would be a good idea
Review of attachment 360694 [details] [review]: ::: gdk/win32/gdkdnd-win32.c @@ +122,1 @@ */ Some of this should probably end up in win32-specific documentation for application developers, and not just hidden away in a backend source comment... @@ +1507,3 @@ + result->formats = g_array_new (FALSE, FALSE, sizeof (GdkSelTargetFormat)); + + for (p = context->targets; p; p = g_list_next (p)) For GList, this is generally open-coded in gtk: p = p->next @@ +2020,3 @@ + mask = GDK_WA_X | GDK_WA_Y | GDK_WA_VISUAL | GDK_WA_TYPE_HINT; + + return gdk_window_new (gdk_screen_get_root_window (screen), &attrs, mask); We've landed changes to get rid of GdkScreen as far as possible. This should probably use gdk_display_get_root_window now, or just avoid non-backend api altogether.
Created attachment 363683 [details] [review] W32: Massive W32 DnD and selection fix v11 v11: * Fix the DnD indicator reverse movement when a drop is refused (the match with the hotspot coordinates was slightly off) * Use list->next instead of g_list_next() * Expand the use of dummy property deletion code slightly
Created attachment 363684 [details] [review] GDK W32: Update layered windows on opacity changes v2 v2: * Remove some code repetition * Just update layered window contents from the cache on opacity changes, no need to do the invalidate->repaint dance.
Created attachment 363685 [details] [review] GDK W32: Refuse to release mouse grab while in DnD mode v2 v2: * Copy the commit message into the code as a comment
Created attachment 363686 [details] [review] GDK W32: Ensure that selection request is processed v2 v2: * Don't loop longer than 1 second
I didn't do anything with the big comment that should go into app developer documentation for 2 reasons: 1) gtk-doc is broken, and i can't do any documentation changes 2) even if it wasn't broken, i have no idea where to put this comment (both GDK and GTK introductory doc sections on DnD are awfully short).
(In reply to LRN from comment #44) > I didn't do anything with the big comment that should go into app developer > documentation for 2 reasons: > 1) gtk-doc is broken, and i can't do any documentation changes > 2) even if it wasn't broken, i have no idea where to put this comment (both > GDK and GTK introductory doc sections on DnD are awfully short). We can work that out afterwards, no problem
Review of attachment 363683 [details] [review]: I'm just going to set acn here. Waiting for somebody to do a detailed review is not going to result in anything.
Review of attachment 363684 [details] [review]: This sounds like an independent bug fix ?
Review of attachment 363685 [details] [review]: ok
Review of attachment 363686 [details] [review]: the commit message needs an update, now that there's a 1-second limit
Created attachment 364217 [details] [review] GDK W32: Ensure that selection request is processed v3 v3: * Adjusted the commit message
Review of attachment 364217 [details] [review]: thanks, looks good now
Attachment 357971 [details] pushed as 022cf42 - GDK W32: Fix a typo in OLE2 DnD code Attachment 357972 [details] pushed as 2cc7a9c - Only register application/x-rootwindow-drop on X11 Attachment 358157 [details] pushed as 4102698 - GDK W32: Don't leak the atom name string Attachment 363683 [details] pushed as 8caba95 - W32: Massive W32 DnD fix Attachment 363684 [details] pushed as 0ee453a - GDK W32: Update layered windows on opacity changes Attachment 363685 [details] pushed as 7b6efc2 - GDK W32: Refuse to release mouse grab while in DnD mode Attachment 364217 [details] pushed as 934ac3f - GDK W32: Ensure that selection request is processed
The main question for me now is whether i should make OLE2 DnD the default or not. Because currently it's *still* a hidden mode that is only enabled by a special environment variable.
(In reply to LRN from comment #53) > The main question for me now is whether i should make OLE2 DnD the default > or not. Because currently it's *still* a hidden mode that is only enabled by > a special environment variable. I'd make it the default, if people complain we should fix the bugs that araise but if you are confident with this code I'd definitely go ahead with it.
Created attachment 364435 [details] [review] GDK W32: Preserve the target value for change_property() We need to know the target atom value to know when we need to do something with side-effects (since side-effects are expressed via special target values). Previously, the code side-stepped that by looking at the data type (which was rather unique for the one side-effect target that we supported, signalled by the TARGETS target), but for the DELETE target that seems to be no longer an option, hence the new field to carry this information past the convert_selection() routine. This prevents GDK from throwing a warning when trying to convert a DELETE target, which has no format or data objects set. The side-effects for the DELETE target happen earlier, in GTK layer. By the point it gets to change_property(), it's a no-op.
Created attachment 364436 [details] [review] GDK W32: Special handling for DELETE requests 1) Ensure that any DELETE requests from the target are sent to GDK, even if both the source and the target are in the same process and it is therefore possible to use a shortcut and call the handler directly in GTK layer 2) Ensure that target GDK doesn't do anything when GTK asks it to send a DELETE request, just report back immediately (the code up the stack does not check for successfullness when request is DELETE, so not giving it any data is OK). The source code already synthesizes a DELETE request, so that side is also taken care of.
Created attachment 364437 [details] [review] GDK W32: Make sure drag source window is not NULL This prevents GTK from throwing a bunch of warnings when it tries to get drag source window -> screen of that window -> ipc widget for that screen, and then tries to attach a signal handler to that widget. Specifically, this happens when we get a DnD move from another application.
Created attachment 364438 [details] [review] GDK W32: Remove an unnecessary type check No idea why it's here, the hash table can store any kind of data, there's no reason why it wouldn't be able to store an old X string type. Might be a holdout from the old days, when strings were handled in a special way (stored directly in the clipboard?).
Review of attachment 364436 [details] [review]: not a big fan of adding more backend-specific code in gtk/, but this is only temporary anyway - gtkselection.c is going away when Benjamin's clipboard rework lands.
Review of attachment 364437 [details] [review]: This is also a temporary workaround at best. The root window is going away, and drag source windows are going away too.
Review of attachment 364438 [details] [review]: ok
Attachment 364435 [details] pushed as 56074fb - GDK W32: Preserve the target value for change_property() Attachment 364436 [details] pushed as 3fd23fc - GDK W32: Special handling for DELETE requests Attachment 364437 [details] pushed as c329940 - GDK W32: Make sure drag source window is not NULL Attachment 364438 [details] pushed as 8df7f88 - GDK W32: Remove an unnecessary type check