Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Completion refactor V3 #2295

Merged
merged 163 commits into from Jul 21, 2017
Merged

Completion refactor V3 #2295

merged 163 commits into from Jul 21, 2017

Conversation

@rcorre
Copy link
Collaborator

rcorre commented Feb 8, 2017

This is a combination of my previous efforts for #74 and #1765.
It refactors completion models into functions that instantiate models on-demand. In order to avoid taking a performace hit, some models are backed by SQL tables.

This approach is a little different than last time in that the SQL does not
exist solely in the completion module. Instead, the bookmark-manager,
quickmark-manager, and web-history are now backed by an in-memory SQL
database rather than a python dict (see e130063 and f095dbe). On startup, they
read from a file on disk and write entries into the in-memory database. This is
the main part I'm looking for feedback on right now. How does it feel? The
advantage of this approach is that SQL-based completion models can be
returned on-demand just as the simple completion models are.

It 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
be a usability hit for those that like to mess with these files manually.

P.S. don't expect this to work just yet, but it will be there soon.


This change is Reviewable

@The-Compiler
Copy link
Member

The-Compiler commented Feb 8, 2017

Can we close #2144 and/or #2178 with this open?

It 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
be a usability hit for those that like to mess with these files manually.

I've been thinking about that too. I originally wanted everything to be plain-text, but at least for cookies I can't do that anymore with QtWebEngine either.

I wonder if we could just ship a script (say, qutebrowser-dumpsql or whatever) which dumps those SQL databases to the earlier plain-text formats, so at least thinks like https://github.com/qutebrowser/qutebrowser/blob/master/misc/userscripts/qutedmenu could be ported easily.

We need the conversion code in qutebrowser anyways (to migrate existing data), so the only question is whether the performance hit on start is too big. If it is, I'm open to doing that.

@rcorre rcorre force-pushed the rcorre:really_complete branch 2 times, most recently from a89ac2f to 8001680 Feb 8, 2017
Copy link
Member

The-Compiler left a comment

I've only taken a relatively quick look, and I already love how this makes everything simpler, despite involving SQL 😆 Great work so far!

The only thing I'm really unsure about is that it's a QAbstractItemModel actually used in the view, and not the SQL model. Doesn't this hinder performance a lot again? Can we use the Qt model directly?

Signals:
add_completion_item: Emitted before a new Entry is added.
Used to sync with the completion.
arg: The new Entry.
item_added: Emitted after a new Entry is added.
Used to tell the savemanager that the history is dirty.
arg: The new Entry.

This comment has been minimized.

@The-Compiler

The-Compiler Feb 9, 2017 Member

Probably can be removed as the signal is gone.

@@ -53,43 +53,25 @@ class InvalidUrlError(Error):
pass


class DoesNotExistError(Error):

This comment has been minimized.

@The-Compiler

The-Compiler Feb 9, 2017 Member

Why get rid of this? Abusing a KeyError (which I view more of a Python built-in exception and can also happen by accident) for this seems wrong to me.

This comment has been minimized.

@The-Compiler

The-Compiler Feb 10, 2017 Member

To expand on this: I think it's fine to use KeyError on a lower level, i.e. with the SqlTable object with a dict-like interface. However, on this higher level, I think it makes sense to re-raise them as more specific exceptions.

This comment has been minimized.

@rcorre

rcorre Feb 12, 2017 Author Collaborator

done

@@ -48,21 +47,12 @@ def __init__(self, source, parent=None):
self.srcmodel = source
self.pattern = ''
self.pattern_re = None

dumb_sort = self.srcmodel.DUMB_SORT

This comment has been minimized.

@The-Compiler

The-Compiler Feb 9, 2017 Member

I'm guessing all the dumb_sort stuff can go because it was only used for the URL model, which doesn't use QSortFilterProxyModel anymore?

This comment has been minimized.

@rcorre

rcorre Feb 12, 2017 Author Collaborator

done

