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 73240 - GtkNotebook should have drag-and-drop support
GtkNotebook should have drag-and-drop support
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: GtkNotebook
2.8.x
Other All
: Normal enhancement
: ---
Assigned To: gtk-bugs
gtk-bugs
: 122223 326764 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2002-03-03 01:11 UTC by Havoc Pennington
Modified: 2011-02-04 16:10 UTC
See Also:
GNOME target: ---
GNOME version: Unversioned Enhancement


Attachments
preliminary version of a notebook with dragndrop support (5.24 KB, application/octet-stream)
2002-09-02 09:31 UTC, Christophe Fergeau
  Details
proposed patch, first draft (27.16 KB, patch)
2005-12-15 20:39 UTC, Carlos Garnacho
none Details | Review
proposed patch, second draft (37.26 KB, patch)
2006-01-09 14:44 UTC, Carlos Garnacho
none Details | Review
proposed patch, third draft (46.92 KB, patch)
2006-01-26 16:54 UTC, Carlos Garnacho
none Details | Review
tab detaching example, quite hackish (2.44 KB, text/x-csrc)
2006-01-28 20:31 UTC, Carlos Garnacho
  Details
testdetach.c (6.24 KB, text/x-csrc)
2006-02-02 19:38 UTC, Matthias Clasen
  Details
treat rejected drops on the background like root drops (1.19 KB, patch)
2006-02-03 17:19 UTC, Matthias Clasen
none Details | Review
patch, fourth draft (59.22 KB, patch)
2006-02-12 17:34 UTC, Carlos Garnacho
none Details | Review
test sample, a bit more complete and using the new API (3.15 KB, text/x-csrc)
2006-02-12 17:37 UTC, Carlos Garnacho
  Details
fifth draft (60.62 KB, patch)
2006-02-15 17:05 UTC, Carlos Garnacho
none Details | Review
test sample (3.30 KB, text/x-csrc)
2006-02-15 17:06 UTC, Carlos Garnacho
  Details
sixth try (60.46 KB, patch)
2006-02-15 19:33 UTC, Carlos Garnacho
none Details | Review
test with entries instead of labels, to test text dnd (3.34 KB, text/x-csrc)
2006-02-16 18:01 UTC, Matthias Clasen
  Details
another patch (65.26 KB, patch)
2006-02-17 16:40 UTC, Carlos Garnacho
none Details | Review
updated patch (66.43 KB, patch)
2006-02-18 17:53 UTC, Carlos Garnacho
none Details | Review
updated patch (77.45 KB, patch)
2006-02-23 01:38 UTC, Carlos Garnacho
none Details | Review

Description Havoc Pennington 2002-03-03 01:11:58 UTC
Notebook should support an API for dragging tabs between notebooks, and
reordering tabs in the notebook. This would be used for IDE-style MDI,
tabbed apps like browsers/terminals, etc.

The minimum support would be a tab_dropped signal, a way to enable DND, 
and a way to specify which notebooks are in a "tab exchange group" 
and can accept tabs from each other.
Comment 1 Christophe Fergeau 2002-09-02 09:29:48 UTC
Just to try to avoid duplicated work, I mention here I started to work
on something like that (I didn't read your bug report and came up with
mostly the same requirements).
I'd add to these requirements that it should be able to do "something"
when a tab is dropped outside of an app window (eg on the desktop).
I'd do that by letting the user specify a "create_new_notebook"
callback which is called when a tab is dropped outside of the app.
Alternatively, it might be possible to handle that with the
"tab_dropped" signal.
Concerning the control of DND, it should be possible to choose between
no dnd, dnd within notebook, dnd between several notebooks.
Atm the code I'm attaching is quite hacky (I intend to clean it up a
little), but it already is able to drag and drop tabs between
notebooks in a few apps (it works with gedit, will work with galeon
when some fixes are made on galeon side, but doesn't work yet with
profterm)
This code is also lacking the signal emission atm, and also the
various configuration possibility evoked by Havoc.

