From bc21904fef99c0a131a6083abdff805b9cede2b3 Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Sat, 22 Jul 2017 15:12:31 -0400 Subject: [PATCH] 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. --- .../completion/models/completionmodel.py | 10 +++++++++- qutebrowser/completion/models/histcategory.py | 9 ++------- qutebrowser/completion/models/listcategory.py | 9 --------- tests/unit/completion/test_completionmodel.py | 18 +++++++++++++++++ tests/unit/completion/test_histcategory.py | 20 +++++++------------ tests/unit/completion/test_listcategory.py | 20 ------------------- 6 files changed, 36 insertions(+), 50 deletions(-) diff --git a/qutebrowser/completion/models/completionmodel.py b/qutebrowser/completion/models/completionmodel.py index 3e48076e5..398673200 100644 --- a/qutebrowser/completion/models/completionmodel.py +++ b/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() diff --git a/qutebrowser/completion/models/histcategory.py b/qutebrowser/completion/models/histcategory.py index 612eb0bf4..56b7c3819 100644 --- a/qutebrowser/completion/models/histcategory.py +++ b/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() diff --git a/qutebrowser/completion/models/listcategory.py b/qutebrowser/completion/models/listcategory.py index 187ebcad6..5f537d4d7 100644 --- a/qutebrowser/completion/models/listcategory.py +++ b/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()) diff --git a/tests/unit/completion/test_completionmodel.py b/tests/unit/completion/test_completionmodel.py index 4d1d3f123..9e73e533a 100644 --- a/tests/unit/completion/test_completionmodel.py +++ b/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 diff --git a/tests/unit/completion/test_histcategory.py b/tests/unit/completion/test_histcategory.py index c53c86ee1..7044f1281 100644 --- a/tests/unit/completion/test_histcategory.py +++ b/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', '')]) diff --git a/tests/unit/completion/test_listcategory.py b/tests/unit/completion/test_listcategory.py index 3b1c1478a..df1d1de5f 100644 --- a/tests/unit/completion/test_listcategory.py +++ b/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')])