From bc21904fef99c0a131a6083abdff805b9cede2b3 Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Sat, 22 Jul 2017 15:12:31 -0400 Subject: [PATCH 1/4] 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')]) From b61691684ec4f3a6c2f4682dd087bcd142a2bd63 Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Sat, 22 Jul 2017 18:06:16 -0400 Subject: [PATCH 2/4] Clear selection when setting completion pattern. It doesn't make sense to have an active selection while you are filtering by entering text. You should be in one of two states: 1. Tabbing through completions (valid selection) 2. Entering a filter pattern (invalid selection) Fixes #2843, where a crash would occur after the following: 1. tab to an item other than the first 2. 3. re-type last character 4. This would try to delete an out of range index. --- qutebrowser/completion/completionwidget.py | 1 + tests/unit/completion/test_completionwidget.py | 1 + 2 files changed, 2 insertions(+) diff --git a/qutebrowser/completion/completionwidget.py b/qutebrowser/completion/completionwidget.py index 6e1e51680..39fe16e86 100644 --- a/qutebrowser/completion/completionwidget.py +++ b/qutebrowser/completion/completionwidget.py @@ -297,6 +297,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() diff --git a/tests/unit/completion/test_completionwidget.py b/tests/unit/completion/test_completionwidget.py index 207e557a8..22d150fd8 100644 --- a/tests/unit/completion/test_completionwidget.py +++ b/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): From 00be9e3c7fb269aa26942f6fcaf374c9b88cc8f6 Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Sat, 22 Jul 2017 18:09:10 -0400 Subject: [PATCH 3/4] Remove obsolete TODO. New aliases will now show up without a signal, as completions are generated on-demand. --- qutebrowser/completion/completionwidget.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/qutebrowser/completion/completionwidget.py b/qutebrowser/completion/completionwidget.py index 39fe16e86..65f1de76c 100644 --- a/qutebrowser/completion/completionwidget.py +++ b/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 From ff9efe22ae24dc215ca896ded69fd8939582f990 Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Sun, 23 Jul 2017 17:17:03 -0400 Subject: [PATCH 4/4] Fix unused imports and removeRow override. Override removeRows instead of removeRow. > removeRow is not virtual in C++, so if this gets called by Qt > internally for some reason, it wouldn't use the overloaded version - > so I think it'd be better to implement removeRows and then use > removeRow without overloading that - The-Compiler --- qutebrowser/completion/models/histcategory.py | 6 +++--- qutebrowser/completion/models/listcategory.py | 3 +-- tests/unit/completion/test_histcategory.py | 6 ++---- tests/unit/completion/test_listcategory.py | 3 --- 4 files changed, 6 insertions(+), 12 deletions(-) diff --git a/qutebrowser/completion/models/histcategory.py b/qutebrowser/completion/models/histcategory.py index 56b7c3819..6fdab0cdb 100644 --- a/qutebrowser/completion/models/histcategory.py +++ b/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,9 +89,10 @@ class HistoryCategory(QSqlQueryModel): self._query.run(pat=pattern) self.setQuery(self._query) - def removeRow(self, _row, _col): - """Re-run sql query to respond to a row removal.""" + def removeRows(self, _row, _count, _parent=None): + """Override QAbstractItemModel::removeRows to re-run sql query.""" # 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 diff --git a/qutebrowser/completion/models/listcategory.py b/qutebrowser/completion/models/listcategory.py index 5f537d4d7..b1ad77bae 100644 --- a/qutebrowser/completion/models/listcategory.py +++ b/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): diff --git a/tests/unit/completion/test_histcategory.py b/tests/unit/completion/test_histcategory.py index 7044f1281..a0b2b0144 100644 --- a/tests/unit/completion/test_histcategory.py +++ b/tests/unit/completion/test_histcategory.py @@ -19,7 +19,6 @@ """Test the web history completion category.""" -import unittest.mock import datetime import pytest @@ -27,7 +26,6 @@ from PyQt5.QtCore import QModelIndex from qutebrowser.misc import sql from qutebrowser.completion.models import histcategory -from qutebrowser.commands import cmdexc @pytest.fixture @@ -141,7 +139,7 @@ def test_sorting(max_items, before, after, model_validator, hist, config_stub): model_validator.validate(after) -def test_remove_row(hist, model_validator): +def test_remove_rows(hist, model_validator): hist.insert({'url': 'foo', 'title': 'Foo'}) hist.insert({'url': 'bar', 'title': 'Bar'}) cat = histcategory.HistoryCategory() @@ -149,5 +147,5 @@ def test_remove_row(hist, model_validator): cat.set_pattern('') hist.delete('url', 'foo') # histcategory does not care which index was removed, it just regenerates - cat.removeRow(QModelIndex(), QModelIndex()) + cat.removeRows(QModelIndex(), 1) model_validator.validate([('bar', 'Bar', '')]) diff --git a/tests/unit/completion/test_listcategory.py b/tests/unit/completion/test_listcategory.py index df1d1de5f..8d8936167 100644 --- a/tests/unit/completion/test_listcategory.py +++ b/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', [