I'm attaching the code here, even if I feel I should keep it hidden
until I clean it a little :)
If you want me to post updates to this code here, please let me know
Comment 2 Christophe Fergeau 2002-09-02 09:31:29 UTC
Created attachment 10851 [details]
preliminary version of a notebook with dragndrop support
Comment 3 Sebastian Rittau 2003-07-03 17:54:17 UTC
I just want to point out that a common implementation of this is quite
interesting. This would e.g. help applications like Galeon that
implement this by themselves, would lead to a consistent handling of
this, ensure that a11y issues are taken care of, and would application
programmers to easily enable this. (Think of e.g. gnome-terminal.)
Comment 4 Matthias Clasen 2003-09-13 18:39:01 UTC
*** Bug 122223 has been marked as a duplicate of this bug. ***
Comment 5 Hakon 2003-10-28 15:43:59 UTC
What user interface design are you thinking of employing?
For example, in Gaim, you see an insertion pointer as you drag the
tab, though the subject tab stays until you drop it somewhere.
Whereas in Epiphany, the tab is moved in real time as you move it.
I vote for the Gaim way, because I think it's more clear. And with
Epiphany it's easy to mistakenly drop the subject tab into a new
window of its own. This happens if you may drop it beneath the tab
row. With Gaim you would notice such a mistake, because then the
insertion pointer wouldn't show.
Comment 6 Reinout van Schouwen 2005-04-15 13:28:00 UTC
I think the Epiphany method is better in principle, since it provides more
direct feedback. However: it would be even better and easier if the tabs would
slide smoothly from position when dragging or inserting them, like for instance
Epiphany toolbar items do. The instantaneous switching of position makes it
harder to track with the human eye where the change is happening.
Comment 7 Reinout van Schouwen 2005-12-09 23:09:21 UTC
Still valid for latest GTK.
Comment 8 Carlos Garnacho 2005-12-15 20:39:47 UTC
Created attachment 56045 [details] [review]
proposed patch, first draft

This patch implements drag and drop in the same GtkNotebook, it adds the
following API:

- a "reorderable" property
- gtk_notebook_[sg]et_reorderable()

a "tabs-reordered" signal when the tabs positions change would be desirable,
but still not implemented.

Regarding the dropping outside (or inside other tabs), it would be nice to add
a "detachable" property, but that's quite harder to do, and maybe (IMHO)
outside the scope of a supposedly simple notebook widget
Comment 9 Carlos Garnacho 2005-12-15 20:41:18 UTC
err, the last paragraph was supposed to say "... dropping outside the notebook
(or inside other notebooks)..."