else:
self.setSortRole(completion.Role.sort)
self._sort_order = dumb_sort
self.lessThan = self.intelligentLessThan

This comment has been minimized.

@The-Compiler

The-Compiler Feb 9, 2017 Member

You can just rename the method now 😉

This comment has been minimized.

@rcorre

rcorre Feb 10, 2017 Author Collaborator

oh, right 😀

from qutebrowser.utils import usertypes, log


class SqlCompletionModel(QAbstractItemModel):

This comment has been minimized.

@The-Compiler

The-Compiler Feb 9, 2017 Member

The original reason we wanted to have sqlite models was performance, as Qt probably implements lazy loading and all that there very efficiently. Don't we lose all that by having this be a QAbstractItemModel?

This comment has been minimized.

@rcorre

rcorre Feb 10, 2017 Author Collaborator

There's no 'natural' way to make a SqlTableModel/SqlQueryModel look like a tree. This is a wrapper around several SqlTableModels, each of which populates a single category. I hope I'm not losing out on lazy loading here! At the very least I'm still relying on SqlTableModel for filtering!

This comment has been minimized.

@The-Compiler

The-Compiler Feb 10, 2017 Member

Makes sense - I originally thought about ditching the tree model entirely and using a QVBoxLayout of QTableViews, but that'd probably make things like sizing a lot more difficult.

Makes me wonder whether it somehow helps to implement canFetchMore and fetchMore - though we don't really have a slow "data generation" step, so I guess it won't help.

