GNOME Bugzilla – Bug 561136
Undo Close Tabs
Last modified: 2017-05-31 13:02:26 UTC
I like the tabbed file browsing in Nautilus, it makes life easier. I would love the tabbed browsing if there were an "Undo Close Tab". As a bonus, I would also love to middle click the tab to close it.
I second this request. This would provide an intuitive model for those of us who are accustomed to other tabbed browsers such as Mozilla Firefox. Ideally the keyboard shortcuts would match, too, so I recommend Shift+Ctrl+T for Undo Close Tab.
Created attachment 145255 [details] [review] Adds ability to reopen a closed tab with Ctrl+Shift+T
I'm not sure I've done this correctly but it seems to be working. Also, should we have a menu item for this or just a keyboard shortcut? Please try it out and comment.
Comment on attachment 145255 [details] [review] Adds ability to reopen a closed tab with Ctrl+Shift+T Couple of minor fixes: you're leaking the list of closed locations, and g_file_get_uri_scheme() then strcmp() then g_free() is wasteful, just use g_file_has_uri_scheme(). Would probably make sense not to even store the closed locations if tabs are off (not familiar enough with the slot code to know if this already happens) and to sanity check the length of that list to some number of items so we're not blowing up in RAM usage for windows that stay open for a very long time.
Hmm.. this got more complex now with panes. I think we need to keep a list of list of locations. Right?
(In reply to comment #5) > Hmm.. this got more complex now with panes. I think we need to keep a list of > list of locations. Right? No, I don't think so. Just like some other formerly window specific items (the list of slots, the pointer to the active slot, ...), it would move from NautilusWindow to NautilusWindowPane. So tab history would be something pane specific, not window specific.
*** Bug 757067 has been marked as a duplicate of this bug. ***
What is the current status of this? I would also like to have a undo close tab as sometimes I accidentally close the wrong work tab and have it ingrained in my Ctrl + Shift + T reopens closed tabs as has been defacto in Google Chrome, Firefox & other browsers for years.
(In reply to Electric Prism from comment #8) > What is the current status of this? I would also like to have a undo close > tab as sometimes I accidentally close the wrong work tab and have it > ingrained in my Ctrl + Shift + T reopens closed tabs as has been defacto in > Google Chrome, Firefox & other browsers for years. Status is what you can see here, nothing happened. But this looks like an interesting addition, hope someone can make the time to post a patch. Marking for 3.24 for now.
Created attachment 347584 [details] [review] add "restore tab" action Adds option to reopen closed tabs with Ctrl+Shift+T. This is enabled when there are no selected folders in the active slot, as in this case this shortcut opens the selected folders in new tabs.
Review of attachment 347584 [details] [review]: Heya, code looks mostly good! But, the shrotcut is already taken... we should find a new one ::: src/nautilus-files-view.c @@ +7516,3 @@ g_simple_action_set_enabled (G_SIMPLE_ACTION (action), item_opens_in_view); + action = g_action_map_lookup_action (G_ACTION_MAP (window), + "restore-tab"); why do you need to deactivate it? You are trying to avoid the shortcut collision? That's not the way... this is really confusing. ::: src/nautilus-window.c @@ +1284,3 @@ + NautilusWindow *window = NAUTILUS_WINDOW (user_data); + NautilusWindowOpenFlags flags; + GFile *location; auto @@ +1298,3 @@ + location = window->priv->closed_locations->data; + + { Why this? We can actually store the "search location", similar to what we do with history in the back/forward buttons in the toolbar. @@ +2190,3 @@ nautilus_application_set_accelerator (app, "win.prompt-home-location", "asciitilde"); nautilus_application_set_accelerator (app, "win.view-menu", "F10"); + nautilus_application_set_accelerator (app, "win.restore-tab", "<shift><control>t"); this shortcut is already taken :( I vote to change the shortcut in files-view so we have similar shortcut as chrome for this, but we need a new one for opening a file in a new tab.
Created attachment 348831 [details] [review] Remove Shift+Control+T accelerator for open in new tab Opening in a new tab can be done with Shift+Return. This shortcut is removed so that it can be used for reopening closed tabs.
Created attachment 348832 [details] [review] add "restore tab" action Adds option to reopen closed tabs with Ctrl+Shift+T.
Review of attachment 348831 [details] [review]: +1
Review of attachment 348832 [details] [review]: as mentioned in IRC search tabs doesn't work, and I think we can do something similar as window-slot where it works (hope that doesn't make the code much ugly though).
Created attachment 348869 [details] [review] add "restore tab" action Adds option to reopen closed tabs with Ctrl+Shift+T.
Review of attachment 348869 [details] [review]: Now! This is the kind of approach I would like to see more... not trying to workaround but fixing the underlying issue, nice! There are just some details that still doesn't work, but now the logic is correct. Search -> close tab -> restore tab -> click one of the directories. The view uses list-view. I saw this in the code, tried it, and effectively that happens. Keep in mind that now in your code you don't save the "previous_view_if_search", this is fine, but what you do afterwards for this case ^ it misbehaves. Also, the comments should be updated to reflect this new use case. And the commit message, something happened with it that now is wrong when it was right before.
*** Bug 750227 has been marked as a duplicate of this bug. ***
Created attachment 349739 [details] [review] add "restore tab" action Adds option to reopen closed tabs with Ctrl+Shift+T. In order to do so, keep a list with data needed to restore closed tabs. So, this list keeps the location bookmark, the view id before search, which is needed in case the closed tab was a search and the back/forward history.
Review of attachment 349739 [details] [review]: Looking good, and it works great, nice solution with the history handling. ::: src/nautilus-window-slot.h @@ +111,3 @@ GFile *location); +void nautilus_window_slot_set_history (NautilusWindowSlot *self, maybe we should have a single function for just getting a struct with all the history data, instead of this specific functions. It's already a little ugly we have to peek on the private data of the slot, so if we can isolate that and not really peek into it individually would be better. So something like: nautilus_window_slot_get_restore_data. nautilus_window_slot_restore_from_data. ::: src/nautilus-window.c @@ +1308,3 @@ + data = window->priv->tab_data_list->data; + + if (!window->priv->tab_data_list) cannot we use just a GFile? Why using the bookmark? @@ +1317,3 @@ + nautilus_window_slot_set_view_before_search (slot, data->view_before_search); + + flags = g_settings_get_enum (nautilus_preferences, NAUTILUS_PREFERENCES_NEW_TAB_POSITION); this already initializes the slot. Couldn't we omite the previous call to window_iniaitlize_slot and the creation of the slot and instead just open a new locaiton in a new tab with this call? then we can set the history and the set_view_before_search. @@ +1320,3 @@ + + window->priv->tab_data_list = g_list_delete_link (window->priv->tab_data_list, + missaligment @@ +1323,3 @@ + + g_object_unref (data->location_bookmark); + flags |= NAUTILUS_WINDOW_OPEN_FLAG_DONT_MAKE_ACTIVE; do a method free_restore_tab_data and do these things in there. Also you are missing to unref the back_list and forward_list (and ref them inside set_history). The ownership should be clear. @@ +1324,3 @@ + g_object_unref (data->location_bookmark); + g_free (data); + flags |= NAUTILUS_WINDOW_OPEN_FLAG_DONT_MAKE_ACTIVE; spurious line @@ +1515,3 @@ { NautilusWindowSlot *next_slot; + GList *back_list, * forward_list; one per line pls @@ +1539,3 @@ + data->view_before_search = nautilus_window_slot_get_view_before_search (slot); + + data->back_list = back_list; can we use a gqueue? We are trying to use it more and more so we can push from both ends and query the lenght in O(1) @@ +2423,3 @@ + for (l = window->priv->tab_data_list; l != NULL; l = l->next) + { ideally this will just be a g_list_foreach (list, free_restore_data, NULL)
Created attachment 351426 [details] [review] add "restore tab" action Adds option to reopen closed tabs with Ctrl+Shift+T. In order to do so, keep a list with data needed to restore closed tabs. So, this list keeps the location bookmark, the view id before search, which is needed in case the closed tab was a search and the back/forward history.
Created attachment 351431 [details] [review] add "restore tab" action Adds option to reopen closed tabs with Ctrl+Shift+T. In order to do so, keep a list with data needed to restore closed tabs. So, this list keeps the location bookmark, the view id before search, which is needed in case the closed tab was a search and the back/forward history.
Review of attachment 351431 [details] [review]: Looking better and better, still two details to take care about. ::: src/nautilus-window-slot.c @@ +124,3 @@ gint view_mode_before_search; + + gboolean restored_slot; Oh this is pretty ugly, and I believe not necesary. I guess you want to avoid the forward list to be cleaned, but doesn't NAUTILUS_LOCATION_CHANGE_RELOAD preciselly exists for this situation, when you are kinda "reloading" rather than opening a new location? The nautilus_window_slot_restore_from_data should do the work already. @@ +156,3 @@ gpointer user_data); +void nautilus_window_slot_restore_from_data (NautilusWindowSlot *self, new line @@ +171,3 @@ +} + + priv->back_list = g_list_copy_deep (data->back_list, (GCopyFunc) g_object_ref, NULL); new line ::: src/nautilus-window.c @@ +1376,3 @@ + + slot = nautilus_window_create_slot (window, location); + if (g_queue_get_length (priv->tab_data_queue) == 0) is this still necesary? This seems to be handled in nautilus_window_open_location_full if NAUTILUS_WINDOW_OPEN_FLAG_NEW_TAB is there. This should go away in one way or another... @@ +1379,3 @@ + + nautilus_window_slot_open_location_full (slot, location, flags, NULL); + return; detail, but this looks better withouth a new line here, as in opening the location and restoring are close related. @@ +1382,3 @@ + nautilus_window_slot_restore_from_data (slot, data); + + } use free_restore_tab_data @@ +2531,3 @@ static void +free_restore_tab_data (gpointer data, + gpointer user_data) wrong identation
Created attachment 352900 [details] [review] add "restore tab" action Adds option to reopen closed tabs with Ctrl+Shift+T. In order to do so, keep a list with data needed to restore closed tabs. So, this list keeps the location bookmark, the view id before search, which is needed in case the closed tab was a search and the back/forward history.
Review of attachment 352900 [details] [review]: ::: src/nautilus-window.c @@ +144,3 @@ + + GQueue *tab_data_queue; + spurious line @@ +1375,3 @@ + data = g_queue_pop_head (priv->tab_data_queue); + + GFile, bookmarks are not necesary
Created attachment 352936 [details] [review] add "restore tab" action Adds option to reopen closed tabs with Ctrl+Shift+T. In order to do so, keep a list with data needed to restore closed tabs. So, this list keeps the location bookmark, the view id before search, which is needed in case the closed tab was a search and the back/forward history.
Created attachment 352938 [details] [review] nautilus-window: add wrapper for creating slot Always when creating a slot in NautilusWindow, it is also initialized, so create a wrapper which does both the creating and initializing.
Review of attachment 352936 [details] [review]: Overall looks good now, just a nit ::: src/nautilus-window-slot.c @@ +191,3 @@ + data->back_list = back_list; + data->forward_list = forward_list; +nautilus_window_slot_get_restore_tab_data (NautilusWindowSlot *self) Add a clear comment here why this is necessary. When investigating leaks I'm pretty sure this will be necessary. Also update OVERVIEW.md to reflect the saved tabs own a reference to previous files.
Created attachment 352945 [details] [review] nautilus-window: add wrapper for creating slot Always when creating a slot in NautilusWindow, it is also initialized, so create a wrapper which does both the creating and initializing.
Created attachment 352946 [details] [review] add "restore tab" action Adds option to reopen closed tabs with Ctrl+Shift+T. In order to do so, keep a list with data needed to restore closed tabs. So, this list keeps the location bookmark, the view id before search, which is needed in case the closed tab was a search and the back/forward history.
Review of attachment 352945 [details] [review]: +1
Review of attachment 352946 [details] [review]: LGTM!!
Attachment 352945 [details] pushed as 9ab9de9 - nautilus-window: add wrapper for creating slot Attachment 352946 [details] pushed as 6e16bc3 - add "restore tab" action