sorry for the spam
Comment 10 Matthias Clasen 2005-12-16 00:56:08 UTC
some comments from looking over the patch (i'll try it later tonight)

- i think a simple approach to detaching might be to just emit a signal with the 
ppointer and page number when the drop happens outside the notebook

- is there any visual feedback during the drag ? (i guess i should just try it)
possible approaches to feedback would be just a drag icon (firefox does it like 
this), or something like the column drag in gtktreeview or evolutions table 
widget.

- we need this to be accessible with the keyboard. its probably worth looking at 
the key combinations used for column reordering in gtktreeview
Comment 11 Matthias Clasen 2005-12-16 04:33:40 UTC
ok, I tried it now. Works relatively nicely, with visual feedback similar to the
one in GtkTreeView. One additional idea to consider: should reorderable be a
child property of the tabs, like it is for treeview columns ?
Comment 12 Christian Persch 2005-12-16 11:13:39 UTC
I didn't see a way to get notified about the tab reordering; IMHO there should
be a "tabs-reordered" signal emitted when this happens.
Comment 13 Carlos Garnacho 2005-12-16 12:08:38 UTC
Matthias:

- You're right regarding using a child property, just for consistency sake
- I'll have a look too at the keybindings issue
- Regardless of having a signal for dropping outside the notebook, maybe should
we add another child property to enable "detachability"? without it enabled the
notebook should be permissive with the drop point (i.e: pretty much like the
Gtk[VH]Scale). This patch already implements such behavior.

Chpe:
In my first comment I told that it's still unimplemented :)

I'll try to work on it all this weekend
Comment 14 Matthias Clasen 2005-12-16 16:43:02 UTC
Ok, the first impression from other people in the office is that they like the
general visual feedback. Of course, allowing detaching by dragging is a bit
harder with this kind of feedback, because you have to allow the "shake loose"
the tab label somehow.

There is a little visual defect when dragging a label all the way to the edge in
non-scrolled mode.
Comment 15 Carlos Garnacho 2006-01-09 14:44:25 UTC
Created attachment 57034 [details] [review]
proposed patch, second draft

Matthias, I've tried to detect the visual defect you talked about, but can't see anything... which kind of visual defect are we talking about? :/

Other issues about this patch:
- Implements the "reorderable" and "detachable" child properties.
- Uses the same keybindings than GtkTreeView for column reordering
- Implements the "tab_reordered" and "tab_detached" signals
- In these signals, unlike in "switch_page", the second argument is the child widget, not the GtkNotebookPage containing it
- Leaves only one padding pointer in GtkNotebookClass

Any comment is welcome
Comment 16 Christian Persch 2006-01-09 18:52:36 UTC
I've tried this patch, and have a few comments:
- it's a bit weird that 'reorderable' is a child property for each tab instead of global, esp. since if you for example have tab 0 reorderable, tab 1 non-reorderable you can still drag tab 0 after tab 1 and have in effect changed the position of tab 1! Also I just don't see any use cases for some-tabs-reorderable-some-not ?
- if there's room left to the right of the tabs, you can drag the tab all the way to the end of the notebook, but releasing will just make it the last tab. IMHO it shouldn't follow the mouse beyond the last tab position.
- do you plan to make drag-to-another-notebook possible for detachable tabs?
Comment 17 Matthias Clasen 2006-01-09 19:40:43 UTC
Christian, its the same with reorderable treeview columns. I agree that it is not very useful. It does have some effect though: you can't start a drag on a column header thats not reorderable, and you can't change the relative positioning of two non-reorderable columns wrt to each other (though you can drop stuff in between).

Maybe you are right though, there is no use case, and consistency with the tree view is not very important here.

Carlos, the visual defect we saw was that if you can drag a tab label all the way to the left or right, part of it vanishes outside the area of the notebook.

I'll try your second patch now...
Comment 18 Matthias Clasen 2006-01-09 20:20:53 UTC
Hmm, so the detachable thing seems totally undiscoverable. We need to add some (large) threshold distance and visually detach the tab and make it jump to the mouse to make it clear that the tab is detached. 

I still the the "visual defect" I tried to explain in the last comment.
Comment 19 Matthias Clasen 2006-01-12 21:17:47 UTC
*** Bug 326764 has been marked as a duplicate of this bug. ***
Comment 20 Got 2006-01-21 13:54:17 UTC
Just something about the cursor mutation when drag n dropping. (I see it in Gajim where MOVE icon is used for dnd tabs) It show a "move file" icon, it may be confusing about what it really do when people see that. I see a "closed hand" icon (as if we catch something) used in Epiphany when drag n drop tabs that show better what we do (catch tab, move it, and let it fall where we decide).
Comment 21 Carlos Garnacho 2006-01-26 16:54:10 UTC
Created attachment 58161 [details] [review]
proposed patch, third draft

This patch implements visual DnD when detaching tabs, handles quite better the drag threshold and implements Christian's suggestion about not moving tabs past the last position when reordering
Comment 22 Carlos Garnacho 2006-01-26 16:59:26 UTC
Oh, and also found a possible cause for the visual defect, anyway, limiting the tabs movement should have minimized it a lot, if not removed.
Comment 23 Matthias Clasen 2006-01-26 18:59:51 UTC
I think this really looks and feels great now.

The one thing it is missing now is a way to undo the tearoff without ending the drag. Ie I want the tab to snap back in the notebook when I get close to the tab row. 
Comment 24 Matthias Clasen 2006-01-27 02:19:20 UTC
Carlos, one other thing that would be great to have is a testcase demonstrating dnd between notebooks and/or to the desktop.
Comment 25 Carlos Garnacho 2006-01-28 20:28:18 UTC
Thinking a bit more about how detaching is currently implemented, maybe it's so simple that will require gross hacks on the api users' side (and wouldn't even be suitable for DnD between several processes, nor for detecting the tab position below the pointer). To improve this situation, something like the following could be implemented:

* Functions to define a notebook group:

void   gtk_notebook_set_group_name (GtkNotebook*, gchar*);
gchar *gtk_notebook_get_group_name (GtkNotebook*);

GtkNotebook would use these groups names to know which tabs may interchange tabs

* a boolean "tab-detached" signal:

gboolean (* tab_detached) (GtkNotebook *notebook,
                           GtkWidget   *child,
                           guint        page_num);

if the signal handler returns TRUE, the GtkNotebook DnD code will automatically remove the tab from the source notebook and put it in the dest one, if it returns FALSE nothing would happen.

This approach would only work with drags within the same application, but would offer a way better API than the current, and handling multiprocess DnD would be really painful anyway. What do you think?

Regarding your suggestion about snapping back the tab, I've tried it doing gtk_drag_finish() during _drag_motion()... but doing so leaves the pointer grabbed, I'll try to have a deeper look at the gtkdnd.c code to see if it can be fixed cleanly.

Dropping to the desktop still requires other modifications to gtkdnd.c, I think I've identified the code piece Owen talked about in IRC, but fixing this might require another signal hook for knowing that a drop has happened in a dest widget that doesn't accept such targets, please correct me if I'm wrong
Comment 26 Carlos Garnacho 2006-01-28 20:31:10 UTC
Created attachment 58289 [details]
tab detaching example, quite hackish
Comment 27 Matthias Clasen 2006-02-02 19:38:28 UTC
Created attachment 58603 [details]
testdetach.c

Here is a slightly improved version of the detach example, stealing some code from testgtk to detect the widget under the pointer, and carry child properties over, so that you can detach tabs more than once.
Comment 28 Matthias Clasen 2006-02-02 21:35:23 UTC
Re #25, I think you are right about dropping on non-accepting targets.
GTK+ has support for dropping on the root window (I just fixed it to work again).
That is handled in part by gdk, by setting a special protocol, which is then handled in gtk_drag_drop(). Doing something similar for drops on non-accepting targets could be done in two ways:

1) add a signal like GtkWidget::no-drop 
2) using another special target like application/x-no-drop

