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

Switch browsing history away from QWebHistoryInterface and add titles. #1350

Merged
merged 4 commits into from Jun 8, 2016

Conversation

@toofar
Copy link
Contributor

@toofar toofar commented Mar 20, 2016

Changes history items to be added when initialLayoutCompleted is fired as opposed to when navigation is initiated. Regarding #1345 this method saves the final url after redirects and doesn't save the url of the redirect.

There are some things I am unsure about:

  • Encoding. I haven't tested the serialization with unicode titles (and urls) on windows.
  • Testing. I was looking at the history behavior for redirects and errors and was wondering if this was worth adding to the test suite and how.

Here is some comparisons with dwb and otter:

Otter (for the qtwebkit backend at least) calls handleHistory() at a few
places but calls after the first one update an entry instead of adding a new
one. It gets called on title changed, navigate (load started I think) and load
finished. Due to events that would update current url and title not firing for
redirect urls otter saves the originally requested url as url and title into
history so if there is a chain of redirects all but the first will be saved
wrong.

dwb and QupZilla use load finished. They only save the final page on
redirects.

This method, the old method, dwb and otter all save the requested url to
history on DNS and 404 errors.


This change is Reviewable

@toofar toofar force-pushed the toofar:history-title branch 2 times, most recently from 9643181 to 3d48895 Mar 22, 2016
@The-Compiler The-Compiler added this to the v0.6.0 milestone Mar 29, 2016
@toofar toofar force-pushed the toofar:history-title branch from 3d48895 to 2fa349a Mar 31, 2016
@The-Compiler The-Compiler removed this from the v0.6.0 milestone Apr 1, 2016
@The-Compiler
Copy link
Member

@The-Compiler The-Compiler commented Apr 1, 2016

I decided to not add this to v0.6.0 - it looks good and much simpler than I thought it'd be, but as you said, there's potential for subtle issues. That's why I'd rather have it cook in -git a bit before releasing a version with it 😉

@@ -208,4 +212,3 @@ def init(parent=None):
"""
history = WebHistory(parent)
objreg.register('web-history', history)
QWebHistoryInterface.setDefaultInterface(history)

This comment has been minimized.

@The-Compiler

The-Compiler Apr 1, 2016
Member

This will still be needed to color visited links. I suggest you re-add this, simply ignore what's happening in addHistoryEntry, and add another method (say add_url or so).

@toofar toofar force-pushed the toofar:history-title branch from 2fa349a to c627b1d Apr 4, 2016
@toofar
Copy link
Contributor Author

@toofar toofar commented Apr 4, 2016

I added the call to setDefaultInterface back and that works. Though with the whole not saving redirects thing sites, like search engine result pages, that link via redirectors don't show their links as visited. Maybe it would be best to save both urls clicked on and pages visited. Bit more noise in the history file but that isn't an issue. Just need to control for duplicates...

@toofar toofar force-pushed the toofar:history-title branch from 45fedfc to f1a289c Apr 5, 2016
@toofar toofar force-pushed the toofar:history-title branch from f1a289c to 60768ca Apr 22, 2016
@toofar toofar force-pushed the toofar:history-title branch from 60768ca to f557a7a May 4, 2016
# QUrl.UrlFormattingOption.
if not self._orig_url.matches(self.cur_url,
getattr(QUrl, 'None',
getattr(QUrl, 'None_'))):

This comment has been minimized.

@The-Compiler

The-Compiler May 15, 2016
Member

This is a bit unfortunate indeed... But QUrl.UrlFormattingOption(0) seems to work as well and seems cleaner.

@@ -145,13 +146,29 @@ def __init__(self, win_id, parent=None):
self.loadProgress.connect(lambda p: setattr(self, 'progress', p))
objreg.get('config').changed.connect(self.on_config_changed)

@pyqtSlot()
def on_initial_layout_complete(self):

This comment has been minimized.

@The-Compiler

The-Compiler May 15, 2016
Member

nitpick: please call this _completed like the signal.

toofar added 4 commits Mar 20, 2016
Adds a title to the HistoryEntry class and includes it in the serialization
stuff. Not currently set from anywhere.

Not sure if anything more needs to be done to support non-ascii characters.
Everything works fine for me with unicode chars in url and title but
everything in my stack is utf-8.
Now adds a url to browser history once we have connected and got enough data
to start rendering the page. The previous approach saved urls as soon as
navigation was initiated, so upon encountering a redirect the final url wasn't
saved.

Using layout started rather than load finished means that pages whose contents
manage to load minus one troublesome asset will still be saved.
This allows webkit to color links that are clicked on but never rendered as
visited too. It also means if you get redirected from eg http://site.com to
http://site.com/ you have essentially duplicates in your history. This makes
the history completion a bit noisier. I suppose normalising paths before
checking for duplicates might help. Also note that otter has an isTypedIn flag
which might be used for dealing with this.
@toofar toofar force-pushed the toofar:history-title branch from f557a7a to cd21f63 May 17, 2016
@The-Compiler
Copy link
Member

@The-Compiler The-Compiler commented Jun 8, 2016

I'd like to get this in before I release v0.7 this week and start to refactoring everything for QtWebEngine.

If this is okay for you I'll just take over and add some tests myself.

QUrl.UrlFormattingOption(0)):
# If the url of the page is different than the url of the link
# originally clicked, save them both.
history.add_url(self._orig_url.toDisplayString())

This comment has been minimized.

@The-Compiler

The-Compiler Jun 8, 2016
Member

Is there a reason you're not adding the title here, or is that just an oversight?

@The-Compiler
Copy link
Member

@The-Compiler The-Compiler commented Jun 8, 2016

I'm adding a few comments about things I find out - you can ignore them, I'll fix it up as I'm writing tests anyways.

# If the url of the page is different than the url of the link
# originally clicked, save them both.
history.add_url(self._orig_url.toDisplayString())
history.add_url(self.cur_url.toDisplayString(), self.title())

This comment has been minimized.

@The-Compiler

The-Compiler Jun 8, 2016
Member

toDisplayString has the problem of e.g. decoding spaces in the URL which is undesirable here. This should probably be toString(QUrl.FullyEncoded | QUrl.RemovePassword) instead.

@The-Compiler The-Compiler merged commit cd21f63 into qutebrowser:master Jun 8, 2016
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants