Add url history completion for open. #531
Conversation
|
Awesome, thanks for your work! :)
The newhistory branch shouldn't be an issue - it's just a
The list of models is mapped to the arguments - take a look at In the commit linked above I just added an exception if there are more
Currently you'd have to create a new The API for managing completions really should be simpler, ideally taking a
I think the Qt documentation says the slots (methods/functions) connected to But either way, I think it'd be clearer to add a new argument to the signal
I remember funny things happening when mutating existing completion models and Honestly I'm surprised adding new items seems to be working so well - I think
Oh, nice! I used Though I think |
|
Thanks for the reply. Pretty much exactly what I was looking for. I will look into fixing this up and changing the history interface next week. I haven't exactly been dogfooding so consider me suitably chastised for just treating the symptom and ignoring the cause of crashes. |
|
Here is what I have so far. I made a new completion model UrlCompletionModel I also am now sorting the history and the But ... It doesn't work right now because I was getting the history entries to |
|
Yeah, it's currently a set for performance reasons - for some reason QtWebKit called I think the best way would be to add an Though here I also worry a bit about the performance and memory usage. Have you tried how this performs with some thousand history entries? I have around 13000 entries after a bit more than a month, so let's say 150k or 200k entries after a year? Maybe it should only use the most recent N items (e.g. using itertools.islice), or only stuff visited more than a certain threshold? Regarding the code style: You maybe already know I'm sometimes a bit more obsessive about this than I should be ;) If you prefer me to fix such issues directly, please tell me! For |
Adds a basic completion model implementation around the global browser history and registers that for the open command. Modifies WebHistory to add an __iter__ method and to use a dict instead of a set to store an entire HistoryEntry for each archived item instead of just the URL. Brief tests showed that the lookup time for set and dict are very similar. They are at least on the same order of magnitude. Testing membership of a list on the other hand, as was the case before a set was used, was four orders of magnitude slower on my machine.
I went to some effort to avoid duplipcating code which which leads to some arguably ugly class method calling.
Each new HistoryEntry is emitted after being added to the global history store. Current members of the HistoryEntry are `url` and `atime`. `title` should be coming soon.
|
I changed the I added the I made the completion model fill_model methods into static methods because Anyway, if this is alright I am going to look into calling the addHistoryEntry hook |
|
+1 for this. |
This makes it possible to use Qt's QSortFilterProxyModel::lessThan option for completions where it doesn't make sense to priorize matches starting with the entered string, e.g. for URLs. In return, we get a *much* better performance (several seconds when opening the completion). See #531.
|
Whoa, this works a lot better than I thought it would, and it feels awesome to use! 👍 I also noticed some other subtle completion bugs while testing this, fixed above. Also I implemented a "dumb" sort in the I still want to do some other changes (e.g. the thing mentioned above) so I won't merge this yet, but I hope I'll have time for it this evening. If you want to experiment, rebase to the current master and apply this patch on top of your branch: commit 6674f74b07175fe17248b7890f7937dd7735f957
Author: Florian Bruhin <git@the-compiler.org>
Date: Wed Mar 11 07:12:18 2015 +0100
Use CompletionFilterModel's sort implementation.
diff --git a/qutebrowser/completion/completer.py b/qutebrowser/completion/completer.py
index 2f7b223..e57b3a8 100644
--- a/qutebrowser/completion/completer.py
+++ b/qutebrowser/completion/completer.py
@@ -19,7 +19,7 @@
"""Completer attached to a CompletionView."""
-from PyQt5.QtCore import pyqtSlot, QObject, QTimer
+from PyQt5.QtCore import pyqtSlot, QObject, QTimer, Qt
from qutebrowser.config import config, configdata
from qutebrowser.commands import cmdutils, runners
@@ -86,7 +86,8 @@ class Completer(QObject):
self._models[usertypes.Completion.helptopic] = CFM(
models.HelpCompletionModel(self), self)
self._models[usertypes.Completion.url_history_and_quickmarks] = CFM(
- models.UrlCompletionModel('url', self), self)
+ models.UrlCompletionModel('url', self), self,
+ dumb_sort=Qt.DescendingOrder)
def _init_setting_completions(self):
"""Initialize setting completion models."""
diff --git a/qutebrowser/completion/models/completion.py b/qutebrowser/completion/models/completion.py
index cdfce48..c562f59 100644
--- a/qutebrowser/completion/models/completion.py
+++ b/qutebrowser/completion/models/completion.py
@@ -234,12 +234,6 @@ class UrlCompletionModel(base.BaseCompletionModel):
WebHistoryCompletionModel.history_changed(
self, e, self._histcat))
- def sort(self, column, order=Qt.AscendingOrder):
- # sort on atime, descending
- # Ignoring the arguments because they are hardcoded in the CFM
- # anyway.
- self._histcat.sortChildren(2, Qt.DescendingOrder)
-
class WebHistoryCompletionModel(base.BaseCompletionModel):
@@ -266,11 +260,14 @@ class WebHistoryCompletionModel(base.BaseCompletionModel):
cat = model.new_category("History")
for entry in histstore:
- model.new_item(cat, entry.url, "", entry.atime)
+ atime = int(entry.atime)
+ model.new_item(cat, entry.url, "", str(atime), sort=atime)
def history_changed(self, entry, cat):
if entry.url:
- self.new_item(cat, entry.url, "", str(entry.atime))
+ atime = int(entry.atime)
+ self.new_item(cat, entry.url, "", str(atime), sort=atime)
+
class QuickmarkCompletionModel(base.BaseCompletionModel): |
The-Compiler
added
enhancement
component: ui
|
Some notes to myself (or you, just tell me if you start working on it!) what to check/change:
I'll also some line notes - feel free to fix them or not (then I'll do this evening hopefully). In either case, don't take it personally because I'm a perfectionist ;) |
| self._old_urls.add(url) | ||
| atime, url = line.rstrip().split(maxsplit=1) | ||
| # This de-duplicates history entries. We keep later ones in the | ||
| # file which usually the last ones accessed. If you want |
There was a problem hiding this comment.
"which usually the last ones accessed" doesn't make sense, but I also don't understand if it was "which usually are the last ones accessed" - do you mean "in the dict"?
|
I just got an exception: Log: |
|
I improved/changed some things based on this PR and pushed that to the histcomplete branch. I opened a new pull request at #547 for the outstanding changes - to keep things organized let's continue discussing this there. This way, further PRs can also be made against the Unfortunately this still needs some changes before being merged because of the exceptions and segfaults I'm seeing. |
The-Compiler
added
the
component: completion
Add url history completion for open.
I mostly just copied this off of the quickmark completion. It probably
isn't done yet but I had some questions to raise. Also I see you have a
newhistory branch but can't see any conflicts on there yet.
I wanted to have the open command complete on history and on quickmarks
(well, I don't care about completing on quickmarks but I don't want to
break it). When commands are registered multiple completion models can
be passed in but it seems that only one of them is used. Is there any
way to chain completions or should I just create a new
OpenCommandCompletionModel or something which does both?
When the WebHistoryCompletionModel gets a history changed signal it just
pops off the most recent history to add to the completion model. I am
not sure how these signals are handled. Is it possible the history to be
appended to a second time before the signal gets handled? If so I will
either change it to walk backwards through history until it sees one it
has seen before or change the signal to a typed signal (I think these
are a thing) that passes the new history entry to the handlers.
Lastly there was a NoneType has no method casefold() exception being
thrown when using this new completion model and I don't know why that
wasn't showing up when using the quickmarks completion. To reproduce it
just remove the
if not datacheck and try to open a new page by usingthe open command and not selecting one of the completion options. I don't
know whether this is a bug I have caused or just exposed.
Lastly it would be nice to be able to complete based on page title too.
Since the QWebHistoryInterface API doesn't pass this information would
you consider adding history items at an appropriate stage of loading
from in qutebrowser? Like these guys did: https://git.reviewboard.kde.org/r/104257/diff/2/#1
This change is