Repository: qutebrowser/qutebrowser
Pull request: Added window id parameter in _tabs funciton used it in delete_tabs function
As delete_tab was assuming that completion column contains window ID, it was showing
exception in case of tab-focus, as it doesn't have the window ID in completion column.
So instead a new parameter named current_win_id is used in _tabs which is also passed
in all uses of the _tabs function. This PR Closes #6967.

<!-- 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:
completion-item-del with tab-focus: ValueError: not enough values to unpack
From a crash report:

- Open the `:tab-focus` menu with `T`
- Select using the arrow keys, some tab from the list (not pressing enter, just hovering)
- Press Shift+Delete

and indeed:

```pytb
Traceback (most recent call last):
  File "/home/florian/proj/qutebrowser/git/qutebrowser/keyinput/eventfilter.py", line 104, in eventFilter
    return handler(cast(QKeyEvent, event))
  File "/home/florian/proj/qutebrowser/git/qutebrowser/keyinput/eventfilter.py", line 74, in _handle_key_event
    return man.handle_event(event)
  File "/home/florian/proj/qutebrowser/git/qutebrowser/keyinput/modeman.py", line 468, in handle_event
    return handler(cast(QKeyEvent, event))
  File "/home/florian/proj/qutebrowser/git/qutebrowser/keyinput/modeman.py", line 289, in _handle_keypress
    match = parser.handle(event, dry_run=dry_run)
  File "/home/florian/proj/qutebrowser/git/qutebrowser/keyinput/basekeyparser.py", line 323, in handle
    self.execute(result.command, count)
  File "/home/florian/proj/qutebrowser/git/qutebrowser/keyinput/modeparsers.py", line 76, in execute
    self._commandrunner.run(cmdstr, count)
  File "/home/florian/proj/qutebrowser/git/qutebrowser/commands/runners.py", line 188, in run
    result.cmd.run(self._win_id, args, count=count)
  File "/home/florian/proj/qutebrowser/git/qutebrowser/commands/command.py", line 553, in run
    self.handler(*posargs, **kwargs)
  File "/home/florian/proj/qutebrowser/git/qutebrowser/completion/completionwidget.py", line 461, in completion_item_del
    self.model().delete_cur_item(index)
  File "/home/florian/proj/qutebrowser/git/qutebrowser/completion/models/completionmodel.py", line 230, in delete_cur_item
    cat.delete_func(data)
  File "/home/florian/proj/qutebrowser/git/qutebrowser/completion/models/miscmodels.py", line 116, in delete_tab
    win_id, tab_index = data[0].split('/')
ValueError: not enough values to unpack (expected 2, got 1)
```

This looks like it *should* be a good bugfix for new contributors: From what it looks like, in `qutebrowser/completion/models/miscmodels.py`, we use `delete_tab` as deletion function for both `tab-focus` (displays tabs from current window) and `tab-select` (displays all tabs].

For `tab-focus`, we pass `add_win_id=False` to the `_tabs` helper generating the completion, so that it doesn't prepend the window id:

```python
            tab_str = ("{}/{}".format(win_id, idx + 1) if add_win_id
                       else str(idx + 1))
```

but the `delete_tab` function assumes that there always is a slash:

```python
    def delete_tab(data):
        """Close the selected tab."""
        win_id, tab_index = data[0].split('/')
```

causing the crash. To fix this, we should probably:

- Pass the current `win_id` (from `info.win_id`) to `_tabs` in all places where it gets called
- Adjust `delete_tab` so that it uses that (instead of getting it from the text in the first completion column) if `add_win_id=False` is given
- Ideally, add a test testing for this, but I'm not sure how unit-testable that part of the code is currently

Hints from issue discussion:
@mohite-abhi please create a PR indeed, then we can take it from there - that way, we can see your code changes and the full test output.

Commit message: Fix linter errors

Patch to review:
diff --git a/qutebrowser/completion/models/miscmodels.py b/qutebrowser/completion/models/miscmodels.py
index d8ebafb2979..0aa30d18490 100644
--- a/qutebrowser/completion/models/miscmodels.py
+++ b/qutebrowser/completion/models/miscmodels.py
@@ -103,17 +103,21 @@ def session(*, info=None):
     return model
 
 
-def _tabs(*, win_id_filter=lambda _win_id: True, add_win_id=True):
+def _tabs(*, win_id_filter=lambda _win_id: True, add_win_id=True, cur_win_id=0):
     """Helper to get the completion model for tabs/other_tabs.
 
     Args:
         win_id_filter: A filter function for window IDs to include.
                        Should return True for all included windows.
         add_win_id: Whether to add the window ID to the completion items.
+        cur_win_id: Window ID to be passed from info.win_id
     """
     def delete_tab(data):
         """Close the selected tab."""
-        win_id, tab_index = data[0].split('/')
+        win_id = cur_win_id
+        # data[0] can be 'tabInd' or 'winID/tabInd'
+        tab_index = data[0].split('/')[-1]
+
         tabbed_browser = objreg.get('tabbed-browser', scope='window',
                                     window=int(win_id))
         tabbed_browser.on_tab_close_requested(int(tab_index) - 1)
@@ -177,13 +181,15 @@ def other_tabs(*, info):
 
     Used for the tab-take command.
     """
-    return _tabs(win_id_filter=lambda win_id: win_id != info.win_id)
+    return _tabs(
+        win_id_filter=lambda win_id: win_id != info.win_id,
+        cur_win_id=info.win_id)
 
 
 def tab_focus(*, info):
     """A model to complete on open tabs in the current window."""
     model = _tabs(win_id_filter=lambda win_id: win_id == info.win_id,
-                  add_win_id=False)
+                  add_win_id=False, cur_win_id=info.win_id)
 
     special = [
         ("last", "Focus the last-focused tab"),


Code review comments:
diff_hunk: @@ -103,17 +103,21 @@ def session(*, info=None):
     return model
 
 
-def _tabs(*, win_id_filter=lambda _win_id: True, add_win_id=True):
+def _tabs(*, win_id_filter=lambda _win_id: True, add_win_id=True, cur_win_id=0):
     """Helper to get the completion model for tabs/other_tabs.
 
     Args:
         win_id_filter: A filter function for window IDs to include.
                        Should return True for all included windows.
         add_win_id: Whether to add the window ID to the completion items.
+        cur_win_id: Window ID to be passed from info.win_id
     """
     def delete_tab(data):
         """Close the selected tab."""
-        win_id, tab_index = data[0].split('/')
+        win_id = cur_win_id
+        # data[0] can be 'tabInd' or 'winID/tabInd'
+        tab_index = data[0].split('/')[-1]
path: qutebrowser/completion/models/miscmodels.py
text: @user1:
I think we still need to get a window ID from `data`, and only fall back to `cur_win_id` if the former is missing. Otherwise we might delete the wrong tab if there are multiple windows.