From eb6126906899e5d957bd55adf915468e2ab25c2f Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Sun, 7 May 2017 22:21:46 -0400 Subject: [PATCH] Fix qute://history javascript for SQL. Returning "next" was no longer possible as the SQL query does not fetch more items than necessary. This is solved by using a start time, a limit, and an offset. The offset is needed to prevent fetching duplicate items if multiple entries have the same timestamp. Two of the history tests that relied on qute://history were changed to rely on qute://history/data instead to make them less failure-prone. --- qutebrowser/browser/history.py | 19 +++++++++++++ qutebrowser/browser/qutescheme.py | 26 ++++++++++++----- qutebrowser/javascript/history.js | 34 +++++++++++++++-------- tests/end2end/features/history.feature | 4 +-- tests/unit/browser/webkit/test_history.py | 27 ++++++++++++++++++ 5 files changed, 90 insertions(+), 20 deletions(-) diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py index dc22d37a8..2f7040c21 100644 --- a/qutebrowser/browser/history.py +++ b/qutebrowser/browser/history.py @@ -111,6 +111,25 @@ class WebHistory(sql.SqlTable): rec = result.record() yield self.Entry(*[rec.value(i) for i in range(rec.count())]) + def entries_before(self, latest, limit, offset): + """Iterate non-redirect, non-qute entries occuring before a timestamp. + + Args: + latest: Omit timestamps more recent than this. + limit: Max number of entries to include. + offset: Number of entries to skip. + """ + result = sql.run_query('SELECT * FROM History ' + 'where not redirect ' + 'and not url like "qute://%" ' + 'and atime <= {} ' + 'ORDER BY atime desc ' + 'limit {} offset {}' + .format(latest, limit, offset)) + while result.next(): + rec = result.record() + yield self.Entry(*[rec.value(i) for i in range(rec.count())]) + @cmdutils.register(name='history-clear', instance='web-history') def clear(self, force=False): """Clear all browsing history. diff --git a/qutebrowser/browser/qutescheme.py b/qutebrowser/browser/qutescheme.py index ba8486ff4..6039fe55e 100644 --- a/qutebrowser/browser/qutescheme.py +++ b/qutebrowser/browser/qutescheme.py @@ -186,15 +186,22 @@ def qute_bookmarks(_url): return 'text/html', html -def history_data(start_time): +def history_data(start_time, offset=None): """Return history data. Arguments: - start_time -- select history starting from this timestamp. + start_time: select history starting from this timestamp. + offset: number of items to skip """ - # end is 24hrs earlier than start - end_time = start_time - 24*60*60 - entries = objreg.get('web-history').entries_between(end_time, start_time) + hist = objreg.get('web-history') + if offset is not None: + # end is 24hrs earlier than start + entries = hist.entries_before(start_time, limit=1000, offset=offset) + else: + # end is 24hrs earlier than start + end_time = start_time - 24*60*60 + entries = hist.entries_between(end_time, start_time) + return [{"url": e.url, "title": e.title or e.url, "time": e.atime * 1000} for e in entries] @@ -203,6 +210,11 @@ def history_data(start_time): def qute_history(url): """Handler for qute://history. Display and serve history.""" if url.path() == '/data': + try: + 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) # Use start_time in query or current time. try: start_time = QUrlQuery(url).queryItemValue("start_time") @@ -210,7 +222,7 @@ def qute_history(url): except ValueError as e: raise QuteSchemeError("Query parameter start_time is invalid", e) - return 'text/html', json.dumps(history_data(start_time)) + return 'text/html', json.dumps(history_data(start_time, offset)) else: if ( config.get('content', 'allow-javascript') and @@ -244,7 +256,7 @@ def qute_history(url): (i["url"], i["title"], datetime.datetime.fromtimestamp(i["time"]/1000), QUrl(i["url"]).host()) - for i in history_data(start_time) if "next" not in i + for i in history_data(start_time) ] return 'text/html', jinja.render( diff --git a/qutebrowser/javascript/history.js b/qutebrowser/javascript/history.js index f46ceb49d..a6cf1d7aa 100644 --- a/qutebrowser/javascript/history.js +++ b/qutebrowser/javascript/history.js @@ -23,8 +23,12 @@ window.loadHistory = (function() { // Date of last seen item. var lastItemDate = null; - // The time to load next. + // Each request for new items includes the time of the last item and an + // offset. The offset is equal to the number of items from the previous + // request that had time=nextTime, and causes the next request to skip + // those items to avoid duplicates. var nextTime = null; + var nextOffset = 0; // The URL to fetch data from. var DATA_URL = "qute://history/data"; @@ -157,7 +161,15 @@ window.loadHistory = (function() { return; } - for (var i = 0, len = history.length - 1; i < len; i++) { + if (history.length == 0) { + // Reached end of history + window.onscroll = null; + EOF_MESSAGE.style.display = "block"; + LOAD_LINK.style.display = "none"; + return + } + + for (var i = 0, len = history.length; i < len; i++) { var item = history[i]; var currentItemDate = new Date(item.time); getSessionNode(currentItemDate).appendChild(makeHistoryRow( @@ -166,14 +178,12 @@ window.loadHistory = (function() { lastItemDate = currentItemDate; } - var next = history[history.length - 1].next; - if (next === -1) { - // Reached end of history - window.onscroll = null; - EOF_MESSAGE.style.display = "block"; - LOAD_LINK.style.display = "none"; - } else { - nextTime = next; + nextTime = history[history.length - 1].time + nextOffset = 0; + for (var i = history.length - 1; i >= 0; i--) { + if (history[i].time == nextTime) { + nextOffset++; + } } } @@ -183,9 +193,11 @@ window.loadHistory = (function() { */ function loadHistory() { if (nextTime === null) { - getJSON(DATA_URL, receiveHistory); + var url = DATA_URL.concat("?offset=0"); + getJSON(url, receiveHistory); } else { var url = DATA_URL.concat("?start_time=", nextTime.toString()); + var url = url.concat("&offset=", nextOffset.toString()); getJSON(url, receiveHistory); } } diff --git a/tests/end2end/features/history.feature b/tests/end2end/features/history.feature index 3ef602712..6efd9ad0e 100644 --- a/tests/end2end/features/history.feature +++ b/tests/end2end/features/history.feature @@ -44,14 +44,14 @@ Feature: Page history Scenario: History with an error When I run :open file:///does/not/exist And I wait for "Error while loading file:///does/not/exist: Error opening /does/not/exist: *" in the log - And I open qute://history + And I open qute://history/data Then the page should contain the plaintext "Error loading page: file:///does/not/exist" @qtwebengine_todo: Error page message is not implemented Scenario: History with a 404 When I open status/404 without waiting And I wait for "Error while loading http://localhost:*/status/404: NOT FOUND" in the log - And I open qute://history + And I open qute://history/data Then the page should contain the plaintext "Error loading page: http://localhost:" And the page should contain the plaintext "/status/404" diff --git a/tests/unit/browser/webkit/test_history.py b/tests/unit/browser/webkit/test_history.py index 4f1a3362d..2118a6c84 100644 --- a/tests/unit/browser/webkit/test_history.py +++ b/tests/unit/browser/webkit/test_history.py @@ -80,6 +80,33 @@ def test_get_recent(hist): ] +def test_entries_between(hist): + hist.add_url(QUrl('http://www.example.com/1'), atime=12345) + hist.add_url(QUrl('http://www.example.com/2'), atime=12346) + hist.add_url(QUrl('http://www.example.com/3'), atime=12347) + hist.add_url(QUrl('http://www.example.com/4'), atime=12348) + hist.add_url(QUrl('http://www.example.com/5'), atime=12348) + hist.add_url(QUrl('http://www.example.com/6'), atime=12349) + hist.add_url(QUrl('http://www.example.com/7'), atime=12350) + + times = [x.atime for x in hist.entries_between(12346, 12349)] + assert times == [12349, 12348, 12348, 12347] + + +def test_entries_before(hist): + hist.add_url(QUrl('http://www.example.com/1'), atime=12346) + hist.add_url(QUrl('http://www.example.com/2'), atime=12346) + hist.add_url(QUrl('http://www.example.com/3'), atime=12347) + hist.add_url(QUrl('http://www.example.com/4'), atime=12348) + hist.add_url(QUrl('http://www.example.com/5'), atime=12348) + hist.add_url(QUrl('http://www.example.com/6'), atime=12348) + hist.add_url(QUrl('http://www.example.com/7'), atime=12349) + hist.add_url(QUrl('http://www.example.com/8'), atime=12349) + + times = [x.atime for x in hist.entries_before(12348, limit=3, offset=2)] + 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)