in either case, the code in gtk_drag_button_release_cb() will have to be changed
to call gtk_drag_drop even if action is 0, and gtk_drag_drop needs to do the right thing if the dest_window is non-NULL but action is 0.

I haven't investigated the problems with reattaching yet.
Comment 29 Matthias Clasen 2006-02-03 00:02:16 UTC
another alternative, maybe more elegant, would be to simply treat nonaccepted
drops on rhe desktop background window like root window drops.
Comment 30 Matthias Clasen 2006-02-03 17:19:18 UTC
Created attachment 58660 [details] [review]
treat rejected drops on the background like root drops
Comment 31 Matthias Clasen 2006-02-03 17:29:59 UTC
With this patch, you should be able to detect drops of notebook tabs on the desktop background, by registering the "application/x-rootwindow-drop" target.
You can look at testdnd.c for an example how to do this.
Comment 32 Matthias Clasen 2006-02-03 17:43:30 UTC
Re #25: I think inter-app dnd is completely irrelevant for this. 
I think the proposed higher level api is a good idea. I think there
should also be a corresponding tab-attached signal. On open question 
is how you tell the tab_detached handler wether you are going to reattach the
tab to another notebook or drop it on the floor (for root window drops)
Comment 33 Carlos Garnacho 2006-02-12 17:34:34 UTC
Created attachment 59195 [details] [review]
patch, fourth draft
Comment 34 Carlos Garnacho 2006-02-12 17:35:42 UTC
Issues in the last patch:

- GtkNotebook runs out of signal slots
- GtkNotebook hasn't any pointer to private data, we should probably replace some private pointer in the GtkNotebook struct with gpointer priv, maybe GList *first_tab is a good candidate
- Regarding root window dropping, I've added gtk_notebook_set_window_creation_function(), which specifies a function to create the window that contains the dest notebook (applications might want to create custom ones, with menus, toolbars, etc...), such function has to be responsible of doing window moving/resizing, and creating the notebook. Perhaps this is becoming too awkward for a little win?

this is the window creation API:

typedef GtkNotebook* (*GtkNotebookWindowCreationFunc) (gint     x,
						       gint     y,
						       gpointer data);
void gtk_notebook_set_window_creation_function (GtkNotebook                   *notebook,
						GtkNotebookWindowCreationFunc  func,
						gpointer                       data,
						GDestroyNotify                 destroy);
Comment 35 Carlos Garnacho 2006-02-12 17:37:21 UTC
Created attachment 59196 [details]
test sample, a bit more complete and using the new API
Comment 36 Matthias Clasen 2006-02-13 07:00:51 UTC
Nice, in general. Some comments:

- None of the fields in GtkNotebook are explicitly marked as private,
  thus I would not want to replace any with a private pointer. I don't
  think this is too terrible. 

- Regarding exhausting the space in the class, note that signals don't 
  absolutely need to have a slot in the class, unless you want to have
  a non-NULL class handler.

- Would it not enough to have a global hook for window creation, and pass
  the source notebook to it ? like

typedef GtkNoteBook* (*GtkNotebookWindowCreationFunc) (GtkNotebook *source,
                                                       GtkWidget   *page,
                                                       gint         x,
                                                       gint         y,
                                                       gpointer     data);
void gtk_nodebook_set_window_creation_hook (GtkNotebookWindowCreationFunc func,
                                            gpointer data,
                                            GDestroyNotify destroy);

Passing the source notebook as a parameter allows you to avoid stuffing the
group id in via the userdata. I think this function should be allowed to
return NULL, causing the drop to fail.

One thing I'm worried about is that making the whole notebook a dnd target will
interfere with dnd targets in the content. This needs testing, e.g. does dropping text on an entry in a dnd enabled notebook still work ?

Comment 37 Matthias Clasen 2006-02-13 07:04:52 UTC
There are some minor todos left in the actual dnd handling.
- reattaching was mentioned already (but I think we can handle it 
  in a second step after getting basic dnd support in, if needs be)
- when switching from sliding to dnd, the dragged tab jumps back to
  its original position. It should snap to the closest position near
  to its current sliding position
- when dropping a tab onto a notebook, it jumps to the last position.
  thats fine when dropping on the content, but when dropping in the
  tab row, it should go to the position closest to where it was dropped.
Comment 38 Matthias Clasen 2006-02-13 07:06:46 UTC
Oh, and destroy needs to set pointers to NULL after freeing them, since
it may be called more than once (got warnings due to that).
Comment 39 Carlos Garnacho 2006-02-15 17:02:23 UTC
Re #36:

- I've removed tab_detached, tab_attached and tab_reordered from GtkNotebookClass declaration, so this still leaves some empty slots.
- Good idea with the global hook, the next patch implements it.
- As far as I've tested, notebook DnD doesn't interfere with contained widgets DnD, but it feels a little strange to have "holes" where notebook DnD doesn't work (i.e.: inside GtkEntries), maybe we shouldn't allow notebook DnD outside tabs area.

Re #37:
The only issue left is the first one, as far as I could investigate, gtk_drag_dest_motion() in gtkdnd.c should need a way to know that the drag has been finished to ungrab pointer and so, but couldn't see a way to get such information.

Re #38: oops, fixed :)
Comment 40 Carlos Garnacho 2006-02-15 17:05:55 UTC
Created attachment 59414 [details] [review]
fifth draft
Comment 41 Carlos Garnacho 2006-02-15 17:06:51 UTC
Created attachment 59415 [details]
test sample
Comment 42 Carlos Garnacho 2006-02-15 19:33:28 UTC
Created attachment 59424 [details] [review]
sixth try

Oops, I saw a case where it would emit spurious "tab-reordered" signals, it's now fixed
Comment 43 Matthias Clasen 2006-02-16 18:01:49 UTC
Created attachment 59512 [details]
test with entries instead of labels, to test text dnd
Comment 44 Matthias Clasen 2006-02-16 19:27:32 UTC
I think the current patch is basically ready to be committed. 
Some small things to clean up before committing:

- Add "Since: 2.10" to all doc comments for new api.
- Add a comment explaining the commented out code block in
  gtk_notebook_drag_motion ()
- It should be documented that window_creation_hook may return 
  NULL. One small problem with that is that you emit a 
  tab-detached signal before calling window_creation_hook, so
  if you get NULL, you probably have to emit tab-attached on 
  the original notebook, since the page does not actually
  move somewhere else.
- The docs for gtk_notebook_set_group_id should probably
  explain what -1 means: "drop everywhere", or "no dnd". 
- The setters for group-id, reorderable and detachable
  code should probably check if the value actually changed
  before sending notify (although we don't promise this,
  we normally do it for integer properties).

Comment 45 Matthias Clasen 2006-02-16 19:34:11 UTC
Oh, one more thing to do before committing this is to write a little
mail to gtk-devel explaining the new api, so people get a chance
to comment on it. Will you do that, or should I ?
Comment 46 Christian Persch 2006-02-16 19:44:13 UTC
Looking at the tests between the earlier attachment 58603 [details] and the newest attachment 59512 [details], I see that the first moved the tab from old to new notebook itself, while the new one doesn't do that... so I assume that gtknotebook DND code does that for you now with the latest patch? Is there still a way to do this manually? That'll be required to port Epiphany to this (we create special tab label widgets for each tab before inserting it into the notebook).
Comment 47 Carlos Garnacho 2006-02-17 00:20:48 UTC
Matthias:
I'm already writing the mail, will handle the other issues tomorrow morning.

