Created attachment 161939 [details] [review]
tab groups v1

This patch works, but it has all the debug and it has some hackish things. I'm going to make a first review myself telling the suspicious parts.

Review of attachment 161939 [details] [review]:

I added some comments bellow. Also say that maybe we should split this out in another class once it is ready. i.e: GeditTabGroupManager

::: gedit/gedit-notebook.c
@@ +758,3 @@
+	   in the button as the notebook doesn't get if we don't. Probably
+	   the button doesn't allow to set the focus to it */
+	gtk_widget_grab_focus (GTK_WIDGET (notebook));

This is a bit hackish, but I think the close button can't take the focus so, better options?

::: gedit/gedit-window-private.h
@@ +126,3 @@
 	gboolean        removing_tabs : 1;
 	gboolean        dispose_has_run : 1;
+	guint           removing_notebook : 1;

We need this as when we close the notebook we are removing the tabs too, so as in the tab-removed handler we also remove the notebook if it is the last tab. Better suggestions?

::: gedit/gedit-window.c
@@ +77,3 @@
 enum
 {
+	NOTEBOOK_ADDED,

mainly added for when we implement the new documents panel.

@@ +737,3 @@
+	 * notebook?
+	 */
+	if (!GTK_IS_ACTION_GROUP (window->priv->action_group))

Somehow when we destroy the window it can happen that the action group and the uimanager are destroyed first. am I right with this?

@@ +1963,2 @@
+			g_object_set_data (G_OBJECT (action),
+					   DOCUMENT_MENU_DATA,

Needed because when we disconnect the activate signal we need to know which notebook is releated to the action.

@@ +1998,3 @@
+	current_notebook = window->priv->notebooks;
+
+	/* FIXME: this looks a bit hacky, though not sure of a better way */

Well, maybe this is not that hacky. Though a better suggestion to manage all this actions would be good.

@@ +3516,3 @@
 	GtkAction *action;
 
+	if (!GTK_IS_ACTION_GROUP (window->priv->action_group))

Here we are under the same reason of the window destroyed.

@@ +3639,3 @@
+	gint aux = 0;
+	
+	if (window->priv->notebooks != NULL)

debug crap

@@ +4232,3 @@
+
+		page_num = gtk_notebook_get_current_page (GTK_NOTEBOOK (container));
+		/* FIXME: refactor notebook_switch_page */

well, maybe at the end, it is not really needed to factor out this.

@@ +4243,3 @@
 {
 	g_signal_connect (notebook,
+			  "set-focus-child",

jesse proposed set-focus, but set-focus-child works ok for our case and with managing less code in the listener.

@@ +4847,3 @@
 
+	/* We just remove the tabs of the active_notebook as it is switched
+	   automatically in the tab_removed listener */

This can be confusing, better ideas?

@@ +5352,3 @@
+	g_signal_handlers_block_by_func (notebook, notebook_switch_page, window);
+
+	gedit_notebook_add_tab (GEDIT_NOTEBOOK (notebook),

add tab calls set_current_page that emits a notebook-switch signal. We are going to do this already when we switch the focus of the notebook.

@@ +5360,3 @@
+	g_signal_handlers_unblock_by_func (notebook, on_notebook_set_focus, window);
+
+	on_notebook_set_focus (GTK_CONTAINER (notebook), NULL, window);

maybe instead of this grab_focus (notebook) ?

@@ +5366,3 @@
+_gedit_window_remove_document_panel (GeditWindow *window)
+{
+	remove_notebook (window,

We need to check that there are tabs modified and launch the dialog. This should go in the commands.

Created attachment 161978 [details] [review]
tab groups v2

Fixed comment:
@@ +4847,3 @@

+    /* We just remove the tabs of the active_notebook as it is switched
+       automatically in the tab_removed listener */

This can be confusing, better ideas?


Also now when you close a notebook the close dialog is opened to tell you if there is some unsaved document.

Created attachment 162148 [details] [review]
tab groups v3

This one is a different approach. I added a GeditNotebookGroup that is the one that manages the notebooks stuff.

In my github I have the 2 approaches working:
notebook2 branch -> everything in GeditWindow
notebook3 branch -> this patch.

Created attachment 162389 [details] [review]
tab groups v4

I think this is pretty much ready. I still don't know if we should allow horizontal splitting and I'm not sure about the names for the menuitems.

Created attachment 162548 [details] [review]
tab groups v5

Rebase with master, and fixes a few things

Review of attachment 162548 [details] [review]:

I started reviewing from the bottom, here is a first batch of comments (along with the ones made on irc)

::: gedit/gedit-window.c
@@ +820,1 @@
 				  GeditTab    *tab)

I don't think this refactoring is needed: as far as I can see in all the places you call it, you use get_current_notebook, so you may as well simply call that function here

@@ +3519,3 @@
+		gtk_window_present (GTK_WINDOW (window));
+	}
+

Why is this needed? sounds something in the wrong place and would also break focus-stealing prevention

@@ +3870,2 @@
 	/* focus the document */
+	if (!visible)

why this change (it may well be correct, but seems unrelated to all the rest)

@@ +3903,3 @@
 					      "GeditWindowDocumentsPanel",
 					      _("Documents"),
+					      GTK_STOCK_FILE);*/

This part looks unfinished, at least let's mark it with TODO or something

@@ +4424,3 @@
+	                                                     create,
+	                                                     jump_to);
+

I don't think the multi notebook widget should know about location etc, it should be a "dumb" gtk widget which implements composited notebooks

@@ +4497,3 @@
 	g_return_val_if_fail (GEDIT_IS_WINDOW (window), NULL);
+
+	tabs = gedit_notebook_group_get_all_tabs (window->priv->notebook_group);

We may want a to add a foreach_tab function to the multinotebook or override container foreach so that it loops on the tabs

::: gedit/gedit-window.h
@@ +37,2 @@
 #include <gedit/gedit-tab.h>
+#include <gedit/gedit-notebook.h>

I am not fond of this change: I am not sure why notebook.h is even public, it should be an internal implementation detail. As far as I can see this is the only place where it is used in another public header

@@ +91,3 @@
+					 GeditNotebook *notebook);
+	void	 (* notebook_removed)   (GeditWindow   *window,
+					 GeditNotebook *notebook);

git grep did not turn up any place where these are used... as said before I'd like to keep notebooks as an internal detail

@@ +97,3 @@
+	void	 (* tab_removed)    	(GeditWindow   *window,
+					 GeditNotebook *notebook,
+					 GeditTab      *tab);

If we really need to spill out the implementation detail here (after all since we already talk about tab it's not much of a change) we can use a GtkNotebook pointer

Created attachment 162566 [details] [review]
tab groups v6

Renamed to multi-notebook, rebased in master, and fixed some of the comments.

Created attachment 162569 [details] [review]
tab groups v7

Fixed missing parts.

This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.