GNOME Bugzilla – Bug 128184
Add "undo tab close" to Web
Last modified: 2013-02-07 22:06:33 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.
Mass reassigning of Epiphany bugs to epiphany-maint@b.g.o
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.
*** Bug 171244 has been marked as a duplicate of this bug. ***
The Provide an extention that does this.
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.
*** Bug 458513 has been marked as a duplicate of this bug. ***
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?
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.
*** Bug 363592 has been marked as a duplicate of this bug. ***
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.
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.
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?
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).
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.
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.
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.
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 :)
*** Bug 469668 has been marked as a duplicate of this bug. ***
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.
I also miss "Undo Close" for reopening closing tabs. Thanks a lot :-)
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.
*** Bug 675195 has been marked as a duplicate of this bug. ***
This is basic functionality in any modern browser. Has any progress been made in implementing this feature?
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.
(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.
(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.
Shall we mark this as a duplicate of bug 603978 then?
*** Bug 603978 has been marked as a duplicate of this bug. ***
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.
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.
Working on this.
Created attachment 230924 [details] [review] ephy-window: implement undo-close-tab WIP Proof of concept
I think I am mostly done except for hooking the shortcut.
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.
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.
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.
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?
(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.
*** Bug 678412 has been marked as a duplicate of this bug. ***
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.
With your permission I'll try to finish this now, because it's annoying me.
(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.
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.
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>
Created attachment 234624 [details] [review] ephy-session: add can-undo-tab-closed boolean property
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>
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.
Review of attachment 234622 [details] [review]: OK (hopefully these magic numbers are not stored anywhere, but I don't think so)
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.
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>
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.
Created attachment 234654 [details] [review] ephy-shell: add application menu item for reopening closed tabs
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.
(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?
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.
Also we *really* need unit tests for the patches up to this point. It's even pretty easy to test.
Review of attachment 234653 [details] [review]: OK.
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.
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>
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>
Created attachment 235350 [details] [review] ephy-session-test: add tests for tab restoring API
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...
Review of attachment 235350 [details] [review]: Great!
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