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 128184 - Add "undo tab close" to Web
Add "undo tab close" to Web
Status: RESOLVED FIXED
Product: epiphany
Classification: Core
Component: Tabs
git master
Other Linux
: Normal enhancement
: ---
Assigned To: Epiphany Maintainers
Marco Pesenti Gritti
: 171244 363592 469668 603978 675195 678412 (view as bug list)
Depends on: 693297
Blocks: 693349
 
 
Reported: 2003-11-29 20:08 UTC by Keith Lea
Modified: 2013-02-07 22:06 UTC
See Also:
GNOME target: ---
GNOME version: 3.7/3.8


Attachments
adds a close-confirm dialog when there are multiple tabs in the window (9.01 KB, patch)
2007-08-01 15:39 UTC, Cosimo Cecchi
needs-work Details | Review
tab restore patch and extension (4.02 KB, application/x-compressed-tar)
2007-08-15 18:27 UTC, Michael Opitz
  Details
ephy-window: implement undo-close-tab WIP (3.43 KB, patch)
2012-12-06 21:15 UTC, Diego Escalante Urrelo (not reading bugmail)
none Details | Review
ephy-window: implement undo-close-tab WIP (4.68 KB, patch)
2012-12-07 19:34 UTC, Diego Escalante Urrelo (not reading bugmail)
none Details | Review
ephy-window: implement undo-close-tab WIP (7.28 KB, patch)
2012-12-07 23:06 UTC, Diego Escalante Urrelo (not reading bugmail)
none Details | Review
ephy-shell: Make it possible to prepend a tab in a window (1.93 KB, patch)
2013-01-28 14:52 UTC, Claudio Saavedra
committed Details | Review
ephy-session: add API to restore closed tabs (8.05 KB, patch)
2013-01-28 14:53 UTC, Claudio Saavedra
reviewed Details | Review
ephy-session: add can-undo-tab-closed boolean property (3.17 KB, patch)
2013-01-28 14:53 UTC, Claudio Saavedra
committed Details | Review
ephy-window: add menu item to allow undoing tab closure (3.57 KB, patch)
2013-01-28 14:53 UTC, Claudio Saavedra
none Details | Review
ephy-session: add API to restore closed tabs (8.92 KB, patch)
2013-01-28 18:10 UTC, Claudio Saavedra
needs-work Details | Review
ephy-embed-shell: add EPHY_EMBED_SHELL_MODE_INCOGNITO (4.93 KB, patch)
2013-01-28 20:18 UTC, Claudio Saavedra
committed Details | Review
ephy-shell: add application menu item for reopening closed tabs (4.26 KB, patch)
2013-01-28 20:18 UTC, Claudio Saavedra
committed Details | Review
ephy-session: add API to restore closed tabs (9.00 KB, patch)
2013-02-06 13:37 UTC, Claudio Saavedra
none Details | Review
This version of the patch uses the EphyNotebook instead of the EphyWindow (8.92 KB, patch)
2013-02-06 21:16 UTC, Claudio Saavedra
committed Details | Review
ephy-session-test: add tests for tab restoring API (3.63 KB, patch)
2013-02-07 08:26 UTC, Claudio Saavedra
committed Details | Review

