From 8909e03f1cea998b6e997486e2541bce3d94d71b Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Sun, 10 Dec 2017 15:06:45 -0500 Subject: [PATCH 1/7] Match url completion terms in any order. Perviously, 'foo bar' would match 'foo/bar' but not 'bar/foo'. Now it will match both, using a query with a WHERE clause like: WHERE ((url || title) like '%foo%' AND (url || title) like '%bar%') This does not seem to change the performance benchmark. However, it does create a new query for every character added rather than re-running the same query with different parameters. We could re-use queries if we maintained a list like self._queries=[1_arg_query, 2_arg_query, ...]. However, it isn't clear that such a complexity would be necessary. Resolves #1651. --- qutebrowser/completion/models/histcategory.py | 42 ++++++++++--------- tests/unit/completion/test_histcategory.py | 2 +- tests/unit/completion/test_models.py | 4 +- 3 files changed, 26 insertions(+), 22 deletions(-) diff --git a/qutebrowser/completion/models/histcategory.py b/qutebrowser/completion/models/histcategory.py index fe89dc79b..03ddb5ff8 100644 --- a/qutebrowser/completion/models/histcategory.py +++ b/qutebrowser/completion/models/histcategory.py @@ -37,21 +37,6 @@ class HistoryCategory(QSqlQueryModel): super().__init__(parent=parent) self.name = "History" - # replace ' in timestamp-format to avoid breaking the query - timestamp_format = config.val.completion.timestamp_format - timefmt = ("strftime('{}', last_atime, 'unixepoch', 'localtime')" - .format(timestamp_format.replace("'", "`"))) - - self._query = sql.Query(' '.join([ - "SELECT url, title, {}".format(timefmt), - "FROM CompletionHistory", - # the incoming pattern will have literal % and _ escaped with '\' - # we need to tell sql to treat '\' as an escape character - "WHERE ((url || title) LIKE :pat escape '\\')", - self._atime_expr(), - "ORDER BY last_atime DESC", - ]), forward_only=False) - # advertise that this model filters by URL and title self.columns_to_filter = [0, 1] self.delete_func = delete_func @@ -86,11 +71,30 @@ class HistoryCategory(QSqlQueryModel): # escape to treat a user input % or _ as a literal, not a wildcard pattern = pattern.replace('%', '\\%') pattern = pattern.replace('_', '\\_') - # treat spaces as wildcards to match any of the typed words - pattern = re.sub(r' +', '%', pattern) - pattern = '%{}%'.format(pattern) + words = ['%{}%'.format(w) for w in pattern.split(' ')] + + wheres = ' AND '.join([ + "(url || title) LIKE :pat{} escape '\\'".format(i) + for i in range(len(words))]) + + # replace ' in timestamp-format to avoid breaking the query + timestamp_format = config.val.completion.timestamp_format + timefmt = ("strftime('{}', last_atime, 'unixepoch', 'localtime')" + .format(timestamp_format.replace("'", "`"))) + + self._query = sql.Query(' '.join([ + "SELECT url, title, {}".format(timefmt), + "FROM CompletionHistory", + # the incoming pattern will have literal % and _ escaped with '\' + # we need to tell sql to treat '\' as an escape character + 'WHERE ({})'.format(wheres), + self._atime_expr(), + "ORDER BY last_atime DESC", + ]), forward_only=False) + with debug.log_time('sql', 'Running completion query'): - self._query.run(pat=pattern) + self._query.run(**{ + 'pat{}'.format(i): w for i, w in enumerate(words)}) self.setQuery(self._query) def removeRows(self, row, _count, _parent=None): diff --git a/tests/unit/completion/test_histcategory.py b/tests/unit/completion/test_histcategory.py index b87eb6ac2..8458c3311 100644 --- a/tests/unit/completion/test_histcategory.py +++ b/tests/unit/completion/test_histcategory.py @@ -61,7 +61,7 @@ def hist(init_sql, config_stub): ('foo bar', [('foo', ''), ('bar foo', ''), ('xfooyybarz', '')], - [('xfooyybarz', '')]), + [('bar foo', ''), ('xfooyybarz', '')]), ('foo%bar', [('foo%bar', ''), ('foo bar', ''), ('foobar', '')], diff --git a/tests/unit/completion/test_models.py b/tests/unit/completion/test_models.py index 8879f3201..c4d224dcc 100644 --- a/tests/unit/completion/test_models.py +++ b/tests/unit/completion/test_models.py @@ -414,12 +414,12 @@ def test_url_completion_no_bookmarks(qtmodeltester, web_history_populated, ('example.com', 'Site Title', 'am', 1), ('example.com', 'Site Title', 'com', 1), ('example.com', 'Site Title', 'ex com', 1), - ('example.com', 'Site Title', 'com ex', 0), + ('example.com', 'Site Title', 'com ex', 1), ('example.com', 'Site Title', 'ex foo', 0), ('example.com', 'Site Title', 'foo com', 0), ('example.com', 'Site Title', 'exm', 0), ('example.com', 'Site Title', 'Si Ti', 1), - ('example.com', 'Site Title', 'Ti Si', 0), + ('example.com', 'Site Title', 'Ti Si', 1), ('example.com', '', 'foo', 0), ('foo_bar', '', '_', 1), ('foobar', '', '_', 0), From 6a20f9d4c9bcce45c4fb17727b85de8f76096d94 Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Tue, 12 Dec 2017 07:37:31 -0500 Subject: [PATCH 2/7] Cache url query when possible. We don't need to regenerate a new query every keystroke, but rather every time the user adds a new word. --- qutebrowser/completion/models/histcategory.py | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/qutebrowser/completion/models/histcategory.py b/qutebrowser/completion/models/histcategory.py index 03ddb5ff8..da1e1b8ab 100644 --- a/qutebrowser/completion/models/histcategory.py +++ b/qutebrowser/completion/models/histcategory.py @@ -36,6 +36,7 @@ class HistoryCategory(QSqlQueryModel): """Create a new History completion category.""" super().__init__(parent=parent) self.name = "History" + self._query = None # advertise that this model filters by URL and title self.columns_to_filter = [0, 1] @@ -82,15 +83,16 @@ class HistoryCategory(QSqlQueryModel): timefmt = ("strftime('{}', last_atime, 'unixepoch', 'localtime')" .format(timestamp_format.replace("'", "`"))) - self._query = sql.Query(' '.join([ - "SELECT url, title, {}".format(timefmt), - "FROM CompletionHistory", - # the incoming pattern will have literal % and _ escaped with '\' - # we need to tell sql to treat '\' as an escape character - 'WHERE ({})'.format(wheres), - self._atime_expr(), - "ORDER BY last_atime DESC", - ]), forward_only=False) + if not self._query or len(wheres) != len(self._query.boundValues()): + self._query = sql.Query(' '.join([ + "SELECT url, title, {}".format(timefmt), + "FROM CompletionHistory", + # the incoming pattern will have literal % and _ escaped + # we need to tell sql to treat '\' as an escape character + 'WHERE ({})'.format(wheres), + self._atime_expr(), + "ORDER BY last_atime DESC", + ]), forward_only=False) with debug.log_time('sql', 'Running completion query'): self._query.run(**{ From 158cfa1194807f566756ebf14f6df4c674f548d9 Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Tue, 12 Dec 2017 17:28:38 -0500 Subject: [PATCH 3/7] Clean up "any order" SQL query code. - Replace a list with a generator - Add commments to the less obvious parts - Simplify the binding variable names --- qutebrowser/completion/models/histcategory.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/qutebrowser/completion/models/histcategory.py b/qutebrowser/completion/models/histcategory.py index da1e1b8ab..95d905208 100644 --- a/qutebrowser/completion/models/histcategory.py +++ b/qutebrowser/completion/models/histcategory.py @@ -74,9 +74,12 @@ class HistoryCategory(QSqlQueryModel): pattern = pattern.replace('_', '\\_') words = ['%{}%'.format(w) for w in pattern.split(' ')] - wheres = ' AND '.join([ - "(url || title) LIKE :pat{} escape '\\'".format(i) - for i in range(len(words))]) + # build a where clause to match all of the words in any order + # given the search term "a b", the WHERE clause would be: + # ((url || title) like '%a%') AND ((url || title) LIKE '%b') + wheres = ' AND '.join( + "(url || title) LIKE :{} escape '\\'".format(i) + for i in range(len(words))) # replace ' in timestamp-format to avoid breaking the query timestamp_format = config.val.completion.timestamp_format @@ -84,6 +87,8 @@ class HistoryCategory(QSqlQueryModel): .format(timestamp_format.replace("'", "`"))) if not self._query or len(wheres) != len(self._query.boundValues()): + # if the number of words changed, we need to generate a new query + # otherwise, we can reuse the prepared query for performance self._query = sql.Query(' '.join([ "SELECT url, title, {}".format(timefmt), "FROM CompletionHistory", @@ -96,7 +101,7 @@ class HistoryCategory(QSqlQueryModel): with debug.log_time('sql', 'Running completion query'): self._query.run(**{ - 'pat{}'.format(i): w for i, w in enumerate(words)}) + str(i): w for i, w in enumerate(words)}) self.setQuery(self._query) def removeRows(self, row, _count, _parent=None): From 8358c76f869c7c2c02134975445326a948f230d2 Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Tue, 12 Dec 2017 20:26:30 -0500 Subject: [PATCH 4/7] Fix casing of LIKE in comment --- qutebrowser/completion/models/histcategory.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qutebrowser/completion/models/histcategory.py b/qutebrowser/completion/models/histcategory.py index 95d905208..554f7f7f0 100644 --- a/qutebrowser/completion/models/histcategory.py +++ b/qutebrowser/completion/models/histcategory.py @@ -76,7 +76,7 @@ class HistoryCategory(QSqlQueryModel): # build a where clause to match all of the words in any order # given the search term "a b", the WHERE clause would be: - # ((url || title) like '%a%') AND ((url || title) LIKE '%b') + # ((url || title) LIKE '%a%') AND ((url || title) LIKE '%b') wheres = ' AND '.join( "(url || title) LIKE :{} escape '\\'".format(i) for i in range(len(words))) From ae294e92ad7a48b7d209ae831c9aba5432356427 Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Tue, 12 Dec 2017 20:27:06 -0500 Subject: [PATCH 5/7] Remove unused re import --- qutebrowser/completion/models/histcategory.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/qutebrowser/completion/models/histcategory.py b/qutebrowser/completion/models/histcategory.py index 554f7f7f0..6ec203787 100644 --- a/qutebrowser/completion/models/histcategory.py +++ b/qutebrowser/completion/models/histcategory.py @@ -19,8 +19,6 @@ """A completion category that queries the SQL History store.""" -import re - from PyQt5.QtSql import QSqlQueryModel from qutebrowser.misc import sql From 2e36e5151e122550d58bab500cfbe9a34027a077 Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Wed, 13 Dec 2017 08:21:48 -0500 Subject: [PATCH 6/7] Fix comment in histcategory. --- qutebrowser/completion/models/histcategory.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qutebrowser/completion/models/histcategory.py b/qutebrowser/completion/models/histcategory.py index 6ec203787..7db6ca902 100644 --- a/qutebrowser/completion/models/histcategory.py +++ b/qutebrowser/completion/models/histcategory.py @@ -74,7 +74,7 @@ class HistoryCategory(QSqlQueryModel): # build a where clause to match all of the words in any order # given the search term "a b", the WHERE clause would be: - # ((url || title) LIKE '%a%') AND ((url || title) LIKE '%b') + # ((url || title) LIKE '%a%') AND ((url || title) LIKE '%b%') wheres = ' AND '.join( "(url || title) LIKE :{} escape '\\'".format(i) for i in range(len(words))) From 6420037dd9afbb6aaf09fd669e11b543be06f0eb Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Wed, 13 Dec 2017 08:39:34 -0500 Subject: [PATCH 7/7] Fix histcategory query reuse logic. I mistakenly checked the length of wheres instead of words. This fixes that check, renames 'wheres' to 'where_clause' to be clear that it is a string and not an array, and adds a test. --- qutebrowser/completion/models/histcategory.py | 6 ++-- tests/unit/completion/test_histcategory.py | 32 +++++++++++++++++++ 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/qutebrowser/completion/models/histcategory.py b/qutebrowser/completion/models/histcategory.py index 7db6ca902..57a2aa936 100644 --- a/qutebrowser/completion/models/histcategory.py +++ b/qutebrowser/completion/models/histcategory.py @@ -75,7 +75,7 @@ class HistoryCategory(QSqlQueryModel): # build a where clause to match all of the words in any order # given the search term "a b", the WHERE clause would be: # ((url || title) LIKE '%a%') AND ((url || title) LIKE '%b%') - wheres = ' AND '.join( + where_clause = ' AND '.join( "(url || title) LIKE :{} escape '\\'".format(i) for i in range(len(words))) @@ -84,7 +84,7 @@ class HistoryCategory(QSqlQueryModel): timefmt = ("strftime('{}', last_atime, 'unixepoch', 'localtime')" .format(timestamp_format.replace("'", "`"))) - if not self._query or len(wheres) != len(self._query.boundValues()): + if not self._query or len(words) != len(self._query.boundValues()): # if the number of words changed, we need to generate a new query # otherwise, we can reuse the prepared query for performance self._query = sql.Query(' '.join([ @@ -92,7 +92,7 @@ class HistoryCategory(QSqlQueryModel): "FROM CompletionHistory", # the incoming pattern will have literal % and _ escaped # we need to tell sql to treat '\' as an escape character - 'WHERE ({})'.format(wheres), + 'WHERE ({})'.format(where_clause), self._atime_expr(), "ORDER BY last_atime DESC", ]), forward_only=False) diff --git a/tests/unit/completion/test_histcategory.py b/tests/unit/completion/test_histcategory.py index 8458c3311..e42f9b91f 100644 --- a/tests/unit/completion/test_histcategory.py +++ b/tests/unit/completion/test_histcategory.py @@ -93,6 +93,38 @@ def test_set_pattern(pattern, before, after, model_validator, hist): model_validator.validate(after) +def test_set_pattern_repeated(model_validator, hist): + """Validate multiple subsequent calls to set_pattern.""" + hist.insert({'url': 'example.com/foo', 'title': 'title1', 'last_atime': 1}) + hist.insert({'url': 'example.com/bar', 'title': 'title2', 'last_atime': 1}) + hist.insert({'url': 'example.com/baz', 'title': 'title3', 'last_atime': 1}) + cat = histcategory.HistoryCategory() + model_validator.set_model(cat) + + cat.set_pattern('b') + model_validator.validate([ + ('example.com/bar', 'title2'), + ('example.com/baz', 'title3'), + ]) + + cat.set_pattern('ba') + model_validator.validate([ + ('example.com/bar', 'title2'), + ('example.com/baz', 'title3'), + ]) + + cat.set_pattern('ba ') + model_validator.validate([ + ('example.com/bar', 'title2'), + ('example.com/baz', 'title3'), + ]) + + cat.set_pattern('ba z') + model_validator.validate([ + ('example.com/baz', 'title3'), + ]) + + @pytest.mark.parametrize('max_items, before, after', [ (-1, [ ('a', 'a', '2017-04-16'),