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

feat: firefox bookmarks #1593

Closed
wants to merge 10 commits into from

Conversation

tangxinfa
Copy link

ivy interface of firefox bookmarks

@tangxinfa
Copy link
Author

I created a standalone package https://github.com/tangxinfa/counsel-firefox-bookmarks, enjoy it:)

counsel.el Outdated
;;** `counsel-firefox-bookmarks`
(defvar counsel-firefox-bookmarks-html-file
(car (file-expand-wildcards "~/.mozilla/firefox/*/bookmarks.html"))
"Firefox's auto exported html bookmarks file.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • HTML is an acronym, so it should be written in upper case.
  • "Firefox's auto exported" is ambiguous; it should be "Firefox's automatically exported" or "Firefox's auto-exported".

counsel.el Outdated

After closing firefox, you will be able to browse your bookmarks."
(interactive)
(ivy-read "Firefox Bookmarks: " (counsel-firefox-bookmarks--candidates)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: the prompt is usually singular, not plural, as the intention is to complete to a single bookmark; and bookmark is not a proper noun, so there is no need to capitalise it. In other words, I think the prompt should read "Firefox bookmark: ".

counsel.el Outdated
(interactive)
(ivy-read "Firefox Bookmarks: " (counsel-firefox-bookmarks--candidates)
:history 'counsel-firefox-bookmarks-history
:action 'counsel-firefox-bookmarks-action
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: counsel-firefox-bookmarks-action is being used as a function here, so we should quote it as #'counsel-firefox-bookmarks-action to avail of byte-compiler warnings (and possibly future optimisations).

counsel.el Outdated

user_pref(\"browser.bookmarks.autoExportHTML\", true);

After closing firefox, you will be able to browse your bookmarks."
Copy link
Collaborator

@basil-conto basil-conto May 27, 2018

Choose a reason for hiding this comment

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

  • Firefox is a proper noun, so it should consistently be capitalised.
  • HTML is an acronym, so it should be consistently written in upper case.
  • "Ivy interface of" implies possession; better to phrase it as "Ivy interface to".
  • about:config is a URL, so Emacs can hyperlink it if you quote it with backquote and apostrophe (see (elisp) Documentation Tips).

Given these comments, I propose the following wording instead:

  "Ivy interface to Firefox bookmarks.
This requires HTML bookmark export to be enabled in Firefox.
To do this, open URL `about:config' in Firefox, make sure that
the value of the setting \"browser.bookmarks.autoExportHTML\" is
\"true\" by, say, double-clicking it, and then restart Firefox."

or:

  "Complete Firefox bookmarks with Ivy.
This requires HTML bookmark export to be enabled in Firefox.
To do this, open URL `about:config' in Firefox, make sure that
the value of the setting \"browser.bookmarks.autoExportHTML\" is
\"true\" by, say, double-clicking it, and then restart Firefox."

counsel.el Outdated
(browse-url (get-text-property 0 'href x)))

(defun counsel-firefox-bookmarks--candidates ()
(let ((candidates))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: this is usually written (let (candidates) ...), as support for (let ((candidates)) ...) is only documented in the manual ((elisp) Local Variables), not let's docstring:

let is a special form in `C source code'.

(let VARLIST BODY...)

Bind variables according to VARLIST then eval BODY.
The value of the last form in BODY is returned.
Each element of VARLIST is a symbol (which is bound to nil)
or a list (SYMBOL VALUEFORM) (which binds SYMBOL to the value of VALUEFORM).
All the VALUEFORMs are evalled before any symbols are bound.

counsel.el Outdated
(while (re-search-forward "*?<A HREF=\"\\([^\"]+\\)\"[^>]*>\\([^<]+\\)</A>" nil t)
(let ((a (match-string 0))
(href (match-string 1))
(text (match-string 2))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will include HTML entities such as &amp; in text. You should decode them first:

(text (save-match-data
        (xml-substitute-special (match-string 2))))

(after calling (require 'xml) at the beginning of the function).

counsel.el Outdated
(tags nil))
(if (string-match "TAGS=\"\\([^\"]+\\)\"" a)
(setq tags (match-string 1 a)))
(push (propertize (format "%s%s" text (if tags (concat " :" (replace-regexp-in-string "," ":" tags) ":") "")) 'href href) candidates))))
Copy link
Collaborator

@basil-conto basil-conto May 27, 2018

Choose a reason for hiding this comment

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

This line is very long and hard to read, and I think can be optimised a bit, as each one of replace-regexp-in-string, format, concat, and propertize may allocate a new string. Furthermore, horizontal space is limited, so I think the four spaces should be reduced to a single space. How about:

(let* ((a (match-string 0))
       (href (match-string 1))
       (text (save-match-data
               (xml-substitute-special (match-string 2))))
       (tags (and (string-match "TAGS=\"\\([^\"]+?\\)\"" a)
                  (match-string 1 a))))
  (put-text-property 0 (length text) 'href href text)
  (push (if tags
            (concat text " :" (subst-char-in-string ?, ?: tags t) ":")
          text)
        candidates))

counsel.el Outdated
(if (string-match "TAGS=\"\\([^\"]+\\)\"" a)
(setq tags (match-string 1 a)))
(push (propertize (format "%s%s" text (if tags (concat " :" (replace-regexp-in-string "," ":" tags) ":") "")) 'href href) candidates))))
(warn "`counsel-firefox-bookmarks-html-file` not exists"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • "not exists" should be "does not exist".
  • warn should not be used by packages/libraries, as it hard-codes the emacs custom group.
  • It doesn't really make sense to display a warning before passing an empty collection to ivy-read; better to error or signal a file-error with a more descriptive error message and abort completion.

counsel.el Outdated

(defun counsel-firefox-bookmarks--candidates ()
(let ((candidates))
(if (and counsel-firefox-bookmarks-html-file (file-exists-p counsel-firefox-bookmarks-html-file))
Copy link
Collaborator

Choose a reason for hiding this comment

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

insert-file-contents expects its argument to satisfy file-readable-p, not just file-exists-p.

counsel.el Outdated
"Browse candidate X."
(browse-url (get-text-property 0 'href x)))

(defun counsel-firefox-bookmarks--candidates ()
Copy link
Collaborator

Choose a reason for hiding this comment

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

WIBNI every function had a docstring?

@basil-conto
Copy link
Collaborator

basil-conto commented May 27, 2018

I created a standalone package https://github.com/tangxinfa/counsel-firefox-bookmarks

This feature shouldn't be maintained in two separate places; it should either live as a separate package, or be included in counsel.el, but not both.

@basil-conto
Copy link
Collaborator

basil-conto commented May 27, 2018

Given my comments above, I suggest the following refactor:

(declare-function xml-substitute-special "xml")

(defun counsel-firefox-bookmarks--candidates ()
  "Return list of `counsel-firefox-bookmarks' candidates."
  (unless (and counsel-firefox-bookmarks-html-file
               (file-readable-p counsel-firefox-bookmarks-html-file))
    (signal 'file-error (list "Opening `counsel-firefox-bookmarks-html-file'"
                              "No such readable file"
                              counsel-firefox-bookmarks-html-file)))
  (require 'xml)
  (with-temp-buffer
    (insert-file-contents counsel-firefox-bookmarks-html-file)
    (let ((case-fold-search t)
          candidates)
      (while (re-search-forward
              "<a href=\"\\([^\"]+?\\)\"[^>]*?>\\([^<]*?\\)</a>" nil t)
        (let* ((a (match-string 0))
               (href (match-string 1))
               (text (if (= (match-beginning 2) (match-end 2))
                         href
                       (save-match-data
                         (xml-substitute-special (match-string 2)))))
               (tags (and (string-match "tags=\"\\([^\"]+?\\)\"" a)
                          (match-string 1 a))))
          (put-text-property 0 (length text) 'href href text)
          (push (if tags
                    (concat text " :" (subst-char-in-string ?, ?: tags t) ":")
                  text)
                candidates)))
      candidates)))

Alternatively, you can construct an alist instead of using text properties:

(defun counsel-firefox-bookmarks-action (x)
  "Browse candidate X."
  (browse-url (cdr x)))

(declare-function xml-substitute-special "xml")

(defun counsel-firefox-bookmarks--candidates ()
  "Return list of `counsel-firefox-bookmarks' candidates."
  (unless (and counsel-firefox-bookmarks-html-file
               (file-readable-p counsel-firefox-bookmarks-html-file))
    (signal 'file-error (list "Opening `counsel-firefox-bookmarks-html-file'"
                              "No such readable file"
                              counsel-firefox-bookmarks-html-file)))
  (require 'xml)
  (with-temp-buffer
    (insert-file-contents counsel-firefox-bookmarks-html-file)
    (let ((case-fold-search t)
          candidates)
      (while (re-search-forward
              "<a href=\"\\([^\"]+?\\)\"[^>]*?>\\([^<]*?\\)</a>" nil t)
        (let* ((a (match-string 0))
               (href (match-string 1))
               (text (if (= (match-beginning 2) (match-end 2))
                         href
                       (save-match-data
                         (xml-substitute-special (match-string 2)))))
               (tags (and (string-match "tags=\"\\([^\"]+?\\)\"" a)
                          (match-string 1 a))))
          (push (cons (if tags
                          (concat text
                                  " :" (subst-char-in-string ?, ?: tags t) ":")
                        text)
                      href)
                candidates)))
      candidates)))

In the future, IWBNI we used libxml-parse-html-region (available since Emacs 24) when (libxml-available-p), instead of parsing HTML with regexps, which is brittle.

counsel.el Outdated
(defun counsel-firefox-bookmarks--candidates ()
(let ((candidates))
(if (and counsel-firefox-bookmarks-html-file (file-exists-p counsel-firefox-bookmarks-html-file))
(with-temp-buffer
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indentation seems off in this function; are you using custom settings?

@basil-conto
Copy link
Collaborator

IWBNI if the commits in this PR were squashed into a single commit.

By the way, this feature is pretty cool! Please don't take my comments as anything other than suggestions for improvement.

@basil-conto
Copy link
Collaborator

basil-conto commented May 28, 2018

In the future, IWBNI we used libxml-parse-html-region (available since Emacs 24) when (libxml-available-p), instead of parsing HTML with regexps, which is brittle.

Here's an attempt to default to libxml parsing, with regexps as a fallback:

(defun counsel--firefox-bookmarks-cand (href text tags)
  "Propertize a string candidate for `counsel-firefox-bookmarks'.
HREF is the bookmark's non-empty URL.  TEXT is the bookmark's
description, which, when empty, defaults to HREF.  TAGS, when
non-nil, is a string of comma-separated tags to append to TEXT."
  (when (zerop (length text))
    (setq text href))
  (put-text-property 0 1 'href href text)
  (if tags
      (concat text " :" (subst-char-in-string ?, ?: tags t) ":")
    text))

(defun counsel--firefox-bookmarks-libxml ()
  "Parse current buffer contents as Firefox HTML bookmarks.
Return list of propertized string candidates for
`counsel-firefox-bookmarks'.
Note: This function requires libxml2 support."
  ;; Perform iterative pre-order depth-first search instead of using
  ;; `dom.el' because the latter is new to Emacs 25 and uses recursion.
  (let ((stack (cddr (libxml-parse-html-region (point-min) (point-max))))
        cands)
    (while (let ((node (pop stack)))
             (if (eq (car-safe node) 'a)
                 (let* ((text (cl-caddr node))
                        (attrs (cadr node))
                        (href (cdr (assq 'href attrs)))
                        (tags (cdr (assq 'tags attrs))))
                   (unless (zerop (length href))
                     (push (counsel--firefox-bookmarks-cand href text tags)
                           cands)))
               (dolist (child (nreverse (cddr node)))
                 (when (consp child)
                   (push child stack))))
             stack))
    cands))

(declare-function xml-substitute-special "xml")

(defun counsel--firefox-bookmarks-regexp ()
  "Like `counsel--firefox-bookmarks-libxml', but using regexps.
This function is used when Emacs lacks libxml support."
  (require 'xml)
  (let ((case-fold-search t)
        cands)
    (while (re-search-forward
            "<a href=\"\\([^\"]+?\\)\"[^>]*?>\\([^<]*?\\)</a>" nil t)
      (let* ((a (match-string 0))
             (href (match-string 1))
             (text (save-match-data
                     (xml-substitute-special (match-string 2))))
             (tags (and (string-match "tags=\"\\([^\"]+?\\)\"" a)
                        (match-string 1 a))))
        (push (counsel--firefox-bookmarks-cand href text tags) cands)))
    cands))

(defalias 'counsel--libxml-available-p
  (if (fboundp 'libxml-available-p)
      #'libxml-available-p
    (lambda ()
      (and (fboundp 'libxml-parse-xml-region)
           (with-temp-buffer
             (insert "<xml/>")
             (libxml-parse-xml-region (point-min) (point))))))
  "Compatibility shim for `libxml-available-p'.
See URL `https://lists.gnu.org/archive/html/emacs-devel/2017-10\
/msg00687.html'.")

(defun counsel--firefox-bookmarks ()
  "Return list of `counsel-firefox-bookmarks' candidates."
  (unless (and counsel-firefox-bookmarks-file
               (file-readable-p counsel-firefox-bookmarks-file))
    (signal 'file-error (list "Opening `counsel-firefox-bookmarks-file'"
                              "No such readable file"
                              counsel-firefox-bookmarks-file)))
  (with-temp-buffer
    (insert-file-contents counsel-firefox-bookmarks-file)
    (if (counsel--libxml-available-p)
        (counsel--firefox-bookmarks-libxml)
      (counsel--firefox-bookmarks-regexp))))

In my benchmarks, the libxml approach is about twice as fast and results in three times less GC. WDYT?

tangxinfa added a commit to tangxinfa/counsel-firefox-bookmarks that referenced this pull request Jun 2, 2018
@tangxinfa
Copy link
Author

@basil-conto Thank you very much, your refactors is very excellent. I select one of your refactors and many other suggests, and added a face to highlight tags.

I also tested the libxml version, it works, but the regex version already have acceptable performance on my 6000+ bookmarks file, and i prefer to keep the code base concise in counsel.el , so the libxml version is not selected.

Welcome to give me more suggests:)

tangxinfa added a commit to tangxinfa/counsel-firefox-bookmarks that referenced this pull request Jun 2, 2018
tangxinfa added a commit to tangxinfa/counsel-firefox-bookmarks that referenced this pull request Jun 2, 2018
@basil-conto
Copy link
Collaborator

basil-conto commented Jun 2, 2018

@tangxinfa

I also tested the libxml version, it works, but the regex version already have acceptable performance on my 6000+ bookmarks file, and i prefer to keep the code base concise in counsel.el, so the libxml version is not selected.

Indeed, I never said that the regexp implementation suffers from poor performance; but I think it's worth including the libxml implementation for better speed, memory consumption, and robustness. Anyway, I can always open a new PR for this, so no worries. :)

counsel.el Outdated

(defface counsel-firefox-bookmarks-tag
'((t :inherit font-lock-comment-face))
"Face used by `counsel-firefox-bookmarks' for tag."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better to write "...for tags." than "...for tag."

Copy link
Author

Choose a reason for hiding this comment

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

Done

counsel.el Outdated
(push (cons (if tags
(concat text
" :"
(string-join
Copy link
Collaborator

Choose a reason for hiding this comment

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

string-join was only added in Emacs 24.4, so Counsel does not support it. If Counsel supported it, you would have to (eval-when-compile (require 'subr-x)) in order to use it.

counsel.el Outdated
(string-join
(mapcar #'(lambda (tag)
(propertize tag 'face 'counsel-firefox-bookmarks-tag))
(split-string (match-string 1 a) "," t)) ":")
Copy link
Collaborator

@basil-conto basil-conto Jun 2, 2018

Choose a reason for hiding this comment

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

  • (match-string 1 a) is already bound to the local variable tags; use it.
  • propertize needlessly allocates a new string; use put-text-property or similar instead.
  • You are needlessly traversing the list of tags twice by calling mapcar and then string-join; use mapconcat instead.
  • ":" is hidden behind the call to split-string; put it on a separate line.
  • The lambda macro is self-quoting; no need for #'.

Do you really want to apply the counsel-firefox-bookmarks-tag face only to the tag names, and not the colon : delimiters? I don't mind either way, but applying the face to the whole :foo:bar:baz: string would be simpler, faster, and more similar to, say, Org Mode tags.

@basil-conto
Copy link
Collaborator

Given my latest comments, I propose the following refactor:

(defun counsel-firefox-bookmarks--candidates ()
  "Return list of `counsel-firefox-bookmarks' candidates."
  (unless (and counsel-firefox-bookmarks-file
               (file-readable-p counsel-firefox-bookmarks-file))
    (signal 'file-error (list "Opening `counsel-firefox-bookmarks-file'"
                              "No such readable file"
                              counsel-firefox-bookmarks-file)))
  (require 'xml)
  (with-temp-buffer
    (insert-file-contents counsel-firefox-bookmarks-file)
    (let ((case-fold-search t)
          candidates)
      (while (re-search-forward
              "<a href=\"\\([^\"]+?\\)\"[^>]*?>\\([^<]*?\\)</a>" nil t)
        (let* ((a (match-string 0))
               (href (match-string 1))
               (text (if (= (match-beginning 2) (match-end 2))
                         href
                       (save-match-data
                         (xml-substitute-special (match-string 2)))))
               (tags (and (string-match "tags=\"\\([^\"]+?\\)\"" a)
                          (mapconcat
                           (lambda (tag)
                             (put-text-property 0 (length tag) 'face
                                                'counsel-firefox-bookmarks-tag
                                                tag)
                             tag)
                           (split-string (match-string 1 a) "," t)
                           ":"))))
          (push (cons (if tags (concat text " :" tags ":") text)
                      href)
                candidates)))
      candidates)))

Other than that, LGTM. :)

tangxinfa added a commit to tangxinfa/counsel-firefox-bookmarks that referenced this pull request Jun 7, 2018
@abo-abo
Copy link
Owner

abo-abo commented Jun 14, 2018

@tangxinfa Thanks for the code. I really like the new command. I'm having some problems with setting up automatic bookmark export on Firefox, but it works fine when I export bookmarks.html manually.

Do you have an Emacs Copyright Assignment? I need it to merge code over 15 lines, see README.md for more info.

@basil-conto
Copy link
Collaborator

Any news on the copyright assignment?

counsel.el Outdated
;;** `counsel-firefox-bookmarks'
(ivy-set-actions
'counsel-firefox-bookmarks
`(("n" ,(lambda (x) (kill-new (second x))) "copy name")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please do not use cl.el functions here and elsewhere in the PR; see (cl) Organization.

I recommend using built-ins such as car, cadr, caddr, etc. or nth for clarity. If you think CL functions are clearer than nth, then use the ones defined in cl-lib.el, e.g. cl-first, cl-second, etc. as counsel-org-entity does.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why add two "copy" actions "n" and "l" when there is already a default "copy" action under "w"?

Copy link
Author

Choose a reason for hiding this comment

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

done

Copy link
Collaborator

Choose a reason for hiding this comment

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

@tangxinfa I wasn't requesting you remove the "n" and "l" actions, I was just wondering whether we can combine one or the other with the default "w" action, in order to save a letter. I realise now that users may be used to copying the whole line with "w", so maybe we shouldn't override it. Why did you remove the "n" and "l" actions?

@@ -4527,13 +4532,18 @@ If there is no such buffer, start a new `shell' with NAME."
"Face used by `counsel-firefox-bookmarks' for tags."
:group 'ivy-faces)

(defface counsel-firefox-bookmarks-location
'((t :inherit link))
"Face used by `counsel-firefox-bookmarks' for locations."
Copy link
Collaborator

@basil-conto basil-conto Jul 22, 2018

Choose a reason for hiding this comment

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

"Location" can mean several things, please specify that you are talking about URLs instead, definitely in the docstring, and possibly also in the face's name.

Copy link
Author

Choose a reason for hiding this comment

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

Firefox's bookmark manager use the "location" term, so i prefer to use the same one

Copy link
Collaborator

Choose a reason for hiding this comment

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

But the user does not know this. If you insist on using the term "location" then you should at least add another sentence explaining what it is.

counsel.el Outdated
(push (list
(concat (if tags (concat name " :" tags ":") name)
" "
(propertize href 'face 'counsel-firefox-bookmarks-location))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use put-text-property instead of propertize to avoid unnecessary string allocation.

Copy link
Author

Choose a reason for hiding this comment

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

done

counsel.el Outdated
(push (list
(concat (if tags (concat name " :" tags ":") name)
" "
(propertize href 'face 'counsel-firefox-bookmarks-location))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This unconditionally appends the href even when it is equal to the name. Wouldn't it be better if only one of the two copies was presented to the user?

Copy link
Author

Choose a reason for hiding this comment

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

done

@basil-conto
Copy link
Collaborator

@tangxinfa Are you willing to start / have you started the Emacs Copyright Assignment process?

@tangxinfa
Copy link
Author

@basil-conto I already sent the Copyright/request-assign.future mail to [email protected] 3 hours ago

@abo-abo
Copy link
Owner

abo-abo commented Jul 30, 2018

@tangxinfa Thanks for the update.

@basil-conto
Copy link
Collaborator

@tangxinfa Any updates on the CA?

@tangxinfa
Copy link
Author

I apologize for the delay. I just emailed a scanned copy of my assignments in PDF format to [email protected] yestoday, still wait for response.

@basil-conto
Copy link
Collaborator

@tangxinfa How about now? :)

@tangxinfa
Copy link
Author

@basil-conto
My assignment process with the FSF is currently complete, i received the fully executed PDF.

What should i do next?

@abo-abo
Copy link
Owner

abo-abo commented Jan 15, 2020

Hi. I tried the about:config setting, but I still don't have a bookmarks.html.

I think it's better to keep counsel-firefox-bookmarks as a separate package. So I close this PR.

@abo-abo abo-abo closed this Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants