Hi Owen,
Sorry for the delay in getting back to you on this - been on a two week
holiday. Anyway, I have read through most of your thoughts and ideas on
this and they seem fine to me. Thanks very much for putting all that
effort into it!
I will download the code and see how it all works.
Damian
On Fri, 2001-10-19 at 10:26, Owen Taylor wrote:
>
> 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
>
>
>
--
Damian Ivereigh
CEPS Team Lead
http://wwwin-print.cisco.com
Desk: +61 2 8446 6344
Mob: +61 418 217 582
Attachment:
pgpKdPCjYwUlH.pgp
Description: PGP signature