-
-
Notifications
You must be signed in to change notification settings - Fork 427
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
Import history from other browsers #3077
Conversation
@hgluka I don't know whether you consider it, but you could have added a new mode with its own file (say The change in |
@aadcg I dismissed that idea at first because it seems way too small for its own mode, but if the end goal is 4 or 5 different commands (one for each browser) then maybe it's warranted. |
I understand. For the sake of tidiness, I'd suggest creating a mode nonetheless. See |
I dismissed that idea at first because it seems way too small for its own mode, but if the end goal is 4 or 5 different commands (one for each browser) then maybe it's warranted.
We have modes with 0 commands, so you're safe here!
|
Done! But this is my first time adding a mode, so maybe I missed something. |
Maybe not so important right now. But I'd suggest setting slots |
I hate to change the direction, but why not putting it into the We have |
That was my idea originally. The issue is (or could be) that the command uses |
Looks good to me. Maybe the syntax of the macro is a bit unconventional in the sense that the sql query string seems out of place. Perhaps it could be preceded by a keyword. Nonetheless, it's not very important in my opinion. The functionality is the most important thing. |
I see your point. I just ordered the parameters by when they appear in the macro. My thinking right now is that the In that case, the SQL string should definitely come before it. |
> I hate to change the direction, but why not putting it into the `history-mode`?
>
> We have `import-bookmarks-from-html` in `bookmark-mode`, so there's a precedent for import commands belonging to the mode they influence.
That was my idea originally. The issue is (or could be) that the command uses `nyxt/mode/file-manager:file-source` and that isn't loaded until after `history-mode`. My solution was to just reorder the components in `nyxt.asd` and while it worked well enough, it might be dangerous later down the line. I assume that there's a reason why `history-mode` is one of the Core modes (and thus loaded earlier than the Prompter modes).
A bad, but nonetheless working solution could be offloading the symbol reading until file manager mode is available. Something like (read-from-string "nyxt/mode/file-manager:file-source") or (sym:resolve-symbol ...) should be enough.
This way, you don't have to reorder files, while having both convenient prompt sources and commands belonging to the right modes.
|
Perhaps someone else (maybe @Ambrevar or @jmercouris?) can chime in, because I don't really know the pros and cons of using On the other hand, I think that a separate mode could still be okay. Yes, all the commands would fit very well in Anyway, I'm still working on the feature in the meantime, so feel free to discuss! I have a moderate amount of popcorn at the ready. |
Resolve sym is effectively a hack to defer loading something so the compiler is happy. I would put it in a separate mode for now. |
So, it turns out that I don't have access to a Mac to test out an import from Safari. @jmercouris mentioned that it's not a priority. We can add that and Luakit in later. As far as Chrome and Firefox go, there are a couple of questions regarding them.
|
I think a separate command would be best for the user, otherwise when they search for their browser and don't find it, they will be confused. |
If that helps, in my case, |
@aadcg Interesting. What does your Maybe we can work on that after this is merged, but my lazy solution (showing the folder with all the profiles in the prompt buffer) works for now. The only thing I'd add to it is some kind of |
It's very simple because I use a single profile.
I'll test your work right now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested by issuing import-history-from-firefox
.
I think we must improve the UI, since these commands should be usable by the simplest of users. I don't think path-filter
will work, because the predicate will filter given a certain input, i.e. within a folder. I would suggest creating a custom class that inherits from prompter:source
or nyxt/mode/file-manager:file-source
(probably the former). In the process, you'll get acquainted with one of the key Nyxt classes (those related to the prompt buffer).
The constructor
would be set to the output of:
(uiop:run-program (list "find" (uiop:native-namestring "~/") "-name" "places.sqlite") :output t)
This is just a draft idea. What do you think?
Ideally we should also catch errors such as the one below, and tell the user what to do via echo
.
Could not open sqlite3 database /home/aadcg/.mozilla/firefox/apez2u28.aadcg/
Code CANTOPEN: no message.
[Condition of type SQLITE:SQLITE-ERROR]
After closing Firefox and attempting to import history again I was greeted with another error:
The value
NIL
is not of type
REAL
[Condition of type TYPE-ERROR]
Restarts:
0: [ABORT] abort thread (#<THREAD tid=22714 "Nyxt run-async command" RUNNING {1005CC0AA3}>)
Backtrace:
0: (LOCAL-TIME:UNIX-TO-TIMESTAMP NIL :NSEC 0)
Locals:
NSEC = 0
UNIX = NIL
1: ((:METHOD NYXT/MODE/HISTORY-MIGRATION:IMPORT-HISTORY-FROM-FIREFOX NIL)) [fast-method]
Locals:
NYXT/MODE/HISTORY-MIGRATION::CONN = #<DBD.SQLITE3:DBD-SQLITE3-CONNECTION {10095FDF93}>
NYXT/MODE/HISTORY-MIGRATION::HISTORY = #<HISTORY-TREE:HISTORY-TREE {1005248403}>
2: (NYXT::RUN-COMMAND #<NYXT:COMMAND NYXT/MODE/HISTORY-MIGRATION:IMPORT-HISTORY-FROM-FIREFOX (1)> NIL)
Locals:
ARGS = NIL
#:G7 = #<NYXT:COMMAND NYXT/MODE/HISTORY-MIGRATION:IMPORT-HISTORY-FROM-FIREFOX (1)>
OLD-%BUFFER = NIL
Have you seen it?
Interesting! It didn't occur to me to just find the
Yeah, I still need to handle some errors and also echo messages at certain steps like "Import started", "Import finished", etc.
I've never run into this issue. I think it means that there's a row in your |
|
@aadcg I implemented the changes you mentioned. Let me know what you think. If I didn't miss anything, I think this is pretty much ready to merge. I can squash it into a couple of commits + a changelog commit when someone approves it. |
@hgluka will review soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're getting there @hgluka.
I've left some comments.
Finally, users may be confused as to what needs to be done for the imported history to be available. Issuing set-url
after importing didn't show the entries in the "Global history" source. If restarting Nyxt is required (what I did), it needs to be echoed to the user. If there's an alternative, the user needs to be notified about it as well.
That's weird, because for me the imported stuff shows up as entries in the "Global history" source immediately. |
Let's not give this too much importance, since the issue may be on my side. I will try to reproduce it in the next review. |
source/mode/history-migration.lisp
Outdated
(str:split #\newline | ||
(nth-value 0 | ||
(uiop:run-program (list "find" (uiop:native-namestring "~/") | ||
"-regex" (browser-pattern source)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is OK for a draft, but the final version should not rely on find
.
Instead, use uiop:collect-sub*directories
to search for files recursively.
Also don't use (user-homedir-pathname)
to resolve the HOME dir.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And use a lambda instead of a regex, it's more powerful, readable, and efficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hgluka you may find inspiration to resolve HOME in the nfiles
library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also don't use
(user-homedir-pathname)
to resolve the HOME dir.
I'm assuming you mean "don't use ~/
" here, because that's what I'm using?
It seems like (user-homedir-pathname)
is used in a couple of places in Nyxt as well as to resolve HOME in nfiles
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, sorry, yes, I meant "don't use ~/
" :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having some trouble with collect-sub*directories
. I tried this, for example:
(let ((results '()))
(uiop:collect-sub*directories
(uiop:ensure-directory-pathname (user-homedir-pathname))
(constantly t)
(constantly t)
(lambda (subdir)
(when (uiop:file-exists-p (nfiles:join subdir "places.sqlite"))
(push (nfiles:join subdir "places.sqlite") results)))))
And it takes a very long time to run (more than 10 minutes).
Now, my home directory is huge, so restricting the search to ~/.mozilla
should be a lot faster (although it might exclude some possible locations).
But I still feel like I must be missing something, since find
takes a couple of seconds at most. Any idea why this would happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hgluka Restricting the search to ~/.mozilla
may be less future-proof, but it's not critical.
I have never used uiop:collect-sub*directories
so I can't provide much insight. Personally, I don't consider the previous solution that used find
to be a huge sin since no non-POSIX compliant flag is being used. @Ambrevar can explain why he considers this solution subpar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I figured out that the issue is following symlinks (that happen to form a loop). find
apparently detects those loops, so I either need to do the same or just avoid all symlinks outright.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, if it loops, then it should not terminate, how come it took "10 minutes"?
@hgluka since me and @Ambrevar may have different views on this topic, I would suggest that you should follow your own view. After all, you're the one that has been putting the effort on the task. Ideally this PR should be merged no later than August 11 - the date of the next release is August 13. Also, I'd encourage you to write a small article announcing this feature given its importance to attract new users (https://nyxt.atlas.engineer/articles). CC @jmercouris. |
That's a huge performance penalty. Maybe use UIOP as a fallback when find is not available or something like that. We'll talk about it during our call today :-) |
Please let me know when you have squashed/rebased and are ready to merge this in! |
Add history-migration mode with commands for importing history from Firefox and several Chromium-based browsers.
Switch from cl-dbi to cl-sqlite because `mode/history-migration` only works with SQLite databases.
Use `uiop:collect-sub*directories` to search for files, instead of GNU find.
@jmercouris Well, after our talk yesterday, I think it's ready to merge now. |
That's perfectly fine :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You did a great job at switching to uiop:collect-sub*directories
, thanks!
Can you open a new pull request to address my review?
(echo "Searching for history file. This may take some time.") | ||
(or (let ((results '())) | ||
(uiop:collect-sub*directories | ||
(user-homedir-pathname) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't start from here, as the search space may be much too big.
Instead, start from a collection of well known location for Firefox, Chrome, etc.
Like ~/.mozilla
, ~/.config/chromium
, etc.
(user-homedir-pathname) | ||
(constantly t) | ||
(lambda (subdir) | ||
(equal (iolib/os:file-kind subdir) :directory)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very slow, instead use uiop:directory-pathname-p
.
But I'm not sure what you are trying to do here, since it looks like this does nothing more than (constantly t)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for avoiding symbolic links, since uiop:directory-pathname-p
doesn't detect them.
I could change it to (not (equal (iolib/os:file-kind subdir) :symbolic-link)))
to make that clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still, no need for iolib
just for this. You can use uiop:truename*
, or uiop:resolve-symlinks
. To avoid cycling, you can push each directory to a hash-table and check if the next subdir was visited already.
|
||
(define-class external-browser-history-source (prompter:source) | ||
((prompter:name "History files") | ||
(browser-lambda :accessor browser-lambda :initarg :browser-lambda) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name is much too generic:
- It's not clear if "browser" refers to the
browser
class or third-party web browsers like Firefox. lambda
is too vague: here it is about a "history-finder" or something like that.
(equal (iolib/os:file-kind subdir) :directory)) | ||
(lambda (subdir) | ||
(let ((browser-history-file (funcall* (browser-lambda source) subdir))) | ||
(when browser-history-file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: Replace this (let ... (when
by alex:when-let
.
(let ((browser-history-file (funcall* (browser-lambda source) subdir))) | ||
(when browser-history-file | ||
(push browser-history-file results))))) | ||
results) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: You may want to reverse (nreverse results)
instead, if you care about the ordering. (May not be significant anyways.)
:sources (make-instance 'external-browser-history-source | ||
:browser-lambda (lambda (subdir) | ||
(when (uiop:file-exists-p (uiop:merge-pathnames* "places.sqlite" subdir)) | ||
(uiop:merge-pathnames* "places.sqlite" subdir)))))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're calling uiop:merge-pathnames*
twice. Let-bind it to save this extra call.
(when (and (string= "google-chrome" | ||
(nfiles:basename (nfiles:parent subdir))) | ||
(uiop:file-exists-p (uiop:merge-pathnames* "History" subdir))) | ||
(uiop:merge-pathnames* "History" subdir)))))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same.
(when (and (string= "chromium" | ||
(nfiles:basename (nfiles:parent subdir))) | ||
(uiop:file-exists-p (uiop:merge-pathnames* "History" subdir))) | ||
(uiop:merge-pathnames* "History" subdir)))))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same.
(when (and (string= "Brave-Browser" | ||
(nfiles:basename (nfiles:parent subdir))) | ||
(uiop:file-exists-p (uiop:merge-pathnames* "History" subdir))) | ||
(uiop:merge-pathnames* "History" subdir)))))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same.
(when (and (str:starts-with-p "vivaldi" | ||
(nfiles:basename (nfiles:parent subdir))) | ||
(uiop:file-exists-p (uiop:merge-pathnames* "History" subdir))) | ||
(uiop:merge-pathnames* "History" subdir)))))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same.
I don't think it's important to address those points now. I've assigned Luka higher priorities right now. Those changes are easy enough to make, but i don't think the context switch is good. |
I don't mind context switching every once in a while! Plus, there aren't any big changes to make. I'll do this when I have the time (and need a break from looking at |
Would be a good idea to wait for user feedback too. The next release featuring this change lands on August 14. |
Considering this was just merged, I would argue the other way around: better address this now that the code is fresh, instead of coming back to it weeks later when it looks like a stranger wrote it. |
Most of it are trivial changes. The main significant change is the home-dir traversal, still, it's an easy fix. |
Description
This PR aims to add commands for importing history from other browsers that use sqlite for data storage.
Some browsers that are on the docket:
Safari(Not yet)Luakit(Not yet)Resolves #2435
Discussion
I'm not sure if reordering the components in
nyxt.asd
was the right way to go, but I needed to usenyxt/mode/file-manager:file-source
in my function because theplaces.sqlite
file is user-supplied.The idea is to make the command search all the usual places and find it automatically, so I might be able to revert the order in
nyxt.asd
when that's done.As for the actual history side of things, this imports everything as separate entries with
htree:add-entry
, but maybe we want a different structure?Checklist:
Everything in this checklist is required for each PR. Please do not approve a PR that does not have all of these items.
cd /path/to/nyxt/checkout git submodule add https://gitlab.common-lisp.net/nyxt/py-configparser _build/py-configparser
:documentation
s written in the aforementioned style. (It's OK to skip the docstring for really trivial parts.)changelog.lisp
with my changes if it's anything user-facing (new features, important bug fix, compatibility breakage).migration.lisp
entry for all compatibility-breaking changes.(asdf:test-system :nyxt)
and(asdf:test-system :nyxt/gi-gtk)
) and they pass.