From c60753731973946c4e8a565970fcd8af24ac44cf Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Mon, 14 Aug 2017 21:37:43 -0400 Subject: [PATCH 1/5] Consistently format urls in history. Encode urls that are inserted into the history, but do not encode urls for completion (other than removing passwords). Also ensure that urls read from the history text file are formatted consistenly with those added while browsing. Fixes #2903. --- qutebrowser/browser/history.py | 16 ++++++++++----- tests/unit/browser/test_history.py | 32 +++++++++++++++++------------- 2 files changed, 29 insertions(+), 19 deletions(-) diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py index 86e597bcd..60c3f3e36 100644 --- a/qutebrowser/browser/history.py +++ b/qutebrowser/browser/history.py @@ -159,13 +159,12 @@ class WebHistory(sql.SqlTable): return atime = int(atime) if (atime is not None) else int(time.time()) - url_str = url.toString(QUrl.FullyEncoded | QUrl.RemovePassword) - self.insert({'url': url_str, + self.insert({'url': self._format_url(url), 'title': title, 'atime': atime, 'redirect': redirect}) if not redirect: - self.completion.insert({'url': url_str, + self.completion.insert({'url': self._format_completion_url(url), 'title': title, 'last_atime': atime}, replace=True) @@ -249,12 +248,13 @@ class WebHistory(sql.SqlTable): if parsed is None: continue url, title, atime, redirect = parsed - data['url'].append(url) + data['url'].append(self._format_url(url)) data['title'].append(title) data['atime'].append(atime) data['redirect'].append(redirect) if not redirect: - completion_data['url'].append(url) + completion_data['url'].append( + self._format_completion_url(url)) completion_data['title'].append(title) completion_data['last_atime'].append(atime) except ValueError as ex: @@ -263,6 +263,12 @@ class WebHistory(sql.SqlTable): self.insert_batch(data) self.completion.insert_batch(completion_data, replace=True) + def _format_url(self, url): + return url.toString(QUrl.RemovePassword | QUrl.FullyEncoded) + + def _format_completion_url(self, url): + return url.toString(QUrl.RemovePassword) + @cmdutils.register(instance='web-history', debug=True) def debug_dump_history(self, dest): """Dump the history to a file in the old pre-SQL format. diff --git a/tests/unit/browser/test_history.py b/tests/unit/browser/test_history.py index c109f44eb..0c6f65683 100644 --- a/tests/unit/browser/test_history.py +++ b/tests/unit/browser/test_history.py @@ -143,23 +143,27 @@ def test_delete_url(hist): assert completion_diff == {('http://example.com/1', '', 0)} -@pytest.mark.parametrize('url, atime, title, redirect, expected_url', [ - ('http://www.example.com', 12346, 'the title', False, - 'http://www.example.com'), - ('http://www.example.com', 12346, 'the title', True, - 'http://www.example.com'), - ('http://www.example.com/spa ce', 12346, 'the title', False, - 'http://www.example.com/spa%20ce'), - ('https://user:pass@example.com', 12346, 'the title', False, - 'https://user@example.com'), -]) -def test_add_url(qtbot, hist, url, atime, title, redirect, expected_url): +@pytest.mark.parametrize( + 'url, atime, title, redirect, history_url, completion_url', [ + + ('http://www.example.com', 12346, 'the title', False, + 'http://www.example.com', 'http://www.example.com'), + ('http://www.example.com', 12346, 'the title', True, + 'http://www.example.com', None), + ('http://www.example.com/sp ce', 12346, 'the title', False, + 'http://www.example.com/sp%20ce', 'http://www.example.com/sp ce'), + ('https://user:pass@example.com', 12346, 'the title', False, + 'https://user@example.com', 'https://user@example.com'), + ] +) +def test_add_url(qtbot, hist, url, atime, title, redirect, history_url, + completion_url): hist.add_url(QUrl(url), atime=atime, title=title, redirect=redirect) - assert list(hist) == [(expected_url, title, atime, redirect)] - if redirect: + assert list(hist) == [(history_url, title, atime, redirect)] + if completion_url is None: assert not len(hist.completion) else: - assert list(hist.completion) == [(expected_url, title, atime)] + assert list(hist.completion) == [(completion_url, title, atime)] def test_add_url_invalid(qtbot, hist, caplog): From 8c6133e29d3e6ac3489f2d5d4456e5527830613d Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Thu, 17 Aug 2017 07:57:14 -0400 Subject: [PATCH 2/5] Regenerate history completion table if needed. If the HistoryCompletion table is removed, regenerate it from the History table. This allows users to manually edit History, then remove HistoryCompletion to prompt regeneration. See #2903. --- qutebrowser/browser/history.py | 13 +++++++++++++ tests/unit/browser/test_history.py | 14 ++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py index 60c3f3e36..e046dc72b 100644 --- a/qutebrowser/browser/history.py +++ b/qutebrowser/browser/history.py @@ -48,6 +48,8 @@ class WebHistory(sql.SqlTable): super().__init__("History", ['url', 'title', 'atime', 'redirect'], parent=parent) self.completion = CompletionHistory(parent=self) + if len(self.completion) == 0: + self._rebuild_completion() self.create_index('HistoryIndex', 'url') self.create_index('HistoryAtimeIndex', 'atime') self._contains_query = self.contains_query('url') @@ -71,6 +73,17 @@ class WebHistory(sql.SqlTable): def __contains__(self, url): return self._contains_query.run(val=url).value() + def _rebuild_completion(self): + data = {'url': [], 'title': [], 'last_atime': []} + # select the latest entry for each url + q = sql.Query('SELECT url, title, max(atime) AS atime FROM History ' + 'WHERE NOT redirect GROUP BY url') + for entry in q.run(): + data['url'].append(self._format_completion_url(QUrl(entry.url))) + data['title'].append(entry.title) + data['last_atime'].append(entry.atime) + self.completion.insert_batch(data) + def get_recent(self): """Get the most recent history entries.""" return self.select(sort_by='atime', sort_order='desc', limit=100) diff --git a/tests/unit/browser/test_history.py b/tests/unit/browser/test_history.py index 0c6f65683..ebb8ff206 100644 --- a/tests/unit/browser/test_history.py +++ b/tests/unit/browser/test_history.py @@ -353,3 +353,17 @@ def test_debug_dump_history_nonexistent(hist, tmpdir): histfile = tmpdir / 'nonexistent' / 'history' with pytest.raises(cmdexc.CommandError): hist.debug_dump_history(str(histfile)) + + +def test_rebuild_completion(hist): + hist.add_url(QUrl('example.com/1'), redirect=False, atime=1) + hist.add_url(QUrl('example.com/1'), redirect=False, atime=2) + hist.add_url(QUrl('example.com/2%203'), redirect=False, atime=3) + hist.add_url(QUrl('example.com/3'), redirect=True, atime=4) + hist.completion.delete_all() + + hist2 = history.WebHistory() + assert list(hist2.completion) == [ + ('example.com/1', '', 2), + ('example.com/2 3', '', 3), + ] From 5f45b9b40e76bcd5378d45ed781b5965faf02686 Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Sun, 20 Aug 2017 20:43:31 -0400 Subject: [PATCH 3/5] Fix pylint and coverage for history. --- qutebrowser/browser/history.py | 2 +- tests/unit/browser/test_history.py | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py index e046dc72b..0b3cf108e 100644 --- a/qutebrowser/browser/history.py +++ b/qutebrowser/browser/history.py @@ -48,7 +48,7 @@ class WebHistory(sql.SqlTable): super().__init__("History", ['url', 'title', 'atime', 'redirect'], parent=parent) self.completion = CompletionHistory(parent=self) - if len(self.completion) == 0: + if not self.completion: self._rebuild_completion() self.create_index('HistoryIndex', 'url') self.create_index('HistoryAtimeIndex', 'atime') diff --git a/tests/unit/browser/test_history.py b/tests/unit/browser/test_history.py index ebb8ff206..6fa160d31 100644 --- a/tests/unit/browser/test_history.py +++ b/tests/unit/browser/test_history.py @@ -367,3 +367,13 @@ def test_rebuild_completion(hist): ('example.com/1', '', 2), ('example.com/2 3', '', 3), ] + + +def test_no_rebuild_completion(hist): + """Ensure that completion is not regenerated unless completely empty.""" + hist.add_url(QUrl('example.com/1'), redirect=False, atime=1) + hist.add_url(QUrl('example.com/2'), redirect=False, atime=2) + hist.completion.delete('url', 'example.com/2') + + hist2 = history.WebHistory() + assert list(hist2.completion) == [('example.com/1', '', 1)] From d35b47c9d8c139986834f2c6603c8d81fd7e00de Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Mon, 21 Aug 2017 08:38:14 -0400 Subject: [PATCH 4/5] Regenerate history completion on version change. Incrementing _USER_VERSION in the source will cause the HistoryCompletion table to regenerate when users update. This is currently necessary to support some recent formatting fixes, but could be incremented again in the future for other changes. --- qutebrowser/browser/history.py | 8 ++++++++ tests/unit/browser/test_history.py | 17 +++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py index 0b3cf108e..9948992ea 100644 --- a/qutebrowser/browser/history.py +++ b/qutebrowser/browser/history.py @@ -30,6 +30,10 @@ from qutebrowser.utils import (utils, objreg, log, usertypes, message, from qutebrowser.misc import objects, sql +# increment to indicate that HistoryCompletion must be regenerated +_USER_VERSION = 1 + + class CompletionHistory(sql.SqlTable): """History which only has the newest entry for each URL.""" @@ -48,7 +52,10 @@ class WebHistory(sql.SqlTable): super().__init__("History", ['url', 'title', 'atime', 'redirect'], parent=parent) self.completion = CompletionHistory(parent=self) + if sql.Query('pragma user_version').run().value() < _USER_VERSION: + self.completion.delete_all() if not self.completion: + # either the table is out-of-date or the user wiped it manually self._rebuild_completion() self.create_index('HistoryIndex', 'url') self.create_index('HistoryAtimeIndex', 'atime') @@ -83,6 +90,7 @@ class WebHistory(sql.SqlTable): data['title'].append(entry.title) data['last_atime'].append(entry.atime) self.completion.insert_batch(data) + sql.Query('pragma user_version = {}'.format(_USER_VERSION)).run() def get_recent(self): """Get the most recent history entries.""" diff --git a/tests/unit/browser/test_history.py b/tests/unit/browser/test_history.py index 6fa160d31..b49a52a2a 100644 --- a/tests/unit/browser/test_history.py +++ b/tests/unit/browser/test_history.py @@ -377,3 +377,20 @@ def test_no_rebuild_completion(hist): hist2 = history.WebHistory() assert list(hist2.completion) == [('example.com/1', '', 1)] + + +def test_user_version(hist, monkeypatch): + """Ensure that completion is regenerated if user_version is incremented.""" + hist.add_url(QUrl('example.com/1'), redirect=False, atime=1) + hist.add_url(QUrl('example.com/2'), redirect=False, atime=2) + hist.completion.delete('url', 'example.com/2') + + hist2 = history.WebHistory() + assert list(hist2.completion) == [('example.com/1', '', 1)] + + monkeypatch.setattr(history, '_USER_VERSION', history._USER_VERSION + 1) + hist3 = history.WebHistory() + assert list(hist3.completion) == [ + ('example.com/1', '', 1), + ('example.com/2', '', 2), + ] From b89caf04580ffccefcc1f1482f3f9c7f8e7d86f2 Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Tue, 22 Aug 2017 07:19:04 -0400 Subject: [PATCH 5/5] Use REPLACE when rebuilding completion table. When upgrading from an old table that used different url formatting, two entries might map to the same key, so we'll need to replace the previous entry to avoid a primary key conflict. --- qutebrowser/browser/history.py | 4 ++-- tests/unit/browser/test_history.py | 18 ++++++++++++------ 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py index 9948992ea..2d5c263bc 100644 --- a/qutebrowser/browser/history.py +++ b/qutebrowser/browser/history.py @@ -84,12 +84,12 @@ class WebHistory(sql.SqlTable): data = {'url': [], 'title': [], 'last_atime': []} # select the latest entry for each url q = sql.Query('SELECT url, title, max(atime) AS atime FROM History ' - 'WHERE NOT redirect GROUP BY url') + 'WHERE NOT redirect GROUP BY url ORDER BY atime asc') for entry in q.run(): data['url'].append(self._format_completion_url(QUrl(entry.url))) data['title'].append(entry.title) data['last_atime'].append(entry.atime) - self.completion.insert_batch(data) + self.completion.insert_batch(data, replace=True) sql.Query('pragma user_version = {}'.format(_USER_VERSION)).run() def get_recent(self): diff --git a/tests/unit/browser/test_history.py b/tests/unit/browser/test_history.py index b49a52a2a..2462a1141 100644 --- a/tests/unit/browser/test_history.py +++ b/tests/unit/browser/test_history.py @@ -356,16 +356,22 @@ def test_debug_dump_history_nonexistent(hist, tmpdir): def test_rebuild_completion(hist): - hist.add_url(QUrl('example.com/1'), redirect=False, atime=1) - hist.add_url(QUrl('example.com/1'), redirect=False, atime=2) - hist.add_url(QUrl('example.com/2%203'), redirect=False, atime=3) - hist.add_url(QUrl('example.com/3'), redirect=True, atime=4) + hist.insert({'url': 'example.com/1', 'title': 'example1', + 'redirect': False, 'atime': 1}) + hist.insert({'url': 'example.com/1', 'title': 'example1', + 'redirect': False, 'atime': 2}) + hist.insert({'url': 'example.com/2%203', 'title': 'example2', + 'redirect': False, 'atime': 3}) + hist.insert({'url': 'example.com/3', 'title': 'example3', + 'redirect': True, 'atime': 4}) + hist.insert({'url': 'example.com/2 3', 'title': 'example2', + 'redirect': False, 'atime': 5}) hist.completion.delete_all() hist2 = history.WebHistory() assert list(hist2.completion) == [ - ('example.com/1', '', 2), - ('example.com/2 3', '', 3), + ('example.com/1', 'example1', 2), + ('example.com/2 3', 'example2', 5), ]