From c6cb6ccd077fd98bb07c3fd385048e607398a0c1 Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Fri, 28 Jul 2017 08:14:38 -0400 Subject: [PATCH] Fix fetch/delete sql category bug. Fixes #2868, where pressing then in history completion (with > 256 items) would cause later items to disappear (and cause a crash if you try to delete again). Cause: Scrolling to the bottom would fetch an additional 256 items (in addition to the 256 that are fetched at first). Deleting causes the query to re-run, but it only fetches the initial 256 items, so the current index is now invalid. Fix: After deleting from the history category, call fetchMore until it has enough rows populated that the current index is valid. --- qutebrowser/completion/models/histcategory.py | 4 +++- tests/unit/completion/test_histcategory.py | 21 ++++++++++++++++--- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/qutebrowser/completion/models/histcategory.py b/qutebrowser/completion/models/histcategory.py index 42301c0dd..606351440 100644 --- a/qutebrowser/completion/models/histcategory.py +++ b/qutebrowser/completion/models/histcategory.py @@ -93,10 +93,12 @@ class HistoryCategory(QSqlQueryModel): self._query.run(pat=pattern) self.setQuery(self._query) - def removeRows(self, _row, _count, _parent=None): + 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) + while self.rowCount() < row: + self.fetchMore() return True diff --git a/tests/unit/completion/test_histcategory.py b/tests/unit/completion/test_histcategory.py index db4071502..1397b8b5f 100644 --- a/tests/unit/completion/test_histcategory.py +++ b/tests/unit/completion/test_histcategory.py @@ -22,7 +22,6 @@ import datetime import pytest -from PyQt5.QtCore import QModelIndex from qutebrowser.misc import sql from qutebrowser.completion.models import histcategory @@ -147,6 +146,22 @@ def test_remove_rows(hist, model_validator): model_validator.set_model(cat) cat.set_pattern('') hist.delete('url', 'foo') - # histcategory does not care which index was removed, it just regenerates - cat.removeRows(QModelIndex(), 1) + cat.removeRows(0, 1) model_validator.validate([('bar', 'Bar', '')]) + + +def test_remove_rows_fetch(hist): + """removeRows should fetch enough data to make the current index valid.""" + # we cannot use model_validator as it will fetch everything up front + hist.insert_batch({'url': [str(i) for i in range(300)]}) + cat = histcategory.HistoryCategory() + cat.set_pattern('') + + # sanity check that we didn't fetch everything up front + assert cat.rowCount() < 300 + cat.fetchMore() + assert cat.rowCount() == 300 + + hist.delete('url', '298') + cat.removeRows(297, 1) + assert cat.rowCount() == 299