From 4361009546f6f39862cd0ea7d539b55746586a83 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 5 Jan 2021 18:30:21 +0100 Subject: history: Add timer for sql/history init --- qutebrowser/app.py | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/qutebrowser/app.py b/qutebrowser/app.py index d2e9468aa..c596fab55 100644 --- a/qutebrowser/app.py +++ b/qutebrowser/app.py @@ -66,7 +66,7 @@ from qutebrowser.misc import (ipc, savemanager, sessions, crashsignal, earlyinit, sql, cmdhistory, backendproblem, objects, quitter) from qutebrowser.utils import (log, version, message, utils, urlutils, objreg, - usertypes, standarddir, error, qtutils) + usertypes, standarddir, error, qtutils, debug) # pylint: disable=unused-import # We import those to run the cmdutils.register decorators. from qutebrowser.mainwindow.statusbar import command @@ -445,17 +445,18 @@ def _init_modules(*, args): downloads.init() quitter.instance.shutting_down.connect(downloads.shutdown) - try: - log.init.debug("Initializing SQL...") - sql.init(os.path.join(standarddir.data(), 'history.sqlite')) - - log.init.debug("Initializing web history...") - history.init(q_app) - except sql.KnownError as e: - error.handle_fatal_exc(e, 'Error initializing SQL', - pre_text='Error initializing SQL', - no_err_windows=args.no_err_windows) - sys.exit(usertypes.Exit.err_init) + with debug.log_time("init", "Initializing SQL/history"): + try: + log.init.debug("Initializing SQL...") + sql.init(os.path.join(standarddir.data(), 'history.sqlite')) + + log.init.debug("Initializing web history...") + history.init(q_app) + except sql.KnownError as e: + error.handle_fatal_exc(e, 'Error initializing SQL', + pre_text='Error initializing SQL', + no_err_windows=args.no_err_windows) + sys.exit(usertypes.Exit.err_init) log.init.debug("Initializing command history...") cmdhistory.init() -- cgit v1.2.3-54-g00ecf From 2ab12af3e1ae9d9ee76e8ac8aff7a570fd9e8526 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 5 Jan 2021 11:35:13 +0100 Subject: history: Refactor early excludes Move the check to a method and move some end2end to unit tests. --- qutebrowser/browser/history.py | 21 +++++++++++++++------ tests/end2end/features/history.feature | 15 --------------- tests/unit/browser/test_history.py | 8 ++++++++ 3 files changed, 23 insertions(+), 21 deletions(-) diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py index cf944f184..57aa30e94 100644 --- a/qutebrowser/browser/history.py +++ b/qutebrowser/browser/history.py @@ -204,11 +204,22 @@ class WebHistory(sql.SqlTable): except sql.KnownError as e: message.error("Failed to write history: {}".format(e.text())) - def _is_excluded(self, url): + def _is_excluded_from_completion(self, url): """Check if the given URL is excluded from the completion.""" patterns = config.cache['completion.web_history.exclude'] return any(pattern.matches(url) for pattern in patterns) + def _is_excluded_entirely(self, url): + """Check if the given URL is excluded from the entire history. + + This is the case for URLs which can't be visited at a later point; or which are + usually excessively long. + """ + return ( + url.scheme() in ['data', 'view-source'] or + (url.scheme() == 'qute' and url.host() == 'back') + ) + def _rebuild_completion(self): data: Mapping[str, MutableSequence[str]] = { 'url': [], @@ -228,7 +239,7 @@ class WebHistory(sql.SqlTable): self._progress.tick() url = QUrl(entry.url) - if self._is_excluded(url): + if self._is_excluded_from_completion(url): continue data['url'].append(self._format_completion_url(url)) data['title'].append(entry.title) @@ -289,9 +300,7 @@ class WebHistory(sql.SqlTable): @pyqtSlot(QUrl, QUrl, str) def add_from_tab(self, url, requested_url, title): """Add a new history entry as slot, called from a BrowserTab.""" - if any(url.scheme() in ('data', 'view-source') or - (url.scheme(), url.host()) == ('qute', 'back') - for url in (url, requested_url)): + if self._is_excluded_entirely(url) or self._is_excluded_entirely(requested_url): return if url.isEmpty(): # things set via setHtml @@ -331,7 +340,7 @@ class WebHistory(sql.SqlTable): 'atime': atime, 'redirect': redirect}) - if redirect or self._is_excluded(url): + if redirect or self._is_excluded_from_completion(url): return self.completion.insert({ diff --git a/tests/end2end/features/history.feature b/tests/end2end/features/history.feature index 00e22297d..034b1c90a 100644 --- a/tests/end2end/features/history.feature +++ b/tests/end2end/features/history.feature @@ -57,21 +57,6 @@ Feature: Page history And I run :click-element id open-invalid Then "load status for * LoadStatus.success" should be logged - Scenario: History with data URL - When I open data/data_link.html - And I run :click-element id link - And I wait until data:;base64,cXV0ZWJyb3dzZXI= is loaded - Then the history should contain: - http://localhost:(port)/data/data_link.html data: link - - @qtwebkit_skip - Scenario: History with view-source URL - When I open data/title.html - And I run :view-source - And I wait for regex "Changing title for idx \d+ to 'view-source:(http://)?localhost:\d+/data/title.html'" in the log - Then the history should contain: - http://localhost:(port)/data/title.html Test title - Scenario: Clearing history When I run :tab-only And I open data/title.html diff --git a/tests/unit/browser/test_history.py b/tests/unit/browser/test_history.py index cac167028..8f3a487fd 100644 --- a/tests/unit/browser/test_history.py +++ b/tests/unit/browser/test_history.py @@ -210,9 +210,17 @@ class TestAdd: (logging.DEBUG, 'a.com', 'b.com', [('a.com', 'title', 12345, False), ('b.com', 'title', 12345, True)]), (logging.WARNING, 'a.com', '', [('a.com', 'title', 12345, False)]), + (logging.WARNING, '', '', []), + (logging.WARNING, 'data:foo', '', []), (logging.WARNING, 'a.com', 'data:foo', []), + + (logging.WARNING, 'view-source:foo', '', []), + (logging.WARNING, 'a.com', 'view-source:foo', []), + + (logging.WARNING, 'qute://back', '', []), + (logging.WARNING, 'a.com', 'qute://back', []), ]) def test_from_tab(self, web_history, caplog, mock_time, level, url, req_url, expected): -- cgit v1.2.3-54-g00ecf From cc7fedd2108a4166cf9235c1f82be3e9b86fc0f3 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 5 Jan 2021 11:51:52 +0100 Subject: Make HistoryProgress more reusable --- qutebrowser/browser/history.py | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py index 57aa30e94..0640be16c 100644 --- a/qutebrowser/browser/history.py +++ b/qutebrowser/browser/history.py @@ -45,14 +45,24 @@ class HistoryProgress: This makes WebHistory simpler as it can call methods of this class even when we don't want to show a progress dialog (for very small imports). This means tick() and finish() can be called even when start() wasn't. + + Class attributes: + _PROGRESS_THRESHOLD: When to start showing progress dialogs. """ + PROGRESS_THRESHOLD = 1000 + def __init__(self): + self._reset() + + def _reset(self): self._progress = None self._value = 0 def start(self, text, maximum): - """Start showing a progress dialog.""" + """Start showing a progress dialog, if needed.""" + if maximum < self.PROGRESS_THRESHOLD: + return self._progress = QProgressDialog() self._progress.setMinimumDuration(500) self._progress.setLabelText(text) @@ -69,9 +79,13 @@ class HistoryProgress: QApplication.processEvents() def finish(self): - """Finish showing the progress dialog.""" + """Finish showing the progress dialog. + + After this is called, the object can be reused. + """ if self._progress is not None: self._progress.hide() + self._reset() class CompletionMetaInfo(sql.SqlTable): @@ -133,9 +147,6 @@ class WebHistory(sql.SqlTable): completion: A CompletionHistory instance. metainfo: A CompletionMetaInfo instance. _progress: A HistoryProgress instance. - - Class attributes: - _PROGRESS_THRESHOLD: When to start showing progress dialogs. """ # All web history cleared @@ -143,8 +154,6 @@ class WebHistory(sql.SqlTable): # one url cleared url_cleared = pyqtSignal(QUrl) - _PROGRESS_THRESHOLD = 1000 - def __init__(self, progress, parent=None): super().__init__("History", ['url', 'title', 'atime', 'redirect'], constraints={'url': 'NOT NULL', @@ -232,8 +241,7 @@ class WebHistory(sql.SqlTable): 'GROUP BY url ORDER BY atime asc') entries = list(q.run()) - if len(entries) > self._PROGRESS_THRESHOLD: - self._progress.start("Rebuilding completion...", len(entries)) + self._progress.start("Rebuilding completion...", len(entries)) for entry in entries: self._progress.tick() -- cgit v1.2.3-54-g00ecf From adaecfe803eb6da45a4c627b7cd3cea9122b5f5f Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 5 Jan 2021 12:39:14 +0100 Subject: sql: Run a one-time history cleanup --- qutebrowser/browser/history.py | 56 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 51 insertions(+), 5 deletions(-) diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py index 0640be16c..1591a2d1e 100644 --- a/qutebrowser/browser/history.py +++ b/qutebrowser/browser/history.py @@ -32,9 +32,15 @@ from qutebrowser.api import cmdutils from qutebrowser.utils import utils, log, usertypes, message, qtutils from qutebrowser.misc import objects, sql +# Increment for schema changes, or if HistoryCompletion needs to be regenerated. +# +# Changes from 0 -> 1 and 1 -> 2: +# - None (only needs history regeneration) +# +# Changes from 2 -> 3: +# - History cleanup is run +_USER_VERSION = 3 -# increment to indicate that HistoryCompletion must be regenerated -_USER_VERSION = 2 web_history = cast('WebHistory', None) @@ -165,10 +171,12 @@ class WebHistory(sql.SqlTable): # Store the last saved url to avoid duplicate immediate saves. self._last_url = None + version_changed = self._run_migrations() + self.completion = CompletionHistory(parent=self) self.metainfo = CompletionMetaInfo(parent=self) - if sql.Query('pragma user_version').run().value() < _USER_VERSION: + if version_changed: self.completion.delete_all() # Get a string of all patterns @@ -213,6 +221,26 @@ class WebHistory(sql.SqlTable): except sql.KnownError as e: message.error("Failed to write history: {}".format(e.text())) + def _run_migrations(self): + """Run migrations needed, based on the stored user_version. + + NOTE: This runs before self.completion or self.metainfo are available! + + Return: + True if the version changed, False otherwise. + """ + db_version = sql.Query('pragma user_version').run().value() + if db_version in [0, 1]: + pass # Only needs completion rebuild which is done in __init__ + elif db_version == 2: + self._cleanup_history() + else: + assert db_version == _USER_VERSION, db_version + return False + + sql.Query(f'PRAGMA user_version = {_USER_VERSION}').run() + return True + def _is_excluded_from_completion(self, url): """Check if the given URL is excluded from the completion.""" patterns = config.cache['completion.web_history.exclude'] @@ -229,6 +257,25 @@ class WebHistory(sql.SqlTable): (url.scheme() == 'qute' and url.host() == 'back') ) + def _cleanup_history(self): + """Do a one-time cleanup of the entire history. + + This is run only once after the v2.0.0 upgrade, based on the database's + user_version. + """ + q = sql.Query('SELECT url FROM History') + entries = list(q.run()) + + self._progress.start("Cleaning up history...", len(entries)) + + for entry in entries: + self._progress.tick() + url = QUrl(entry.url) + if self._is_excluded_entirely(url): + self.delete('url', entry.url) + + self._progress.finish() + def _rebuild_completion(self): data: Mapping[str, MutableSequence[str]] = { 'url': [], @@ -237,7 +284,7 @@ class WebHistory(sql.SqlTable): } # select the latest entry for each url q = sql.Query('SELECT url, title, max(atime) AS atime FROM History ' - 'WHERE NOT redirect and url NOT LIKE "qute://back%" ' + 'WHERE NOT redirect ' 'GROUP BY url ORDER BY atime asc') entries = list(q.run()) @@ -256,7 +303,6 @@ class WebHistory(sql.SqlTable): self._progress.finish() 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.""" -- cgit v1.2.3-54-g00ecf From 5894e01a93eedd3f1ec4bfe1c5f9bc22dbaaf138 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 5 Jan 2021 18:00:23 +0100 Subject: Rename CompletionMetaInfo to MetaInfo --- qutebrowser/browser/history.py | 14 ++++++++------ tests/unit/browser/test_history.py | 6 +++--- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py index 1591a2d1e..35037ed24 100644 --- a/qutebrowser/browser/history.py +++ b/qutebrowser/browser/history.py @@ -38,6 +38,7 @@ from qutebrowser.misc import objects, sql # - None (only needs history regeneration) # # Changes from 2 -> 3: +# - CompletionMetaInfo is renamed to MetaInfo # - History cleanup is run _USER_VERSION = 3 @@ -94,7 +95,7 @@ class HistoryProgress: self._reset() -class CompletionMetaInfo(sql.SqlTable): +class MetaInfo(sql.SqlTable): """Table containing meta-information for the completion.""" @@ -103,7 +104,8 @@ class CompletionMetaInfo(sql.SqlTable): } def __init__(self, parent=None): - super().__init__("CompletionMetaInfo", ['key', 'value'], + super().__init__("MetaInfo", + ['key', 'value'], constraints={'key': 'PRIMARY KEY'}) for key, default in self.KEYS.items(): if key not in self: @@ -123,8 +125,7 @@ class CompletionMetaInfo(sql.SqlTable): def __getitem__(self, key): self._check_key(key) - query = sql.Query('SELECT value FROM CompletionMetaInfo ' - 'WHERE key = :key') + query = sql.Query('SELECT value FROM MetaInfo WHERE key = :key') return query.run(key=key).value() def __setitem__(self, key, value): @@ -151,7 +152,7 @@ class WebHistory(sql.SqlTable): Attributes: completion: A CompletionHistory instance. - metainfo: A CompletionMetaInfo instance. + metainfo: A MetaInfo instance. _progress: A HistoryProgress instance. """ @@ -174,7 +175,7 @@ class WebHistory(sql.SqlTable): version_changed = self._run_migrations() self.completion = CompletionHistory(parent=self) - self.metainfo = CompletionMetaInfo(parent=self) + self.metainfo = MetaInfo(parent=self) if version_changed: self.completion.delete_all() @@ -234,6 +235,7 @@ class WebHistory(sql.SqlTable): pass # Only needs completion rebuild which is done in __init__ elif db_version == 2: self._cleanup_history() + sql.Query('ALTER TABLE CompletionMetaInfo RENAME TO MetaInfo').run() else: assert db_version == _USER_VERSION, db_version return False diff --git a/tests/unit/browser/test_history.py b/tests/unit/browser/test_history.py index 8f3a487fd..7f52e1062 100644 --- a/tests/unit/browser/test_history.py +++ b/tests/unit/browser/test_history.py @@ -465,11 +465,11 @@ class TestRebuild: assert progress._started == patch_threshold -class TestCompletionMetaInfo: +class TestMetaInfo: @pytest.fixture def metainfo(self): - return history.CompletionMetaInfo() + return history.MetaInfo() def test_contains_keyerror(self, metainfo): with pytest.raises(KeyError): @@ -489,7 +489,7 @@ class TestCompletionMetaInfo: def test_delete_old_key(self, monkeypatch, metainfo): metainfo.insert({'key': 'force_rebuild', 'value': False}) - info2 = history.CompletionMetaInfo() + info2 = history.MetaInfo() monkeypatch.setitem(info2.KEYS, 'force_rebuild', False) assert 'force_rebuild' not in info2 -- cgit v1.2.3-54-g00ecf From cce5bfc5553fdb1ac4c8a59abb0c6ea1a21bb881 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 5 Jan 2021 18:02:58 +0100 Subject: Revert "Rename CompletionMetaInfo to MetaInfo" This reverts commit 3ffc46444aac5c7e1aca3443a1e380308d841b54. Not actually worth the trouble it brings (making databases incompatible between qutebrowser versions). --- qutebrowser/browser/history.py | 14 ++++++-------- tests/unit/browser/test_history.py | 6 +++--- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py index 35037ed24..1591a2d1e 100644 --- a/qutebrowser/browser/history.py +++ b/qutebrowser/browser/history.py @@ -38,7 +38,6 @@ from qutebrowser.misc import objects, sql # - None (only needs history regeneration) # # Changes from 2 -> 3: -# - CompletionMetaInfo is renamed to MetaInfo # - History cleanup is run _USER_VERSION = 3 @@ -95,7 +94,7 @@ class HistoryProgress: self._reset() -class MetaInfo(sql.SqlTable): +class CompletionMetaInfo(sql.SqlTable): """Table containing meta-information for the completion.""" @@ -104,8 +103,7 @@ class MetaInfo(sql.SqlTable): } def __init__(self, parent=None): - super().__init__("MetaInfo", - ['key', 'value'], + super().__init__("CompletionMetaInfo", ['key', 'value'], constraints={'key': 'PRIMARY KEY'}) for key, default in self.KEYS.items(): if key not in self: @@ -125,7 +123,8 @@ class MetaInfo(sql.SqlTable): def __getitem__(self, key): self._check_key(key) - query = sql.Query('SELECT value FROM MetaInfo WHERE key = :key') + query = sql.Query('SELECT value FROM CompletionMetaInfo ' + 'WHERE key = :key') return query.run(key=key).value() def __setitem__(self, key, value): @@ -152,7 +151,7 @@ class WebHistory(sql.SqlTable): Attributes: completion: A CompletionHistory instance. - metainfo: A MetaInfo instance. + metainfo: A CompletionMetaInfo instance. _progress: A HistoryProgress instance. """ @@ -175,7 +174,7 @@ class WebHistory(sql.SqlTable): version_changed = self._run_migrations() self.completion = CompletionHistory(parent=self) - self.metainfo = MetaInfo(parent=self) + self.metainfo = CompletionMetaInfo(parent=self) if version_changed: self.completion.delete_all() @@ -235,7 +234,6 @@ class WebHistory(sql.SqlTable): pass # Only needs completion rebuild which is done in __init__ elif db_version == 2: self._cleanup_history() - sql.Query('ALTER TABLE CompletionMetaInfo RENAME TO MetaInfo').run() else: assert db_version == _USER_VERSION, db_version return False diff --git a/tests/unit/browser/test_history.py b/tests/unit/browser/test_history.py index 7f52e1062..8f3a487fd 100644 --- a/tests/unit/browser/test_history.py +++ b/tests/unit/browser/test_history.py @@ -465,11 +465,11 @@ class TestRebuild: assert progress._started == patch_threshold -class TestMetaInfo: +class TestCompletionMetaInfo: @pytest.fixture def metainfo(self): - return history.MetaInfo() + return history.CompletionMetaInfo() def test_contains_keyerror(self, metainfo): with pytest.raises(KeyError): @@ -489,7 +489,7 @@ class TestMetaInfo: def test_delete_old_key(self, monkeypatch, metainfo): metainfo.insert({'key': 'force_rebuild', 'value': False}) - info2 = history.MetaInfo() + info2 = history.CompletionMetaInfo() monkeypatch.setitem(info2.KEYS, 'force_rebuild', False) assert 'force_rebuild' not in info2 -- cgit v1.2.3-54-g00ecf From bdab7b35b6b3b7cd807f975d3449ac0d1fc68e55 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 5 Jan 2021 18:00:38 +0100 Subject: sql: Add .delete(..., like=True) --- qutebrowser/misc/sql.py | 7 ++++--- tests/unit/misc/test_sql.py | 12 ++++++++++++ 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/qutebrowser/misc/sql.py b/qutebrowser/misc/sql.py index 50ae0a251..ae77dde58 100644 --- a/qutebrowser/misc/sql.py +++ b/qutebrowser/misc/sql.py @@ -318,19 +318,20 @@ class SqlTable(QObject): q.run() return q.value() - def delete(self, field, value, *, optional=False): + def delete(self, field, value, *, optional=False, like=False): """Remove all rows for which `field` equals `value`. Args: field: Field to use as the key. value: Key value to delete. optional: If set, non-existent values are ignored. + like: If set, LIKE is used instead of = Return: The number of rows deleted. """ - q = Query("DELETE FROM {table} where {field} = :val" - .format(table=self._name, field=field)) + op = "LIKE" if like else "=" + q = Query(f"DELETE FROM {self._name} where {field} {op} :val") q.run(val=value) if not q.rows_affected(): if optional: diff --git a/tests/unit/misc/test_sql.py b/tests/unit/misc/test_sql.py index 5f14ebec4..8d628c12b 100644 --- a/tests/unit/misc/test_sql.py +++ b/tests/unit/misc/test_sql.py @@ -181,6 +181,18 @@ def test_delete_optional(qtbot): table.delete('name', 'doesnotexist', optional=True) +def test_delete_like(): + table = sql.SqlTable('Foo', ['val']) + table.insert({'val': 'helloworld'}) + + with pytest.raises(KeyError): + table.delete('val', 'hello%') + + with qtbot.waitSignal(table.changed): + table.delete('val', 'hello%', like=True) + assert not list(table) + + def test_len(): table = sql.SqlTable('Foo', ['name', 'val', 'lucky']) assert len(table) == 0 -- cgit v1.2.3-54-g00ecf From b6a543284af7534e3106e6eba00cb5503d96cce2 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 5 Jan 2021 15:48:45 +0100 Subject: history: Use some more f-strings --- qutebrowser/browser/history.py | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py index 1591a2d1e..0bf2ecd00 100644 --- a/qutebrowser/browser/history.py +++ b/qutebrowser/browser/history.py @@ -219,7 +219,7 @@ class WebHistory(sql.SqlTable): try: yield except sql.KnownError as e: - message.error("Failed to write history: {}".format(e.text())) + message.error(f"Failed to write history: {e.text()}") def _run_migrations(self): """Run migrations needed, based on the stored user_version. @@ -437,20 +437,15 @@ def debug_dump_history(dest): """ dest = os.path.expanduser(dest) - lines = ('{}{} {} {}'.format(int(x.atime), - '-r' * x.redirect, - x.url, - x.title) - for x in web_history.select(sort_by='atime', - sort_order='asc')) + lines = (f'{int(x.atime)}{"-r" * x.redirect} {x.url} {x.title}' + for x in web_history.select(sort_by='atime', sort_order='asc')) try: with open(dest, 'w', encoding='utf-8') as f: f.write('\n'.join(lines)) - message.info("Dumped history to {}".format(dest)) + message.info(f"Dumped history to {dest}") except OSError as e: - raise cmdutils.CommandError('Could not write history: {}' - .format(e)) + raise cmdutils.CommandError(f'Could not write history: {e}') def init(parent=None): -- cgit v1.2.3-54-g00ecf From b2fcc270ec5427df1b77ed28c459d295d77dd28f Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 5 Jan 2021 15:59:55 +0100 Subject: history: Do clean up using SQL There's some more code duplication, but it's more reliable and a lot faster. --- qutebrowser/browser/history.py | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py index 0bf2ecd00..e1d0a5c41 100644 --- a/qutebrowser/browser/history.py +++ b/qutebrowser/browser/history.py @@ -251,6 +251,10 @@ class WebHistory(sql.SqlTable): This is the case for URLs which can't be visited at a later point; or which are usually excessively long. + + NOTE: If you add new filters here, it might be a good idea to adjust the + _USER_VERSION code and _cleanup_history so that older histories get cleaned up + accordingly as well. """ return ( url.scheme() in ['data', 'view-source'] or @@ -263,18 +267,15 @@ class WebHistory(sql.SqlTable): This is run only once after the v2.0.0 upgrade, based on the database's user_version. """ - q = sql.Query('SELECT url FROM History') - entries = list(q.run()) - - self._progress.start("Cleaning up history...", len(entries)) - - for entry in entries: - self._progress.tick() - url = QUrl(entry.url) - if self._is_excluded_entirely(url): - self.delete('url', entry.url) - - self._progress.finish() + terms = [ + 'data:%', + 'view-source:%', + 'qute://back%', + ] + where_clause = ' OR '.join(f"url LIKE '{term}'" for term in terms) + q = sql.Query(f'DELETE FROM History WHERE {where_clause}') + entries = q.run() + log.sql.debug(f"Cleanup removed {entries.rows_affected()} items") def _rebuild_completion(self): data: Mapping[str, MutableSequence[str]] = { -- cgit v1.2.3-54-g00ecf From b4513df5e5ebf751a89fead9f8eed28d383e6078 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 5 Jan 2021 16:02:58 +0100 Subject: Remove qute://pdfjs URLs from history The original URL is already in the history anyways - the qute://pdfjs URL is not really something that gets visited "separately" from the PDF download. With #5000, these URLs can also get very big if the PDF is in a data: URL, see #1099. --- qutebrowser/browser/history.py | 3 ++- tests/unit/browser/test_history.py | 3 +++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py index e1d0a5c41..c8c942528 100644 --- a/qutebrowser/browser/history.py +++ b/qutebrowser/browser/history.py @@ -258,7 +258,7 @@ class WebHistory(sql.SqlTable): """ return ( url.scheme() in ['data', 'view-source'] or - (url.scheme() == 'qute' and url.host() == 'back') + (url.scheme() == 'qute' and url.host() in ['back', 'pdfjs']) ) def _cleanup_history(self): @@ -271,6 +271,7 @@ class WebHistory(sql.SqlTable): 'data:%', 'view-source:%', 'qute://back%', + 'qute://pdfjs%', ] where_clause = ' OR '.join(f"url LIKE '{term}'" for term in terms) q = sql.Query(f'DELETE FROM History WHERE {where_clause}') diff --git a/tests/unit/browser/test_history.py b/tests/unit/browser/test_history.py index 8f3a487fd..c981e661d 100644 --- a/tests/unit/browser/test_history.py +++ b/tests/unit/browser/test_history.py @@ -221,6 +221,9 @@ class TestAdd: (logging.WARNING, 'qute://back', '', []), (logging.WARNING, 'a.com', 'qute://back', []), + + (logging.WARNING, 'qute://pdfjs/', '', []), + (logging.WARNING, 'a.com', 'qute://pdfjs/', []), ]) def test_from_tab(self, web_history, caplog, mock_time, level, url, req_url, expected): -- cgit v1.2.3-54-g00ecf From 9ab17548c819b401e7eca95cabc750353b2248d5 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 5 Jan 2021 16:35:39 +0100 Subject: history: Show progress dialog immediately Even just finding out how many entries we have takes multiple seconds on my (rather recent) system - let's try to show a window as early as possible so that the user knows something is going on. This might be rather distracting if things only take a very short time, but that's probably the exception rather than the rule. --- qutebrowser/browser/history.py | 29 ++++++++++++++++------------- tests/helpers/stubs.py | 5 ++++- tests/unit/browser/test_history.py | 21 ++++++++++----------- 3 files changed, 30 insertions(+), 25 deletions(-) diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py index c8c942528..dab7560dc 100644 --- a/qutebrowser/browser/history.py +++ b/qutebrowser/browser/history.py @@ -51,13 +51,8 @@ class HistoryProgress: This makes WebHistory simpler as it can call methods of this class even when we don't want to show a progress dialog (for very small imports). This means tick() and finish() can be called even when start() wasn't. - - Class attributes: - _PROGRESS_THRESHOLD: When to start showing progress dialogs. """ - PROGRESS_THRESHOLD = 1000 - def __init__(self): self._reset() @@ -65,18 +60,21 @@ class HistoryProgress: self._progress = None self._value = 0 - def start(self, text, maximum): - """Start showing a progress dialog, if needed.""" - if maximum < self.PROGRESS_THRESHOLD: - return + def start(self, text): + """Start showing a progress dialog.""" self._progress = QProgressDialog() - self._progress.setMinimumDuration(500) + self._progress.setMaximum(0) # unknown + self._progress.setMinimumDuration(0) self._progress.setLabelText(text) - self._progress.setMaximum(maximum) self._progress.setCancelButton(None) self._progress.show() QApplication.processEvents() + def set_maximum(self, maximum): + """Set the progress maximum as soon as we know about it.""" + self._progress.setMaximum(maximum) + QApplication.processEvents() + def tick(self): """Increase the displayed progress value.""" self._value += 1 @@ -284,13 +282,18 @@ class WebHistory(sql.SqlTable): 'title': [], 'last_atime': [] } + + self._progress.start("Rebuilding completion...") + # 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') - entries = list(q.run()) + result = q.run() + QApplication.processEvents() + entries = list(result) - self._progress.start("Rebuilding completion...", len(entries)) + self._progress.set_maximum(len(entries)) for entry in entries: self._progress.tick() diff --git a/tests/helpers/stubs.py b/tests/helpers/stubs.py index 47fb78f85..a23d8fd3b 100644 --- a/tests/helpers/stubs.py +++ b/tests/helpers/stubs.py @@ -633,9 +633,12 @@ class FakeHistoryProgress: self._finished = False self._value = 0 - def start(self, _text, _maximum): + def start(self, _text): self._started = True + def set_maximum(self, _maximum): + pass + def tick(self): self._value += 1 diff --git a/tests/unit/browser/test_history.py b/tests/unit/browser/test_history.py index c981e661d..876ffb9bc 100644 --- a/tests/unit/browser/test_history.py +++ b/tests/unit/browser/test_history.py @@ -450,22 +450,17 @@ class TestRebuild: ('http://example.org', '', 2) ] - @pytest.mark.parametrize('patch_threshold', [True, False]) - def test_progress(self, web_history, config_stub, monkeypatch, stubs, - patch_threshold): + def test_progress(self, web_history, config_stub, stubs): web_history.add_url(QUrl('example.com/1'), redirect=False, atime=1) web_history.add_url(QUrl('example.com/2'), redirect=False, atime=2) # Change cached patterns to trigger a completion rebuild web_history.metainfo['excluded_patterns'] = 'http://example.org' - if patch_threshold: - monkeypatch.setattr(history.WebHistory, '_PROGRESS_THRESHOLD', 1) - progress = stubs.FakeHistoryProgress() history.WebHistory(progress=progress) assert progress._value == 2 + assert progress._started assert progress._finished - assert progress._started == patch_threshold class TestCompletionMetaInfo: @@ -512,12 +507,13 @@ class TestHistoryProgress: def test_no_start(self, progress): """Test calling tick/finish without start.""" progress.tick() + assert progress._value == 1 progress.finish() assert progress._progress is None - assert progress._value == 1 + assert progress._value == 0 def test_gui(self, qtbot, progress): - progress.start("Hello World", 42) + progress.start("Hello World") dialog = progress._progress qtbot.add_widget(dialog) progress.tick() @@ -525,9 +521,12 @@ class TestHistoryProgress: assert dialog.isVisible() assert dialog.labelText() == "Hello World" assert dialog.minimum() == 0 - assert dialog.maximum() == 42 assert dialog.value() == 1 - assert dialog.minimumDuration() == 500 + assert dialog.minimumDuration() == 0 + + assert dialog.maximum() == 0 + progress.set_maximum(42) + assert dialog.maximum() == 42 progress.finish() assert not dialog.isVisible() -- cgit v1.2.3-54-g00ecf From 74671c167f6f18a5494ffe44809e6a0e1c6ea8e9 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 5 Jan 2021 18:04:02 +0100 Subject: history: Run migrations on all old versions --- qutebrowser/browser/history.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py index dab7560dc..3a25284e3 100644 --- a/qutebrowser/browser/history.py +++ b/qutebrowser/browser/history.py @@ -228,16 +228,18 @@ class WebHistory(sql.SqlTable): True if the version changed, False otherwise. """ db_version = sql.Query('pragma user_version').run().value() - if db_version in [0, 1]: - pass # Only needs completion rebuild which is done in __init__ - elif db_version == 2: + assert db_version >= 0, db_version + + if db_version != _USER_VERSION: + sql.Query(f'PRAGMA user_version = {_USER_VERSION}').run() + + if db_version < 3: self._cleanup_history() - else: - assert db_version == _USER_VERSION, db_version - return False + return True - sql.Query(f'PRAGMA user_version = {_USER_VERSION}').run() - return True + # FIXME handle too new user_version + assert db_version == _USER_VERSION, db_version + return False def _is_excluded_from_completion(self, url): """Check if the given URL is excluded from the completion.""" -- cgit v1.2.3-54-g00ecf From fcfa069a06ade76d91bac38127f3235c13d78eb1 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 5 Jan 2021 17:51:19 +0100 Subject: sql: Add major/minor user version infra Not used at the moment, but allows for future changes. --- qutebrowser/misc/sql.py | 56 +++++++++++++++++++++++++++++++++++++++++++++ tests/unit/misc/test_sql.py | 51 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+) diff --git a/qutebrowser/misc/sql.py b/qutebrowser/misc/sql.py index ae77dde58..a0d2cb730 100644 --- a/qutebrowser/misc/sql.py +++ b/qutebrowser/misc/sql.py @@ -21,12 +21,54 @@ import collections +import attr from PyQt5.QtCore import QObject, pyqtSignal from PyQt5.QtSql import QSqlDatabase, QSqlQuery, QSqlError from qutebrowser.utils import log, debug +@attr.s +class UserVersion: + + """The version of data stored in the history database. + + When we originally started using user_version, we only used it to signify that the + completion database should be regenerated. However, sometimes there are + backwards-incompatible changes. + + Instead, we now (ab)use the fact that the user_version in sqlite is a 32-bit integer + to store both a major and a minor part. If only the minor part changed, we can deal + with it (there are only new URLs to clean up or somesuch). If the major part changed, + there are backwards-incompatible changes in how the database works, so newer + databases are not compatible with older qutebrowser versions. + """ + + major: int = attr.ib() + minor: int = attr.ib() + + @classmethod + def from_int(cls, num): + """Parse a number from sqlite into a major/minor user version.""" + assert 0 <= num <= 0x7FFF_FFFF, num # signed integer, but shouldn't be negative + major = (num & 0x7FFF_0000) >> 16 + minor = num & 0x0000_FFFF + return cls(major, minor) + + def to_int(self): + """Get a sqlite integer from a major/minor user version.""" + assert 0 <= self.major <= 0x7FFF # signed integer + assert 0 <= self.minor <= 0xFFFF + return self.major << 16 | self.minor + + def __str__(self): + return f'{self.major}.{self.minor}' + + +db_user_version = None # The user version we got from the database +USER_VERSION = UserVersion(0, 3) # The current / newest user version + + class SqliteErrorCode: """Error codes as used by sqlite. @@ -134,6 +176,20 @@ def init(db_path): error.text()) raise_sqlite_error(msg, error) + global db_user_version + version = Query('pragma user_version').run().value() + db_user_version = UserVersion.from_int(version) + + if db_user_version.major > USER_VERSION.major: + raise KnownError( + "Database is too new for this qutebrowser version (database version " + f"{db_user_version}, but {USER_VERSION.major}.x is supported)") + + if db_user_version < USER_VERSION: + log.sql.debug(f"Migrating from version {db_user_version} to {USER_VERSION}") + # FIXME ... + Query(f'PRAGMA user_version = {USER_VERSION.to_int()}').run() + # Enable write-ahead-logging and reduce disk write frequency # see https://sqlite.org/pragma.html and issues #2930 and #3507 Query("PRAGMA journal_mode=WAL").run() diff --git a/tests/unit/misc/test_sql.py b/tests/unit/misc/test_sql.py index 8d628c12b..be5803c18 100644 --- a/tests/unit/misc/test_sql.py +++ b/tests/unit/misc/test_sql.py @@ -21,6 +21,8 @@ import pytest +import hypothesis +from hypothesis import strategies from PyQt5.QtSql import QSqlError from qutebrowser.misc import sql @@ -29,6 +31,55 @@ from qutebrowser.misc import sql pytestmark = pytest.mark.usefixtures('init_sql') +class TestUserVersion: + + @pytest.mark.parametrize('val, major, minor', [ + (0x0008_0001, 8, 1), + (0x7FFF_FFFF, 0x7FFF, 0xFFFF), + ]) + def test_from_int(self, val, major, minor): + version = sql.UserVersion.from_int(val) + assert version.major == major + assert version.minor == minor + + @pytest.mark.parametrize('major, minor, val', [ + (8, 1, 0x0008_0001), + (0x7FFF, 0xFFFF, 0x7FFF_FFFF), + ]) + def test_to_int(self, major, minor, val): + version = sql.UserVersion(major, minor) + assert version.to_int() == val + + @pytest.mark.parametrize('val', [0x8000_0000, -1]) + def test_from_int_invalid(self, val): + with pytest.raises(AssertionError): + sql.UserVersion.from_int(val) + + @pytest.mark.parametrize('major, minor', [ + (-1, 0), + (0, -1), + (0, 0x10000), + (0x8000, 0), + ]) + def test_to_int_invalid(self, major, minor): + version = sql.UserVersion(major, minor) + with pytest.raises(AssertionError): + version.to_int() + + @hypothesis.given(val=strategies.integers(min_value=0, max_value=0x7FFF_FFFF)) + def test_from_int_hypothesis(self, val): + version = sql.UserVersion.from_int(val) + assert version.to_int() == val + + @hypothesis.given( + major=strategies.integers(min_value=0, max_value=0x7FFF), + minor=strategies.integers(min_value=0, max_value=0xFFFF) + ) + def test_to_int_hypothesis(self, major, minor): + version = sql.UserVersion(major, minor) + assert version.from_int(version.to_int()) == version + + @pytest.mark.parametrize('klass', [sql.KnownError, sql.BugError]) def test_sqlerror(klass): text = "Hello World" -- cgit v1.2.3-54-g00ecf From 4414a349d9f44736eb10724e7fa332f422a95db6 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 5 Jan 2021 18:07:05 +0100 Subject: history: Use new user version handling --- qutebrowser/browser/history.py | 42 +++++++++--------------------------------- 1 file changed, 9 insertions(+), 33 deletions(-) diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py index 3a25284e3..d39519bed 100644 --- a/qutebrowser/browser/history.py +++ b/qutebrowser/browser/history.py @@ -32,14 +32,6 @@ from qutebrowser.api import cmdutils from qutebrowser.utils import utils, log, usertypes, message, qtutils from qutebrowser.misc import objects, sql -# Increment for schema changes, or if HistoryCompletion needs to be regenerated. -# -# Changes from 0 -> 1 and 1 -> 2: -# - None (only needs history regeneration) -# -# Changes from 2 -> 3: -# - History cleanup is run -_USER_VERSION = 3 web_history = cast('WebHistory', None) @@ -169,12 +161,18 @@ class WebHistory(sql.SqlTable): # Store the last saved url to avoid duplicate immediate saves. self._last_url = None - version_changed = self._run_migrations() - self.completion = CompletionHistory(parent=self) self.metainfo = CompletionMetaInfo(parent=self) - if version_changed: + if sql.db_user_version != sql.USER_VERSION: + # If the DB user version changed, run a full cleanup and rebuild the + # completion history. + # + # In the future, this could be improved to only be done when actually needed + # - but version changes happen very infrequently, rebuilding everything + # gives us less corner-cases to deal with, and we can run a VACUUM to make + # things smaller. + self._cleanup_history() self.completion.delete_all() # Get a string of all patterns @@ -219,28 +217,6 @@ class WebHistory(sql.SqlTable): except sql.KnownError as e: message.error(f"Failed to write history: {e.text()}") - def _run_migrations(self): - """Run migrations needed, based on the stored user_version. - - NOTE: This runs before self.completion or self.metainfo are available! - - Return: - True if the version changed, False otherwise. - """ - db_version = sql.Query('pragma user_version').run().value() - assert db_version >= 0, db_version - - if db_version != _USER_VERSION: - sql.Query(f'PRAGMA user_version = {_USER_VERSION}').run() - - if db_version < 3: - self._cleanup_history() - return True - - # FIXME handle too new user_version - assert db_version == _USER_VERSION, db_version - return False - def _is_excluded_from_completion(self, url): """Check if the given URL is excluded from the completion.""" patterns = config.cache['completion.web_history.exclude'] -- cgit v1.2.3-54-g00ecf From 5c21ce976e525fa8550831e6fd424e3536692c64 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 5 Jan 2021 18:14:59 +0100 Subject: sql: Only alter tables if user version changed This avoids having to do various unneeded SQL queries on every start, thus increasing startup time. While all those queries are idempotent, doing them on every startup is still kind of expensive and might involve disk I/O. --- qutebrowser/browser/history.py | 15 +++++++++------ qutebrowser/misc/sql.py | 42 +++++++++++++++++++++++++++++------------- 2 files changed, 38 insertions(+), 19 deletions(-) diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py index d39519bed..ecd96eecc 100644 --- a/qutebrowser/browser/history.py +++ b/qutebrowser/browser/history.py @@ -95,17 +95,20 @@ class CompletionMetaInfo(sql.SqlTable): def __init__(self, parent=None): super().__init__("CompletionMetaInfo", ['key', 'value'], constraints={'key': 'PRIMARY KEY'}) - for key, default in self.KEYS.items(): - if key not in self: - self[key] = default - - # force_rebuild is not in use anymore - self.delete('key', 'force_rebuild', optional=True) + if sql.user_version_changed(): + self._init_default_values() + # force_rebuild is not in use anymore + self.delete('key', 'force_rebuild', optional=True) def _check_key(self, key): if key not in self.KEYS: raise KeyError(key) + def _init_default_values(self): + for key, default in self.KEYS.items(): + if key not in self: + self[key] = default + def __contains__(self, key): self._check_key(key) query = self.contains_query('key') diff --git a/qutebrowser/misc/sql.py b/qutebrowser/misc/sql.py index a0d2cb730..84c2d075f 100644 --- a/qutebrowser/misc/sql.py +++ b/qutebrowser/misc/sql.py @@ -69,6 +69,11 @@ db_user_version = None # The user version we got from the database USER_VERSION = UserVersion(0, 3) # The current / newest user version +def user_version_changed(): + """Whether the version stored in the database is different from the current one.""" + return db_user_version != USER_VERSION + + class SqliteErrorCode: """Error codes as used by sqlite. @@ -185,16 +190,17 @@ def init(db_path): "Database is too new for this qutebrowser version (database version " f"{db_user_version}, but {USER_VERSION.major}.x is supported)") - if db_user_version < USER_VERSION: + if user_version_changed(): log.sql.debug(f"Migrating from version {db_user_version} to {USER_VERSION}") - # FIXME ... + # Enable write-ahead-logging and reduce disk write frequency + # see https://sqlite.org/pragma.html and issues #2930 and #3507 + # + # We might already have done this (without a migration) in earlier versions, but + # as those are idempotent, let's make sure we run them once again. + Query("PRAGMA journal_mode=WAL").run() + Query("PRAGMA synchronous=NORMAL").run() Query(f'PRAGMA user_version = {USER_VERSION.to_int()}').run() - # Enable write-ahead-logging and reduce disk write frequency - # see https://sqlite.org/pragma.html and issues #2930 and #3507 - Query("PRAGMA journal_mode=WAL").run() - Query("PRAGMA synchronous=NORMAL").run() - def close(): """Close the SQL connection.""" @@ -321,9 +327,7 @@ class SqlTable(QObject): changed = pyqtSignal() def __init__(self, name, fields, constraints=None, parent=None): - """Create a new table in the SQL database. - - Does nothing if the table already exists. + """Wrapper over a table in the SQL database. Args: name: Name of the table. @@ -332,22 +336,34 @@ class SqlTable(QObject): """ super().__init__(parent) self._name = name + self._create_table(fields, constraints) + + def _create_table(self, fields, constraints): + """Create the table if the database is uninitialized. + + If the table already exists, this does nothing, so it can e.g. be called on + every user_version change. + """ + if not user_version_changed(): + return constraints = constraints or {} column_defs = ['{} {}'.format(field, constraints.get(field, '')) for field in fields] q = Query("CREATE TABLE IF NOT EXISTS {name} ({column_defs})" - .format(name=name, column_defs=', '.join(column_defs))) - + .format(name=self._name, column_defs=', '.join(column_defs))) q.run() def create_index(self, name, field): - """Create an index over this table. + """Create an index over this table if the database is uninitialized. Args: name: Name of the index, should be unique. field: Name of the field to index. """ + if not user_version_changed(): + return + q = Query("CREATE INDEX IF NOT EXISTS {name} ON {table} ({field})" .format(name=name, table=self._name, field=field)) q.run() -- cgit v1.2.3-54-g00ecf From 33dc1288cbfe857ad6e6a46ea4365f125c78f044 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 5 Jan 2021 18:25:27 +0100 Subject: history: Don't query SQL to rebuild completion Instead of using an empty table as a trigger to rebuild, use a simple flag. If the user wants to force a rebuild, they can set the user version to 0 instead. --- qutebrowser/browser/history.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py index ecd96eecc..2d330c269 100644 --- a/qutebrowser/browser/history.py +++ b/qutebrowser/browser/history.py @@ -167,6 +167,8 @@ class WebHistory(sql.SqlTable): self.completion = CompletionHistory(parent=self) self.metainfo = CompletionMetaInfo(parent=self) + rebuild_completion = False + if sql.db_user_version != sql.USER_VERSION: # If the DB user version changed, run a full cleanup and rebuild the # completion history. @@ -176,7 +178,7 @@ class WebHistory(sql.SqlTable): # gives us less corner-cases to deal with, and we can run a VACUUM to make # things smaller. self._cleanup_history() - self.completion.delete_all() + rebuild_completion = True # Get a string of all patterns patterns = config.instance.get_str('completion.web_history.exclude') @@ -184,10 +186,9 @@ class WebHistory(sql.SqlTable): # If patterns changed, update them in database and rebuild completion if self.metainfo['excluded_patterns'] != patterns: self.metainfo['excluded_patterns'] = patterns - self.completion.delete_all() + rebuild_completion = True - if not self.completion: - # either the table is out-of-date or the user wiped it manually + if rebuild_completion: self._rebuild_completion() self.create_index('HistoryIndex', 'url') @@ -266,7 +267,11 @@ class WebHistory(sql.SqlTable): self._progress.start("Rebuilding completion...") - # select the latest entry for each url + # Delete old entries + self.completion.delete_all() + QApplication.processEvents() + + # 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') -- cgit v1.2.3-54-g00ecf From 3106c1a79ece6cc405a6685105fe48e6eec719eb Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 5 Jan 2021 18:39:13 +0100 Subject: sql: Run VACUUM after rebuilding completion See #6007 --- qutebrowser/browser/history.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py index 2d330c269..59dcb7126 100644 --- a/qutebrowser/browser/history.py +++ b/qutebrowser/browser/history.py @@ -293,6 +293,9 @@ class WebHistory(sql.SqlTable): self._progress.finish() + # We might have caused fragmentation - let's clean up. + sql.Query('VACUUM').run() + self.completion.insert_batch(data, replace=True) def get_recent(self): -- cgit v1.2.3-54-g00ecf From e0001cb7d071c4efe0508a50a282bda6549dee05 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 5 Jan 2021 18:49:59 +0100 Subject: history: Improve progress dialog UX --- qutebrowser/browser/history.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py index 59dcb7126..362d0c3e9 100644 --- a/qutebrowser/browser/history.py +++ b/qutebrowser/browser/history.py @@ -59,6 +59,7 @@ class HistoryProgress: self._progress.setMinimumDuration(0) self._progress.setLabelText(text) self._progress.setCancelButton(None) + self._progress.setAutoClose(False) self._progress.show() QApplication.processEvents() @@ -265,7 +266,11 @@ class WebHistory(sql.SqlTable): 'last_atime': [] } - self._progress.start("Rebuilding completion...") + self._progress.start( + "Rebuilding completion...
" + "This is a one-time operation and happens because the database version " + "or completion.web_history.exclude was changed." + ) # Delete old entries self.completion.delete_all() @@ -291,12 +296,16 @@ class WebHistory(sql.SqlTable): data['title'].append(entry.title) data['last_atime'].append(entry.atime) - self._progress.finish() + self._progress.set_maximum(0) # We might have caused fragmentation - let's clean up. sql.Query('VACUUM').run() + QApplication.processEvents() self.completion.insert_batch(data, replace=True) + QApplication.processEvents() + + self._progress.finish() def get_recent(self): """Get the most recent history entries.""" -- cgit v1.2.3-54-g00ecf From 91bdc748ad3e6859a5311977c877bb37c35337c8 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 5 Jan 2021 18:56:22 +0100 Subject: history: Skip cleanup steps if tables are empty --- qutebrowser/browser/history.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py index 362d0c3e9..83c4ef2f7 100644 --- a/qutebrowser/browser/history.py +++ b/qutebrowser/browser/history.py @@ -178,7 +178,10 @@ class WebHistory(sql.SqlTable): # - but version changes happen very infrequently, rebuilding everything # gives us less corner-cases to deal with, and we can run a VACUUM to make # things smaller. - self._cleanup_history() + # + # If no history exists, we skip cleaning it up. + if self: + self._cleanup_history() rebuild_completion = True # Get a string of all patterns @@ -189,7 +192,9 @@ class WebHistory(sql.SqlTable): self.metainfo['excluded_patterns'] = patterns rebuild_completion = True - if rebuild_completion: + if rebuild_completion and self.completion: + # If no completion history exists, we don't need to spawn a dialog for + # cleaning it up. self._rebuild_completion() self.create_index('HistoryIndex', 'url') -- cgit v1.2.3-54-g00ecf From 0e4b807beb3897700fbf7695eed009f3c4e7bfb8 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Thu, 14 Jan 2021 10:58:06 +0100 Subject: sql: Fix test --- tests/unit/misc/test_sql.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/misc/test_sql.py b/tests/unit/misc/test_sql.py index be5803c18..3edbbffb0 100644 --- a/tests/unit/misc/test_sql.py +++ b/tests/unit/misc/test_sql.py @@ -232,7 +232,7 @@ def test_delete_optional(qtbot): table.delete('name', 'doesnotexist', optional=True) -def test_delete_like(): +def test_delete_like(qtbot): table = sql.SqlTable('Foo', ['val']) table.insert({'val': 'helloworld'}) -- cgit v1.2.3-54-g00ecf From 0f0a59c279c60f9a327cbf976637387c39bcf7b5 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Thu, 14 Jan 2021 11:21:13 +0100 Subject: sql: Harden rows_affected() See https://doc.qt.io/qt-5/qsqlquery.html#numRowsAffected --- qutebrowser/misc/sql.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/qutebrowser/misc/sql.py b/qutebrowser/misc/sql.py index 84c2d075f..7844253f0 100644 --- a/qutebrowser/misc/sql.py +++ b/qutebrowser/misc/sql.py @@ -307,7 +307,11 @@ class Query: return self.query.record().value(0) def rows_affected(self): - return self.query.numRowsAffected() + assert not self.query.isSelect(), self + assert self.query.isActive(), self + rows = self.query.numRowsAffected() + assert rows != -1 + return rows def bound_values(self): return self.query.boundValues() -- cgit v1.2.3-54-g00ecf From 4d893e75a175d7f05c5627a1dca4e176ce1e70e6 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Thu, 14 Jan 2021 11:22:56 +0100 Subject: sql: Add Query.__bool__ Makes it clearer what the intent is (rather than implicitly falling back on __len__ via Python) and also happens to be a small (most probably insignificant) performance improvement. On my machine, without a __bool__: Name (time in us) Min Max Median test_bool_benchmark 144.2940 879.5960 167.2730 With it: Name (time in us) Min Max Median test_bool_benchmark 133.3990 876.1080 152.1879 --- qutebrowser/misc/sql.py | 6 ++++++ tests/unit/misc/test_sql.py | 20 ++++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/qutebrowser/misc/sql.py b/qutebrowser/misc/sql.py index 7844253f0..090c3a5d2 100644 --- a/qutebrowser/misc/sql.py +++ b/qutebrowser/misc/sql.py @@ -394,6 +394,12 @@ class SqlTable(QObject): q.run() return q.value() + def __bool__(self): + """Check whether there's any data in the table.""" + q = Query(f"SELECT 1 FROM {self._name} LIMIT 1") + q.run() + return q.query.next() + def delete(self, field, value, *, optional=False, like=False): """Remove all rows for which `field` equals `value`. diff --git a/tests/unit/misc/test_sql.py b/tests/unit/misc/test_sql.py index 3edbbffb0..ab34727a9 100644 --- a/tests/unit/misc/test_sql.py +++ b/tests/unit/misc/test_sql.py @@ -255,6 +255,26 @@ def test_len(): assert len(table) == 3 +def test_bool(): + table = sql.SqlTable('Foo', ['name']) + assert not table + table.insert({'name': 'one'}) + assert table + + +def test_bool_benchmark(benchmark): + table = sql.SqlTable('Foo', ['number']) + + # Simulate a history table + table.create_index('NumberIndex', 'number') + table.insert_batch({'number': [str(i) for i in range(100_000)]}) + + def run(): + assert table + + benchmark(run) + + def test_contains(): table = sql.SqlTable('Foo', ['name', 'val', 'lucky']) table.insert({'name': 'one', 'val': 1, 'lucky': False}) -- cgit v1.2.3-54-g00ecf From 84ad7c990b27264f9ed03ef81a47e9d4acdffaf0 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Thu, 14 Jan 2021 11:53:09 +0100 Subject: sql: Improve/fix rows_affected() tests --- tests/unit/misc/test_sql.py | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/tests/unit/misc/test_sql.py b/tests/unit/misc/test_sql.py index ab34727a9..d7c6fd4d5 100644 --- a/tests/unit/misc/test_sql.py +++ b/tests/unit/misc/test_sql.py @@ -376,10 +376,24 @@ class TestSqlQuery: match='No result for single-result query'): q.value() - def test_num_rows_affected(self): - q = sql.Query('SELECT 0') + def test_num_rows_affected_not_active(self): + with pytest.raises(AssertionError): + q = sql.Query('SELECT 0') + q.rows_affected() + + def test_num_rows_affected_select(self): + with pytest.raises(AssertionError): + q = sql.Query('SELECT 0') + q.run() + q.rows_affected() + + @pytest.mark.parametrize('condition', [0, 1]) + def test_num_rows_affected(self, condition): + table = sql.SqlTable('Foo', ['name']) + table.insert({'name': 'helloworld'}) + q = sql.Query(f'DELETE FROM Foo WHERE {condition}') q.run() - assert q.rows_affected() == 0 + assert q.rows_affected() == condition def test_bound_values(self): q = sql.Query('SELECT :answer') -- cgit v1.2.3-54-g00ecf From 23847d5e0e36feeb6ecc4acd6e5e7500441a887a Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Thu, 14 Jan 2021 11:53:25 +0100 Subject: history: Skip unnecessary check It doesn't really matter if we run a DELETE query on an empty table vs. running a query to find out whether the table is empty. --- qutebrowser/browser/history.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py index 83c4ef2f7..8fe7edb64 100644 --- a/qutebrowser/browser/history.py +++ b/qutebrowser/browser/history.py @@ -178,10 +178,7 @@ class WebHistory(sql.SqlTable): # - but version changes happen very infrequently, rebuilding everything # gives us less corner-cases to deal with, and we can run a VACUUM to make # things smaller. - # - # If no history exists, we skip cleaning it up. - if self: - self._cleanup_history() + self._cleanup_history() rebuild_completion = True # Get a string of all patterns -- cgit v1.2.3-54-g00ecf From fb5e105c1998350cd89512323c641be0d5d9d1b5 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Thu, 14 Jan 2021 11:55:34 +0100 Subject: sql: Remove .delete(like=True) Mostly reverts bdab7b35b6b3b7cd807f975d3449ac0d1fc68e55 as we didn't end up using it in b2fcc270ec5427df1b77ed28c459d295d77dd28f. --- qutebrowser/misc/sql.py | 6 ++---- tests/unit/misc/test_sql.py | 12 ------------ 2 files changed, 2 insertions(+), 16 deletions(-) diff --git a/qutebrowser/misc/sql.py b/qutebrowser/misc/sql.py index 7016e60f6..b14c96c14 100644 --- a/qutebrowser/misc/sql.py +++ b/qutebrowser/misc/sql.py @@ -400,20 +400,18 @@ class SqlTable(QObject): q.run() return q.query.next() - def delete(self, field, value, *, optional=False, like=False): + def delete(self, field, value, *, optional=False): """Remove all rows for which `field` equals `value`. Args: field: Field to use as the key. value: Key value to delete. optional: If set, non-existent values are ignored. - like: If set, LIKE is used instead of = Return: The number of rows deleted. """ - op = "LIKE" if like else "=" - q = Query(f"DELETE FROM {self._name} where {field} {op} :val") + q = Query(f"DELETE FROM {self._name} where {field} = :val") q.run(val=value) if not q.rows_affected(): if optional: diff --git a/tests/unit/misc/test_sql.py b/tests/unit/misc/test_sql.py index d7c6fd4d5..9910b3ef1 100644 --- a/tests/unit/misc/test_sql.py +++ b/tests/unit/misc/test_sql.py @@ -232,18 +232,6 @@ def test_delete_optional(qtbot): table.delete('name', 'doesnotexist', optional=True) -def test_delete_like(qtbot): - table = sql.SqlTable('Foo', ['val']) - table.insert({'val': 'helloworld'}) - - with pytest.raises(KeyError): - table.delete('val', 'hello%') - - with qtbot.waitSignal(table.changed): - table.delete('val', 'hello%', like=True) - assert not list(table) - - def test_len(): table = sql.SqlTable('Foo', ['name', 'val', 'lucky']) assert len(table) == 0 -- cgit v1.2.3-54-g00ecf From 5526dce6c36042f711cdb80f6b1b342cd6b0628c Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Thu, 14 Jan 2021 11:58:30 +0100 Subject: Fix lint --- qutebrowser/browser/history.py | 4 ++-- qutebrowser/misc/sql.py | 11 ++++++----- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py index 8fe7edb64..eaeb274f3 100644 --- a/qutebrowser/browser/history.py +++ b/qutebrowser/browser/history.py @@ -240,8 +240,8 @@ class WebHistory(sql.SqlTable): accordingly as well. """ return ( - url.scheme() in ['data', 'view-source'] or - (url.scheme() == 'qute' and url.host() in ['back', 'pdfjs']) + url.scheme() in {'data', 'view-source'} or + (url.scheme() == 'qute' and url.host() in {'back', 'pdfjs'}) ) def _cleanup_history(self): diff --git a/qutebrowser/misc/sql.py b/qutebrowser/misc/sql.py index b14c96c14..c71ac6eaa 100644 --- a/qutebrowser/misc/sql.py +++ b/qutebrowser/misc/sql.py @@ -39,9 +39,9 @@ class UserVersion: Instead, we now (ab)use the fact that the user_version in sqlite is a 32-bit integer to store both a major and a minor part. If only the minor part changed, we can deal - with it (there are only new URLs to clean up or somesuch). If the major part changed, - there are backwards-incompatible changes in how the database works, so newer - databases are not compatible with older qutebrowser versions. + with it (there are only new URLs to clean up or somesuch). If the major part + changed, there are backwards-incompatible changes in how the database works, so + newer databases are not compatible with older qutebrowser versions. """ major: int = attr.ib() @@ -182,8 +182,8 @@ def init(db_path): raise_sqlite_error(msg, error) global db_user_version - version = Query('pragma user_version').run().value() - db_user_version = UserVersion.from_int(version) + version_int = Query('pragma user_version').run().value() + db_user_version = UserVersion.from_int(version_int) if db_user_version.major > USER_VERSION.major: raise KnownError( @@ -307,6 +307,7 @@ class Query: return self.query.record().value(0) def rows_affected(self): + """Return how many rows were affected by a non-SELECT query.""" assert not self.query.isSelect(), self assert self.query.isActive(), self rows = self.query.numRowsAffected() -- cgit v1.2.3-54-g00ecf From f1b925e37fea016939a35126bfc29a0c3e2d547b Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Thu, 14 Jan 2021 11:59:58 +0100 Subject: history: Simplify HistoryProgress It doesn't actually need to be reusable, also make sure it's cleaned up when unneeded. --- qutebrowser/browser/history.py | 21 +++++----------- tests/unit/browser/test_history.py | 51 ++++++++++++++------------------------ 2 files changed, 25 insertions(+), 47 deletions(-) diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py index eaeb274f3..7a9dd7fe8 100644 --- a/qutebrowser/browser/history.py +++ b/qutebrowser/browser/history.py @@ -46,15 +46,11 @@ class HistoryProgress: """ def __init__(self): - self._reset() - - def _reset(self): - self._progress = None + self._progress = QProgressDialog() self._value = 0 def start(self, text): """Start showing a progress dialog.""" - self._progress = QProgressDialog() self._progress.setMaximum(0) # unknown self._progress.setMinimumDuration(0) self._progress.setLabelText(text) @@ -71,18 +67,13 @@ class HistoryProgress: def tick(self): """Increase the displayed progress value.""" self._value += 1 - if self._progress is not None: - self._progress.setValue(self._value) - QApplication.processEvents() + self._progress.setValue(self._value) + QApplication.processEvents() def finish(self): - """Finish showing the progress dialog. - - After this is called, the object can be reused. - """ - if self._progress is not None: - self._progress.hide() - self._reset() + """Finish showing the progress dialog.""" + self._progress.hide() + self._progress.deleteLater() class CompletionMetaInfo(sql.SqlTable): diff --git a/tests/unit/browser/test_history.py b/tests/unit/browser/test_history.py index 876ffb9bc..1aa873503 100644 --- a/tests/unit/browser/test_history.py +++ b/tests/unit/browser/test_history.py @@ -498,35 +498,22 @@ class TestCompletionMetaInfo: assert metainfo['excluded_patterns'] == value -class TestHistoryProgress: - - @pytest.fixture - def progress(self): - return history.HistoryProgress() - - def test_no_start(self, progress): - """Test calling tick/finish without start.""" - progress.tick() - assert progress._value == 1 - progress.finish() - assert progress._progress is None - assert progress._value == 0 - - def test_gui(self, qtbot, progress): - progress.start("Hello World") - dialog = progress._progress - qtbot.add_widget(dialog) - progress.tick() - - assert dialog.isVisible() - assert dialog.labelText() == "Hello World" - assert dialog.minimum() == 0 - assert dialog.value() == 1 - assert dialog.minimumDuration() == 0 - - assert dialog.maximum() == 0 - progress.set_maximum(42) - assert dialog.maximum() == 42 - - progress.finish() - assert not dialog.isVisible() +def test_history_progress(qtbot): + progress = history.HistoryProgress() + progress.start("Hello World") + dialog = progress._progress + qtbot.add_widget(dialog) + progress.tick() + + assert dialog.isVisible() + assert dialog.labelText() == "Hello World" + assert dialog.minimum() == 0 + assert dialog.value() == 1 + assert dialog.minimumDuration() == 0 + + assert dialog.maximum() == 0 + progress.set_maximum(42) + assert dialog.maximum() == 42 + + progress.finish() + assert not dialog.isVisible() -- cgit v1.2.3-54-g00ecf From fd4f0663e76e9ff9f8e11ac6a8c3b40e4c700bc2 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Thu, 14 Jan 2021 20:10:38 +0100 Subject: Revert "history: Simplify HistoryProgress" Seems to cause random dialogs This reverts commit f1b925e37fea016939a35126bfc29a0c3e2d547b. --- qutebrowser/browser/history.py | 21 +++++++++++----- tests/unit/browser/test_history.py | 51 ++++++++++++++++++++++++-------------- 2 files changed, 47 insertions(+), 25 deletions(-) diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py index 7a9dd7fe8..eaeb274f3 100644 --- a/qutebrowser/browser/history.py +++ b/qutebrowser/browser/history.py @@ -46,11 +46,15 @@ class HistoryProgress: """ def __init__(self): - self._progress = QProgressDialog() + self._reset() + + def _reset(self): + self._progress = None self._value = 0 def start(self, text): """Start showing a progress dialog.""" + self._progress = QProgressDialog() self._progress.setMaximum(0) # unknown self._progress.setMinimumDuration(0) self._progress.setLabelText(text) @@ -67,13 +71,18 @@ class HistoryProgress: def tick(self): """Increase the displayed progress value.""" self._value += 1 - self._progress.setValue(self._value) - QApplication.processEvents() + if self._progress is not None: + self._progress.setValue(self._value) + QApplication.processEvents() def finish(self): - """Finish showing the progress dialog.""" - self._progress.hide() - self._progress.deleteLater() + """Finish showing the progress dialog. + + After this is called, the object can be reused. + """ + if self._progress is not None: + self._progress.hide() + self._reset() class CompletionMetaInfo(sql.SqlTable): diff --git a/tests/unit/browser/test_history.py b/tests/unit/browser/test_history.py index 1aa873503..876ffb9bc 100644 --- a/tests/unit/browser/test_history.py +++ b/tests/unit/browser/test_history.py @@ -498,22 +498,35 @@ class TestCompletionMetaInfo: assert metainfo['excluded_patterns'] == value -def test_history_progress(qtbot): - progress = history.HistoryProgress() - progress.start("Hello World") - dialog = progress._progress - qtbot.add_widget(dialog) - progress.tick() - - assert dialog.isVisible() - assert dialog.labelText() == "Hello World" - assert dialog.minimum() == 0 - assert dialog.value() == 1 - assert dialog.minimumDuration() == 0 - - assert dialog.maximum() == 0 - progress.set_maximum(42) - assert dialog.maximum() == 42 - - progress.finish() - assert not dialog.isVisible() +class TestHistoryProgress: + + @pytest.fixture + def progress(self): + return history.HistoryProgress() + + def test_no_start(self, progress): + """Test calling tick/finish without start.""" + progress.tick() + assert progress._value == 1 + progress.finish() + assert progress._progress is None + assert progress._value == 0 + + def test_gui(self, qtbot, progress): + progress.start("Hello World") + dialog = progress._progress + qtbot.add_widget(dialog) + progress.tick() + + assert dialog.isVisible() + assert dialog.labelText() == "Hello World" + assert dialog.minimum() == 0 + assert dialog.value() == 1 + assert dialog.minimumDuration() == 0 + + assert dialog.maximum() == 0 + progress.set_maximum(42) + assert dialog.maximum() == 42 + + progress.finish() + assert not dialog.isVisible() -- cgit v1.2.3-54-g00ecf From bc50d9ced42d09a8f27433d98d874401a6c3ff15 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Fri, 15 Jan 2021 10:16:06 +0100 Subject: Use sql.user_version_changed() everywhere --- qutebrowser/browser/history.py | 2 +- qutebrowser/misc/sql.py | 18 +++++++++--------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py index eaeb274f3..6fcc140a1 100644 --- a/qutebrowser/browser/history.py +++ b/qutebrowser/browser/history.py @@ -170,7 +170,7 @@ class WebHistory(sql.SqlTable): rebuild_completion = False - if sql.db_user_version != sql.USER_VERSION: + if sql.user_version_changed(): # If the DB user version changed, run a full cleanup and rebuild the # completion history. # diff --git a/qutebrowser/misc/sql.py b/qutebrowser/misc/sql.py index c71ac6eaa..5cd3246cb 100644 --- a/qutebrowser/misc/sql.py +++ b/qutebrowser/misc/sql.py @@ -65,13 +65,13 @@ class UserVersion: return f'{self.major}.{self.minor}' -db_user_version = None # The user version we got from the database -USER_VERSION = UserVersion(0, 3) # The current / newest user version +_db_user_version = None # The user version we got from the database +_USER_VERSION = UserVersion(0, 3) # The current / newest user version def user_version_changed(): """Whether the version stored in the database is different from the current one.""" - return db_user_version != USER_VERSION + return _db_user_version != _USER_VERSION class SqliteErrorCode: @@ -181,17 +181,17 @@ def init(db_path): error.text()) raise_sqlite_error(msg, error) - global db_user_version + global _db_user_version version_int = Query('pragma user_version').run().value() - db_user_version = UserVersion.from_int(version_int) + _db_user_version = UserVersion.from_int(version_int) - if db_user_version.major > USER_VERSION.major: + if _db_user_version.major > _USER_VERSION.major: raise KnownError( "Database is too new for this qutebrowser version (database version " - f"{db_user_version}, but {USER_VERSION.major}.x is supported)") + f"{_db_user_version}, but {_USER_VERSION.major}.x is supported)") if user_version_changed(): - log.sql.debug(f"Migrating from version {db_user_version} to {USER_VERSION}") + log.sql.debug(f"Migrating from version {_db_user_version} to {_USER_VERSION}") # Enable write-ahead-logging and reduce disk write frequency # see https://sqlite.org/pragma.html and issues #2930 and #3507 # @@ -199,7 +199,7 @@ def init(db_path): # as those are idempotent, let's make sure we run them once again. Query("PRAGMA journal_mode=WAL").run() Query("PRAGMA synchronous=NORMAL").run() - Query(f'PRAGMA user_version = {USER_VERSION.to_int()}').run() + Query(f'PRAGMA user_version = {_USER_VERSION.to_int()}').run() def close(): -- cgit v1.2.3-54-g00ecf From 8007f2b258a6233eae2249494dbd1c634a7ded86 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Fri, 15 Jan 2021 10:47:31 +0100 Subject: sql: Clean up init slightly --- qutebrowser/misc/sql.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/qutebrowser/misc/sql.py b/qutebrowser/misc/sql.py index 5cd3246cb..bae9b5aa5 100644 --- a/qutebrowser/misc/sql.py +++ b/qutebrowser/misc/sql.py @@ -192,6 +192,11 @@ def init(db_path): if user_version_changed(): log.sql.debug(f"Migrating from version {_db_user_version} to {_USER_VERSION}") + # Note we're *not* updating the _db_user_version global here. We still want + # user_version_changed() to return True, as other modules (such as history.py) + # use it to create the initial table structure. + Query(f'PRAGMA user_version = {_USER_VERSION.to_int()}').run() + # Enable write-ahead-logging and reduce disk write frequency # see https://sqlite.org/pragma.html and issues #2930 and #3507 # @@ -199,7 +204,6 @@ def init(db_path): # as those are idempotent, let's make sure we run them once again. Query("PRAGMA journal_mode=WAL").run() Query("PRAGMA synchronous=NORMAL").run() - Query(f'PRAGMA user_version = {_USER_VERSION.to_int()}').run() def close(): -- cgit v1.2.3-54-g00ecf From 6af6f5c79b67bdee143e65a0dcfebfb069466e02 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Fri, 15 Jan 2021 10:55:56 +0100 Subject: history: Fix tests --- tests/unit/browser/test_history.py | 49 ++++++++++++-------------------------- 1 file changed, 15 insertions(+), 34 deletions(-) diff --git a/tests/unit/browser/test_history.py b/tests/unit/browser/test_history.py index 876ffb9bc..e80f30d4c 100644 --- a/tests/unit/browser/test_history.py +++ b/tests/unit/browser/test_history.py @@ -368,33 +368,12 @@ class TestDump: class TestRebuild: - def test_delete(self, web_history, stubs): - web_history.insert({'url': 'example.com/1', 'title': 'example1', - 'redirect': False, 'atime': 1}) - web_history.insert({'url': 'example.com/1', 'title': 'example1', - 'redirect': False, 'atime': 2}) - web_history.insert({'url': 'example.com/2%203', 'title': 'example2', - 'redirect': False, 'atime': 3}) - web_history.insert({'url': 'example.com/3', 'title': 'example3', - 'redirect': True, 'atime': 4}) - web_history.insert({'url': 'example.com/2 3', 'title': 'example2', - 'redirect': False, 'atime': 5}) - web_history.completion.delete_all() - - hist2 = history.WebHistory(progress=stubs.FakeHistoryProgress()) - assert list(hist2.completion) == [ - ('example.com/1', 'example1', 2), - ('example.com/2 3', 'example2', 5), - ] - - def test_no_rebuild(self, web_history, stubs): - """Ensure that completion is not regenerated unless empty.""" - web_history.add_url(QUrl('example.com/1'), redirect=False, atime=1) - web_history.add_url(QUrl('example.com/2'), redirect=False, atime=2) - web_history.completion.delete('url', 'example.com/2') - - hist2 = history.WebHistory(progress=stubs.FakeHistoryProgress()) - assert list(hist2.completion) == [('example.com/1', '', 1)] + # FIXME: Some of those tests might be a bit misleading, as creating a new + # history.WebHistory will regenerate the completion either way with the SQL changes + # in v2.0.0 (because the user version changed from 0 -> 3). + # + # They should be revisited once we can actually create two independent sqlite + # databases and copy the data over, for a "real" test. def test_user_version(self, web_history, stubs, monkeypatch): """Ensure that completion is regenerated if user_version changes.""" @@ -402,11 +381,12 @@ class TestRebuild: web_history.add_url(QUrl('example.com/2'), redirect=False, atime=2) web_history.completion.delete('url', 'example.com/2') - hist2 = history.WebHistory(progress=stubs.FakeHistoryProgress()) - assert list(hist2.completion) == [('example.com/1', '', 1)] + # User version always changes, so this won't work + # hist2 = history.WebHistory(progress=stubs.FakeHistoryProgress()) + # assert list(hist2.completion) == [('example.com/1', '', 1)] + + monkeypatch.setattr(sql, 'user_version_changed', lambda: True) - monkeypatch.setattr(history, '_USER_VERSION', - history._USER_VERSION + 1) hist3 = history.WebHistory(progress=stubs.FakeHistoryProgress()) assert list(hist3.completion) == [ ('example.com/1', '', 1), @@ -450,11 +430,12 @@ class TestRebuild: ('http://example.org', '', 2) ] - def test_progress(self, web_history, config_stub, stubs): + def test_progress(self, monkeypatch, web_history, config_stub, stubs): web_history.add_url(QUrl('example.com/1'), redirect=False, atime=1) web_history.add_url(QUrl('example.com/2'), redirect=False, atime=2) - # Change cached patterns to trigger a completion rebuild - web_history.metainfo['excluded_patterns'] = 'http://example.org' + + # Trigger a completion rebuild + monkeypatch.setattr(sql, 'user_version_changed', lambda: True) progress = stubs.FakeHistoryProgress() history.WebHistory(progress=progress) -- cgit v1.2.3-54-g00ecf From 75b39896c3aa221f24e392aace041aad71386e71 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Fri, 15 Jan 2021 11:06:50 +0100 Subject: sql: Improve and quieten logging --- qutebrowser/misc/sql.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/qutebrowser/misc/sql.py b/qutebrowser/misc/sql.py index bae9b5aa5..4c3ceddef 100644 --- a/qutebrowser/misc/sql.py +++ b/qutebrowser/misc/sql.py @@ -238,7 +238,7 @@ class Query: """ self.query = QSqlQuery(QSqlDatabase.database()) - log.sql.debug('Preparing SQL query: "{}"'.format(querystr)) + log.sql.vdebug(f'Preparing: {querystr}') ok = self.query.prepare(querystr) self._check_ok('prepare', ok) self.query.setForwardOnly(forward_only) @@ -266,16 +266,20 @@ class Query: def _bind_values(self, values): for key, val in values.items(): self.query.bindValue(':{}'.format(key), val) - if any(val is None for val in self.bound_values().values()): + + bound_values = self.bound_values() + if any(val is None for val in bound_values.values()): raise BugError("Missing bound values!") + return bound_values + def run(self, **values): """Execute the prepared query.""" - log.sql.debug('Running SQL query: "{}"'.format( - self.query.lastQuery())) + log.sql.debug(self.query.lastQuery()) - self._bind_values(values) - log.sql.debug('query bindings: {}'.format(self.bound_values())) + bound_values = self._bind_values(values) + if bound_values: + log.sql.debug(f' {bound_values}') ok = self.query.exec() self._check_ok('exec', ok) -- cgit v1.2.3-54-g00ecf From f6fedbbec921bd138a8d29c276edd09ffee58d0b Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Fri, 15 Jan 2021 11:07:28 +0100 Subject: sql: Simplify condition --- qutebrowser/misc/sql.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qutebrowser/misc/sql.py b/qutebrowser/misc/sql.py index 4c3ceddef..2ed99e4b6 100644 --- a/qutebrowser/misc/sql.py +++ b/qutebrowser/misc/sql.py @@ -268,7 +268,7 @@ class Query: self.query.bindValue(':{}'.format(key), val) bound_values = self.bound_values() - if any(val is None for val in bound_values.values()): + if None in bound_values.values(): raise BugError("Missing bound values!") return bound_values -- cgit v1.2.3-54-g00ecf From 8015988bb05f8a1174c582412b941c697a4e96b4 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Fri, 15 Jan 2021 11:42:17 +0100 Subject: sql: Fix mypy --- qutebrowser/misc/sql.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qutebrowser/misc/sql.py b/qutebrowser/misc/sql.py index 2ed99e4b6..bb0e4ac88 100644 --- a/qutebrowser/misc/sql.py +++ b/qutebrowser/misc/sql.py @@ -238,7 +238,7 @@ class Query: """ self.query = QSqlQuery(QSqlDatabase.database()) - log.sql.vdebug(f'Preparing: {querystr}') + log.sql.vdebug(f'Preparing: {querystr}') # type: ignore[attr-defined] ok = self.query.prepare(querystr) self._check_ok('prepare', ok) self.query.setForwardOnly(forward_only) -- cgit v1.2.3-54-g00ecf From 485a3173797a356f90cb539802c73685d9b93371 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Fri, 15 Jan 2021 11:42:42 +0100 Subject: sql: Remove HistoryProgress reusability Not actually needed and complicates things --- qutebrowser/browser/history.py | 5 +---- tests/unit/browser/test_history.py | 1 - 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py index 6fcc140a1..d81accc95 100644 --- a/qutebrowser/browser/history.py +++ b/qutebrowser/browser/history.py @@ -46,9 +46,6 @@ class HistoryProgress: """ def __init__(self): - self._reset() - - def _reset(self): self._progress = None self._value = 0 @@ -65,6 +62,7 @@ class HistoryProgress: def set_maximum(self, maximum): """Set the progress maximum as soon as we know about it.""" + assert self._progress is not None self._progress.setMaximum(maximum) QApplication.processEvents() @@ -82,7 +80,6 @@ class HistoryProgress: """ if self._progress is not None: self._progress.hide() - self._reset() class CompletionMetaInfo(sql.SqlTable): diff --git a/tests/unit/browser/test_history.py b/tests/unit/browser/test_history.py index e80f30d4c..e5a3f1ee8 100644 --- a/tests/unit/browser/test_history.py +++ b/tests/unit/browser/test_history.py @@ -491,7 +491,6 @@ class TestHistoryProgress: assert progress._value == 1 progress.finish() assert progress._progress is None - assert progress._value == 0 def test_gui(self, qtbot, progress): progress.start("Hello World") -- cgit v1.2.3-54-g00ecf