Description Keith Lea 2003-11-29 20:08:39 UTC
Sometimes I close a tab and then I realize I still wanted it open. I think
I should be able to go to Edit->Undo to bring it back.
Comment 1 Christian Persch 2004-10-13 10:54:46 UTC
Mass reassigning of Epiphany bugs to epiphany-maint@b.g.o
Comment 2 spark 2004-11-08 19:27:55 UTC
Thanks for the bug report. However, we feel that tabs act like windows who's
operations also can't be "undone". Perhaps a better solution would be to provide
easier access to recently visited locations in case you do accidentally close a
tab. Marking WONTFIX.
Comment 3 Reinout van Schouwen 2005-03-23 00:28:12 UTC
*** Bug 171244 has been marked as a duplicate of this bug. ***
Comment 4 shimon 2005-03-23 03:10:01 UTC
The Provide an extention that does this.
Comment 5 Reinout van Schouwen 2007-07-24 17:49:41 UTC
I'm reopening this bug. We don't give a warning when the user clicks the window-close button on purpose, but the "forgive the user" principle tells me that undoing closing 50 tabs in a window at once should be easier than digging through recently visited locations.
Comment 6 Reinout van Schouwen 2007-07-24 17:50:28 UTC
*** Bug 458513 has been marked as a duplicate of this bug. ***
Comment 7 Jean-François Fortin Tam 2007-07-25 21:37:32 UTC
Yeah, the ability to undo individual tab closes is not as important, in my opinion, as the *need* for a protection from multiple tab close accidents that happen when closing windows. Firefox does this, and since it exists as an epiphany extension, could it be integrated as part of epiphany's core?
Comment 8 Nathaniel Smith 2007-08-01 02:33:33 UTC
Even in the single-tab close case, it seems quite unfair to just say "oops, user's error, they should have been smarter".  With firefox I find myself using the undo-tab-close button a few times a week, because I misclicked with the mouse, or my finger stuttered on C-w, or I just realized a few minutes later that I wasn't done with that window after all.  Certainly I'm tough enough to survive such situations, and I can understand not having the engineering resources to get this right, but providing undo-close-tab does seem like the unambiguously user-friendly thing to do.

Also re: #2, a real undo-close-tab implementation is qualitatively different from what you get from any kind of recent-history browser.  Undo-close-tab, like session recovery, should also restore the state of any forms that were in the closed tab.  I guess this could be a lot of work, but, Firefox does do it.  We've all had that stomach-sinking feeling when we lose a long text entry we've spent half an hour writing but haven't yet hit submit on; Firefox appears to have classified that experience as a bug and fixed it, and it would be lovely if Epiphany did the same.
Comment 9 Reinout van Schouwen 2007-08-01 07:47:21 UTC
*** Bug 363592 has been marked as a duplicate of this bug. ***
Comment 10 Cosimo Cecchi 2007-08-01 15:39:50 UTC
Created attachment 92860 [details] [review]
adds a close-confirm dialog when there are multiple tabs in the window

This patch adds a deatcivable firefox-like behaviour, adding a close-confirm dialog when you close a window with multiple tabs opened. Don't know if this is the desired behaviour to fix this bug, but IMHO firefox function is useful to prevent wrong clicks.
Comment 11 Reinout van Schouwen 2007-08-01 16:17:22 UTC
To clarify, this bug asks for Undo tab close functionality, not for a confirm close dialog. http://live.gnome.org/Epiphany/ThirdPartyExtensions already contains a "Confirm Window Close" extension.
Comment 12 Cosimo Cecchi 2007-08-01 16:22:10 UTC
In #7 Jeff asked for it to be integrated in the main Epiphany, so I did this patch. Is there any chance this will be merged?
Comment 13 Jean-François Fortin Tam 2007-08-01 16:23:59 UTC
my bug (bug 458513) was about having a close confirmation dialog when
multiple tabs are open. Since it seems it's a different issue, I am reforking
it (reopening the bug report).
Comment 14 Christian Persch 2007-08-03 20:26:42 UTC
Generally speaking, I'm not sure we want either the pref UI or the checkbox in the question dialogue.

Specific concerns with the patch:

+	dialog = (gtk_message_dialog_new
+		(GTK_WINDOW (window),
+		 GTK_DIALOG_MODAL | 
+		 GTK_DIALOG_DESTROY_WITH_PARENT,
+		 GTK_MESSAGE_WARNING,
+		 GTK_BUTTONS_CANCEL,
+	 	 g_strdup_printf (_("You are about to close a window with %i open tabs"), n_pages)));

That "g_strdup_printf" is not necessary, since gtk_message_dialog_new has printf syntax itself.

However, this needs to use ngettext for correct i18n.

