Adventures in clipboard land

[Owen Taylor <otaylor redhat com>]

Hi Damian, 

I've been working on your patches:

 60854: Add a bunch of functions to detect if pasting is appropriate
 60793: Fix to entry popup menu
 60796: Fix to GtkTextView widget popup 

So, I started with your API:

 gboolean       gtk_clipboard_wait_is_text_available   (GtkClipboard *clipboard);
 gboolean       gtk_clipboard_wait_is_target_available (GtkClipboard *clipboard,
                                                       const GtkTargetEntry *tar gets,
                                                       gint                  ntargets);
 GtkTargetList *gtk_clipboard_wait_for_target_list     (GtkClipboard         *clipboard);

There are a couple of things here that aren't ideal:

 * Returning a 'GtkTargetList *' is awkward, because it's an opaque type,
   so you don't have access to the targets, and it has the flags field
   you don't need. So, it works better to have:

   gboolean gtk_clipboard_wait_for_targets (GtkClipboard *clipboard,
                        		    GdkAtom     **targets,
					    gint         *n_targets);

 * There is no async API; APIs that call the main loop are 
   dangerous, and I don't like forcing people to use them. 
   It seems natural to add:

   typedef void (* GtkClipboardTargetsReceivedFunc) (GtkClipboard     *clipboard,
	  					     GdkAtom          *targets,
						     gint              n_targets,
						     gpointer          data);
   void gtk_clipboard_request_targets  (GtkClipboard                    *clipboard,
				        GtkClipboardTargetsReceivedFunc  callback,
				        gpointer                         user_data);

 * I'm don't think gtk_clipboard_wait_is_target_available() is actually all
   that useful; if you just return an array of GdkAtom, it's about as easy
   to iterate through that array as to construct a GtkTargetEntry * array
   statically, and a lot easier to iterate through if you are doing something
   dynamic.

   So, I experimentally removed this function.

I then went on to the GtkEntry and GtkTextView patches. In the original
form, there was one of the typical "recursive mainloops are dangerous"
pitfalls:

  can_paste = entry->editable && gtk_clipboard_wait_is_text_available (gtk_clipboard_get (GDK_NONE) );

  append_action_signal (entry, entry->popup_menu, _("Cut"), "cut_clipboard",
                        have_selection);
                        can_cut);

Note that when you call gtk_clipboard_wait_is_text_available() the entry could
have been destroyed, and entry->popup_menu would no longer exist.

So, you need to do something like move the calll to wait_is_text_available to
the very beginning of the function, before you create entry->popup_menu, and
get something like:

  g_object_ref (entry);

  /* Cut is OK if entry is editable and there is something selected */
  can_cut = entry->editable && (entry->current_pos != entry->selection_bound);
  /* Copy is OK if there is something selected */
  can_copy = (entry->current_pos != entry->selection_bound);
  /* Paste is OK if entry is editable and there something to be pasted */
  can_paste = entry->editable && gtk_clipboard_wait_is_text_available (gtk_clipboard_get (GDK_NONE));

  /* Entry could have been destroyed during wait_is_text_available() */
  if (!GTK_WIDGET_REALIZED (entry))
    {
      g_object_unref (entry);
      return;
    }
  
  if (entry->popup_menu)
     gtk_widget_destroy (entry->popup_menu);

  [...]
  
  g_object_unref (entry);

But this still makes me just a bit nervous, because we've suddenly add
recursing the main loop into GTK+'s event handling. In 99% of the cases,
this is going to be perfectly fine, but someday, someone is going to
try to pop up a menu by emitting the ::popup_menu action signal and 
introduce a impossible-to-find rare bug into their app.

If at all possible, I'd prefer if GTK+ contained no code that recursed
the main loop ... a function like gtk_clipboard_wait_is_text_available()
can be useful for applications, since an application has an exact idea
of when its functions will be called and when not, but GTK+ basically
has to expect anything will happen.

