GNOME Bugzilla – Bug 571582
gtk_selection_convert() not behaving as expected
Last modified: 2018-04-14 23:57:28 UTC
Hi, My program uses a widget (Scintilla) that has its own clipboard using a GdkAtom. When gtk_selection_convert() is called using that atom, I expect the "selection-received" signal to be generated and the text to be pasted. Instead I'm getting nothing.
Could the fact that most of the public API implementations in gdk/quartz/gdkselection-quartz.c and gdkproperty-quartz.c are empty with a /* FIXME: Implement */ comment have anything to do with it?
Created attachment 162707 [details] [review] Gtkselection-quartz implementation against master 41120d2f6753fd6817a622fc66792399e0096665 Patch against current master to implement the Gtk selection functions on Quartz. I decided to follow the lead of gtkclipboard-quartz and ignore as much as possible the gdk layer; instead, I implemented gtkselection in terms of gtkclipboard. I find this consistent with the current widget code, which uses the gtkclipboard interface even for X11 selection pasting.
Created attachment 163947 [details] [review] Corrected patch against master Updated patch: I'd missed that I needed to create a GdkEventSelection to pass to the emission of selection-clear-event in gtk_selection_clear_cb. Not surprisingly, this caused a crash.
John, is there a specific application or test case you used to test this? I need to find some time to thoroughly review this and re-evaluate whether cloning GtkSelection is the best solution short-term. Next to clipboards, selections are also involved in drag-and-drop. As you know, there are still quite some holes in there as well and it might be beneficial to implement the GDK interface as well because of that. But that only makes sense if full GDK intra-application drag-and-drop has any chance of working on Quartz, that's one of the things I need to figure out when thoroughly reviewing this.
Kris, Sorry I missed this; it was just before I went off on a months vacation. Yes, I tested against tests/testselection and verified "real-life" operation with Gnucash (which was, as usual, my primary motivation -- it has a homebrewed table view widget that uses GtkSelection instead of GtkClipboard). I don't think that GdkSelections are involved in much of anything anymore, actually. All of the text widgets are using an approach very similar to the one I adopted here. There are only 8 calls to gdk_selection_foo in gtk/gtkselection.c and one each in gtk/gtkdnd.c and gtk/gtktextbuffer.c (both to gdk_selectin_owner_get_for_display). I rather think it would be better to either relegate it to X11 code or get rid of it altogether. GtkSelections, on the other hand, are used extensively in gtk/gtkdnd.c (and many other places. My patch implements this for quartz.
Created attachment 177032 [details] [review] Updated quartz gtkselection implementation for master as of 2010-12-24
Created attachment 196278 [details] [review] Selection implementation, updated to apply to master of 11Sep2011
Created attachment 196295 [details] [review] Selection implementation, updated to apply to master of 11Sep2011, corrected The first try had some artifacts from rebasing.
It would be nice if this could be applied to gtk+. I'm trying currently to build for 3.4 and this patch is failing due to some small changes in gtk+ (like Makefile.am). Merging it would prevent manually updating the patch every time. Are there any objections to not merge it in gtk+?
Yes -- I am not sure I agree with the approach taken, secondly it needs to be reviewed.
Kris, I think it's time to accept that if you haven't found time to review it in 2 years, you're not going to. If you don't like the approach, what would you prefer?
Fine, I will stop reviewing patches.
I understand it needs to be reviewed, however, consider that this code has now been used anyway in all native gtk+ OS X applications, and that it's probably better to have this than to not having it implemented at all _and_ that we can still change the implementation afterwards. Wouldn't it be easier to push this?
(In reply to comment #12) > Fine, I will stop reviewing patches. Don't get pissy. Just recognize that you've been promising to review this for two years and haven't set it high enough on your priority list to actually look at it.
(In reply to comment #13) > I understand it needs to be reviewed, however, consider that this code has now > been used anyway in all native gtk+ OS X applications, and that it's probably > better to have this than to not having it implemented at all _and_ that we can > still change the implementation afterwards. Wouldn't it be easier to push this? Using GtkSelection is actually pretty rare, and isn't really to be encouraged. The same behavior can be achieved with less pain by using GtkClipboard, and in fact I converted Gnucash to do so a few months ago (see #667900 for why). Most applications use neither, relying instead upon the clipboard and selection behavior built in to individual widgets. That said, this patch *is* well-tested at this point after having been in Gnucash for two years. On the other hand, GdkSelection has been massively simplified and made less dependent upon X11 in the meantime, so it might now be feasible to implement *that* for quartz which would make this unnecessary.
afaik we still use it in several parts in gedit.
I still had this bug starred in my inbox to ever return to it. I never liked how GtkClipboard and GtkDND are currently implemented for Quartz in GTK+, by copying gtkdnd.c and gtkclipboard.c and making modifications where necessary. The main problem of this approach is maintainability, gtkclipboard.c and gtkclipboard-quartz.c become out of sync, because fixes to gtkclipboard.c do not end up in gtkclipboard-quartz.c, and it gradually becomes harder to figure what parts in gtkclipboard-quartz.c are different from gtkclipboard.c. Therefore, I don't think copying gtkselection.c to gtkselection-quartz.c is the right approach. The right approach is, in my opinion, to move as many Cocoa bits as possible to the GDK layer and try to use as much of the generic code in GTK+ as possible (this means using the plain gtkclipboard.c, gtkdnd.c, gtkselection.c). To demonstrate the feasibility of this approach, I implemented GdkSelection for Quartz such that gtkclipboard.c can be used instead of gtkclipboard-quartz.c. The code is unfinished, but it seems to be able to copy image data from the GIMP to Preview and vice versa just fine. I will attach the necessary patches, they are based on gtk-2-24 as of today (Dec 30, 2012). To me, the Quartz-specific GdkSelection code seems much easier to understand than the current gtkclipboard-quartz.c. I would probably continue implementation as follows: - Finish the "clipboard part" of GdkSelection. This includes finishing the code to convert URI lists which is currently commented out and a bunch of testing to see if we missed something. - Add a patch to actually remove gtkclipboard-quartz.c. - Compare gtkdnd-quartz.c with gtkdnd.c and revert all code that should be using GtkSelection (which is now doing Quartz-specific stuff). This will likely require further extension of GdkSelection. - Finally, determine how many differences between gtkdnd-quartz.c and gtkdnd.c are left and see if we can get rid of gtkdnd-quartz.c by using a number of (so not too many) #ifdef GDK_WINDOWING_QUARTZ in gtkdnd.c. I will not have any more spare time to work on this, so if anyone wants to pick this up, feel free.
Created attachment 232393 [details] [review] [PATCH 1/3] quartz: define GDK_NATIVE_WINDOW_POINTER
Created attachment 232394 [details] [review] [PATCH 2/3] quartz: move atom/pasteboard type conversions functions to GDK
Created attachment 232395 [details] [review] [PATCH 3/3] (temporary commit) start implementation of GdkSelection for Quartz
Note: all above patches are against gtk-2-24.
Kris, Thanks for moving the ball forward on this. I'll add it to my stack and hope to get to it sometime this spring.
Comment on attachment 232393 [details] [review] [PATCH 1/3] quartz: define GDK_NATIVE_WINDOW_POINTER Pushed this to gtk-2-24. Doesn't apply to gtk3 because GdkNativeWidndow is gone there.
Comment on attachment 232394 [details] [review] [PATCH 2/3] quartz: move atom/pasteboard type conversions functions to GDK Cleaned up to build properly and pushed to gtk-2-24, gtk-3-8 and master.
Created attachment 253551 [details] [review] Updated Kristian's patch with a minor safety fix I've made a minor change to Kristian's patch, replacing some strcmp() with g_strcmp0() just to be safe from NULL strings getting passed in, but I've tested it pretty thoroughly and I think it can be safely committed once it's been fixed up to apply to master.
Created attachment 253552 [details] [review] Preliminary implementation of GdkDnD for Quartz
Created attachment 253554 [details] [review] False start: Integration with NSDragPboard in gdkselection-quartz I've worked on this some for a week or so and want to document what I've done so far: The GdkDnD Quartz implementation patch gets DnD working inside of tests/testdnd, but doesn't work between applications because it doesn't transfer anything to or from the NSPasteboard system: DnD uses a different selection atom from cut-and-paste, and I don't think that it would be a good idea to use the NSGeneralPboard for DnD. The NSDragPboard patch extends gdkselection-quartz and GdkQuartzWindow to take items off of the pasteboard passed in with an NSDragInfo, but it fails to work because it can't find the right GdkWindow to pass the data to: For example, in tests/testdnd it will drop only on the trashcan pixmap, and that's only because it special cases the setting of GdkWindow's user_data to get around the virtual GtkWidgets gtkdnd creates to talk to X11 selections. I've got to work on other things for a few months, so I'm putting the patches up for others to work on it if they feel motivated and to remind me of where I left off when I can come back to it. I've got to
We're moving to gitlab! As part of this move, we are moving bugs to NEEDINFO if they haven't seen activity in more than a year. If this issue is still important to you and still relevant with GTK+ 3.22 or master, please reopen it and we will migrate it to gitlab.
As announced a while ago, we are migrating to gitlab, and bugs that haven't seen activity in the last year or so will be not be migrated, but closed out in bugzilla. If this bug is still relevant to you, you can open a new issue describing the symptoms and how to reproduce it with gtk 3.22.x or master in gitlab: https://gitlab.gnome.org/GNOME/gtk/issues/new