Repository: qutebrowser/qutebrowser
Pull request: Always close tabs when given or taken
Add an optional boolean argument to `tabbed_browser.close_tab()` called
`transfer` which indicates whether the tab is closing as a result of
being given to another window (`tab-give`) or taken by another window
(`tab-take`). If so, the tab will always close, even if it is the last
tab in the window and `tabs.last_close` is not set to 'close'.

Closes #5836.

<!-- Thanks for submitting a pull request! Please pick a descriptive title (not just "issue 12345"). If there is an open issue associated to your PR, please add a line like "Closes #12345" somewhere in the PR description (outside of this comment) -->


Problem statement:
Taking a tab doesn't close the tab if it's the only tab in a window
**Version info**:
qutebrowser v1.11.1
Backend: QtWebEngine (Chromium 73.0.3683.105)
Qt: 5.13.2

**Does the bug happen if you start with `--temp-basedir`?**:
Yes.

**Description**
If you take a tab to window A from window B using `tab-take`, it's opened in window A and the instance in window B closes, likely through `tab-close`. (This, I presume, is the expected behavior.) However, if the tab is the only tab in window B, `tab-take` doesn't close the tab, so there's a copy in both window A and window B.

**How to reproduce**
1. Open two windows of qutebrowser.
2. From the first window, run: `tab-take 1/0`

Hints from issue discussion:
This is consistent with what happens when you try to close the last tab in a window normally (i.e. via `d`). If you prefer the window to be closed in such cases, you can set `tabs.last_close` to `close`.

However, since this isn't the first time this is coming up (#4979, cc @jhultman), I'm okay with forcing the window to close (no matter what `tabs.last_close` is) in that case.
I didn't know about that option, it seems to solve the problem. Thanks for the quick reply!
This is also an issue with tab-give, though in that case it's less inconvenient to close it as a separate step.

I have tabs.last_close set to ignore because I found that it made it easier to recover from accidentally closing tabs. This usually happened due to thinking I was in insert mode, and then typing something with a 'd' in it.

tab-give and tab-take don't have default key-bindings (and I wouldn't expect anyone add a single-key binding), so I think the main reasons for using the tabs.last_close setting don't apply here. So I'd support either always closing in this situation, or having a "tabs.last_give" setting to determine behaviour in this case. However, I don't think it's a particular important issue.
@andrewcarlotti FWIW with `:undo --window` (`Shift-U`) being a thing now, this would only apply if you accidentally close the last tab in the last window (causing qutebrowser to quit).

Commit message: Always close tabs when given or taken

Add an optional boolean argument to `tabbed_browser.close_tab()` called
`transfer` which indicates whether the tab is closing as a result of
being given to another window (`tab-give`) or taken by another window
(`tab-take`). If so, the tab will always close, even if it is the last
tab in the window and `tabs.last_close` is not set to 'close'.

Patch to review:
diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py
index f3438aaa8f6..5937e760444 100644
--- a/qutebrowser/browser/commands.py
+++ b/qutebrowser/browser/commands.py
@@ -451,7 +451,7 @@ def tab_take(self, index, keep=False):
 
         self._open(tab.url(), tab=True)
         if not keep:
-            tabbed_browser.close_tab(tab, add_undo=False)
+            tabbed_browser.close_tab(tab, add_undo=False, transfer=True)
 
     @cmdutils.register(instance='command-dispatcher', scope='window')
     @cmdutils.argument('win_id', completion=miscmodels.window)
@@ -500,7 +500,8 @@ def tab_give(self, win_id: int = None, keep: bool = False,
         tabbed_browser.tabopen(self._current_url())
         if not keep:
             self._tabbed_browser.close_tab(self._current_widget(),
-                                           add_undo=False)
+                                           add_undo=False,
+                                           transfer=True)
 
     def _back_forward(self, tab, bg, window, count, forward, index=None):
         """Helper function for :back/:forward."""
diff --git a/qutebrowser/mainwindow/tabbedbrowser.py b/qutebrowser/mainwindow/tabbedbrowser.py
index e081284eeba..47bb32a6f11 100644
--- a/qutebrowser/mainwindow/tabbedbrowser.py
+++ b/qutebrowser/mainwindow/tabbedbrowser.py
@@ -406,13 +406,14 @@ def tab_close_prompt_if_pinned(
         else:
             yes_action()
 
-    def close_tab(self, tab, *, add_undo=True, new_undo=True):
+    def close_tab(self, tab, *, add_undo=True, new_undo=True, transfer=False):
         """Close a tab.
 
         Args:
             tab: The QWebView to be closed.
             add_undo: Whether the tab close can be undone.
             new_undo: Whether the undo entry should be a new item in the stack.
+            transfer: Whether the tab is closing because it is moving to a new window.
         """
         if config.val.tabs.tabs_are_windows:
             last_close = 'close'
@@ -421,13 +422,13 @@ def close_tab(self, tab, *, add_undo=True, new_undo=True):
 
         count = self.widget.count()
 
-        if last_close == 'ignore' and count == 1:
+        if last_close == 'ignore' and count == 1 and not transfer:
             return
 
         self._remove_tab(tab, add_undo=add_undo, new_undo=new_undo)
 
         if count == 1:  # We just closed the last tab above.
-            if last_close == 'close':
+            if last_close == 'close' or transfer:
                 self.close_window.emit()
             elif last_close == 'blank':
                 self.load_url(QUrl('about:blank'), newtab=True)


Code review comments:
diff_hunk: @@ -406,13 +406,14 @@ def tab_close_prompt_if_pinned(
         else:
             yes_action()
 
-    def close_tab(self, tab, *, add_undo=True, new_undo=True):
+    def close_tab(self, tab, *, add_undo=True, new_undo=True, transfer=False):
         """Close a tab.
 
         Args:
             tab: The QWebView to be closed.
             add_undo: Whether the tab close can be undone.
             new_undo: Whether the undo entry should be a new item in the stack.
+            transfer: Whether the tab is closing because it is moving to a new window.
         """
         if config.val.tabs.tabs_are_windows:
path: qutebrowser/mainwindow/tabbedbrowser.py
text: @user1:
wouldn't it be simpler to just add `or transfer` here? That way the more complicated set of conditionals down below don't have to get more clauses.