Christian:
In the current code GtkNotebook moves both the content and the tab label widget from one tab to another (assuming they're both in the same group), can't the tab labels be reused this way when moving tabs in ephy?
Comment 48 Matthias Clasen 2006-02-17 15:46:22 UTC
chpe, you currently have two signals, tab-detached and tab-attached which are 
emitted before/after the actual transfer happens. Is that not enough for you 
to modify the tab labels as required ?
Comment 49 Carlos Garnacho 2006-02-17 16:40:51 UTC
Created attachment 59585 [details] [review]
another patch

The commented code block has been removed, it was merely an sketch of what should do when snapping back the tab to the source notebook, GtkNotebookWindowCreationFunc has been documented, the rest of the issues have been fixed. Besides that, the reorder_tab() function has been slightly improved to avoid unnecessary reordering under some side cases (tabs with alternate packing)
Comment 50 Matthias Clasen 2006-02-17 17:02:36 UTC
One small thing: treeview header reordering supports
Alt-Home and Alt-End to go to either end. We should probably do
that here, too.
Comment 51 Matthias Clasen 2006-02-17 17:33:06 UTC
Also, with the DND keynav change I just submitted, you could 
theoretically let Alt-Up/Down start a drag (and suitable other
arrows for rotated notebooks). Not sure its a good idea, but it 
should give some keynavigatable way to detach tabs and move them
to another notebook. 

A better ui for that may be a dialog listing the notebooks that you 
could transfer the tab to, popped up from a tab context menu. I'm not 
sure if such a dialog should be proved by the notebook, or if each 
app should do its own. If apps were to do that on their own, they
probably need api to list all notebooks with a given group id.
If the notebook were to provide it, it needs some way to refer to 
the notebooks, like a "title" or something.

(even cooler, but way more complicated would be 
Alt-Tab style cycling between all candidate notebooks)
Comment 52 Carlos Garnacho 2006-02-17 20:40:35 UTC
You're right with the alt+[home|end], I'll work on this.

Regarding doing detaching keynavigable, with the current API there's no way to know which (nor how many) notebooks are in the same group, that could require something a la GtkSizeGroup.

In my humble opinion, most consumers of this new API (I'm mostly thinking of ephy, gnome-terminal and gedit) will only need one group for their tabs, and will know much better than gtk itself the candidates for DnD (at least with the current API), so maybe a GtkNotebookGroup class is way too much for their needs? can they manage this task themselves better than gtk could?

Another smallish issue is that Alt+[Up|Down] is already used in notebooks with tabs to the right or left, so alternating too much the keybindings could be confusing...

But that's just an opinion, I'll be glad to work on it if you still feel that it's worth the effort :).
Comment 53 Matthias Clasen 2006-02-17 22:53:22 UTC
Yes, I would not worry about the group thing for now. And the Alt-Up/Down
is something that we can try out after landing the dnd support. Its a 
5 line patch on top of your work, so it'll be easy to add afterwards.

The only thing that currently holds off committing this is getting
a comment from chpe on the sufficience of this for epiphany.
Comment 54 Carlos Garnacho 2006-02-18 17:53:07 UTC
Created attachment 59655 [details] [review]
updated patch

It adds support for Alt+[Home|End] keybindings
Comment 55 Christian Persch 2006-02-20 13:48:21 UTC
I've looked over epiphany's notebook code again to see how this will fit with it.

Epiphany needs:
- a wrapper around gtk_notebook_insert_page, that 
  * builds a custom tab label widget for the tab, and
  * emits a tab-added signal after the tab has been inserted, and
- a wrapper around gtk_notebook_remove_page which emits a tab-removed signal after the tab has been removed.

The tab labels that we build are not reusable in a different notebook, because
- older epiphany versions connected a signal to the notebook with tab as user_data (current version doesn't anymore, but I think it should still be supported)
- sets tooltip on the tab label in a GtkTooltips object that belongs to the notebook.


I've looked at the patch here, and have a few problems with it:
- the tab_attached signal works like my tab-added signal, but is only emitted on newly attached-by-DND tabs.
- same for the tab_detached signal wrt. my tab-removed signal
- the comment for tab_detached says
+   * A handler for the ::tab-detached signal should do any aditional
+   * cleanup needed for detaching the tab and then return %FALSE to
+   * allow tab detaching, or return %TRUE to deny it.
  so what happens if one handler does that 'cleanup', and then a later handler (added from an extension in epiphany, for example) decides to veto the detach?
- the notebook does the moving from one notebook to another one on DND itself, reusing the tab label, breaking everything epiphany needs to do here (tab-removed signal AFTER removal, and using a new tab label).
- the tab_reorderer signal is fine

So how can we fix this?
- we could make tab-attached into tab-added
- split tab-detached in tab-detach-request (return TRUE to deny) and tab-removed after the removal has been done

For building the tab label I could see two ways:
- make gtk_notebook_insert/remove_page call class vfuncs where you could build the tab label and ignore the passed-in widget
or
- add a GtkWidget * (*GtkNotebookBuildTabLabel)(GtkNotebook*nb, GtkWidget*tab, gpointer userdata) function with gtk_notebook_set_tab_label_creation_hook that builds a tab label for a given tab in the notebook, and use this function when DND moves the tab to another notebook, falling back to the existing label in the origin notebook when this func isn't set?
Comment 56 Matthias Clasen 2006-02-20 15:45:42 UTC
I think these are good ideas

- replace ::tab-attached and ::tab-detached by
  ::tab-added which gets called after any page addition, and
  ::tab-removed which gets called after any page removal

- add vfuncs for insert/remove, with default implementations
  that add/remove the page and emit ::tab-added/::tab-removed.
  
- is a separate ::tab-detach signal (which gets emitted before 
  detaching and can veto it) actually necessary ? Is there
  any use case for dynamically determining detachability, instead
  of just setting the detachable child property ?









Comment 57 Christian Persch 2006-02-20 16:05:04 UTC
I don't think the ::tab-detach signal is necessary. Epiphany has it to create the new window + move the tab, but this patch deals with that differently, which looks fine to me (window creation func).
The only real reason to have a veto-able detach signal would be to check the target notebook, but the signal doesn't carry that, and the group ID thing works just as well, I think.
Comment 58 Carlos Garnacho 2006-02-23 01:38:03 UTC
Created attachment 59969 [details] [review]
updated patch

- replaces ::tab-detached and ::tab-attached with ::tab-added and ::tab-removed
- implements insert_page() vfunc, implementation (gtk_notebook_real_insert_page ()) mostly comes from the old gtk_notebook_insert_page_menu() code
- instead of implementing remove_page() vfunc, I've made gtk_notebook_remove_page to use gtk_container_remove(), which actually removes the page and emits ::tab-removed
Comment 59 Carlos Garnacho 2006-02-23 01:47:43 UTC
Forgot to mention that tabs detaching is no longer vetoable, and that a GktNotebookWindowCreationFunc handler returning NULL will no longer emit any ::tab-attached/tab-detached signal equivalents
Comment 60 Matthias Clasen 2006-02-23 14:31:31 UTC
Looks very good to me. 
I think we should get this in cvs now, so that ephy and others can
easier try it out, and work on any remaining problems in cvs.

Very small nitpick:

  if (class->insert_page)
     return (class->insert_page) (notebook, child, tab_label, menu_label, position);

the if is pointless if you know that there is an implementation (since
GtkNotebook has one itself)
Comment 61 Matthias Clasen 2006-02-23 18:00:23 UTC
2006-02-23  Matthias Clasen  <mclasen@redhat.com>

	* gtk/gtknotebook.h: Add a reorder_tab keynav signal and an
	insert_page vfunc to GtkNotebook.

	* gtk/gtk.symbols:
	* gtk/gtknotebook.c: Support notebook DND. New API includes
	gtk_notebook_set_window_creation_hook, 
	gtk_notebook_[gs]et_group_id,
	gtk_notebook_[gs]et_tab_reorderable, 
	gtk_notebook_[gs]et_tab_detachable (#73240, Carlos Garnacho)
Comment 62 Thomas D Ahle 2008-03-26 16:22:20 UTC
I have a need to manually decide whether a tab should be accepted. Is this possible with the current API?

Even if I connect to drag-drop and drag-data-received and try to stop them with drag_context.finish, the tab gets moved.