From 6fc61d12fc538c4e129b1e3f31f2cfc8dee6bfdf Mon Sep 17 00:00:00 2001
From: Ryan Roden-Corrent <ryan@rcorre.net>
Date: Wed, 7 Jun 2017 22:45:14 -0400
Subject: [PATCH] Assorted small fixes for sql code review.

---
 qutebrowser/browser/qutescheme.py             |  2 +-
 qutebrowser/completion/completionwidget.py    | 19 +++++++------
 .../completion/models/completionmodel.py      | 12 ++++----
 qutebrowser/completion/models/listcategory.py |  3 +-
 qutebrowser/completion/models/sqlcategory.py  |  6 ++--
 qutebrowser/completion/models/urlmodel.py     |  3 +-
 qutebrowser/misc/sql.py                       | 28 +++++++++++++------
 tests/end2end/features/misc.feature           |  2 --
 tests/end2end/features/test_history_bdd.py    | 19 ++++++-------
 tests/helpers/utils.py                        | 13 +++++++++
 tests/unit/browser/test_qutescheme.py         | 22 ---------------
 tests/unit/browser/webkit/test_history.py     | 20 ++++---------
 .../unit/completion/test_completionwidget.py  |  4 +--
 tests/unit/completion/test_listcategory.py    | 16 ++---------
 tests/unit/completion/test_sqlcategory.py     | 24 ++++------------
 tests/unit/utils/test_version.py              |  2 +-
 16 files changed, 83 insertions(+), 112 deletions(-)

diff --git a/qutebrowser/browser/qutescheme.py b/qutebrowser/browser/qutescheme.py
index 4ebc626d3..0c495d28b 100644
--- a/qutebrowser/browser/qutescheme.py
+++ b/qutebrowser/browser/qutescheme.py
@@ -213,7 +213,7 @@ def qute_history(url):
             offset = QUrlQuery(url).queryItemValue("offset")
             offset = int(offset) if offset else None
         except ValueError as e:
-            raise QuteSchemeError("Query parameter start_time is invalid", e)
+            raise QuteSchemeError("Query parameter offset is invalid", e)
         # Use start_time in query or current time.
         try:
             start_time = QUrlQuery(url).queryItemValue("start_time")
diff --git a/qutebrowser/completion/completionwidget.py b/qutebrowser/completion/completionwidget.py
index 711301b4c..f4d4d80b7 100644
--- a/qutebrowser/completion/completionwidget.py
+++ b/qutebrowser/completion/completionwidget.py
@@ -111,7 +111,6 @@ class CompletionView(QTreeView):
         # objreg.get('config').changed.connect(self.init_command_completion)
         objreg.get('config').changed.connect(self._on_config_changed)
 
-        self._column_widths = (30, 70, 0)
         self._active = False
 
         self._delegate = completiondelegate.CompletionItemDelegate(self)
@@ -150,7 +149,8 @@ class CompletionView(QTreeView):
     def _resize_columns(self):
         """Resize the completion columns based on column_widths."""
         width = self.size().width()
