rnhmjoj/qutebrowser
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Refactor delete_cur_item.

Browse Source 
Taking the completion widget as an argument was overly complex.
The process now looks like:

1. CompletionView gets deletion request
2. CompletionView passes selected index to CompletionModel
3. CompletionModel passes the row data to the owning category
4. The category runs its custom completion function.

This also fixes a bug. With the switch to the hybrid (list/sql)
completion model, the view was no longer updating when items were
deleted. This fixes that by ensuring the correct signals are emitted.

The SQL model must be refreshed by running the query. We could try using
a SqlTableModel so we can call removeRows instead.

The test for deleting a url fails because qmodeltester claims the length
of the query model is still 3.
This commit is contained in:
Ryan Roden-Corrent 2017-06-20 23:04:03 -04:00
parent 866f4653c7
commit 46161c3af0
12 changed files with 220 additions and 127 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
qutebrowser/browser/history.py
View File

@ -116,6 +116,15 @@ class WebHistory(sql.SqlTable):
def _do_clear(self):
self.delete_all()
def delete_url(self, url):
"""Remove all history entries with the given url.
Args:
url: URL string to delete.
"""
self.delete(url, 'url')
self.completion.delete(url, 'url')
@pyqtSlot(QUrl, QUrl, str)
def add_from_tab(self, url, requested_url, title):
"""Add a new history entry as slot, called from a BrowserTab."""

8
qutebrowser/completion/completionwidget.py
View File

@ -364,9 +364,7 @@ class CompletionView(QTreeView):
modes=[usertypes.KeyMode.command], scope='window')
def completion_item_del(self):
"""Delete the current completion item."""
if not self.currentIndex().isValid():
index = self.currentIndex()
if not index.isValid():
raise cmdexc.CommandError("No item selected!")
if self.model().delete_cur_item is None:
raise cmdexc.CommandError("Cannot delete this item.")
else:
self.model().delete_cur_item(self)
self.model().delete_cur_item(index)

15
qutebrowser/completion/models/completionmodel.py
View File

@ -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):
@ -38,13 +39,11 @@ class CompletionModel(QAbstractItemModel):
_categories: The sub-categories.
"""
def __init__(self, *, column_widths=(30, 70, 0),
delete_cur_item=None, parent=None):
def __init__(self, *, column_widths=(30, 70, 0), parent=None):
super().__init__(parent)
self.column_widths = column_widths
self._categories = []
self.pattern = ''
self.delete_cur_item = delete_cur_item
def _cat_from_idx(self, index):
"""Return the category pointed to by the given index.
@ -217,3 +216,13 @@ class CompletionModel(QAbstractItemModel):
"""
cat = self._cat_from_idx(index.parent())
return cat.columns_to_filter if cat else []
def delete_cur_item(self, index):
"""Delete the row at the given index."""
parent = index.parent()
cat = self._cat_from_idx(parent)
if not cat:
raise cmdexc.CommandError("No category selected")
self.beginRemoveRows(parent, index.row(), index.row())
cat.delete_cur_item(cat.index(index.row(), 0))
self.endRemoveRows()

16
qutebrowser/completion/models/listcategory.py
View File

@ -21,17 +21,19 @@
import re
from PyQt5.QtCore import QSortFilterProxyModel
from PyQt5.QtCore import QSortFilterProxyModel, QModelIndex
from PyQt5.QtGui import QStandardItem, QStandardItemModel
from qutebrowser.utils import qtutils
from qutebrowser.commands import cmdexc
class ListCategory(QSortFilterProxyModel):
"""Expose a list of items as a category for the CompletionModel."""
def __init__(self, name, items, columns_to_filter=None, parent=None):
def __init__(self, name, items, columns_to_filter=None,
delete_func=None, parent=None):
super().__init__(parent)
self.name = name
self.srcmodel = QStandardItemModel(parent=self)
@ -41,6 +43,7 @@ class ListCategory(QSortFilterProxyModel):
for item in items:
self.srcmodel.appendRow([QStandardItem(x) for x in item])
self.setSourceModel(self.srcmodel)
self.delete_func = delete_func
def set_pattern(self, val):
"""Setter for pattern.
@ -114,3 +117,12 @@ 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())

18
qutebrowser/completion/models/miscmodels.py
View File

@ -20,7 +20,7 @@
"""Functions that return miscellaneous completion models."""
from qutebrowser.config import config, configdata
from qutebrowser.utils import objreg, log, qtutils
from qutebrowser.utils import objreg, log
from qutebrowser.commands import cmdutils
from qutebrowser.completion.models import completionmodel, listcategory
@ -96,21 +96,14 @@ def buffer():
url_column = 1
text_column = 2
def delete_buffer(completion):
def delete_buffer(data):
"""Close the selected tab."""
index = completion.currentIndex()
qtutils.ensure_valid(index)
category = index.parent()
qtutils.ensure_valid(category)
index = category.child(index.row(), idx_column)
win_id, tab_index = index.data().split('/')
win_id, tab_index = data[0].split('/')
tabbed_browser = objreg.get('tabbed-browser', scope='window',
window=int(win_id))
tabbed_browser.on_tab_close_requested(int(tab_index) - 1)
model = completionmodel.CompletionModel(
column_widths=(6, 40, 54),
delete_cur_item=delete_buffer)
model = completionmodel.CompletionModel(column_widths=(6, 40, 54))
for win_id in objreg.window_registry:
tabbed_browser = objreg.get('tabbed-browser', scope='window',
@ -124,7 +117,8 @@ def buffer():
tab.url().toDisplayString(),
tabbed_browser.page_title(idx)))
cat = listcategory.ListCategory("{}".format(win_id), tabs,
columns_to_filter=[idx_column, url_column, text_column])
columns_to_filter=[idx_column, url_column, text_column],
delete_func=delete_buffer)
model.add_category(cat)
return model

17
qutebrowser/completion/models/sqlcategory.py
View File

