After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 712752 - GtkClipboard: add _get_default and _get_primary
GtkClipboard: add _get_default and _get_primary
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: .General
3.11.x
Other Linux
: Normal enhancement
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks: 579312
 
 
Reported: 2013-11-20 16:55 UTC by Jonas Danielsson
Modified: 2015-01-27 17:12 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
clipboard: add get_default and get_primary (2.60 KB, patch)
2013-11-20 16:56 UTC, Jonas Danielsson
none Details | Review
clipboard: add get_default and get_primary (2.52 KB, patch)
2014-03-09 22:03 UTC, Jonas Danielsson
reviewed Details | Review
clipboard: Add get_default and get_primary (5.43 KB, patch)
2014-03-28 19:47 UTC, Jonas Danielsson
reviewed Details | Review
clipboard: Add link to GdkDisplay in doc (894 bytes, patch)
2014-03-28 19:47 UTC, Jonas Danielsson
committed Details | Review
clipboard: Add get_default and get_primary (5.58 KB, patch)
2014-03-28 21:49 UTC, Jonas Danielsson
needs-work Details | Review
clipboard: Add get_default() helper (2.57 KB, patch)
2015-01-26 13:09 UTC, Bastien Nocera
reviewed Details | Review
clipboard: Add get_default() helper (2.47 KB, patch)
2015-01-26 14:26 UTC, Bastien Nocera
committed Details | Review

Description Jonas Danielsson 2013-11-20 16:55:48 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
Comment 1 Jonas Danielsson 2013-11-20 16:56:33 UTC
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.
Comment 2 Jonas Danielsson 2014-03-09 22:03:08 UTC
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.
Comment 3 Emmanuele Bassi (:ebassi) 2014-03-27 14:34:22 UTC
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.
Comment 4 Matthias Clasen 2014-03-27 14:39:44 UTC
The one concern I have here is whether this api will change / move to gdk anyway.
Comment 5 Jonas Danielsson 2014-03-27 18:30:23 UTC
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.
Comment 6 Emmanuele Bassi (:ebassi) 2014-03-28 01:21:47 UTC
(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).
Comment 7 Emmanuele Bassi (:ebassi) 2014-03-28 01:23:34 UTC
(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.
Comment 8 Jonas Danielsson 2014-03-28 19:47:14 UTC
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.
Comment 9 Jonas Danielsson 2014-03-28 19:47:18 UTC
Created attachment 273194 [details] [review]
clipboard: Add link to GdkDisplay in doc
Comment 10 Jonas Danielsson 2014-03-28 19:49:42 UTC
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?
Comment 11 Emmanuele Bassi (:ebassi) 2014-03-28 20:34:09 UTC
(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.
Comment 12 Emmanuele Bassi (:ebassi) 2014-03-28 20:38:28 UTC
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.
Comment 13 Emmanuele Bassi (:ebassi) 2014-03-28 20:38:49 UTC
Review of attachment 273194 [details] [review]:

this looks good.
Comment 14 Jonas Danielsson 2014-03-28 21:49:15 UTC
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
Comment 15 Jonas Danielsson 2014-03-28 21:49:49 UTC
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.
Comment 16 Jonas Danielsson 2014-03-28 21:51:35 UTC
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.
Comment 17 Emmanuele Bassi (:ebassi) 2014-03-29 12:36:32 UTC
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.
Comment 18 Jonas Danielsson 2014-04-08 09:16:57 UTC
Matthias / Benjamin: Any input now that we are branched?
Comment 19 Benjamin Otte (Company) 2015-01-22 13:24:57 UTC
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.
Comment 20 Emmanuele Bassi (:ebassi) 2015-01-22 14:28:50 UTC
(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.
Comment 21 Bastien Nocera 2015-01-22 14:33:36 UTC
As I mentioned to Jonas on IRC, the easiest and best would be to just bind the main clipboard, and leave it at that.
Comment 22 Bastien Nocera 2015-01-26 13:09:20 UTC
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.
Comment 23 Emmanuele Bassi (:ebassi) 2015-01-26 14:17:50 UTC
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
Comment 24 Bastien Nocera 2015-01-26 14:26:35 UTC
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.
Comment 25 Bastien Nocera 2015-01-27 17:12:02 UTC
Attachment 295453 [details] pushed as 1442299 - clipboard: Add get_default() helper