Created attachment 61857 [details]
testcase as patch for testnotebookdnd.c

Carlos, can you look at that ?

sure! I'm on it

Created attachment 61912 [details] [review]
patch

with this patch the tabs follow the pointer properly, and all flickering is avoided.

A side effect of using event boxes as tab labels is that they get expose events independently of the notebook, so they overlap while reordering (both with other tab labels and with the tab extensions drawn by the notebook), due to the impossibility of imposing a drawing order, the only workaround I've seen is setting "visible-window" to FALSE. This is done already by gnome-terminal, ephy and gedit, so they wouldn't be affected.

Now my question is: should I try to avoid this somehow (I really don't know how) or should it just be considered another case where the event box could cause artifacts?

hmm, yes thats ugly.
I guess you could perhaps create an extra window and move that, that might
help. Check out how the treeview does header reordering...

Thanks for the tip, Matthias :), I'm working on it already, but it will require a completely different approach, right now I'm focusing on refactoring a bit the code (specially the gtk_notebook_pages_allocate beast), I think it will help to make the whole task easier...

In the meantime, I think it could be good to apply this patch, at least it will work for ephy, g-t, and gedit

Makes sense. Feel free to commit the bandaid fix

Turns out I already committed the fix in comment #4.

Created attachment 62951 [details] [review]
a patch

Summary of changes:
- gtk_notebook_pages_allocate() code has been splitted into gtk_notebook_tab_space(), gtk_notebook_calculate_shown_tabs() and gtk_notebook_calculate_tabs_allocation(), some of the code here has been improved/modified, but most of it is still the same. I think this has made the code much more hacker-friendly
- now the reordered tab is shown in an independent GdkWindow. The drawing code has been slightly modified to draw there when it's necessary
- the default DnD icon has been improved, it's now a copy of the new GdkWindow

I've been testing it with apps and testnotebookdnd and I can't appreciate regressions, and works fine with the testcase too

Created attachment 62956 [details] [review]
another patch

Oops, it seems I managed to delete more code than necessary in my last cleanup, this patch fixes DnD icon creation with tab_labels with their own GdkWindow (it seems to be broken too in the current code, sorry...). but AFAICT it won't manage to copy child GdkWindow contents (if tab_label has any), am I wrong? recursing through GdkWindow's seems too much for just creating a DnD icon...

I only had occasion to test this remotely so far, and it felt pretty unusable.
Sometimes the dragged tab stayed black, and the non-dragged tabs would only redraw 
with a big delay. Also, button release only stopped the drag with a big delay.
(admittedly, the current version does not feel much better when used remotely)

Regarding the copying of the window contents for a drag icon, have you considered
simply using the drag window as the drag icon, using gtk_drag_set_icon_widget ?
You'd have to turn it into a GtkWindow for that, of course.

Ugh, right...

I've been investigating using the tc trick that Federico posted [1] and found out that the main culprit seems to be the call to gdk_window_get_pointer() (and thus to XQueryPointer) in the ::motion-notify handler [2].

To improve this I've used the GdkEvent coordinates, but (I don't know if there's something wrong in my setup) with some latency enabled the event coordinates were extremely unreliable (i.e.: having strange values) not only in this case, treeview column reordering from testtreeview also behaved wrongly.

I've also improved gtk_notebook_pages_allocate() for not making superfluous calls to gtk_notebook_redraw_tabs(), which invalidates the tabs area.

I've still got to investigate gtk_drag_set_icon_widget(), but it seems that detaching completely the tab_label widget from the notebook to create an independent window will require more changes to the code, and it was quite pretty to draw the tab border :)

[1] http://primates.ximian.com/~federico/news-2006-03.html#24
[2] selection in GtkTextView and GtkIconView seem to be affected too

Created attachment 63100 [details] [review]
new patch, with some optimization

Created attachment 63870 [details] [review]
updated patch

Fixes a defect with expanded tabs that was introduced in the refactor, plus other slight code improvements.

I still seem to be able to reproduce the GdkEvent coordinates problem, haven't found any cause, though... It'd be great if the patch were tested in a real network environment

Hmm, trying the tc trick to simulate latency hangs my system.
So, I'll give it a remote test run from home tonight.
In principle, the patch looks ok, there is one problem that
is going to be hard to fix: if you position the notebook so that 
part of a  tab label is offscreen, and then start a drag, you get
garbage in the drag icon for the offscreen parts.

Note that that problem already exists with the current code, though, so
its not a regression. 

Hmm, I see what you mean now, both for the notebook tab reordering and
treeview column reordering, latency kills it.

Maybe It's a silly theory, but might the cause be a glitch in the drag_window position between the creation of the ::motion-notify event and the event dispatch?

I'm not sure if it's even possible, but can't think of a better cause...

Created attachment 64925 [details] [review]
new patch

Sorry for the delay handling this bug, here's a new patch with the following new features:

- uses gtk_drag_set_icon_widget, reparenting the tab label to a window, this fixes the "garbage in offscreen parts" case. Also fixes the issue raised in comment #10. Of course, the tab_label is reparented again to the notebook before the drag window is destroyed
- fixes here and there to make the notebook aware that it could not be the parent of tab_label.
- To avoid an annoying borderless drag icon, the expose handler is modified to draw the tab extension in the DnD window
- Other small code improvements

Big patch. It would be easier to review if the refactoring bits were broken out, but lets try anyway...

The behaviour seems good, ie no garbage anymore.

Some comments on style (I notice that you just copied existing
things...)

As a matter of principle, I always add a /* fall thru */
comment if a break is intentionally missing.

+    case GTK_POS_TOP:
+      y += page->allocation.height;
+    case GTK_POS_BOTTOM:
+      height -= page->allocation.height;
+      break;
+    case GTK_POS_LEFT:
+      x += page->allocation.width;
+    case GTK_POS_RIGHT:
+      width -= page->allocation.width;
+      break;


extraneous parens here:

+	    gap_x = (notebook->cur_page->allocation.x - widget->allocation.x - border_width);


missing space before (

+      gtk_paint_extension(widget->style, window,


missing spaces around comment text
+      /*unmap all non-visible tabs*/


With those changes, I think it should be fine to commit.

Created attachment 65059 [details] [review]
updated patch

Yeah, sorry about the big patch, it should have been definitely a 2 step process, but has helped a lot to make these changes (and will help too for future improvements)

gtk_notebook_pages_allocate() was splitted into subtasks, but almost all of its codebase is unchanged to avoid new bugs, I already found other similar style issues that were fixed, but it seems I didn't spot all the offenders...

The attached patch fixes the style issues you raised and other small ones found through grep and code reading

thanks, please commit

Thanks!

2006-05-09  Carlos Garnacho  <carlosg@gnome.org>

        * gtk/gtknotebook.c: create an independent GdkWindow to behave nicely
        during reordering with tab_labels with their own window, also use
        gtk_drag_set_icon_widget() for the DnD icon to avoid drawing garbage
        from offscreen regions. Closes bug #335707.
        (gtk_notebook_tab_space), (gtk_notebook_calculate_shown_tabs),
        (gtk_notebook_calculate_tabs_allocation): new functions,
        gtk_notebook_pages_allocate() functionality has been splitted into
        these.