@ -21,10 +21,12 @@
import re
from PyQt5.QtCore import QModelIndex
from PyQt5.QtSql import QSqlQueryModel
from qutebrowser.misc import sql
from qutebrowser.utils import debug
from qutebrowser.commands import cmdexc
class SqlCategory(QSqlQueryModel):
@ -33,7 +35,7 @@ class SqlCategory(QSqlQueryModel):
def __init__(self, name, *, title=None, filter_fields, sort_by=None,
sort_order=None, select='*', where=None, group_by=None,
parent=None):
delete_func=None, parent=None):
"""Create a new completion category backed by a sql table.
Args:
@ -44,6 +46,7 @@ class SqlCategory(QSqlQueryModel):
where: An optional clause to filter out some rows.
sort_by: The name of the field to sort by, or None for no sorting.
sort_order: Either 'asc' or 'desc', if sort_by is non-None
delete_func: Callback to delete a selected item.
"""
super().__init__(parent=parent)
self.name = title or name
@ -69,6 +72,7 @@ class SqlCategory(QSqlQueryModel):
col_query = sql.Query('SELECT * FROM {} LIMIT 1'.format(name))
rec = col_query.run().record()
self.columns_to_filter = [rec.indexOf(n) for n in filter_fields]
self.delete_func = delete_func
def set_pattern(self, pattern):
"""Set the pattern used to filter results.
@ -86,3 +90,14 @@ class SqlCategory(QSqlQueryModel):
with debug.log_time('sql', 'Running completion query'):
self._query.run(pattern=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)
# re-run query to reload updated table
with debug.log_time('sql', 'Running completion query'):
self._query.run()

57
qutebrowser/completion/models/urlmodel.py
View File

@ -29,31 +29,25 @@ _URLCOL = 0
_TEXTCOL = 1
def _delete_url(completion):
"""Delete the selected item.
def _delete_history(data):
urlstr = data[_URLCOL]
log.completion.debug('Deleting history entry {}'.format(urlstr))
hist = objreg.get('web-history')
hist.delete_url(urlstr)
Args:
completion: The Completion object to use.
"""
index = completion.currentIndex()
qtutils.ensure_valid(index)
category = index.parent()
index = category.child(index.row(), _URLCOL)
catname = category.data()
qtutils.ensure_valid(category)
if catname == 'Bookmarks':
urlstr = index.data()
log.completion.debug('Deleting bookmark {}'.format(urlstr))
bookmark_manager = objreg.get('bookmark-manager')
bookmark_manager.delete(urlstr)
elif catname == 'Quickmarks':
quickmark_manager = objreg.get('quickmark-manager')
sibling = index.sibling(index.row(), _TEXTCOL)
qtutils.ensure_valid(sibling)
name = sibling.data()
log.completion.debug('Deleting quickmark {}'.format(name))
quickmark_manager.delete(name)
def _delete_bookmark(data):
urlstr = data[_URLCOL]
log.completion.debug('Deleting bookmark {}'.format(urlstr))
bookmark_manager = objreg.get('bookmark-manager')
bookmark_manager.delete(urlstr)
def _delete_quickmark(data):
name = data[_TEXTCOL]
quickmark_manager = objreg.get('quickmark-manager')
log.completion.debug('Deleting quickmark {}'.format(name))
quickmark_manager.delete(name)
def url():
@ -61,18 +55,18 @@ def url():
Used for the `open` command.
"""
model = completionmodel.CompletionModel(
column_widths=(40, 50, 10),
delete_cur_item=_delete_url)
model = completionmodel.CompletionModel(column_widths=(40, 50, 10))
quickmarks = ((url, name) for (name, url)
in objreg.get('quickmark-manager').marks.items())
bookmarks = objreg.get('bookmark-manager').marks.items()
model.add_category(listcategory.ListCategory('Quickmarks', quickmarks,
columns_to_filter=[0, 1]))
model.add_category(listcategory.ListCategory('Bookmarks', bookmarks,
columns_to_filter=[0, 1]))
model.add_category(listcategory.ListCategory(
'Quickmarks', quickmarks, columns_to_filter=[0, 1],
delete_func=_delete_quickmark))
model.add_category(listcategory.ListCategory(
'Bookmarks', bookmarks, columns_to_filter=[0, 1],
delete_func=_delete_bookmark))
# replace 's to avoid breaking the query
timefmt = config.get('completion', 'timestamp-format').replace("'", "`")
@ -81,6 +75,7 @@ def url():
'CompletionHistory', title='History',
sort_order='desc', sort_by='last_atime',
filter_fields=['url', 'title'],
select='url, title, {}'.format(select_time))
select='url, title, {}'.format(select_time),
delete_func=_delete_history)
model.add_category(hist_cat)
return model

9
tests/helpers/fixtures.py
View File

@ -257,6 +257,15 @@ def bookmark_manager_stub(stubs):
objreg.delete('bookmark-manager')
@pytest.fixture
def web_history_stub(init_sql, stubs):
"""Fixture which provides a fake web-history object."""
stub = stubs.WebHistoryStub()
objreg.register('web-history', stub)
yield stub
objreg.delete('web-history')
@pytest.fixture
def session_manager_stub(stubs):
"""Fixture which provides a fake session-manager object."""

29
tests/helpers/stubs.py
View File

