Created attachment 60395 [details] [review]
patch

I didn't try it yet, but here are some comments from reading the patch:

+#define SWITCH_TAB_TIMEOUT    1000

The timeout must somehow be tied to the settings we have for timeouts.
All timeouts should be configurable for a11y reasons.
 
   gint  mouse_y;
   gint  pressed_button;
   guint dnd_timer;
+  guint switch_tab_timer;
   GtkTargetList *source_targets;
   gboolean during_detach;
   gboolean has_scrolled;

Source ids should be stored in longs (affects dnd_timer too)

 
+static gboolean
+gtk_notebook_switch_tab_timeout (gpointer data)
+{
+  GtkNotebook *notebook;
+  GtkNotebookPrivate *priv;
+  GList *tab;
+  gint x, y;
+
+  notebook = GTK_NOTEBOOK (data);
+  priv = GTK_NOTEBOOK_GET_PRIVATE (notebook);
+
+  priv->switch_tab_timer = 0;
+  x = priv->mouse_x;
+  y = priv->mouse_y;
+
+  if ((tab = get_clicked_tab (notebook, x, y)) != NULL)

This would probably better be called get_tab_at_pos() or similar - 
no clicking involved here...

Created attachment 60408 [details] [review]
updated patch, fixes those issues

Created attachment 60409 [details] [review]
other patch, undoes guint/glong confusion

Hmm, after reviewing other uses of timeouts, I think we should just stay
with a fixed timeout and address them all at a later point. The initial-timeout
is really about button-repeat functionality and not the right thing here.

One thing still missing is autoscrolling when hovering over the arrows. 
Other than that, I think this is ready to commit.

Will this interfere with other drag dests the app itself sets on the notebook (Epiphany accepts URI drags there) ?

It shouldn't. We already make the whole notebook a drop target for notebook tabs,
and the entries in testnotebookdnd still work fine for dnd

It occurs to me we probably want this expand-on-hover behaviour in GtkExpander
as well. 

I also note that we have a fixed timeout (#define AUTO_EXPAND_TIMEOUT 500) 
for a similar purpose in GtkTreeView. Not sure if they should be the same, or
independent. 1 second felt a little bit too slow for me. We could add an
extra dose of crack, by allowing to press a second button to kill the timeout
and switch immediately. Not sure if that would be easy to implement, though,
since keys are grabbed by gtkdnd.c

I've been trying to reproduce ephy case in testnotebookdnd.c, it's mostly doable (just allowing the default "drag-motion" and "drag-drop" handlers execution), but tabs detaching gets broken in the next code snippet from gtkdnd.c (gtk_drag_selection_received() func):

if (site && site->target_list)
  {
    guint target_info;

    if (gtk_target_list_find (site->target_list, 
			      selection_data->target,
			      &target_info))
      {
        if (!(site->flags & GTK_DEST_DEFAULT_DROP) ||
	    selection_data->length >= 0)
	  g_signal_emit_by_name (drop_widget,
				 "drag_data_received",
				 context, info->drop_x, info->drop_y,
				 selection_data,
				 target_info, time);
      }
  }
else
  {
    g_signal_emit_by_name (drop_widget,
			   "drag_data_received",
			   context, info->drop_x, info->drop_y,
			   selection_data,
			   0, time);
  }

with this patch, GtkNotebook uses gtk_drag_dest_set with a NULL target list, thus making it pass through the else statement of the code snippet. When I add a target list, it passes through the if statement, and doesn't emit ::drag-data-received because it doesn't find the target "GTK_NOTEBOOK_TAB" (used by GtkNotebook) in the target list.

To fix this, couldn't a "magic" target be added that allows any target to match with it? this would also help to get rid of the check_source_is_notebook_tab() hack (in the patch)

Matthias, very good idea with adding this feature to GtkExpander, I can do that now that I'm on it :), regarding the generic timeout, I'd suggest a "gtk-timeout-expand" GtkSettings property, 500ms sounds like a good default

Oh, regarding autoscrolling, the GtkNotebook code relies quite strongly in keeping the current tab label visible, I guess that this should change to get a good autoscrolling effect (we could change cur_page for scrolling, but that feels weird), opinions?

Hmm, not quite sure I understand how the breakage happens ? I mean, the else 
branch (which is taken when site->target_list is NULL) emits drag_data_received
too, the only difference being 0 instead of target_info. And the info parameter
is not even used in the notebook handler of that signal.

I think hovering over the arrows during drag should trigger the same action that
clicking them does normally.

Sorry, I'll try to explain better the details:

- GtkNotebook code in the patch uses gtk_drag_dest_set() with a NULL target list, making the DnD code to always emit ::drag-data-received regardless of the source target (the else statement). The notebook code in the patch checks the source target in order to know what to do with it.

- when an app adds targets to a GtkNotebook like in the ephy case (I've used in my test gtk_drag_dest_add_text_targets()) the target list becomes != NULL (making it pass through the if statement), but doesn't contain the target "GTK_NOTEBOOK_TAB"

- when a tab is attached to a notebook, the source target is "GTK_NOTEBOOK_TAB", which doesn't exist in the dest target list, so ::drag-data-received is not emitted

to properly fix this, my suggestion is to add some kind of universal target (one that matches with any target when doing gtk_target_list_find()) instead of using a NULL target list, this way apps can add more targets safely, still allowing the proper execution of the default handlers

I don't think adding targets to someone elses drop target can ever work reliable.

But you are probably right, in order to prevent this from breaking the notebook
tab switching, a magic "application/x-dnd-anything" target would help. You have
to be a little careful to not mess up priorities of explicitly added targets 
when matching targets. (If the source offers foo and bar, and the target accepts
anything, bar and foo, you still want to use bar, not foo.)

Created attachment 60974 [details] [review]
updated patch

- adds scrolling support when hovering the arrows
- adds a "gtk-timeout-expand" GtkSettings property, with a 500ms default value
- adds an "application/x-dnd-anything" target", what feels the most like a hack to me is that I had to override both gtk_target_list_find() and gtk_drag_dest_find_target() behaviors to use that target if there wasn't a specific match

with this patch I've been able to add more targets without breaking GtkNotebook behavior

Created attachment 60975 [details]
modified testnotebookdnd.c to test the features

Owen, do you want to comment on the application/x-dnd-anything idea ?
You wrote most of the dnd code originally...

Another random idea could be adding a gtk_drag_dest_accept_all (GtkWidget*, gboolean), this way we don't break what ought to be a simple list search with special cases, nor add extra stuff to other GtkSelection uses (x-dnd-anything seems to be only convenient to DnD?), but maybe I'm on crack :)

Sounds like a better idea to me. 

*** Bug 327253 has been marked as a duplicate of this bug. ***

Created attachment 61240 [details] [review]
updated patch

this patch implements gtk_drag_dest_[sg]et_accept_all(), as far as I've tested, seems to work ok, apps that already use gtk_drag_dest_set() on a notebook break tab switching, but keep their behavior

why does it break tab switching if the app call drag_dest_set ? I thought the
whole purpose of the accept_all flag was to keep tab switching working even if
the app adds targets on the notebook ?

You're very right, I'll have to make this independent of the internal GtkDragDestSite struct (which gets destroyed in every gtk_drag_dest_set() call)

But things will still break if there are two gtk_drag_dest_set() calls for the same widget (i.e.: GtkNotebook calls it to set "GTK_NOTEBOOK_TAB" target, and EphyNotebook calls it again for its custom targets, so "GTK_NOTEBOOK_TAB" target is lost, and tab attaching with it)

So besides set_accept_all() and making it persistent across drag_dest_set() calls, I guess that apps should still try a more conservative approach to adding targets

I mentioned this to Matthias on IRC, though I didn't see his response --
the target list is just convenience API ... can you just directly
implement handlers for drag-motion and friends?

do you even get drag-motion signals if the dest does not accept the target ?

from what I've learned, it does the signal emission when you don't set the flag GTK_DEST_DEFAULT_MOTION, but if an app calls later gtk_drag_dest_set() with that flag set, we're back to square one.

So we probably need to special-case the accept_all flag to survive 
gtk_drag_dest_set(), which is slightly evil, and make sure that it does
not invalidate application expectiations when it sets or doesn't set
the various default dnd behaviours. 

Maybe the flag should be renamed to something like "give-me-drag-motion-no-matter-what", since that is what we are really after.

Created attachment 61507 [details] [review]
patch

This patch makes accept_all persistent across gtk_drag_dest_set() calls, also removes an unused var from gtkdnd.c (gtk_drag_get_cursor())

I've been testing things with this patch and seems to work quite nicely

Created attachment 61667 [details] [review]
alternative patch

Here is an alternative patch, which reduces the role of the new flag to overriding
the GTK_DEST_DEFAULT_MOTION flag. This seems to work just as well for the tab switching case, and the patch seems easier to understand to me. I'm not entirely happy with the name track_motion, better proposals welcome.

Hmm, can't think neither of a better name, what about gtk_drag_dest_[gs]et_motion_notification()?

I decided that track_motion is acceptable...

2006-03-22  Matthias Clasen  <mclasen@redhat.com>

	Improved DND support for GtkNotebook  (#332991, Carlos Garnacho)
	
	* gtk/gtk.symbols: 
	* gtk/gtkdnd.h: 
	* gtk/gtkdnd.c: Add a track_motion flag on GtkDragDest
	with getter and setter, for cases where the drag destination
	is interested in drag motion events independent of targets.

	* gtk/gtksettings.c (gtk_settings_class_init): Add a setting
	for the timeout used when expanding during DND.

	* gtk/gtknotebook.c: Use the track_motion flag to switch
	notebook tabs when hovering over tabs during DND.