-        pixel_widths = [(width * perc // 100) for perc in self._column_widths]
+        column_widths = self.model.column_widths
+        pixel_widths = [(width * perc // 100) for perc in column_widths]
 
         if self.verticalScrollBar().isVisible():
             delta = self.style().pixelMetric(QStyle.PM_ScrollBarExtent) + 5
@@ -280,14 +280,8 @@ class CompletionView(QTreeView):
 
         model.setParent(self)
         self.setModel(model)
-        self._column_widths = model.column_widths
         self._active = True
-
-        if (config.get('completion', 'show') == 'always' and
-                model.count() > 0):
-            self.show()
-        else:
-            self.hide()
+        self._maybe_show()
 
         for i in range(model.rowCount()):
             self.expand(model.index(i, 0))
@@ -297,7 +291,14 @@ class CompletionView(QTreeView):
             self.model().set_pattern(pattern)
             self._resize_columns()
             self._maybe_update_geometry()
+            self._maybe_show()
+
+    def _maybe_show(self):
+        if (config.get('completion', 'show') == 'always' and
+                model.count() > 0):
             self.show()
+        else:
+            self.hide()
 
     def _maybe_update_geometry(self):
         """Emit the update_geometry signal if the config says so."""
diff --git a/qutebrowser/completion/models/completionmodel.py b/qutebrowser/completion/models/completionmodel.py
index cbc20470c..2dec0ed46 100644
--- a/qutebrowser/completion/models/completionmodel.py
+++ b/qutebrowser/completion/models/completionmodel.py
@@ -78,12 +78,14 @@ class CompletionModel(QAbstractItemModel):
         if not index.isValid() or role != Qt.DisplayRole:
             return None
         if not index.parent().isValid():
+            # category header
             if index.column() == 0:
                 return self._categories[index.row()].name
-        else:
-            cat = self._categories[index.parent().row()]
-            idx = cat.index(index.row(), index.column())
-            return cat.data(idx)
+            return None
+        # item
+        cat = self._categories[index.parent().row()]
+        idx = cat.index(index.row(), index.column())
+        return cat.data(idx)
 
     def flags(self, index):
         """Return the item flags for index.
@@ -132,7 +134,7 @@ class CompletionModel(QAbstractItemModel):
             # categories have no parent
             return QModelIndex()
         row = self._categories.index(parent_cat)
-        return self.createIndex(row, 0, None)
+        return self.createIndex(row, index.column(), None)
 
     def rowCount(self, parent=QModelIndex()):
         """Override QAbstractItemModel::rowCount."""
diff --git a/qutebrowser/completion/models/listcategory.py b/qutebrowser/completion/models/listcategory.py
index 75cc2642c..b22d795be 100644
--- a/qutebrowser/completion/models/listcategory.py
+++ b/qutebrowser/completion/models/listcategory.py
@@ -71,8 +71,7 @@ class ListCategory(QSortFilterProxyModel):
             parent: The parent item QModelIndex.
 
         Return:
-            True if self.pattern is contained in item, or if it's a root item
-            (category). False in all other cases
+            True if self.pattern is contained in item.
         """
         if not self.pattern:
             return True
diff --git a/qutebrowser/completion/models/sqlcategory.py b/qutebrowser/completion/models/sqlcategory.py
index e2801b072..2060f383f 100644
--- a/qutebrowser/completion/models/sqlcategory.py
+++ b/qutebrowser/completion/models/sqlcategory.py
@@ -28,6 +28,7 @@ from qutebrowser.utils import debug
 
 
 class SqlCategory(QSqlQueryModel):
+
     """Wraps a SqlQuery for use as a completion category."""
 
     def __init__(self, name, *, filter_fields, sort_by=None, sort_order=None,
@@ -57,12 +58,11 @@ class SqlCategory(QSqlQueryModel):
         if group_by:
             querystr += ' group by {}'.format(group_by)
         if sort_by:
-            assert sort_order in ['asc', 'desc']
+            assert sort_order in ['asc', 'desc'], sort_order
             querystr += ' order by {} {}'.format(sort_by, sort_order)
 
-        self._query = sql.Query(querystr)
+        self._query = sql.Query(querystr, forward_only=False)
         self._param_count = len(filter_fields)
-        self.columns_to_filter = None
 
         # map filter_fields to indices
         col_query = sql.Query('SELECT * FROM {} LIMIT 1'.format(name))
diff --git a/qutebrowser/completion/models/urlmodel.py b/qutebrowser/completion/models/urlmodel.py
index 2c92815c9..7e42285a6 100644
--- a/qutebrowser/completion/models/urlmodel.py
+++ b/qutebrowser/completion/models/urlmodel.py
@@ -74,7 +74,8 @@ def url():
     model.add_category(listcategory.ListCategory('Bookmarks', bookmarks,
                                                  columns_to_filter=[0, 1]))
 
-    timefmt = config.get('completion', 'timestamp-format')
+    # replace 's to avoid breaking the query
+    timefmt = config.get('completion', 'timestamp-format').replace("'", "`")
     select_time = "strftime('{}', last_atime, 'unixepoch')".format(timefmt)
     hist_cat = sqlcategory.SqlCategory(
         'CompletionHistory', sort_order='desc', sort_by='last_atime',
diff --git a/qutebrowser/misc/sql.py b/qutebrowser/misc/sql.py
index f174248b3..98eebba43 100644
--- a/qutebrowser/misc/sql.py
+++ b/qutebrowser/misc/sql.py
@@ -59,13 +59,23 @@ class Query(QSqlQuery):
 
     """A prepared SQL Query."""
 
-    def __init__(self, querystr):
+    def __init__(self, querystr, forward_only=True):
+        """Prepare a new sql query.
+
+        Args:
+            querystr: String to prepare query from.
+            forward_only: Optimization for queries that will only step forward.
+                          Must be false for completion queries.
+        """
         super().__init__(QSqlDatabase.database())
         log.sql.debug('Preparing SQL query: "{}"'.format(querystr))
-        self.prepare(querystr)
+        if not self.prepare(querystr):
+            raise SqlException('Failed to prepare query "{}"'.format(querystr))
+        self.setForwardOnly(forward_only)
 
     def __iter__(self):
-        assert self.isActive(), "Cannot iterate inactive query"
+        if not self.isActive():
+            raise SqlException("Cannot iterate inactive query")
         rec = self.record()
         fields = [rec.fieldName(i) for i in range(rec.count())]
         rowtype = collections.namedtuple('ResultRow', fields)
@@ -87,8 +97,8 @@ class Query(QSqlQuery):
 
     def value(self):
         """Return the result of a single-value query (e.g. an EXISTS)."""
-        ok = self.next()
-        assert ok, "No result for single-result query"
+        if not self.next():
+            raise SqlException("No result for single-result query")
         return self.record().value(0)
 
 
@@ -152,7 +162,7 @@ class SqlTable(QObject):
         Args:
             field: Field to match.
         """
-        return Query("Select EXISTS(SELECT * FROM {} where {} = ?)"
+        return Query("SELECT EXISTS(SELECT * FROM {} WHERE {} = ?)"
                      .format(self._name, field))
 
     def __len__(self):
@@ -214,17 +224,19 @@ class SqlTable(QObject):
         self.changed.emit()
 
     def delete_all(self):
-        """Remove all row from the table."""
+        """Remove all rows from the table."""
         Query("DELETE FROM {}".format(self._name)).run()
         self.changed.emit()
 
     def select(self, sort_by, sort_order, limit=-1):
-        """Remove all row from the table.
+        """Prepare, run, and return a select statement on this table.
 
         Args:
             sort_by: name of column to sort by.
             sort_order: 'asc' or 'desc'.
             limit: max number of rows in result, defaults to -1 (unlimited).
+
+        Return: A prepared and executed select query.
         """
         q = Query('SELECT * FROM {} ORDER BY {} {} LIMIT ?'
                   .format(self._name, sort_by, sort_order))
diff --git a/tests/end2end/features/misc.feature b/tests/end2end/features/misc.feature
index f6c5ce84a..1a1c0c95b 100644
--- a/tests/end2end/features/misc.feature
+++ b/tests/end2end/features/misc.feature
@@ -702,8 +702,6 @@ Feature: Various utility commands.
         And I wait for "Renderer process was killed" in the log
         And I open data/numbers/3.txt
         Then no crash should happen
-        And the following tabs should be open:
-            - data/numbers/3.txt (active)
 
     ## Other
 
diff --git a/tests/end2end/features/test_history_bdd.py b/tests/end2end/features/test_history_bdd.py
index 197bc3d20..5249e891c 100644
--- a/tests/end2end/features/test_history_bdd.py
+++ b/tests/end2end/features/test_history_bdd.py
@@ -28,17 +28,16 @@ bdd.scenarios('history.feature')
 
 
 @bdd.then(bdd.parsers.parse("the history should contain:\n{expected}"))
-def check_history(quteproc, expected, httpbin):
-    with tempfile.TemporaryDirectory() as tmpdir:
-        path = os.path.join(tmpdir, 'history')
-        quteproc.send_cmd(':debug-dump-history "{}"'.format(path))
-        quteproc.wait_for(category='message', loglevel=logging.INFO,
-                          message='Dumped history to {}.'.format(path))
+def check_history(quteproc, httpbin, tmpdir, expected):
+    path = tmpdir / 'history'
+    quteproc.send_cmd(':debug-dump-history "{}"'.format(path))
+    quteproc.wait_for(category='message', loglevel=logging.INFO,
+                      message='Dumped history to {}.'.format(path))
 
-        with open(path, 'r', encoding='utf-8') as f:
-            # ignore access times, they will differ in each run
-            actual = '\n'.join(re.sub('^\\d+-?', '', line).strip()
-                               for line in f.read().splitlines())
+    with open(path, 'r', encoding='utf-8') as f:
+        # ignore access times, they will differ in each run
+        actual = '\n'.join(re.sub('^\\d+-?', '', line).strip()
+                           for line in f.read())
 
     expected = expected.replace('(port)', str(httpbin.port))
     assert actual == expected
diff --git a/tests/helpers/utils.py b/tests/helpers/utils.py
index 7dbd7dd25..051b7bd50 100644
--- a/tests/helpers/utils.py
+++ b/tests/helpers/utils.py
@@ -170,3 +170,16 @@ def abs_datapath():
     """Get the absolute path to the end2end data directory."""
     file_abs = os.path.abspath(os.path.dirname(__file__))
     return os.path.join(file_abs, '..', 'end2end', 'data')
+
+
+def validate_model(cat, expected):
+    """Check that a category contains the expected items in the given order.
+
+    Args:
+        cat: The category to inspect.
+        expected: A list of tuples containing the expected items.
+    """
+    assert cat.rowCount() == len(expected)
+    for row, items in enumerate(expected):
+        for col, item in enumerate(items):
+            assert cat.data(cat.index(row, col)) == item
diff --git a/tests/unit/browser/test_qutescheme.py b/tests/unit/browser/test_qutescheme.py
index 0f9f43373..87d0662b5 100644
--- a/tests/unit/browser/test_qutescheme.py
+++ b/tests/unit/browser/test_qutescheme.py
@@ -122,7 +122,6 @@ class TestHistoryHandler:
         url = QUrl("qute://history/data?start_time=" + str(start_time))
         _mimetype, data = qutescheme.qute_history(url)
         items = json.loads(data)
-        items = [item for item in items if 'time' in item]  # skip 'next' item
 
         assert len(items) == expected_item_count
 
@@ -132,27 +131,6 @@ class TestHistoryHandler:
             assert item['time'] <= start_time * 1000
             assert item['time'] > end_time * 1000
 
-    @pytest.mark.skip("TODO: do we need next?")
-    @pytest.mark.parametrize("start_time_offset, next_time", [
-        (0, 24*60*60),
-        (24*60*60, 48*60*60),
-        (48*60*60, -1),
-        (72*60*60, -1)
-    ])
-    def test_qutehistory_next(self, start_time_offset, next_time, now):
-        """Ensure qute://history/data returns correct items."""
-        start_time = now - start_time_offset
-        url = QUrl("qute://history/data?start_time=" + str(start_time))
-        _mimetype, data = qutescheme.qute_history(url)
-        items = json.loads(data)
-        items = [item for item in items if 'next' in item]  # 'next' items
-        assert len(items) == 1
-
-        if next_time == -1:
-            assert items[0]["next"] == -1
-        else:
-            assert items[0]["next"] == now - next_time
-
     def test_qute_history_benchmark(self, fake_web_history, benchmark, now):
         entries = []
         for t in range(100000):  # one history per second
diff --git a/tests/unit/browser/webkit/test_history.py b/tests/unit/browser/webkit/test_history.py
index 5386dddd9..841da4ea7 100644
--- a/tests/unit/browser/webkit/test_history.py
+++ b/tests/unit/browser/webkit/test_history.py
@@ -107,14 +107,6 @@ def test_entries_before(hist):
     assert times == [12348, 12347, 12346]
 
 
-def test_save(tmpdir, hist):
-    hist.add_url(QUrl('http://example.com/'), atime=12345)
-    hist.add_url(QUrl('http://www.qutebrowser.org/'), atime=67890)
-
-    hist2 = history.WebHistory()
-    assert list(hist2) == [('http://example.com/', '', 12345, False),
-                           ('http://www.qutebrowser.org/', '', 67890, False)]
-
 
 def test_clear(qtbot, tmpdir, hist, mocker):
     hist.add_url(QUrl('http://example.com/'))
@@ -132,12 +124,11 @@ def test_clear_force(qtbot, tmpdir, hist):
     assert not len(hist)
 
 
-@pytest.mark.parametrize('item', [
+@pytest.mark.parametrize('url, atime, title, redirect', [
     ('http://www.example.com', 12346, 'the title', False),
     ('http://www.example.com', 12346, 'the title', True)
 ])
-def test_add_item(qtbot, hist, item):
-    (url, atime, title, redirect) = item
+def test_add_item(qtbot, hist, url, atime, title, redirect):
     hist.add_url(QUrl(url), atime=atime, title=title, redirect=redirect)
     assert list(hist) == [(url, title, atime, redirect)]
 
@@ -188,13 +179,14 @@ def test_history_interface(qtbot, webview, hist_interface):
 def cleanup_init():
     # prevent test_init from leaking state
     yield
-    try:
-        hist = objreg.get('web-history')
+    hist = objreg.get('web-history', None)
+    if hist is not None:
         hist.setParent(None)
         objreg.delete('web-history')
+    try:
         from PyQt5.QtWebKit import QWebHistoryInterface
         QWebHistoryInterface.setDefaultInterface(None)
-    except:
+    except Exception:
         pass
 
 
diff --git a/tests/unit/completion/test_completionwidget.py b/tests/unit/completion/test_completionwidget.py
index 66f6e7f89..4cc59deea 100644
--- a/tests/unit/completion/test_completionwidget.py
+++ b/tests/unit/completion/test_completionwidget.py
@@ -229,7 +229,7 @@ def test_completion_item_del_no_selection(completionview):
     model = completionmodel.CompletionModel(delete_cur_item=func)
     model.add_category(listcategory.ListCategory('', [('foo',)]))
     completionview.set_model(model)
-    with pytest.raises(cmdexc.CommandError):
+    with pytest.raises(cmdexc.CommandError, match='No item selected!'):
         completionview.completion_item_del()
     assert not func.called
 
@@ -240,5 +240,5 @@ def test_completion_item_del_no_func(completionview):
     model.add_category(listcategory.ListCategory('', [('foo',)]))
     completionview.set_model(model)
     completionview.completion_item_focus('next')
-    with pytest.raises(cmdexc.CommandError):
+    with pytest.raises(cmdexc.CommandError, match='Cannot delete this item.'):
         completionview.completion_item_del()
diff --git a/tests/unit/completion/test_listcategory.py b/tests/unit/completion/test_listcategory.py
index a0c300473..d8d35ea3c 100644
--- a/tests/unit/completion/test_listcategory.py
+++ b/tests/unit/completion/test_listcategory.py
@@ -21,22 +21,10 @@
 
 import pytest
 
+from helpers import utils
 from qutebrowser.completion.models import listcategory
 
 
-def _validate(cat, expected):
-    """Check that a category contains the expected items in the given order.
-
-    Args:
-        cat: The category to inspect.
-        expected: A list of tuples containing the expected items.
-    """
-    assert cat.rowCount() == len(expected)
-    for row, items in enumerate(expected):
-        for col, item in enumerate(items):
-            assert cat.data(cat.index(row, col)) == item
-
-
 @pytest.mark.parametrize('pattern, filter_cols, before, after', [
     ('foo', [0],
      [('foo', '', ''), ('bar', '', '')],
@@ -68,4 +56,4 @@ def test_set_pattern(pattern, filter_cols, before, after):
     cat = listcategory.ListCategory('Foo', before,
                                     columns_to_filter=filter_cols)
     cat.set_pattern(pattern)
-    _validate(cat, after)
+    utils.validate_model(cat, after)
diff --git a/tests/unit/completion/test_sqlcategory.py b/tests/unit/completion/test_sqlcategory.py
index 1b4190d21..2a5b73445 100644
--- a/tests/unit/completion/test_sqlcategory.py
+++ b/tests/unit/completion/test_sqlcategory.py
@@ -21,6 +21,7 @@
 
 import pytest
 
+from helpers import utils
 from qutebrowser.misc import sql
 from qutebrowser.completion.models import sqlcategory
 
@@ -28,19 +29,6 @@ from qutebrowser.completion.models import sqlcategory
 pytestmark = pytest.mark.usefixtures('init_sql')
 
 
-def _validate(cat, expected):
-    """Check that a category contains the expected items in the given order.
-
-    Args:
-        cat: The category to inspect.
-        expected: A list of tuples containing the expected items.
-    """
-    assert cat.rowCount() == len(expected)
-    for row, items in enumerate(expected):
-        for col, item in enumerate(items):
-            assert cat.data(cat.index(row, col)) == item
-
-
 @pytest.mark.parametrize('sort_by, sort_order, data, expected', [
     (None, 'asc',
      [('B', 'C', 'D'), ('A', 'F', 'C'), ('C', 'A', 'G')],
@@ -77,7 +65,7 @@ def test_sorting(sort_by, sort_order, data, expected):
     cat = sqlcategory.SqlCategory('Foo', filter_fields=['a'], sort_by=sort_by,
                                   sort_order=sort_order)
     cat.set_pattern('')
-    _validate(cat, expected)
+    utils.validate_model(cat, expected)
 
 
 @pytest.mark.parametrize('pattern, filter_cols, before, after', [
@@ -133,7 +121,7 @@ def test_set_pattern(pattern, filter_cols, before, after):
     filter_fields = [['a', 'b', 'c'][i] for i in filter_cols]
     cat = sqlcategory.SqlCategory('Foo', filter_fields=filter_fields)
     cat.set_pattern(pattern)
-    _validate(cat, after)
+    utils.validate_model(cat, after)
 
 
 def test_select():
@@ -141,7 +129,7 @@ def test_select():
     table.insert(['foo', 'bar', 'baz'])
     cat = sqlcategory.SqlCategory('Foo', filter_fields=['a'], select='b, c, a')
     cat.set_pattern('')
-    _validate(cat, [('bar', 'baz', 'foo')])
+    utils.validate_model(cat, [('bar', 'baz', 'foo')])
 
 
 def test_where():
@@ -150,7 +138,7 @@ def test_where():
     table.insert(['baz', 'biz', True])
     cat = sqlcategory.SqlCategory('Foo', filter_fields=['a'], where='not c')
     cat.set_pattern('')
-    _validate(cat, [('foo', 'bar', False)])
+    utils.validate_model(cat, [('foo', 'bar', False)])
 
 
 def test_group():
@@ -162,7 +150,7 @@ def test_group():
     cat = sqlcategory.SqlCategory('Foo', filter_fields=['a'],
                                   select='a, max(b)', group_by='a')
     cat.set_pattern('')
-    _validate(cat, [('bar', 3), ('foo', 2)])
+    utils.validate_model(cat, [('bar', 3), ('foo', 2)])
 
 
 def test_entry():
diff --git a/tests/unit/utils/test_version.py b/tests/unit/utils/test_version.py
index 11bf326fb..13dfb09d2 100644
--- a/tests/unit/utils/test_version.py
+++ b/tests/unit/utils/test_version.py
@@ -798,7 +798,6 @@ def test_chromium_version_unpatched(qapp):
     assert version._chromium_version() not in ['', 'unknown', 'unavailable']
 
 
-# pylint: disable=too-many-locals
 @pytest.mark.parametrize(['git_commit', 'frozen', 'style', 'with_webkit',
                           'known_distribution'], [
     (True, False, True, True, True),  # normal
@@ -812,6 +811,7 @@ def test_chromium_version_unpatched(qapp):
 def test_version_output(git_commit, frozen, style, with_webkit,
                         known_distribution, stubs, monkeypatch, init_sql):
     """Test version.version()."""
+    # pylint: disable=too-many-locals
     class FakeWebEngineProfile:
         def httpUserAgent(self):
             return 'Toaster/4.0.4 Chrome/CHROMIUMVERSION Teapot/4.1.8'