Fix completion-item-del on undeletable item.

Even though no item was deleted, it was manipulating the completion
model because beginRemoveRows was called before the exception was
raised.

This fixes that problem by moving the removal logic (and delete_func
check) into the parent model, so it can check whether deletion is
possible before calling beginRemoveRows.

Fixes #2839.

qutebrowser/completion/models/completionmodel.py

@@ -22,6 +22,7 @@
 from PyQt5.QtCore import Qt, QModelIndex, QAbstractItemModel
 
 from qutebrowser.utils import log, qtutils
+from qutebrowser.commands import cmdexc
 
 
 class CompletionModel(QAbstractItemModel):
@@ -219,6 +220,13 @@ class CompletionModel(QAbstractItemModel):
         parent = index.parent()
         cat = self._cat_from_idx(parent)
         assert cat, "CompletionView sent invalid index for deletion"
+        if not cat.delete_func:
+            raise cmdexc.CommandError("Cannot delete this item.")
+
+        data = [cat.data(cat.index(index.row(), i))
+                for i in range(cat.columnCount())]
+        cat.delete_func(data)
+
         self.beginRemoveRows(parent, index.row(), index.row())
-        cat.delete_cur_item(cat.index(index.row(), 0))
+        cat.removeRow(index.row(), QModelIndex())
         self.endRemoveRows()

qutebrowser/completion/models/histcategory.py

@@ -90,13 +90,8 @@ class HistoryCategory(QSqlQueryModel):
             self._query.run(pat=pattern)
         self.setQuery(self._query)
 
-    def delete_cur_item(self, index):
-        """Delete the row at the given index."""
-        if not self.delete_func:
-            raise cmdexc.CommandError("Cannot delete this item.")
-        data = [self.data(index.sibling(index.row(), i))
-                for i in range(self.columnCount())]
-        self.delete_func(data)
+    def removeRow(self, _row, _col):
+        """Re-run sql query to respond to a row removal."""
         # re-run query to reload updated table
         with debug.log_time('sql', 'Re-running completion query post-delete'):
             self._query.run()

qutebrowser/completion/models/listcategory.py

@@ -91,12 +91,3 @@ class ListCategory(QSortFilterProxyModel):
             return False
         else:
             return left < right
-
-    def delete_cur_item(self, index):
-        """Delete the row at the given index."""
-        if not self.delete_func:
-            raise cmdexc.CommandError("Cannot delete this item.")
-        data = [self.data(index.sibling(index.row(), i))
-                for i in range(self.columnCount())]
-        self.delete_func(data)
-        self.removeRow(index.row(), QModelIndex())

tests/unit/completion/test_completionmodel.py

@@ -28,6 +28,7 @@ from PyQt5.QtCore import QModelIndex
 
 from qutebrowser.completion.models import completionmodel, listcategory
 from qutebrowser.utils import qtutils
+from qutebrowser.commands import cmdexc
 
 
 @hypothesis.given(strategies.lists(min_size=0, max_size=3,
@@ -92,8 +93,25 @@ def test_delete_cur_item():
     func.assert_called_once_with(['foo', 'bar'])
 
 
+def test_delete_cur_item_no_func():
+    callback = mock.Mock(spec=[])
+    model = completionmodel.CompletionModel()
+    cat = listcategory.ListCategory('', [('foo', 'bar')], delete_func=None)
+    model.rowsAboutToBeRemoved.connect(callback)
+    model.rowsRemoved.connect(callback)
+    model.add_category(cat)
+    parent = model.index(0, 0)
+    with pytest.raises(cmdexc.CommandError):
+        model.delete_cur_item(model.index(0, 0, parent))
+    assert not callback.called
+
+
 def test_delete_cur_item_no_cat():
     """Test completion_item_del with no selected category."""
+    callback = mock.Mock(spec=[])
     model = completionmodel.CompletionModel()
+    model.rowsAboutToBeRemoved.connect(callback)
+    model.rowsRemoved.connect(callback)
     with pytest.raises(qtutils.QtValueError):
         model.delete_cur_item(QModelIndex())
+    assert not callback.called

tests/unit/completion/test_histcategory.py

@@ -23,6 +23,7 @@ import unittest.mock
 import datetime
 
 import pytest
+from PyQt5.QtCore import QModelIndex
 
 from qutebrowser.misc import sql
 from qutebrowser.completion.models import histcategory
@@ -140,20 +141,13 @@ def test_sorting(max_items, before, after, model_validator, hist, config_stub):
     model_validator.validate(after)
 
 
-def test_delete_cur_item(hist):
+def test_remove_row(hist, model_validator):
     hist.insert({'url': 'foo', 'title': 'Foo'})
     hist.insert({'url': 'bar', 'title': 'Bar'})
-    func = unittest.mock.Mock(spec=[])
-    cat = histcategory.HistoryCategory(delete_func=func)
-    cat.set_pattern('')
-    cat.delete_cur_item(cat.index(0, 0))
-    func.assert_called_with(['foo', 'Foo', ''])
-
-
-def test_delete_cur_item_no_func(hist):
-    hist.insert({'url': 'foo', 'title': 1})
-    hist.insert({'url': 'bar', 'title': 2})
     cat = histcategory.HistoryCategory()
+    model_validator.set_model(cat)
     cat.set_pattern('')
-    with pytest.raises(cmdexc.CommandError, match='Cannot delete this item'):
-        cat.delete_cur_item(cat.index(0, 0))
+    hist.delete('url', 'foo')
+    # histcategory does not care which index was removed, it just regenerates
+    cat.removeRow(QModelIndex(), QModelIndex())
+    model_validator.validate([('bar', 'Bar', '')])

tests/unit/completion/test_listcategory.py

@@ -51,23 +51,3 @@ def test_set_pattern(pattern, before, after, model_validator):
     model_validator.set_model(cat)
     cat.set_pattern(pattern)
     model_validator.validate(after)
-
-
-def test_delete_cur_item(model_validator):
-    func = mock.Mock(spec=[])
-    cat = listcategory.ListCategory('Foo', [('a', 'b'), ('c', 'd')],
-                                    delete_func=func)
-    model_validator.set_model(cat)
-    idx = cat.index(0, 0)
-    cat.delete_cur_item(idx)
-    func.assert_called_once_with(['a', 'b'])
-    model_validator.validate([('c', 'd')])
-
-
-def test_delete_cur_item_no_func(model_validator):
-    cat = listcategory.ListCategory('Foo', [('a', 'b'), ('c', 'd')])
-    model_validator.set_model(cat)
-    idx = cat.index(0, 0)
-    with pytest.raises(cmdexc.CommandError, match="Cannot delete this item."):
-        cat.delete_cur_item(idx)
-    model_validator.validate([('a', 'b'), ('c', 'd')])