{"action":"created","issue":{"url":"https://api.github.com/repos/qutebrowser/qutebrowser/issues/2295","repository_url":"https://api.github.com/repos/qutebrowser/qutebrowser","labels_url":"https://api.github.com/repos/qutebrowser/qutebrowser/issues/2295/labels{/name}","comments_url":"https://api.github.com/repos/qutebrowser/qutebrowser/issues/2295/comments","events_url":"https://api.github.com/repos/qutebrowser/qutebrowser/issues/2295/events","html_url":"https://github.com/qutebrowser/qutebrowser/pull/2295","id":206210972,"number":2295,"title":"Completion refactor V3","user":{"login":"rcorre","id":2496231,"avatar_url":"https://avatars0.githubusercontent.com/u/2496231?v=3","gravatar_id":"","url":"https://api.github.com/users/rcorre","html_url":"https://github.com/rcorre","followers_url":"https://api.github.com/users/rcorre/followers","following_url":"https://api.github.com/users/rcorre/following{/other_user}","gists_url":"https://api.github.com/users/rcorre/gists{/gist_id}","starred_url":"https://api.github.com/users/rcorre/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/rcorre/subscriptions","organizations_url":"https://api.github.com/users/rcorre/orgs","repos_url":"https://api.github.com/users/rcorre/repos","events_url":"https://api.github.com/users/rcorre/events{/privacy}","received_events_url":"https://api.github.com/users/rcorre/received_events","type":"User","site_admin":false},"labels":[{"id":363622283,"url":"https://api.github.com/repos/qutebrowser/qutebrowser/labels/PR:%20work%20in%20progress","name":"PR: work in progress","color":"f9d0c4","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":118,"created_at":"2017-02-08T14:20:14Z","updated_at":"2017-07-07T10:11:12Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/qutebrowser/qutebrowser/pulls/2295","html_url":"https://github.com/qutebrowser/qutebrowser/pull/2295","diff_url":"https://github.com/qutebrowser/qutebrowser/pull/2295.diff","patch_url":"https://github.com/qutebrowser/qutebrowser/pull/2295.patch"},"body":"This is a combination of my previous efforts for #74 and #1765.\r\nIt [refactors completion models into functions](https://github.com/qutebrowser/qutebrowser/pull/2295/commits/11aca9af1fa8475a8769e177331351d1f7152621) that instantiate models on-demand. In order to avoid taking a performace hit, some models are backed by SQL tables.\r\n\r\nThis approach is a little different than last time in that the SQL does not\r\nexist solely in the completion module. Instead, the `bookmark-manager`,\r\n`quickmark-manager`, and `web-history` are now backed by an in-memory SQL\r\ndatabase rather than a python dict (see e130063 and f095dbe). On startup, they\r\nread from a file on disk and write entries into the in-memory database. This is\r\nthe main part I'm looking for feedback on right now. How does it feel? The\r\nadvantage of this approach is that SQL-based completion models can be \r\n[returned on-demand](https://github.com/qutebrowser/qutebrowser/commit/c2266f88e66f73cc38b1a264282d1d94a4cca154) just as the simple completion models are.\r\n\r\nIt would likely be even more performant and simpler to replace the old bookmark/quickmark/history text files with an on-disk database, but that could\r\nbe a usability hit for those that like to mess with these files manually.\r\n\r\nP.S. don't expect this to work just yet, but it will be there soon.\n\n<!-- Reviewable:start -->\n---\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"34\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/qutebrowser/qutebrowser/2295)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/qutebrowser/qutebrowser/issues/comments/313642911","html_url":"https://github.com/qutebrowser/qutebrowser/pull/2295#issuecomment-313642911","issue_url":"https://api.github.com/repos/qutebrowser/qutebrowser/issues/2295","id":313642911,"user":{"login":"The-Compiler","id":625793,"avatar_url":"https://avatars3.githubusercontent.com/u/625793?v=3","gravatar_id":"","url":"https://api.github.com/users/The-Compiler","html_url":"https://github.com/The-Compiler","followers_url":"https://api.github.com/users/The-Compiler/followers","following_url":"https://api.github.com/users/The-Compiler/following{/other_user}","gists_url":"https://api.github.com/users/The-Compiler/gists{/gist_id}","starred_url":"https://api.github.com/users/The-Compiler/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/The-Compiler/subscriptions","organizations_url":"https://api.github.com/users/The-Compiler/orgs","repos_url":"https://api.github.com/users/The-Compiler/repos","events_url":"https://api.github.com/users/The-Compiler/events{/privacy}","received_events_url":"https://api.github.com/users/The-Compiler/received_events","type":"User","site_admin":false},"created_at":"2017-07-07T10:11:12Z","updated_at":"2017-07-07T10:11:12Z","body":"I did another full review now, most likely the last one. Noticed some more small stuff, hope you're okay with that :laughing: But we're definitely getting close!\n\n---\n\nReviewed 26 of 55 files at r1, 1 of 16 files at r3, 5 of 13 files at r4, 2 of 5 files at r5, 8 of 12 files at r6, 9 of 12 files at r7, 3 of 3 files at r8, 1 of 1 files at r9.\nReview status: all files reviewed at latest revision, 17 unresolved discussions, some commit checks failed.\n\n---\n\n*[qutebrowser/app.py, line 87 at r9](https://reviewable.io:443/reviews/qutebrowser/qutebrowser/2295#-KoR3OaYHZKWOBpAJ7Tg:-KoR3OaYHZKWOBpAJ7Th:b25eo7v) ([raw file](https://github.com/qutebrowser/qutebrowser/blob/1dd5f06a4fa6e7b57af10ed3deeda7c4db119896/qutebrowser/app.py#L87)):*\n> ```Python\n>     if args.version:\n>         # we need to init sql to print the sql version\n>         # we can use an in-memory database as we just want to query the version\n> ```\n\nThis is outdated with cee0aa3adc01faed0f18e982fb3a37e04c8a469f\n\n---\n\n*[qutebrowser/browser/history.py, line 245 at r9](https://reviewable.io:443/reviews/qutebrowser/qutebrowser/2295#-KoRA5575rGEK-ARjbbW:-KoRA5575rGEK-ARjbbX:bk69wws) ([raw file](https://github.com/qutebrowser/qutebrowser/blob/1dd5f06a4fa6e7b57af10ed3deeda7c4db119896/qutebrowser/browser/history.py#L245)):*\n> ```Python\n>                 line = line.strip()\n>                 if not line or line.startswith('#'):\n>                     continue\n> ```\n\nnitpick: Would probably be cleaner to do this check in `_parse_entry` too and just return `None` there.\n(then you can also do `self._parse_entry(line.strip())` and drop the `line = line.strip()`)\n\n---\n\n*[qutebrowser/browser/history.py, line 276 at r9](https://reviewable.io:443/reviews/qutebrowser/qutebrowser/2295#-KoRAe-EFUI0vv0K_VwZ:-KoRAe-EFUI0vv0K_Vw_:b3mquas) ([raw file](https://github.com/qutebrowser/qutebrowser/blob/1dd5f06a4fa6e7b57af10ed3deeda7c4db119896/qutebrowser/browser/history.py#L276)):*\n> ```Python\n>         dirname = os.path.dirname(dest)\n>         if not os.path.exists(dirname):\n>             raise cmdexc.CommandError('Path does not exist', dirname)\n> ```\n\nDo you think it makes sense to add an explicit check here? It'd be caught by the `OSError` handling below anyways, no?\n\n---\n\n*[qutebrowser/completion/models/completionmodel.py, line 62 at r9](https://reviewable.io:443/reviews/qutebrowser/qutebrowser/2295#-KoRE0YNIaMIVf1c7APZ:-KoRE0YNIaMIVf1c7AP_:b-injx5b) ([raw file](https://github.com/qutebrowser/qutebrowser/blob/1dd5f06a4fa6e7b57af10ed3deeda7c4db119896/qutebrowser/completion/models/completionmodel.py#L62)):*\nThe [docs for `layoutChanged`](http://doc.qt.io/qt-5/qabstractitemmodel.html#layoutChanged) say:\n\n> When subclassing QAbstractItemModel or QAbstractProxyModel, ensure that you emit layoutAboutToBeChanged() before changing the order of items or altering the structure of the data you expose to views, and emit layoutChanged() after changing the layout.\n\nApparently it works fine so far without forwarding that, but maybe we should forward both anyways?\n\n---\n\n*[qutebrowser/completion/models/sqlcategory.py, line 61 at r9](https://reviewable.io:443/reviews/qutebrowser/qutebrowser/2295#-KoRKeIf3f_9eZW4-7kU:-KoRKeIf3f_9eZW4-7kV:bhqbiln) ([raw file](https://github.com/qutebrowser/qutebrowser/blob/1dd5f06a4fa6e7b57af10ed3deeda7c4db119896/qutebrowser/completion/models/sqlcategory.py#L61)):*\n> ```Python\n>         if sort_by:\n>             assert sort_order in ['asc', 'desc'], sort_order\n>             querystr += ' order by {} {}'.format(sort_by, sort_order)\n> ```\n\nmaybe add:\n\n```python\nelse:\n    assert sort_order is None\n```\n\nto avoid mistakes down the road?\n\n---\n\n*[tests/helpers/stubs.py, line 526 at r9](https://reviewable.io:443/reviews/qutebrowser/qutebrowser/2295#-KoRMyyRidCY4jKrr-i6:-KoRMyyRidCY4jKrr-i7:blkt2zn) ([raw file](https://github.com/qutebrowser/qutebrowser/blob/1dd5f06a4fa6e7b57af10ed3deeda7c4db119896/tests/helpers/stubs.py#L526)):*\n> ```Python\n> \n> \n> class WebHistoryStub(sql.SqlTable):\n> ```\n\nThis is basically a reimplementation of `history.WebHistory` - we could probably just use that instead, as it seems pretty self-contained (other than using SQL, but the stub does too, and I'm fine with that).\n\n---\n\n*[tests/unit/browser/test_history.py, line 137 at r9](https://reviewable.io:443/reviews/qutebrowser/qutebrowser/2295#-KoRNip-xhe2jbDSOvhc:-KoRNip-xhe2jbDSOvhd:b49q8fw) ([raw file](https://github.com/qutebrowser/qutebrowser/blob/1dd5f06a4fa6e7b57af10ed3deeda7c4db119896/tests/unit/browser/test_history.py#L137)):*\n> ```Python\n>     hist.delete_url(QUrl('http://example.com/1'))\n>     diff = before.difference(set(hist))\n>     assert diff == set([('http://example.com/1', '', 0, False)])\n> ```\n\nnitpick: Use `{[...]}` instead of `set([...])` (`{item1, item2}` is a set in Python)\n\n---\n\n*[tests/unit/browser/test_history.py, line 144 at r9](https://reviewable.io:443/reviews/qutebrowser/qutebrowser/2295#-KoR9MOdyKUicfsosJUc:-KoR9MOdyKUicfsosJUd:bw9dpdo) ([raw file](https://github.com/qutebrowser/qutebrowser/blob/1dd5f06a4fa6e7b57af10ed3deeda7c4db119896/tests/unit/browser/test_history.py#L144)):*\n> ```Python\n>     ('http://www.example.com', 12346, 'the title', True)\n> ])\n> def test_add_item(qtbot, hist, url, atime, title, redirect):\n> ```\n\nCan you add tests to ensure the URL is fully encoded, and that passwords are removed?\n\n---\n\n*[tests/unit/browser/test_history.py, line 167 at r9](https://reviewable.io:443/reviews/qutebrowser/qutebrowser/2295#-KoRNygeVthzAuKU9FjA:-KoRNygeVthzAuKU9FjB:b4jq5jq) ([raw file](https://github.com/qutebrowser/qutebrowser/blob/1dd5f06a4fa6e7b57af10ed3deeda7c4db119896/tests/unit/browser/test_history.py#L167)):*\n> ```Python\n>     with caplog.at_level(level):\n>         hist.add_from_tab(QUrl(url), QUrl(req_url), 'title')\n>     assert set(list(hist)) == set(expected)\n> ```\n\nnitpick: I don't think the `list(...)` is needed here?\n\n---\n\n*[tests/unit/browser/test_history.py, line 338 at r9](https://reviewable.io:443/reviews/qutebrowser/qutebrowser/2295#-KoROMeiU51GukVd46aN:-KoROMeiU51GukVd46aO:b97rhgm) ([raw file](https://github.com/qutebrowser/qutebrowser/blob/1dd5f06a4fa6e7b57af10ed3deeda7c4db119896/tests/unit/browser/test_history.py#L338)):*\n> ```Python\n>     histfile = tmpdir / 'history'\n>     histfile.write('')\n>     os.chmod(str(histfile), 0)\n> ```\n\nI recently noticed those kinds tests fail because the files are still accessible inside Docker containers on my system for some reason. Can you add something like:\n\n```python\n        if os.access(str(histfile), os.R_OK):\n            pytest.skip(\"File was still readable\")\n```\n\nto make sure that doesn't happen please?\n\n---\n\n*[tests/unit/completion/test_completionmodel.py, line 42 at r9](https://reviewable.io:443/reviews/qutebrowser/qutebrowser/2295#-KoRP7X2_23EZPEcwZGh:-KoRP7X2_23EZPEcwZGi:bw3zijc) ([raw file](https://github.com/qutebrowser/qutebrowser/blob/1dd5f06a4fa6e7b57af10ed3deeda7c4db119896/tests/unit/completion/test_completionmodel.py#L42)):*\n> ```Python\n>         cat.rowCount = mock.Mock(return_value=c, spec=[])\n>         model.add_category(cat)\n>     nonempty = [i for i, rowCount in enumerate(counts) if rowCount > 0]\n> ```\n\nnitpick: I find that name kind of confusing, I'd just call it `data` or so.\n\n---\n\n*[tests/unit/completion/test_completionwidget.py, line 215 at r9](https://reviewable.io:443/reviews/qutebrowser/qutebrowser/2295#-KoRWLjKueDSl8o7hIKd:-KoRWLjKueDSl8o7hIKe:bn58okf) ([raw file](https://github.com/qutebrowser/qutebrowser/blob/1dd5f06a4fa6e7b57af10ed3deeda7c4db119896/tests/unit/completion/test_completionwidget.py#L215)):*\n> ```Python\n> \n> \n> def test_completion_item_del(completionview):\n> ```\n\nMaybe add a test for `delete_cur_item` without `delete_func`?\n\n---\n\n*[tests/unit/completion/test_listcategory.py, line 46 at r9](https://reviewable.io:443/reviews/qutebrowser/qutebrowser/2295#-KoRRlDMdiWY2o9HB2Jw:-KoRRlDNMnhk5J1_Ns18:bufjswx) ([raw file](https://github.com/qutebrowser/qutebrowser/blob/1dd5f06a4fa6e7b57af10ed3deeda7c4db119896/tests/unit/completion/test_listcategory.py#L46)):*\n> ```Python\n>      [('foo', 'bar'), ('bar', 'foo')]),\n> ])\n> def test_set_pattern(pattern, before, after):\n> ```\n\nWould it make sense to add `qtmodeltester` here,  to have a test which tests `set_pattern` with the modeltester?\n\n---\n\n*[tests/unit/completion/test_models.py, line 467 at r9](https://reviewable.io:443/reviews/qutebrowser/qutebrowser/2295#-KoRVO8bKL6SLIhuHwKu:-KoRVO8bKL6SLIhuHwKv:bdbxcb8) ([raw file](https://github.com/qutebrowser/qutebrowser/blob/1dd5f06a4fa6e7b57af10ed3deeda7c4db119896/tests/unit/completion/test_models.py#L467)):*\n> ```Python\n> \n> \n> def test_setting_section_completion(qtmodeltester, monkeypatch, stubs):\n> ```\n\nMaybe add a test checking `section()`, `option()` and `value()` which items which don't exist?\n\n---\n\n*[tests/unit/completion/test_sqlcategory.py, line 1 at r9](https://reviewable.io:443/reviews/qutebrowser/qutebrowser/2295#-KoRSSAoY_a4THTJKjC5:-KoRSSAoY_a4THTJKjC6:b19lk8s) ([raw file](https://github.com/qutebrowser/qutebrowser/blob/1dd5f06a4fa6e7b57af10ed3deeda7c4db119896/tests/unit/completion/test_sqlcategory.py#L1)):*\n> ```Python\n> # vim: ft=python fileencoding=utf-8 sts=4 sw=4 et:\n> ```\n\nMaybe add `qtmodeltester` for some (all?) tests in here?\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/qutebrowser/qutebrowser/2295#-:-KoRWsy-nx0kRWh29-2i:bjdmxp3)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"The-Compiler"}}