Merge pull request #2905 from rcorre/urlencode

Consistently format urls in history.

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,6 +52,11 @@ 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')
         self._contains_query = self.contains_query('url')
@@ -71,6 +80,18 @@ 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 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, replace=True)
+        sql.Query('pragma user_version = {}'.format(_USER_VERSION)).run()
+
     def get_recent(self):
         """Get the most recent history entries."""
         return self.select(sort_by='atime', sort_order='desc', limit=100)
@@ -159,13 +180,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': self._format_url(url),
-        self.insert({'url': url_str,
                      '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 +269,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 +284,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.

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', [
+@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'),
         ('http://www.example.com', 12346, 'the title', True,
-        'http://www.example.com'),
+            'http://www.example.com', None),
-    ('http://www.example.com/spa ce', 12346, 'the title', False,
+        ('http://www.example.com/sp ce', 12346, 'the title', False,
-        'http://www.example.com/spa%20ce'),
+            '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', 'https://user@example.com'),
-])
+    ]
-def test_add_url(qtbot, hist, url, atime, title, redirect, expected_url):
+)
+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)]
+    assert list(hist) == [(history_url, title, atime, redirect)]
-    if 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):
@@ -349,3 +353,50 @@ 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.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', 'example1', 2),
+        ('example.com/2 3', 'example2', 5),
+    ]
+
+
+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)]
+
+
+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),
+    ]