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

Quickmark completion #189

Merged
merged 12 commits into from
Oct 19, 2014
Merged

Quickmark completion #189

merged 12 commits into from
Oct 19, 2014

Conversation

claudehohl
Copy link
Contributor

@claudehohl claudehohl commented Oct 14, 2014

gnah, I created a new branch on top of my importer commits... I guess I totally fucked it up this time.

Here is the quickmark completer. With lousy performance, any tips how to access the marks values more 'directly'? :>


This change is Reviewable

@The-Compiler
Copy link
Member

No worries - mind rebasing it though?

From the top of my head:

git checkout master
git pull
git checkout quickmark-completion
git rebase master
git push --force

This looks like the most performant way it can currently be done, I'd have to test and profile it to say more about why it's slow.

@The-Compiler The-Compiler self-assigned this Oct 14, 2014
The-Compiler added a commit that referenced this pull request Oct 14, 2014
@The-Compiler
Copy link
Member

I did a profiler run to find out why it's slow:

profile_completer

I guess I could gain some time by not calling sourceModel() and caching it instead (no idea why it takes so long...). Also maybe there's some way to optimize lessThan a bit. I'll take a look at it.

The-Compiler added a commit that referenced this pull request Oct 15, 2014
For some reason calling sourceModel() takes quite some time, which accounts for
about 1-2s of delay when showing the completion.

This matters for #189 for example.
The-Compiler added a commit that referenced this pull request Oct 15, 2014
It still slows stuff down while typing, but at least it's a lot more responsive
for the initial load.

See #189.
It works, but:
* terrible performance (5s for ~1600 marks)
* split and join operations in the loop - i want direct access to name +
  url in the marks dict! how?
thx to the fix from The-Compiler!
@claudehohl
Copy link
Contributor Author

Something is kaputt now.. When I scroll through the completions, the completer "recompletes" itself with one result - the highlighted quickmark. Hmm...

The-Compiler added a commit that referenced this pull request Oct 15, 2014
I accidentally broke this in fb3682f because
the variable gets reset before the slot is executed now.

See #189.
@The-Compiler
Copy link
Member

Thanks, I fixed this now I hope.

Is it intended that the user can complete the URLs instead of the names? I'd have expected to complete the names with :quickmark-load. What about doing a QuickmarkCompletionModel and a OpenCompletionModel, and the one for :open at the moment just contains the URLs for the quickmarks, while :quickmark-load completes names?

@claudehohl
Copy link
Contributor Author

Excellent idea.

@The-Compiler
Copy link
Member

Also the model currently won't update when the user adds/removes quickmarks.

However doing this would mean we'd have to convert the stuff in quickmarks.py to a class inheriting from QObject, and adding pyqtSignals when the data is changed.

Is it okay if I do that, and then you can update your part accordingly?

@claudehohl
Copy link
Contributor Author

Of course. Thx!

@@ -25,6 +25,7 @@
from qutebrowser.models import basecompletion
from qutebrowser.utils import log, qtutils, objreg
from qutebrowser.commands import cmdutils
from qutebrowser.browser import quickmarks
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove this import now after 74839d7

@The-Compiler
Copy link
Member

Done now - rough guide to get it auto-updated:

  • Move the initializing back into its own function (sorry!)
  • Add a on_quickmarks_changed function decorated with @pyqtSlot() which does self.clear() and calls the initializing function again.
  • Make the _models attribute in utils/completer.py:Completer public (s/_models/models/g)
  • In app.py:_connect_signals, add something like:
quickmark_model = objreg.get('completer').models[usertypes.Completion.quickmarks]
quickmark_manager = objreg.get('quickmark-manager')
quickmark_manager.changed.connect(quickmark_model.on_quickmarks.changed)

Though the question is how long it takes to re-build the whole completion model... if it should take too long, a solution would be to do the changes twice in quickmarks.py (once in the model, once in the marks dict), but I'd like to avoid that.

@claudehohl
Copy link
Contributor Author

Dear Der-Kompilator, it's redi to mörtsch!

super().__init__(parent)
self._on_quickmarks_changed(self, match_field)

def _on_quickmarks_changed(self, parent=None, match_field='url'):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we don't use this as a slot anymore, can you move the stuff below into __init__ directly?

@The-Compiler The-Compiler merged commit 14d8d01 into qutebrowser:master Oct 19, 2014
@The-Compiler The-Compiler added the component: completion Issues related to the commandline completion or history. label Jun 16, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: completion Issues related to the commandline completion or history.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants