Merge pull request #2846 from rcorre/completion-fixes

Completion fixes

qutebrowser/completion/completionwidget.py

@@ -109,8 +109,6 @@ class CompletionView(QTreeView):
         super().__init__(parent)
         self.pattern = ''
         self._win_id = win_id
-        # FIXME handle new aliases.
-        # objreg.get('config').changed.connect(self.init_command_completion)
         objreg.get('config').changed.connect(self._on_config_changed)
 
         self._active = False
@@ -297,6 +295,7 @@ class CompletionView(QTreeView):
         self.pattern = pattern
         with debug.log_time(log.completion, 'Set pattern {}'.format(pattern)):
             self.model().set_pattern(pattern)
+            self.selectionModel().clear()
             self._maybe_update_geometry()
             self._maybe_show()
 

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

@@ -25,7 +25,6 @@ from PyQt5.QtSql import QSqlQueryModel
 
 from qutebrowser.misc import sql
 from qutebrowser.utils import debug
-from qutebrowser.commands import cmdexc
 from qutebrowser.config import config
 
 
@@ -90,14 +89,10 @@ class HistoryCategory(QSqlQueryModel):
             self._query.run(pat=pattern)
         self.setQuery(self._query)
 
-    def delete_cur_item(self, index):
+    def removeRows(self, _row, _count, _parent=None):
-        """Delete the row at the given index."""
+        """Override QAbstractItemModel::removeRows to re-run sql query."""
-        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)
         # re-run query to reload updated table
         with debug.log_time('sql', 'Re-running completion query post-delete'):
             self._query.run()
         self.setQuery(self._query)
+        return True

qutebrowser/completion/models/listcategory.py

@@ -21,11 +21,10 @@
 
 import re
 
-from PyQt5.QtCore import Qt, QSortFilterProxyModel, QModelIndex, QRegExp
+from PyQt5.QtCore import Qt, QSortFilterProxyModel, QRegExp
 from PyQt5.QtGui import QStandardItem, QStandardItemModel
 
 from qutebrowser.utils import qtutils
-from qutebrowser.commands import cmdexc
 
 
 class ListCategory(QSortFilterProxyModel):
@@ -91,12 +90,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_completionwidget.py

@@ -87,6 +87,7 @@ def test_set_pattern(completionview):
     completionview.set_model(model)
     completionview.set_pattern('foo')
     model.set_pattern.assert_called_with('foo')
+    assert not completionview.selectionModel().currentIndex().isValid()
 
 
 def test_set_pattern_no_model(completionview):

tests/unit/completion/test_histcategory.py

@@ -19,14 +19,13 @@
 
 """Test the web history completion category."""
 
-import unittest.mock
 import datetime
 
 import pytest
+from PyQt5.QtCore import QModelIndex
 
 from qutebrowser.misc import sql
 from qutebrowser.completion.models import histcategory
-from qutebrowser.commands import cmdexc
 
 
 @pytest.fixture
@@ -140,20 +139,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_rows(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'):
+    hist.delete('url', 'foo')
-        cat.delete_cur_item(cat.index(0, 0))
+    # histcategory does not care which index was removed, it just regenerates
+    cat.removeRows(QModelIndex(), 1)
+    model_validator.validate([('bar', 'Bar', '')])

tests/unit/completion/test_listcategory.py

@@ -19,12 +19,9 @@
 
 """Tests for CompletionFilterModel."""
 
-from unittest import mock
-
 import pytest
 
 from qutebrowser.completion.models import listcategory
-from qutebrowser.commands import cmdexc
 
 
 @pytest.mark.parametrize('pattern, before, after', [
@@ -51,23 +48,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')])