So, I then tried rewriting it to be fully async.

  typedef struct
  {
    GtkEntry *entry;
    gint button;
    guint time;
  } PopupInfo;

  static void
  popup_targets_received (GtkClipboard     *clipboard,
                          GdkAtom          *targets,
                          gint              n_targets,
                          gpointer          user_data)
  {
    PopupInfo *info = user_data;
    GtkEntry *entry = info->entry;

    if (GTK_WIDGET_REALIZED (entry))
      {
        gboolean clipboard_contains_text = FALSE;
        GtkWidget *menuitem;
        GtkWidget *submenu;

        if (targets)
          {
            if (gtk_selection_targets_include_text (targets, n_targets))
              clipboard_contains_text = TRUE;

            g_free (targets);
          }

        [ Really pop up the menu ]  
      } 

    g_object_unref (entry);
    g_free (info);
  }

  static void
  gtk_entry_do_popup (GtkEntry       *entry,
                      GdkEventButton *event)
  {
    PopupInfo *info = g_new (PopupInfo, 1);

    /* In order to know what entries we should make sensitive, we
     * ask for the current targets of the clipboard, and when
     * we get them, then we actually pop up the menu.
     */
    info->entry = g_object_ref (entry);
    g_signal_connect (entry, "destroy", G_CALLBACK (popup_destroy_func), info);

    if (event)
      {
        info->button = event->button;
        info->time = event->time;
      }
    else
      {
        info->button = 0;
        info->time = gtk_get_current_event_time ();
      }

    gtk_clipboard_request_targets (gtk_clipboard_get (GDK_NONE),
                                   popup_targets_received,
                                   info);
  }

As you can see, the logic here is basically the same as before, except
for the intermediate structure used to carry data to the callback.

An extra utility function is used here:

  gboolean gtk_selection_targets_include_text (GdkAtom *targets,
					       gint     n_targets);


To review, the GtkClipboard additions are then:

 typedef void (* GtkClipboardTargetsReceivedFunc) (GtkClipboard     *clipboard,
						   GdkAtom          *targets,
						   gint              n_targets,
						  gb pointer          data);

  void     gtk_clipboard_request_targets   (GtkClipboard                    *clipboard,
				            GtkClipboardTargetsReceivedFunc  callback,
				            gpointer                         user_data);
  gboolean gtk_clipboard_wait_for_targets  (GtkClipboard *clipboard,
					    GdkAtom     **targets,
					    gint         *n_targets);
  gboolean gtk_clipboard_wait_is_text_available (GtkClipboard         *clipboard);

Can this be reduced? One way of reducing things is to get rid of request_targets()
and wait_for_targets(), and instead have:

 gboolean gtk_selection_data_get_targets (GtkSelectionData  *selection_data,
	 				  GdkAtom          **targets,
					  gint              *n_atoms);

And use wait_for_contents(), request_contents(). If you make this change
in the above code, it gets a few lines longer; I'm not sure if it gets
more clear or less clear. You end up with:

  gtk_clipboard_request_contents (gtk_clipboard_get (GDK_NONE),
				  gdk_atom_intern ("TARGETS", FALSE),
				  popup_targets_received,
				  info);

Which is certainly more complex looking than:

  gtk_clipboard_request_targets (gtk_clipboard_get (GDK_NONE),
                                 popup_targets_received,
                                 info);

But, on the other hand, you have to know how to use
gtk_clipboard_request_contents() and GtkSelectionData if you are
actually going to _do_ anything with the target list you get back.

You could say this is a "give a man a fish" versus "teach a man to fish"
situation; on the other hand, you could say that you are just making
the applications programmer's life harder for no good reason.

The lazy way out would be to add everything, but I'd be curious if you
have opinions on not including request_targets()/wait_for_targets()
and just going with request_contents()/wait_for_contents().

I'll attach my current version of your patches to the bug report once
I get home.

Regards,
                                        Owen