GNOME Bugzilla – Bug 712752
GtkClipboard: add _get_default and _get_primary
Last modified: 2015-01-27 17:12:13 UTC
When I tried to implement a 'copy uri' style function with gjs I found that it wasn't really possible to use the clipboard api without using GdkAtom. And GdkAtom is not usable from gjs because its an opaque struct that it's tricky to refcount. So I still wanted a way of using the clipboard api and this was one way to do it. Hopefully the change is beneficial to the api in general as well as a way to get the default and primary clipboard without using GdkAtom. Patch will be attached. Jonas
Created attachment 260340 [details] [review] clipboard: add get_default and get_primary Add constructor shortcuts for getting the CLIPBOARD and PRIMARY clipboards. This makes the api a bit easier to use. And also allows use of the clipboard api for gjs where GdkAtoms are not available.
Created attachment 271386 [details] [review] clipboard: add get_default and get_primary Add constructor shortcuts for getting the CLIPBOARD and PRIMARY clipboards. This makes the api a bit easier to use. And also allows use of the clipboard api for bindings where GdkAtoms can not be used.
Review of attachment 271386 [details] [review]: I'd argue that no living soul should ever mess around with the PRIMARY clipboard, but if we're going for completeness, then it's fine by me. to be fair, I'd add a GtkClipboard *gtk_x11_clipboard_get_primary (GdkDisplay *display) in the X11-specific GTK API, just to ensure that it's only used on X11. the patch is missing the symbols added to `docs/reference/gtk/gtk3-sections.txt`. ::: gtk/gtkclipboard.c @@ +396,3 @@ +gtk_clipboard_get_default (GdkDisplay *display) +{ + * I think this should allow a NULL display, and if it is NULL, then do the gdk_display_get_default() dance for you.
The one concern I have here is whether this api will change / move to gdk anyway.
Review of attachment 271386 [details] [review]: Thanks for the review! Sorry, but what is the X11-specific GTK API? I.e. where should I add the gtk_x11_clipboard_get_primary function? ::: gtk/gtkclipboard.c @@ +396,3 @@ +gtk_clipboard_get_default (GdkDisplay *display) +{ + return gtk_clipboard_get_for_display (display, GDK_SELECTION_CLIPBOARD); Yeah, that sounds nice.
(In reply to comment #5) > Review of attachment 271386 [details] [review]: > > Thanks for the review! > > Sorry, but what is the X11-specific GTK API? I.e. where should I add the > gtk_x11_clipboard_get_primary function? we have gtk/gtkx.h, which includes gtksocket.h and gtkplug.h, which are known to work only in X11. you should add a gtkclipboard-x11.h that only declares gtk_clipboard_get_primary(), so that it's available only when including gtkx.h. you should also do a type check on the passed GdkDisplay* to see if it's a GdkX11Display, and bail out if it isn't. > ::: gtk/gtkclipboard.c > @@ +396,3 @@ > +gtk_clipboard_get_default (GdkDisplay *display) > +{ > + return gtk_clipboard_get_for_display (display, GDK_SELECTION_CLIPBOARD); > > Yeah, that sounds nice. just remember that the display has to be marked as (allow-none).
(In reply to comment #4) > The one concern I have here is whether this api will change / move to gdk > anyway. if it does, and it should, we can simply mark it as deprecated, and possibly wrap it around the GDK API. right now, though, accessing the clipboard from an introspection-based language binding is basically impossible, given that the atoms are invisible.
Created attachment 273193 [details] [review] clipboard: Add get_default and get_primary Add constructor shortcuts for getting the CLIPBOARD and PRIMARY clipboards. This makes the api a bit easier to use. And also allows use of the clipboard api for bindings where GdkAtoms can not be used.
Created attachment 273194 [details] [review] clipboard: Add link to GdkDisplay in doc
This will not compile as is, since there is no versionmacro for 3_14 yet. When does it get added? When gtk-3-12 branches out?
(In reply to comment #10) > This will not compile as is, since there is no versionmacro for 3_14 yet. When > does it get added? When gtk-3-12 branches out? yes, once gtk-3-12 gets created, the 3.14 version macros will be added as well.
Review of attachment 273193 [details] [review]: looks generally good, apart from a couple of coding style issues. ::: gtk/gtkclipboard.c @@ +35,3 @@ #ifdef GDK_WINDOWING_X11 #include "x11/gdkx.h" +#include <gtkx.h> this should be: #include "gtkx.h" since it's a local header in the inclusion path. @@ +397,3 @@ +GtkClipboard * +gtk_clipboard_get_default (GdkDisplay *display) + * and keyboard shortcuts. missing the: g_return_val_if_fail (display == NULL || ..., NULL); @@ +398,3 @@ +gtk_clipboard_get_default (GdkDisplay *display) +{ + * coding style: do an explicit NULL comparison, instead of the truthy value of a pointer. @@ +422,3 @@ +gtk_clipboard_get_primary (GdkDisplay *display) +{ +} coding style: do an explicit NULL comparison. the type check should be GDK_IS_DISPLAY(), and the check for GDK_IS_X11_DISPLAY() should be done separately with an explicit if. g_return_val_if_fail() can be compiled out, but we want the type check of always be performed. @@ +424,3 @@ + g_return_val_if_fail (!display || GDK_IS_X11_DISPLAY (display), NULL); + + coding style: do an explicit NULL comparison.
Review of attachment 273194 [details] [review]: this looks good.
Comment on attachment 273194 [details] [review] clipboard: Add link to GdkDisplay in doc Attachment 273194 [details] pushed as 45b982d - clipboard: Add link to GdkDisplay in doc
Created attachment 273199 [details] [review] clipboard: Add get_default and get_primary Add constructor shortcuts for getting the CLIPBOARD and PRIMARY clipboards. This makes the api a bit easier to use. And also allows use of the clipboard api for bindings where GdkAtoms can not be used.
Review of attachment 273199 [details] [review]: ::: gtk/gtkclipboard.c @@ +408,3 @@ +/** + * gtk_clipboard_get_primary: + * @display: (allow-none): the #GdkDisplay for which the clipboard is to be Note: I also changed #GdkX11Display to #GdkDisplay here, since that was the argument type is.
Review of attachment 273199 [details] [review]: it looks good to me. if Matthias or Benjamin are also okay with it, I think it should land as soon as master is branched for 3.14.
Matthias / Benjamin: Any input now that we are branched?
Review of attachment 273199 [details] [review]: ::: gtk/gtkclipboard-x11.h @@ +28,3 @@ + +GDK_AVAILABLE_IN_3_14 +GtkClipboard *gtk_clipboard_get_primary (GdkDisplay *display); This should not be an API specific to X11 and as such should go into gtkclipboard.h as a regular function. The reason I'm very much not a fan of backend-specific APIs is that nobody is gonna test them. Everybody is developing against X where this clipboard exists and never returns NULL. And then when somebody runs this app against Wayland and gets a NULL pointer we get a crash. And nobody knows why because the developer on X cannot for the life of him reproduce it. Also, the name is very nondescriptive to anyone not knowing about X. I'm failing to find a name that I like a lot, but gtk_clipboard_get_selection_clipboard() or something? ::: gtk/gtkclipboard.c @@ +401,3 @@ + + if (display == NULL) + display = gdk_display_get_default (); Is it common that we accept NULL for the default display in GDK and GTK? I don't think it is? I think we should just not allow NULL here. @@ +429,3 @@ + display = gdk_display_get_default (); + else if (!GDK_IS_X11_DISPLAY (display)) + return NULL; These 2 lines should go away. gdk-win32 implements a (local) primary selection and we should keep supporting that.
(In reply to comment #19) > Review of attachment 273199 [details] [review]: > > ::: gtk/gtkclipboard-x11.h > @@ +28,3 @@ > + > +GDK_AVAILABLE_IN_3_14 > +GtkClipboard *gtk_clipboard_get_primary (GdkDisplay *display); > > This should not be an API specific to X11 and as such should go into > gtkclipboard.h as a regular function. I vehemently disagree. it'd be a mistake, and this is a good chance to get rid of the whole "let's have two clipboards" idiocy. > The reason I'm very much not a fan of backend-specific APIs is that nobody is > gonna test them. Everybody is developing against X where this clipboard exists > and never returns NULL. And then when somebody runs this app against Wayland > and gets a NULL pointer we get a crash. And nobody knows why because the > developer on X cannot for the life of him reproduce it. that argument is moot: Wayland does not have a PRIMARY, Windows does not have a PRIMARY, Broadway does not have a PRIMARY, and MacOS does not have a PRIMARY. if we want to ensure that GDK works the same way across backends, you'll notice that the X11 is the odd one out, thanks to historical baggage. GDK implements a GDK-only PRIMARY selection (which is pretty useless, since the whole point of the PRIMARY selection is to be shared across applications, not only across GDK-based ones), which means that *all* non-X11 backends will need to implement a local PRIMARY selection — or we'll need to make a local PRIMARY selection for everything, and then override it for X11 — whichever is more broken. the PRIMARY selection is an X11-only concept, which warrants an X11-only implementation and/or API. papering over it with GDK is a great case for introducing broken behaviour inside applications. people will assume selection-driven clipboard works on non-X11 just like it works on X11, and will open bugs complaining that it does not work as expected; or we'll get bugs from non-X11 users who get this weird behaviour they did not ask for.
As I mentioned to Jonas on IRC, the easiest and best would be to just bind the main clipboard, and leave it at that.
Created attachment 295446 [details] [review] clipboard: Add get_default() helper Add helper for getting the main clipboard. This makes the API usable for bindings (as GdkAtoms are usable through gobject-introspection), and easier to use in C.
Review of attachment 295446 [details] [review]: the commit message should say: "as GdkAtoms are not usable through gobject-introspection". looks generally good to me. ::: gtk/gtkclipboard.c @@ +389,3 @@ + * Return value: (transfer none): the default clipboard object. + * + * Since: 3.14 should be: Since: 3.16 @@ +396,3 @@ + g_return_val_if_fail (display == NULL || GDK_IS_DISPLAY (display), NULL); + + if (display == NULL) I think we wanted to drop the support to passing a NULL display. it's not hard to use gdk_display_get_default(). ::: gtk/gtkclipboard.h @@ +190,3 @@ GtkClipboard *gtk_clipboard_get (GdkAtom selection); +GDK_AVAILABLE_IN_3_14 GDK_AVAILABLE_IN_3_16
Created attachment 295453 [details] [review] clipboard: Add get_default() helper Add helper for getting the main clipboard. This makes the API usable for bindings (as GdkAtoms aren't usable through gobject-introspection), and easier to use in C.
Attachment 295453 [details] pushed as 1442299 - clipboard: Add get_default() helper