I think a few benchmark tests (after #2281 is in) would probably help there too, as we can test what's helping and what's not much easier.

fields = (t.record().fieldName(i) for i in self.columns_to_filter)
query = ' or '.join("{} like '%{}%' escape '\\'"
.format(field, pattern)
for field in fields)

This comment has been minimized.

@The-Compiler

The-Compiler Feb 9, 2017 Member

What if there's a ' in the pattern? Can't we use QSqlQuery here too?

return query


class SqlTable(QObject):

This comment has been minimized.

@The-Compiler

The-Compiler Feb 9, 2017 Member

Love the pythonic interface over SQL stuff! 👍

This comment has been minimized.

@rcorre

rcorre Feb 10, 2017 Author Collaborator

Thanks! That actually answers your question about KeyError though. I saw this as a dict-like interface over a sql table. When you try to access or delete a key that doesn't exist, you get a KeyError just as you would if it were a python dict. I didn't see a reason to invent a new exception just for that (I do have SqlException, but that's more for things like 'your SQL connection died')

# You should have received a copy of the GNU General Public License
# along with qutebrowser. If not, see <http://www.gnu.org/licenses/>.

"""Tests for the base sql completion model."""

This comment has been minimized.

@The-Compiler

The-Compiler Feb 9, 2017 Member

Those should probably use qtmodeltester where appropriate, and maybe pytest-benchmark for some interesting operations (which I'm introducing in #2281).

This comment has been minimized.

@rcorre

rcorre Feb 10, 2017 Author Collaborator

I must've lost that while cherry-picking over from my previous branch. Working on this model actually made me really appreciate qmodeltester, it caught like 10 dumb things I was doing!

This comment has been minimized.

@The-Compiler

The-Compiler Feb 20, 2017 Member

Looks like it's still not back?

completion view.
DUMB_SORT: the dumb sorting used by the model
dumb_sort: the dumb sorting used by the model

This comment has been minimized.

@rcorre

rcorre Feb 10, 2017 Author Collaborator

this can go too

@rcorre
Copy link
Collaborator Author

rcorre commented Feb 10, 2017

To answer your main question more fully, no, we can't use a QSqlTableModel/QSqlQueryModel directly as long as we want to group by category using a tree view. We need to wrap the table models in either an item model or a proxy model, and I found the item model to be much easier.

Furthermore, we may actually want a model that combines both SQL and item-based sources. Otherwise, to support #2191, we'd need to create a sql table to include searchengines. I'm imagining something like:

model = CompletionModel()
model.add_sql_category('History')
model.add_sql_category('Bookmarks')
model.add_sql_category('Quickmarks')
model.add_list_category([('ddg', 'duckduckgo'), ...])
@rcorre rcorre force-pushed the rcorre:really_complete branch from 8001680 to bd3635d Feb 10, 2017
Copy link
Member

The-Compiler left a comment

A quick look at Travis shows that you have a circular import Python 3.4 doesn't like, some actual failures, and of course some linting errors, but I'm guessing that's all WIP. The Ubuntu Xenial one fails to init the sqlite backend, I'm guessing some additional package is needed there?

edit: Looks like it's this one. We should also check if QtSql and sqlite is available in earlyinit.py to show an error if it isn't.

@@ -53,43 +53,25 @@ class InvalidUrlError(Error):
pass


class DoesNotExistError(Error):

This comment has been minimized.

@The-Compiler

The-Compiler Feb 10, 2017 Member

To expand on this: I think it's fine to use KeyError on a lower level, i.e. with the SqlTable object with a dict-like interface. However, on this higher level, I think it makes sense to re-raise them as more specific exceptions.

from qutebrowser.utils import usertypes, log


class SqlCompletionModel(QAbstractItemModel):

This comment has been minimized.

@The-Compiler

The-Compiler Feb 10, 2017 Member

Makes sense - I originally thought about ditching the tree model entirely and using a QVBoxLayout of QTableViews, but that'd probably make things like sizing a lot more difficult.

Makes me wonder whether it somehow helps to implement canFetchMore and fetchMore - though we don't really have a slow "data generation" step, so I guess it won't help.

I think a few benchmark tests (after #2281 is in) would probably help there too, as we can test what's helping and what's not much easier.

@The-Compiler
Copy link
Member

The-Compiler commented Feb 10, 2017

I also just noticed this from the Qt docs:

The driver is locked for updates while a select is executed. This may cause problems when using QSqlTableModel because Qt's item views fetch data as needed (with QSqlQuery::fetchMore() in the case of QSqlTableModel).

Not sure if that applies to us, and if/how we need to handle it.

@rcorre
Copy link
Collaborator Author

rcorre commented Feb 12, 2017

Hmm, if I'm understanding that correctly, it could be a problem if you try to enter a history item while you have completion open and it is select()ing. Maybe if you hit o while a page is loading, and then it finishes loading while the completion menu is still filling out.

@rcorre rcorre force-pushed the rcorre:really_complete branch 4 times, most recently from 8a361b7 to 386e227 Feb 12, 2017
@rcorre
Copy link
Collaborator Author

rcorre commented Feb 16, 2017

Hmm ... I can't figure out what's up with this failure. For some reason it only has a problem initializing SQL for those tests. Other tests, like the whole completion test suite, run fine, so Sqlite is clearly installed ...
Those tests worked fine locally even without init_sql

Also, for Xenial, do I need to install libqt5sql5-sqlite in travis_install.sh, or in https://github.com/rcorre/docker-travis, or both? What's the difference?

@rcorre
Copy link
Collaborator Author

rcorre commented Feb 16, 2017

Bleh, never mind. Its one of those ones that only reproes when you run the full test suite (or at least everything under tests/unit/browser). Maybe calling sql.init()/sql.close() a bunch of times is problematic?

@The-Compiler
Copy link
Member

The-Compiler commented Feb 16, 2017

I can reproduce it with tox -e py36 -- tests/unit/browser tests/unit/utils. Right now I don't know what's causing this or what to do about it... 😟

@rcorre
Copy link
Collaborator Author

rcorre commented Feb 17, 2017

Ok, looks like all that's left is one failure on master and that tricky circular import. I thought I solved that early on but for some reason py3.4 still isn't happy with it.

@rcorre
Copy link
Collaborator Author

rcorre commented Feb 17, 2017

Once that's fixed up, I think this is feature-complete and ready for review. There are a few pieces of work that I think are non-critical and could come in a later review:

  • Implement the searchengine completion from #2191 with the new SQL backend. This is non-trivial as it requires a model that mixes SQL and non-SQL sources. Such a model might actually lead to a cleaner API though, I'll have to play with it.
  • Replace history.Entry with SqlTable.Entry, a namedtuple type that is automatically generated from the SQL record.
  • benchmark performance (maybe this should even be a PR that goes in before this one, to get a baseline).
  • de-complicate delete_cur_item (just something I noticed while working on this)
@rcorre
Copy link
Collaborator Author

rcorre commented Feb 17, 2017

Oh, and those sql failures were leaked state. ☹️

@rcorre
Copy link
Collaborator Author

rcorre commented Feb 17, 2017

oh, that's unfortunate. configmodels imports config and config imports configmodels to reference the completion functions. Looks like our options are:

  1. Go back to using usertypes.Completion as a level of indirection between
    commands and completion models.

  2. Move set_command from qutebrowser.config to qutebrowser.browser.commands

  3. Drop python3.4 support

None are great options. Well, ok, I like 3, but I'm guessing a fair number of
people won't 😁

@The-Compiler
Copy link
Member

The-Compiler commented Feb 20, 2017

Also, for Xenial, do I need to install libqt5sql5-sqlite in travis_install.sh, or in https://github.com/rcorre/docker-travis, or both? What's the difference?

Sorry, must've missed that one earlier!

Travis runs various test environments - some of them in their native (Ubuntu Trusty) environment, others (with DOCKER=... set) in Docker images built in the docker-travis repo.

For the native environments, what is installed in travis_install.sh matters. For Docker environments, what's installed in their Dockerfile matters.

Implement the searchengine completion from #2191 with the new SQL backend. This is non-trivial as it requires a model that mixes SQL and non-SQL sources. Such a model might actually lead to a cleaner API though, I'll have to play with it.

Hmm, I guess it'd make sense to close #2191 and reimplement that with the new completion API at some point then? Kind of a bad situation for @rsteube though...

benchmark performance (maybe this should even be a PR that goes in before this one, to get a baseline).

I really like that idea. We badly need some benchmarks for the completion (mainly filtering with many items), and it'd make sense to have them in before this PR, as that performance really shouldn't get worse. Do you want to work on this?

Oh, and those sql failures were leaked state.

Dang. I noticed the first one, but I didn't get how it'd cause SQL errors and fixing it didn't seem to help, so I dropped it again. 😆

oh, that's unfortunate. configmodels imports config and config imports configmodels to reference the completion functions.

Looks like you found a good solution to this, but it makes me wonder if we're going to run into similar issues with this structure in the feature... I vaguely remember something similar was the reason for the usertypes.Completion indirection originally, though I don't really like it.

Drop python3.4 support

I've actually been thinking about that, mostly because of #1456 and because most people on an old Debian/Ubuntu would probably be happy with #1912. Not so sure yet... 😉


Some other observations:

(everthing after this was added as an edit)

  • qutebrowser/completion/models/urlmodel.py has 100% coverage but is not in perfect_files!
  • earlyinit still should check that sqlite is available and abort if not
  • qutebrowser/mainwindow/statusbar/bar.py:147: Unused attribute '_option' (I can look at that one - it seems like vulture now likes to complain about that as well, but with _option we already tell pylint to shut up... So I'll make vulture just shut up about those.)
  • qutebrowser/utils/utils.py:765: Unused function 'newest_slice' (If we can be reasonably sure it's not needed anywhere else, let's drop it.)
  • You'll need to adjust INSTALL to point out those dependencies too
  • Is there some relevant version info (I can't find anything in QtSQL, but it seems like the sqlite version might be relevant?) we should add to version.py? We can probably either use sqlite3.sqlite_version from Python (and hope it's the same version), or use SELECT sqlite_version(); from Qt
  • The Windows build also seems to be missing sqlite, I'm not sure why yet. It currently uses Qt 5.6 installed via the PyQt .exe installer. I want to update it to Qt 5.8 with QtWebEngine only at some point, but I wonder why it's missing?
  • I did rerun the docker builds with qutebrowser/docker-ci#3 in and they pass.
@rcorre
Copy link
Collaborator Author

rcorre commented Feb 20, 2017

Hmm, I guess it'd make sense to close #2191 and reimplement that with the new completion API at some point then? Kind of a bad situation for @rsteube though...

Yeah, that's unfortunate, but I can probably at least re-use the tests.

Do you want to work on [completion benchmarks]?

Yes.

Looks like you found a good solution

Yeah, I listed 3 options and picked the fourth 😁. Its a step backward for #640 but the lesser of four evils...

with _option we already tell pylint to shut up...

Is it not an unused variable? I was about to remove it, along with
newest_slice (which was rendered unnecessary).

@The-Compiler
Copy link
Member

The-Compiler commented Feb 20, 2017

Yeah, I listed 3 options and picked the fourth 😁. Its a step backward for #640 but the lesser of four evils...

I agree - with #640 we'd probably just have a qutebrowser.misc.objects.config there.

Is it not an unused variable? I was about to remove it

Oh, right. I assumed it was an argument for some reason, not an attribute.

@rcorre rcorre force-pushed the rcorre:really_complete branch from dadc21d to 6bd573a Feb 20, 2017
@rcorre
Copy link
Collaborator Author

rcorre commented Feb 20, 2017

I think I fixed all the things, except earlyinit. Is there any advantage to
adding a check earlier? init will throw a SqlException if sqlite is not
available. The only way I know to check is actually trying to open a database
(QSqlDatabase.isDriverAvailable('QSQLITE') will return True even if I
uninstall sqlite). Should sql.init happen in earlyinit instead of init?

@rcorre
Copy link
Collaborator Author

rcorre commented Jul 20, 2017

tests/unit/completion/test_histcategory.py, line 122 at r17 (raw file):

Previously, rcorre (Ryan Roden-Corrent) wrote…

Just pushed the UTC conversion. Hope that fixes it!

oops, shouldn't have submitted my review, just the single comment about timestamps. I actually I didn't push it (except to my WIP branch)


Comments from Reviewable

@The-Compiler
Copy link
Member

The-Compiler commented Jul 20, 2017

Review status: all files reviewed at latest revision, 3 unresolved discussions.


qutebrowser/completion/models/histcategory.py, line 42 at r18 (raw file):

Previously, rcorre (Ryan Roden-Corrent) wrote…

Done.

This one still applies, see #2830.


qutebrowser/javascript/history.js, line 51 at r18 (raw file):

Previously, rcorre (Ryan Roden-Corrent) wrote…

Done.

This one still applies (but no hard feelings either way)


tests/unit/completion/test_histcategory.py, line 122 at r17 (raw file):

Previously, rcorre (Ryan Roden-Corrent) wrote…

oops, shouldn't have submitted my review, just the single comment about timestamps. I actually I didn't push it (except to my WIP branch)

I resolved the discussions which are outdated now with #2830 and things being a bit clearer. I commented on the rest. This is all based on what's pushed right now - i.e. ignoring the WIP branch.


Comments from Reviewable

@rcorre
Copy link
Collaborator Author

rcorre commented Jul 20, 2017

tests/unit/completion/test_histcategory.py, line 122 at r17 (raw file):

Previously, The-Compiler (Florian Bruhin) wrote…

I resolved the discussions which are outdated now with #2830 and things being a bit clearer. I commented on the rest. This is all based on what's pushed right now - i.e. ignoring the WIP branch.

hmm, I can't actually reproduce your failure. I've tried playing around with a bunch of different TZ settings but can't get test_histcategory to fail. e.g. TZ=UTC24 tox -e py36 -- tests/unit/completion/testhistcategory.py should throw dates off by a whole day (you can verify with TZ=UTC24 python -c "import datetime; print(datetime.datetime.now())"


Comments from Reviewable

@The-Compiler
Copy link
Member

The-Compiler commented Jul 20, 2017

Review status: all files reviewed at latest revision, 3 unresolved discussions.


tests/unit/completion/test_histcategory.py, line 122 at r17 (raw file):

Previously, rcorre (Ryan Roden-Corrent) wrote…

hmm, I can't actually reproduce your failure. I've tried playing around with a bunch of different TZ settings but can't get test_histcategory to fail. e.g. TZ=UTC24 tox -e py36 -- tests/unit/completion/testhistcategory.py should throw dates off by a whole day (you can verify with TZ=UTC24 python -c "import datetime; print(datetime.datetime.now())"

That's because tox isolates environment variables by default - you'll need to add TZ to passenv in the [testenv] section in tox.ini:

diff --git a/tox.ini b/tox.ini
index 04cf826d1..ce3da0edb 100644
--- a/tox.ini
+++ b/tox.ini
@@ -13,7 +13,7 @@ skipsdist = true
 setenv =
     QT_QPA_PLATFORM_PLUGIN_PATH={envdir}/Lib/site-packages/PyQt5/plugins/platforms
     PYTEST_QT_API=pyqt5
-passenv = PYTHON DISPLAY XAUTHORITY HOME USERNAME USER CI TRAVIS XDG_* QUTE_* DOCKER
+passenv = PYTHON DISPLAY XAUTHORITY HOME USERNAME USER CI TRAVIS XDG_* QUTE_* DOCKER TZ
 deps =
     -r{toxinidir}/requirements.txt
     -r{toxinidir}/misc/requirements/requirements-tests.txt

Comments from Reviewable

We need to tell sqlite to convert the timestamps to localtime during
formatting, otherwise it formats them as though you are in UTC.

Also fix up a few uses of mktime.
@The-Compiler
Copy link
Member

The-Compiler commented Jul 20, 2017

Reviewed 3 of 3 files at r19.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@The-Compiler
Copy link
Member

The-Compiler commented Jul 20, 2017

Review status: all files reviewed at latest revision, 2 unresolved discussions.


qutebrowser/browser/qutescheme.py, line 254 at r19 (raw file):

            # start_time is the last second of curr_date
            start_time = next_date.timestamp() - 1

Mea culpa, this acually breaks because this is a datetime.date and not a datetime.datetime... So either back to using time.mktime(next_date.timetuple()) here, or make this datetime objects, i.e. drop the .date() above and use something like datetime.datetime.combine(datetime.date.today(), datetime.time()).


Comments from Reviewable

A date object doesn't have a timestamp property. Go back to using
mktime.
@rcorre
Copy link
Collaborator Author

rcorre commented Jul 21, 2017

qutebrowser/browser/qutescheme.py, line 254 at r19 (raw file):

Previously, The-Compiler (Florian Bruhin) wrote…

Mea culpa, this acually breaks because this is a datetime.date and not a datetime.datetime... So either back to using time.mktime(next_date.timetuple()) here, or make this datetime objects, i.e. drop the .date() above and use something like datetime.datetime.combine(datetime.date.today(), datetime.time()).

Fixed, but I'm wondering why tests only failed on a few builds ... and why I couldn't get them to fail locally. I need to look into whether this is actually covered. Also I don't think the datetime.combine method would work here since the html history page wants dhe start and end of the dayay boundaries (so we don't want the current time embedded in there). Though I can't help but wonder if we can consolidate some logic between the js and html history pages, as right now they have pretty divergent logic...


Comments from Reviewable

@The-Compiler
Copy link
Member

The-Compiler commented Jul 21, 2017

I think this is ready apart from the check_coverage.py thing. If you want to, feel free to push the merge button after that's pushed, or let me know things are ready (unless you disagree) and I'll do so 😆 🎉


Reviewed 1 of 1 files at r20.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


qutebrowser/browser/qutescheme.py, line 254 at r19 (raw file):

Previously, rcorre (Ryan Roden-Corrent) wrote…

Fixed, but I'm wondering why tests only failed on a few builds ... and why I couldn't get them to fail locally. I need to look into whether this is actually covered. Also I don't think the datetime.combine method would work here since the html history page wants dhe start and end of the dayay boundaries (so we don't want the current time embedded in there). Though I can't help but wonder if we can consolidate some logic between the js and html history pages, as right now they have pretty divergent logic...

datetime.time() is midnight, so I think this would've been equivalent - but I'm okay with either really.

This only happens on some builds because if the check above which uses the javascript page with a new QtWebKit or QtWebEngine. So it's only tested on legacy QtWebKit.

I don't think it's worth it to consolidate the two. I'll drop the HTML one with v1.0, which will drop legacy QtWebKit and allow to force JS for qute://* via per-domain settings.


scripts/dev/check_coverage.py, line 160 at r20 (raw file):

        'completion/models/urlmodel.py'),
    ('tests/unit/completion/test_histcategory.py',
        'completion/models/histcategory.py'),

Looks like you can add completion/models/listcategory.py too - not sure why we didn't notice earlier though?


Comments from Reviewable

@rcorre
Copy link
Collaborator Author

rcorre commented Jul 21, 2017

pushed a fix for the coverage. I'll wait for one more test run to make sure nothing crazy happens, then one of us can merge it 🎊


Review status: 54 of 55 files reviewed at latest revision, 3 unresolved discussions.


qutebrowser/browser/qutescheme.py, line 254 at r19 (raw file):

Previously, The-Compiler (Florian Bruhin) wrote…

datetime.time() is midnight, so I think this would've been equivalent - but I'm okay with either really.

This only happens on some builds because if the check above which uses the javascript page with a new QtWebKit or QtWebEngine. So it's only tested on legacy QtWebKit.

I don't think it's worth it to consolidate the two. I'll drop the HTML one with v1.0, which will drop legacy QtWebKit and allow to force JS for qute://* via per-domain settings.

oh, good. I'm all for dropping the HTML page


qutebrowser/completion/models/histcategory.py, line 42 at r18 (raw file):

Previously, The-Compiler (Florian Bruhin) wrote…

This one still applies, see #2830.

Done.


qutebrowser/javascript/history.js, line 51 at r18 (raw file):

Previously, The-Compiler (Florian Bruhin) wrote…

This one still applies (but no hard feelings either way)

This is obsolete now, correct?


scripts/dev/check_coverage.py, line 160 at r20 (raw file):

Previously, The-Compiler (Florian Bruhin) wrote…

Looks like you can add completion/models/listcategory.py too - not sure why we didn't notice earlier though?

Done.


Comments from Reviewable

@The-Compiler
Copy link
Member

The-Compiler commented Jul 21, 2017

:lgtm: 🎉


Reviewed 1 of 1 files at r21.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


qutebrowser/javascript/history.js, line 51 at r18 (raw file):

Previously, rcorre (Ryan Roden-Corrent) wrote…

This is obsolete now, correct?

Yep! They could still be UTC, but it really doesn't matter, and if someone reads the source of the page, localtime is probably less confusing.


Comments from Reviewable

@The-Compiler
Copy link
Member

The-Compiler commented Jul 21, 2017

I've never been this excited about clicking a button... let's do this, and thanks so much!

@The-Compiler The-Compiler merged commit fba2533 into qutebrowser:master Jul 21, 2017
3 checks passed
3 checks passed
code-review/reviewable 55 files reviewed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@amosbird
Copy link
Contributor

amosbird commented Jul 21, 2017

Great!

@Quantum-cross
Copy link

Quantum-cross commented Aug 10, 2017

Wow, I felt this change hard after a git pull, it feels really awesome on my older x230. Honestly the old completion system is the only thing that dragged this computer down... now nothing can stop it! Thank you all for the hard work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.