+	hbox = gtk_hbox_new (FALSE, 0);
+	
+	checkbutton = (gtk_check_button_new_with_label ("Remember my choice"));
+
+	g_signal_connect (checkbutton,
+			  "toggled", G_CALLBACK (tabs_confirm_toggled_cb), NULL);
+
+	
+	/* 62px to align checkbutton to label (12 + 36 + 14) 
+	   14px is the spacing of dialog->vbox
+	   12px is the spacing of gtkmessagedialog's priv vbox */
+	gtk_box_pack_start (GTK_BOX (hbox), checkbutton, FALSE, FALSE, 62);
+	
+	gtk_box_pack_start_defaults (GTK_BOX (GTK_DIALOG (dialog)->vbox), GTK_WIDGET (hbox));

Instead of this weird packing, just pack the checkbox into GTK_MESSAGE_DIALOG (dialog)->label->parent (which is a GtkVBox) directly.

+	dialog = construct_confirm_close_with_many_tabs_dialog (window);
+	response = gtk_dialog_run (GTK_DIALOG (dialog));

Never use gtk_dialog_run. You need to connect to "response" signal and do the work from its handler.
Comment 15 Cosimo Cecchi 2007-08-05 23:17:45 UTC
When the dialog comes up, Epiphany is shutting down, so I think I need some kind of lock, otherwise Ephy will quit. How do I get the lock without using gtk_dialog_run there?
Also, I used gtk_dialog_run there, because the dialog popping up when closing tabs with modified forms also used it, and I just adapted that code, so if gtk_dialog_run is evil, we need to change it also for the modified forms case.
Comment 16 Michael Opitz 2007-08-15 18:27:58 UTC
Created attachment 93745 [details]
tab restore patch and extension

I've tried to implement a undo tab-close behaviour with a python extension and an extension for the EphyEmbed interface, which allows a client to set some history-items for an EphyEmbed. 
Unfortunately I've only implemented this method for the Mozilla backend and not for the Webkit backend. Moreover I'm neither very familiar with the GObject type system, nor with Gecko. Thus the code might not be  very good. 

The drawback of my approach is though, that only the history of the tab is restored, and not the state of the forms.
Comment 17 Christian Persch 2007-08-15 21:04:56 UTC
The window isn't destroyed yet before the dialogue is accepted, right? So epiphany shouldn't be shutting down. (I know that the forms dialogue uses gtk_dialog_run, but that doesn't mean we should add more places that do :)
Comment 18 Bastien Nocera 2007-08-25 12:04:07 UTC
*** Bug 469668 has been marked as a duplicate of this bug. ***
Comment 19 Bastien Nocera 2007-08-25 12:05:45 UTC
In bug 469668, I mentioned that I'd like recently closed tabs to be visible under the history dialogue, possible in a "recently closed" section.
Comment 20 Pacho Ramos 2007-11-24 20:28:14 UTC
I also miss "Undo Close" for reopening closing tabs.

Thanks a lot :-)
Comment 21 Daniel Michalik 2009-12-19 12:00:23 UTC
I addressed this issue in the -extension bugreport #603978 which converts the Tab-Restore extension of -gecko to -webkit. It augments it a bit by having a global URL list instead of one list per window. I'd like to raise attention to this patch since I think I fixed all the issues and it should be in a good state.

I'm also aware that it doesn't solve this request perfectly since it only saves the last visited url of a tab for restoring, but not the whole tab itself. However I'm working on the latter and I'll add my (faulty) draft to #603978, hoping that someone see's my mistake.
Comment 22 Reinout van Schouwen 2012-05-03 23:12:53 UTC
*** Bug 675195 has been marked as a duplicate of this bug. ***
Comment 23 erusan 2012-05-27 02:55:45 UTC
This is basic functionality in any modern browser. Has any progress been made in implementing this feature?
Comment 24 Asif Ali Rizvan 2012-05-27 07:18:00 UTC
It seems to me that Gnome developers are working on each component, redesigning, rethinking and remaking the look and feel, along with behavior; which is a good thing.

First the Shell is going to be polished and made usable, whereas the applications, will come later;

it is better to use alternative applications in the meantime. like I have to use 

k3b       instead of   brasero (dvd writing fails)
firefox/qupzilla  instead of   web (flashplugin, addons, etc.)

So, let's wait for every app to get overhauled and improved; have you seen "disks" it looks better in gnome 3.4.x and easier to use.
Comment 25 Bastien Nocera 2012-05-28 08:39:14 UTC
(In reply to comment #24)
> It seems to me that Gnome developers are working on each component,
> redesigning, rethinking and remaking the look and feel, along with behavior;
> which is a good thing.

This isn't a forum. Please refrain from posting irrelevant information in bugs.
Comment 26 Daniel Michalik 2012-05-28 09:44:05 UTC
(In reply to comment #23)
> This is basic functionality in any modern browser. Has any progress been made
> in implementing this feature?

Yes, I implemented it in 2010. Patch was found to be good, see bug report #603978 and status of 2010-04-22, but got never integrated. After bugging people for about half a year after that (asking for either further feedback or integration) I gave up and turned away from developing for epiphany.
Comment 27 kxra 2012-05-28 20:10:32 UTC
Shall we mark this as a duplicate of bug 603978 then?
Comment 28 Jean-François Fortin Tam 2012-05-28 20:30:41 UTC
*** Bug 603978 has been marked as a duplicate of this bug. ***
Comment 29 Jean-François Fortin Tam 2012-05-28 20:35:02 UTC
Marked the newer bug (which had a less clear name/summary) as a dupe of this.

Someone (hello, maintainers! :) should forward-port the patch (attachment #154760 [details]) from bug #603978 (which had ample review it seems) to the new codebase...

Maybe this clashes with whatever plans there are for managing tabs/queues in 3.6 and will be obsolete by then, I'm not sure. Setting the target so that it at least gets considered/evaluated, and then you can change it if it doesn't fit your plans.
Comment 30 Xan Lopez 2012-10-08 17:56:50 UTC
I really want to get this done for 3.8, I'll have a look at the extension and see if we can integrate it.
Comment 31 Diego Escalante Urrelo (not reading bugmail) 2012-12-06 16:36:31 UTC
Working on this.
Comment 32 Diego Escalante Urrelo (not reading bugmail) 2012-12-06 21:15:59 UTC
Created attachment 230924 [details] [review]
ephy-window: implement undo-close-tab WIP

Proof of concept
Comment 33 Diego Escalante Urrelo (not reading bugmail) 2012-12-06 21:17:07 UTC
I think I am mostly done except for hooking the shortcut.
Comment 34 Diego Escalante Urrelo (not reading bugmail) 2012-12-07 19:34:56 UTC
Created attachment 230993 [details] [review]
ephy-window: implement undo-close-tab WIP

Updated
==

This version is working, even with the shortcut.
The limitation is that it works per-window.

I believe this should work globally, to do that I am thinking on moving
the GQueue to EphyShell rather than EphyWindow.

I am thinking something like this:
  EphyShellPrivate adds a @closed_tabs and ephy_shell_has_closed_tabs(),
  ephy_shell_get_last_closed_tab().

  EphyWindow updates the sensitivity of FileUndoCloseTab by consulting
  something like ephy_shell_has_closed_tabs.

  EphyShell watches over EphyWindow's EphyEmbedContainer::remove_child
  to keep its list up to date.

  On FileUndoCloseTab::activate, EphyWindow asks EphyShell for
  ephy_shell_get_last_closed_tab() (which pops the head of the GQueue).

While doing this I believe we could make the closed_tabs list more than
just a URL list.

Chrome restores the back/forward history of un-closed tabs. I wonder if
we could keep a copy of the tab's WebKitWebBackForwardList in the
GQueue.

Considering that @closed_tabs would have a size limit (10?) it should
not be a big memory hog, right?

My preferred implementation would be as described, plus restoring
back/fwd history. The user experience would then feels as a "restored
tab" instead of just re-opened URL.
Comment 35 Joel Dimbernat 2012-12-07 19:47:27 UTC
From what I can see of Firefox, it restores history, but also the place where you are in the page(scroll down a page, close the tab, undo the close, you're still down the page), and it also restores the page VERY fast which makes me think it doesn't reload it.

So I guess Firefox saves much more than URL and history. One would have to dig in the code to see exactly what they do, but it would not surprise me if they'd keep the entire "tab" object stored in memory, considering how fast it's restored.
Comment 36 Diego Escalante Urrelo (not reading bugmail) 2012-12-07 20:16:51 UTC
There used to be a restore tab extension that instead of removing the widgets, just hide them.

The problems with that were that flash still played hidden, or chats still went on.

We could do that if we had some way of freezing the tab entirely, meaning that we can be sure that CPU, network, audio, etc; are not used at all.

I don't know if that is possible with WebKit.
Comment 37 Diego Escalante Urrelo (not reading bugmail) 2012-12-07 23:06:29 UTC
Created attachment 231010 [details] [review]
ephy-window: implement undo-close-tab WIP

===
This is an alternate version that keeps track of closed tabs globally.
I tried to follow the idea on my previous comment, although this is in
EphySession rather than EphyShell.

It works fine, a few issues remain like syncing the sensitivity of the
action.

I would much prefer to have some kind of magical suspend/resume on
EphyEmbed objects, but the back/fwd restore works okish. If you are
paying attention you can see how the back button gets activated after
the BackForwardList gets populated.

Thoughts?
Comment 38 Bastien Nocera 2012-12-10 09:10:54 UTC
(In reply to comment #37)
> Created an attachment (id=231010) [details] [review]
> ephy-window: implement undo-close-tab WIP
> 
> ===
> This is an alternate version that keeps track of closed tabs globally.
> I tried to follow the idea on my previous comment, although this is in
> EphySession rather than EphyShell.
> 
> It works fine, a few issues remain like syncing the sensitivity of the
> action.
> 
> I would much prefer to have some kind of magical suspend/resume on
> EphyEmbed objects, but the back/fwd restore works okish. If you are
> paying attention you can see how the back button gets activated after
> the BackForwardList gets populated.
> 
> Thoughts?

Having the undo not be per-window is a requirement, otherwise closing the last tab in a window would make it impossible to restore.

FWIW, Chrom{e,ium} shows the last closed tabs in the default page, and I quite often go and look in the history to find recently closed tabs (closed time is actually more interesting than the last time it loaded in a lot of cases). This would also allow us to keep more pages to undo, and provide an easy way to restore closed tabs across sessions.
Comment 39 Diego Escalante Urrelo (not reading bugmail) 2012-12-11 10:07:44 UTC
*** Bug 678412 has been marked as a duplicate of this bug. ***
Comment 40 Claudio Saavedra 2013-01-25 17:32:01 UTC
What we should support in an initial patch to add this feature:

1. The BackForwardList should be kept.
2. Tabs should open in the window where they were closed. If such window has already been closed, then it needs to be reopened.
3. Tabs should open in the position where they were before closing.

So your latest patch is already going in a good direction, but it's not taking into account (2) and (3), which I believe to be basic for a good experience.
Comment 41 Claudio Saavedra 2013-01-25 18:03:08 UTC
With your permission I'll try to finish this now, because it's annoying me.
Comment 42 Diego Escalante Urrelo (not reading bugmail) 2013-01-26 00:21:38 UTC
(In reply to comment #41)
> With your permission I'll try to finish this now, because it's annoying me.

Please, do. I think you are in a much better position to quickly iterate this, than me.
Comment 43 Claudio Saavedra 2013-01-28 14:52:54 UTC
Created attachment 234622 [details] [review]
ephy-shell: Make it possible to prepend a tab in a window

We need this in order to open a tab in the first position of a window
when undoing a tab closure.
Comment 44 Claudio Saavedra 2013-01-28 14:53:03 UTC
Created attachment 234623 [details] [review]
ephy-session: add API to restore closed tabs

We add a queue of closed tabs to EphySession, which is later used to
restore them through ephy_session_undo_close_tab().

Based on a patch by Diego Escalante Urrelo <diegoe@igalia.com>
Comment 45 Claudio Saavedra 2013-01-28 14:53:07 UTC
Created attachment 234624 [details] [review]
ephy-session: add can-undo-tab-closed boolean property
Comment 46 Claudio Saavedra 2013-01-28 14:53:11 UTC
Created attachment 234625 [details] [review]
ephy-window: add menu item to allow undoing tab closure

Based on a patch by Diego Escalante Urrelo <diegoe@igalia.com>
Comment 47 Claudio Saavedra 2013-01-28 15:04:41 UTC
So these patches above implement what I described. There are a few limitations regarding the BackForward list, as WK1 is not very flexible regarding this and WK2 has a completely different idea of how session should be handled and there is still some API that we need to add to WKGTK+ in order to use it.

Also, I think ideally we would remember this between sessions and be able to restore filled forms. But for the time being this makes me considerably less irritated.
Comment 48 Xan Lopez 2013-01-28 15:39:38 UTC
Review of attachment 234622 [details] [review]:

OK (hopefully these magic numbers are not stored anywhere, but I don't think so)
Comment 49 Xan Lopez 2013-01-28 16:02:20 UTC
Review of attachment 234623 [details] [review]:

::: src/ephy-session.c
@@ +145,3 @@
+		g_free (data->url);
+		data->url = NULL;
+	}

You are never freeing 'data' itself?

@@ +229,3 @@
+						   (void **)tab->window);
+		else
+			g_free (tab->window);

I think it seems better to make a method that maintains the sanity of the tab->window thing for each item in the queue, and call it at the end of each function where it's relevant. Something like purge_orphan_windows (queue) or something. Seems cleaner than doing different things at different points in different functions?

@@ +290,3 @@
+	}
+
+	tab = g_new0 (ClosedTab, 1);

This should probably use GSlice.

@@ +307,3 @@
+	{
+		tab->window = g_new0 (EphyWindow *, 1);
+		*tab->window = window;

It seems this could just be a gpointer no?

@@ +314,3 @@
+
+	LOG ("Added: %s to the list (%d elements)",
+	     address, g_queue_get_legth (priv->closed_tabs));

I think this whole chunk should go into its own method to construct o ClosedTab

@@ +352,3 @@
+	toplevel = gtk_widget_get_toplevel (notebook);
+	/* When quitting the toplevel might not be the window. In that
+	 * case we to save the closed tab. */

we "don't have" to save, I guess?

@@ +611,3 @@
 
+	g_queue_free_full (session->priv->closed_tabs,
+			   (GDestroyNotify) closed_tab_free);

The style rule is supposedly no space in casts.
Comment 50 Claudio Saavedra 2013-01-28 18:10:43 UTC
Created attachment 234639 [details] [review]
ephy-session: add API to restore closed tabs

We add a queue of closed tabs to EphySession, which is later used to
restore them through ephy_session_undo_close_tab().

Based on a patch by Diego Escalante Urrelo <diegoe@igalia.com>
Comment 51 Claudio Saavedra 2013-01-28 20:18:21 UTC
Created attachment 234653 [details] [review]
ephy-embed-shell: add EPHY_EMBED_SHELL_MODE_INCOGNITO

Since we need to differenciate between incognito and private mode in
order to enable restoring closed tabs only for the latter case.

Add a macro for the cases in which either mode should behave in the
same way to simplify the change.
Comment 52 Claudio Saavedra 2013-01-28 20:18:38 UTC
Created attachment 234654 [details] [review]
ephy-shell: add application menu item for reopening closed tabs
Comment 53 Xan Lopez 2013-02-04 18:10:02 UTC
Review of attachment 234624 [details] [review]:

I think it's wrong to have this kind of API in EphySession. It should be in EphyShell. Add the minimum API necessary to EphySession to do this from EphyShell, but keep the API out IMHO.
Comment 54 Xan Lopez 2013-02-04 18:12:37 UTC
(In reply to comment #53)
> Review of attachment 234624 [details] [review]:
> 
> I think it's wrong to have this kind of API in EphySession. It should be in
> EphyShell. Add the minimum API necessary to EphySession to do this from
> EphyShell, but keep the API out IMHO.

We can even start to define a very simple beginning for an EphyModel like API. So for instance if we want to expose the contents of the "undo close tab" list in the model, here we could add an API point to the session for the shell to ask for its length. Later it could mutate into getting the full blown list of items, and we'd make the "closed tab" item type public. Eventually all that stuff should be split from EphySession into EphyModel proper.

Makes sense?
Comment 55 Xan Lopez 2013-02-04 18:36:42 UTC
Review of attachment 234639 [details] [review]:

::: src/ephy-session.c
@@ +284,3 @@
+			window_location_free (tab->window_location, FALSE);
+	}
+

This looks better. I still think you should factor out the difficult bits of this method into a different function, with a comment explaining the possible cases.

Undo close tab, check the window_location pointer:

* If it is not NULL, the window were this tab was is still around. Restore it there. If there are no more tabs in this window in the queue we can forget about it. This is explained in the comment.

* If it is NULL, it means the window where we were was closed. Create a new one, set window_location to it. Note that this pointer is *shared* with all the other tabs that were in the same window. Now we search the queue, if we find other sibling tabs update the weak ref, otherwise we can forget again about it (see previous bullet point).

Just by writing this down it suggests there's some common code between the two branches too.
Comment 56 Xan Lopez 2013-02-04 18:38:13 UTC
Also we *really* need unit tests for the patches up to this point. It's even pretty easy to test.
Comment 57 Xan Lopez 2013-02-04 18:40:22 UTC
Review of attachment 234653 [details] [review]:

OK.
Comment 58 Xan Lopez 2013-02-04 18:41:56 UTC
Review of attachment 234654 [details] [review]:

Excellent!

::: src/window-commands.c
@@ +86,3 @@
+			   EphyWindow *window)
+{
+	ephy_session_undo_close_tab (EPHY_SESSION (ephy_shell_get_session (ephy_shell_get_default ())));

god let's make EphyShell return an EphySession* there after this patch.
Comment 59 Claudio Saavedra 2013-02-06 13:37:59 UTC
Created attachment 235309 [details] [review]
ephy-session: add API to restore closed tabs

We add a queue of closed tabs to EphySession, which is later used to
restore them through ephy_session_undo_close_tab().

Based on a patch by Diego Escalante Urrelo <diegoe@igalia.com>
Comment 60 Claudio Saavedra 2013-02-06 21:16:32 UTC
Created attachment 235329 [details] [review]
This version of the patch uses the EphyNotebook instead of the EphyWindow

to keep track of where each closed tab belongs. The reason to do this is
that when a window is destroyed the tabs are removed from the notebook
after this has been removed from the window, so previous patches wouldn't
be able to restore windows that have been closed through "delete-event".


ephy-session: add API to restore closed tabs

We add a queue of closed tabs to EphySession, which is later used to
restore them through ephy_session_undo_close_tab().

Based on a patch by Diego Escalante Urrelo <diegoe@igalia.com>
Comment 61 Claudio Saavedra 2013-02-07 08:26:51 UTC
Created attachment 235350 [details] [review]
ephy-session-test: add tests for tab restoring API
Comment 62 Xan Lopez 2013-02-07 17:15:21 UTC
Review of attachment 235329 [details] [review]:

OK then.

::: src/ephy-session.c
@@ +253,3 @@
+		| EPHY_NEW_TAB_JUMP
+		| EPHY_NEW_TAB_DONT_COPY_HISTORY;
+

g_return_if_fail missing.

@@ +292,3 @@
+					      NULL, NULL, tab->url, flags);
+
+		notebook = EPHY_NOTEBOOK (gtk_widget_get_parent (GTK_WIDGET (new_tab)));

Add a FIXME here, and it would be cool to not do these hacks...
Comment 63 Xan Lopez 2013-02-07 17:17:18 UTC
Review of attachment 235350 [details] [review]:

Great!
Comment 64 Claudio Saavedra 2013-02-07 22:06:11 UTC
Attachment 234622 [details] pushed as f3eef36 - ephy-shell: Make it possible to prepend a tab in a window
Attachment 234624 [details] pushed as 946292f - ephy-session: add can-undo-tab-closed boolean property
Attachment 234653 [details] pushed as 180ad78 - ephy-embed-shell: add EPHY_EMBED_SHELL_MODE_INCOGNITO
Attachment 234654 [details] pushed as be7b9d4 - ephy-shell: add application menu item for reopening closed tabs
Attachment 235350 [details] pushed as 0d728a0 - ephy-session-test: add tests for tab restoring API