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

Added history clear command #1264

Merged
merged 2 commits into from Jun 8, 2016
Merged

Conversation

@EliteTK
Copy link
Contributor

@EliteTK EliteTK commented Jan 23, 2016

These two commits add the history clear command, if you are willing to pull this I would be willing to write any required unit tests for it.

To fully implement anything like #58 would take a rather interesting rework of AppendLineParser.

I decided to forego this for a simpler approach for now.

Review on Reviewable

EliteTK added 2 commits Jan 23, 2016
The lineparser clear method, implemented for all lineparser subclasses,
clears the underlying file and also empties any lineparser data
structures.
WebHistory now has a clear() method which is also a command
(history-clear) which clears the qutebrowser history using the new
lineparser clear() method and emits a cleared signal.

The completion model urlmodel connects to the WebHistory.cleared signal
and clears its history category completion list.

I am adding this as a temporary fix before #58 or #1051 get implemented.
@The-Compiler
Copy link
Member

@The-Compiler The-Compiler commented Jan 23, 2016

The fail on AppVeyor wasn't your fault - I fixed it in master and hit rebuild now.


def clear(self):
self.data = []
self.save()

This comment has been minimized.

@The-Compiler

The-Compiler Jun 8, 2016
Member

This isn't needed as it's inherited from LineParser, but I'll take care of it.

@The-Compiler
Copy link
Member

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

As mentioned in IRC I'm picking this one up and adding some tests.

@The-Compiler The-Compiler merged commit 399aaa2 into qutebrowser:master Jun 8, 2016
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
@The-Compiler
Copy link
Member

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

Aaand merged!

I also used the opportunity to clean up the lineparser tests a bit, you can find the ones for clear here if you're curious:

@lahwaacz
Copy link
Contributor

@lahwaacz lahwaacz commented Jun 8, 2016

I think that the documentation should emphasize that the command clears only qutebrowser's history, not QtWebkit's history.

@The-Compiler
Copy link
Member

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

@lahwaacz Do you mean only the global history on disk and not the tab's back/forward history? I'm not sure how I'd phrase that though...

@lahwaacz
Copy link
Contributor

@lahwaacz lahwaacz commented Jun 8, 2016

Well, there are multiple levels of "history":

  • per tab (for going backwards/forwards)
  • per running instance, used for completion
  • in the ~/.local/share/qutebrowser/history file (the previous item is only a subset of this, limited by cmd-history-max-items and web-history-max-items options)
  • the QtWebkit's history (used to mark visited links with different color, and I guess for many other things) -- this is what #58 seems to be about, besides content cache, cookies, etc.

I haven't looked at the code, so I'm not sure which part is cleared by this :history-clear command, but it's definitely not the QtWebkit's history (I've tried it with --temp-basedir and :history-clear did not reset the color of visited links), which is the most relevant for sweeping traces, releasing disk space, keeping privacy and the like.

@The-Compiler
Copy link
Member

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

I just tried as well, and it does reset the color of visited links for me, which is also what I'd expect looking at the code.

I started with --temp-basedir, navigated to http://cmpl.cc/downloads/ and selected disp, then did go back. That shows disp in purple.

Then I did run :history-clear and press r, and the link is blue again.

It might also be worth pointing out that QtWebKit doesn't store any history in disk, it's qutebrowser which does that and provides Qt with a historyContains method.

@lahwaacz
Copy link
Contributor

@lahwaacz lahwaacz commented Jun 8, 2016

Oh, then I probably made a mistake when testing it, works as you say now. I'm sorry for the noise.

Still, I think that the documentation should say that the command currently does not clear cache, cookies, etc. Many other browsers have a "Clear history" entry in the menu, which includes subitems for browsing history, cache, cookies, etc. and by default everything is cleared.

@The-Compiler
Copy link
Member

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

I can see that, but I'm still struggling with how to best bring this across. "Clear all page history entries" in the description maybe, and a bigger explanation in the command help?

@lahwaacz
Copy link
Contributor

@lahwaacz lahwaacz commented Jun 8, 2016

Other browsers seem to call it "browsing history", so maybe "Clear all browsing history" for the description? Namely leaving out "entry", which might be associated with the other things like cache or cookies instead of URLs. And yes, there is enough room for a bigger explanation in the command help.

@The-Compiler
Copy link
Member

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

Updated to:

Clear all browsing history.

Note this only clears the global history
(e.g. ~/.local/share/qutebrowser/history on Linux) but not cookies,
the back/forward history of a tab, cache or other persistent data.

@lahwaacz
Copy link
Contributor

@lahwaacz lahwaacz commented Jun 8, 2016

Looks good, thank you!

@EliteTK EliteTK deleted the EliteTK:history-clear branch Jun 8, 2016
@The-Compiler
Copy link
Member

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

@EliteTK In case you're curious: I added tests for browser.history now, including one for clear.

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

4 participants