@ -33,6 +33,7 @@ from qutebrowser.browser import browsertab
from qutebrowser.config import configexc
from qutebrowser.utils import usertypes, utils
from qutebrowser.mainwindow import mainwindow
from qutebrowser.misc import sql
class FakeNetworkCache(QAbstractNetworkCache):
@ -522,6 +523,34 @@ class QuickmarkManagerStub(UrlMarkManagerStub):
self.delete(key)
class WebHistoryStub(sql.SqlTable):
"""Stub for the web-history object."""
def __init__(self):
super().__init__("History", ['url', 'title', 'atime', 'redirect'])
self.completion = sql.SqlTable("CompletionHistory",
['url', 'title', 'last_atime'])
def add_url(self, url, title="", *, redirect=False, atime=None):
self.insert({'url': url, 'title': title, 'atime': atime,
'redirect': redirect})
if not redirect:
self.completion.insert({'url': url,
'title': title,
'last_atime': atime})
def delete_url(self, url):
"""Remove all history entries with the given url.
Args:
url: URL string to delete.
"""
self.delete(url, 'url')
self.completion.delete(url, 'url')
class HostBlockerStub:
"""Stub for the host-blocker object."""

23
tests/unit/completion/test_completionmodel.py
View File

@ -23,7 +23,11 @@ from unittest import mock
import hypothesis
from hypothesis import strategies
from qutebrowser.completion.models import completionmodel
import pytest
from PyQt5.QtCore import QModelIndex
from qutebrowser.completion.models import completionmodel, listcategory
from qutebrowser.commands import cmdexc
@hypothesis.given(strategies.lists(min_size=0, max_size=3,
@ -72,3 +76,20 @@ def test_set_pattern(pat):
model.add_category(c)
model.set_pattern(pat)
assert all(c.set_pattern.called_with([pat]) for c in cats)
def test_delete_cur_item():
func = mock.Mock()
model = completionmodel.CompletionModel()
cat = listcategory.ListCategory('', [('foo', 'bar')], delete_func=func)
model.add_category(cat)
parent = model.index(0, 0)
model.delete_cur_item(model.index(0, 0, parent))
func.assert_called_once_with(['foo', 'bar'])
def test_delete_cur_item_no_cat():
"""Test completion_item_del with no selected category."""
model = completionmodel.CompletionModel()
with pytest.raises(cmdexc.CommandError, match='No category selected'):
model.delete_cur_item(QModelIndex())

26
tests/unit/completion/test_completionwidget.py
View File

@ -215,30 +215,22 @@ def test_completion_show(show, rows, quick_complete, completionview,
def test_completion_item_del(completionview):
"""Test that completion_item_del invokes delete_cur_item in the model."""
func = mock.Mock()
model = completionmodel.CompletionModel(delete_cur_item=func)
model.add_category(listcategory.ListCategory('', [('foo',)]))
model = completionmodel.CompletionModel()
cat = listcategory.ListCategory('', [('foo', 'bar')], delete_func=func)
model.add_category(cat)
completionview.set_model(model)
completionview.completion_item_focus('next')
completionview.completion_item_del()
assert func.called
func.assert_called_once_with(['foo', 'bar'])
def test_completion_item_del_no_selection(completionview):
"""Test that completion_item_del with no selected index."""
"""Test that completion_item_del with an invalid index."""
func = mock.Mock()
model = completionmodel.CompletionModel(delete_cur_item=func)
model.add_category(listcategory.ListCategory('', [('foo',)]))
model = completionmodel.CompletionModel()
cat = listcategory.ListCategory('', [('foo',)], delete_func=func)
model.add_category(cat)
completionview.set_model(model)
with pytest.raises(cmdexc.CommandError, match='No item selected!'):
completionview.completion_item_del()
assert not func.called
def test_completion_item_del_no_func(completionview):
"""Test completion_item_del with no delete_cur_item in the model."""
model = completionmodel.CompletionModel()
model.add_category(listcategory.ListCategory('', [('foo',)]))
completionview.set_model(model)
completionview.completion_item_focus('next')
with pytest.raises(cmdexc.CommandError, match='Cannot delete this item.'):
completionview.completion_item_del()
func.assert_not_called

120
tests/unit/completion/test_models.py
View File

@ -20,6 +20,7 @@
"""Tests for completion models."""
import collections
import unittest.mock
from datetime import datetime
import pytest
@ -28,6 +29,7 @@ from PyQt5.QtWidgets import QTreeView
from qutebrowser.completion.models import miscmodels, urlmodel, configmodel
from qutebrowser.config import sections, value
from qutebrowser.utils import objreg
from qutebrowser.misc import sql
@ -114,23 +116,6 @@ def _patch_config_section_desc(monkeypatch, stubs, symbol):
monkeypatch.setattr(symbol, section_desc)
def _mock_view_index(model, category_num, child_num, qtbot):
"""Create a tree view from a model and set the current index.
Args:
model: model to create a fake view for.
category_idx: index of the category to select.
child_idx: index of the child item under that category to select.
"""
view = QTreeView()
qtbot.add_widget(view)
view.setModel(model)
parent = model.index(category_num, 0)
child = model.index(child_num, 0, parent=parent)
view.setCurrentIndex(child)
return view
@pytest.fixture
def quickmarks(quickmark_manager_stub):
"""Pre-populate the quickmark-manager stub with some quickmarks."""
@ -152,24 +137,29 @@ def bookmarks(bookmark_manager_stub):
])
return bookmark_manager_stub
@pytest.fixture
def web_history_stub(stubs, init_sql):
def history_completion_table(init_sql):
return sql.SqlTable("CompletionHistory", ['url', 'title', 'last_atime'])
@pytest.fixture
def web_history(web_history_stub, init_sql):
def web_history(web_history_stub):
"""Pre-populate the web-history database."""
web_history_stub.insert({'url': 'http://qutebrowser.org',
'title': 'qutebrowser',
'last_atime': datetime(2015, 9, 5).timestamp()})
web_history_stub.insert({'url': 'https://python.org',
'title': 'Welcome to Python.org',
'last_atime': datetime(2016, 3, 8).timestamp()})
web_history_stub.insert({'url': 'https://github.com',
'title': 'https://github.com',
'last_atime': datetime(2016, 5, 1).timestamp()})
web_history_stub.add_url(
url='http://qutebrowser.org',
title='qutebrowser',
atime=datetime(2015, 9, 5).timestamp()
)
web_history_stub.add_url(
url='https://python.org',
title='Welcome to Python.org',
atime=datetime(2016, 3, 8).timestamp()
)
web_history_stub.add_url(
url='https://github.com',
title='https://github.com',
atime=datetime(2016, 5, 1).timestamp()
)
return web_history_stub
@ -280,7 +270,6 @@ def test_url_completion(qtmodeltester, config_stub, web_history, quickmarks,
Verify that:
- quickmarks, bookmarks, and urls are included
- entries are sorted by access time
- redirect entries are not included
- only the most recent entry is included for each url
"""
config_stub.data['completion'] = {'timestamp-format': '%Y-%m-%d'}
@ -331,16 +320,15 @@ def test_url_completion_pattern(config_stub, web_history_stub,
url, title, pattern, rowcount):
"""Test that url completion filters by url and title."""
config_stub.data['completion'] = {'timestamp-format': '%Y-%m-%d'}
web_history_stub.insert({'url': url, 'title': title, 'last_atime': 0})
web_history_stub.add_url(url, title)
model = urlmodel.url()
model.set_pattern(pattern)
# 2, 0 is History
assert model.rowCount(model.index(2, 0)) == rowcount
def test_url_completion_delete_bookmark(qtmodeltester, config_stub,
web_history, quickmarks, bookmarks,
qtbot):
def test_url_completion_delete_bookmark(qtmodeltester, config_stub, bookmarks,
web_history, quickmarks):
"""Test deleting a bookmark from the url completion model."""
config_stub.data['completion'] = {'timestamp-format': '%Y-%m-%d'}
model = urlmodel.url()
@ -348,12 +336,18 @@ def test_url_completion_delete_bookmark(qtmodeltester, config_stub,
qtmodeltester.data_display_may_return_none = True
qtmodeltester.check(model)
# delete item (1, 1) -> (bookmarks, 'https://github.com')
view = _mock_view_index(model, 1, 1, qtbot)
model.delete_cur_item(view)
parent = model.index(1, 0)
idx = model.index(1, 0, parent)
# sanity checks
assert model.data(parent) == "Bookmarks"
assert model.data(idx) == 'https://github.com'
assert 'https://github.com' in bookmarks.marks
len_before = len(bookmarks.marks)
model.delete_cur_item(idx)
assert 'https://github.com' not in bookmarks.marks
assert 'https://python.org' in bookmarks.marks
assert 'http://qutebrowser.org' in bookmarks.marks
assert len_before == len(bookmarks.marks) + 1
def test_url_completion_delete_quickmark(qtmodeltester, config_stub,
@ -366,28 +360,39 @@ def test_url_completion_delete_quickmark(qtmodeltester, config_stub,
qtmodeltester.data_display_may_return_none = True
qtmodeltester.check(model)
# delete item (0, 0) -> (quickmarks, 'ddg' )
view = _mock_view_index(model, 0, 0, qtbot)
model.delete_cur_item(view)
assert 'aw' in quickmarks.marks
parent = model.index(0, 0)
idx = model.index(0, 0, parent)
# sanity checks
assert model.data(parent) == "Quickmarks"
assert model.data(idx) == 'https://duckduckgo.com'
assert 'ddg' in quickmarks.marks
len_before = len(quickmarks.marks)
model.delete_cur_item(idx)
assert 'ddg' not in quickmarks.marks
assert 'wiki' in quickmarks.marks
assert len_before == len(quickmarks.marks) + 1
def test_url_completion_delete_history(qtmodeltester, config_stub,
web_history, quickmarks, bookmarks,
qtbot):
"""Test that deleting a history entry is a noop."""
web_history_stub, web_history,
quickmarks, bookmarks):
"""Test deleting a history entry."""
config_stub.data['completion'] = {'timestamp-format': '%Y-%m-%d'}
model = urlmodel.url()
model.set_pattern('')
qtmodeltester.data_display_may_return_none = True
qtmodeltester.check(model)
hist_before = list(web_history)
view = _mock_view_index(model, 2, 0, qtbot)
model.delete_cur_item(view)
assert list(web_history) == hist_before
parent = model.index(2, 0)
idx = model.index(1, 0, parent)
# sanity checks
assert model.data(parent) == "History"
assert model.data(idx) == 'https://python.org'
model.delete_cur_item(idx)
assert not web_history_stub.contains('url', 'https://python.org')
def test_session_completion(qtmodeltester, session_manager_stub):
@ -431,7 +436,7 @@ def test_tab_completion(qtmodeltester, fake_web_tab, app_stub, win_registry,
})
def test_tab_completion_delete(qtmodeltester, fake_web_tab, qtbot, app_stub,
def test_tab_completion_delete(qtmodeltester, fake_web_tab, app_stub,
win_registry, tabbed_browser_stubs):
"""Verify closing a tab by deleting it from the completion widget."""
tabbed_browser_stubs[0].tabs = [
@ -447,9 +452,14 @@ def test_tab_completion_delete(qtmodeltester, fake_web_tab, qtbot, app_stub,
qtmodeltester.data_display_may_return_none = True
qtmodeltester.check(model)
view = _mock_view_index(model, 0, 1, qtbot)
qtbot.add_widget(view)
model.delete_cur_item(view)
parent = model.index(0, 0)
idx = model.index(1, 0, parent)
# sanity checks
assert model.data(parent) == "0"
assert model.data(idx) == '0/2'
model.delete_cur_item(idx)
actual = [tab.url() for tab in tabbed_browser_stubs[0].tabs]
assert actual == [QUrl('https://github.com'),
QUrl('https://duckduckgo.com')]