Some changes (dynamic completion table, etc)

[karthink]

Hi @agzam,

Thanks for this package, I can see it being very handy.

Are you accepting PRs? I tried it out today and noticed a couple of things:

1. Search is slow: completing-read and Emacs don't like collections of 80,000 entries (as returned by the SQL query). It pretty much locks up Emacs on my laptop.

2. It causes a big garbage collection spike afterwards, which makes sense, especially since you double the size the of the results by adding the description to the url.

---

In my fork I switched to a fully dynamic completion table, which queries the database with each keystroke and updates the results. (This implements #8.)

It's much better at winnowing large collections now, and is more responsive and generates almost no garbage. If responsiveness is still an issue it's easy to add a debounce on top.

It also gives us searching through annotations (descriptions) for free, since that's folded into the SQL query.

There are disadvantages also: completion styles like Orderless won't work, and since everything is passed to sql you can't search for regexps, by initialism or other fancy ways. This is hard to mitigate without using something like Consult, which splits the input into an "external search" string and a "search with completing-read" string and lets you do both.

I also cleaned up the code a bit, made browser-hist a feature, avoided runtime checks, and ensured that the db is closed afterwards (too many open connections to the same file messes up or crashes the Emacs session, something to do with sqlite being a C library in Emacs 29.)

[agzam]

Are you accepting PRs?

Of course, why wouldn't I?

I really love the improvements you've made. Sounds very cool.

This is hard to mitigate without using something like Consult

Initially, I wanted it to be simple and provide value for every Emacs user, but then I quickly realized that it would be nice to utilize the full potential of Consult.

Do you think it would be better to turn it into a separate Consult extension, such as "consult-browser-hist", or should that be done within the package? Or maybe in a different module. Or perhaps you believe that maintaining both the Consult extension and the vanilla search feature within the same package would become harder to maintain and extend later?

I'd be down for whatever option you think is best, but I prefer not to break backward compatibility for people who don't use Consult.

[karthink]

Are you accepting PRs?

Of course, why wouldn't I?

It kinda changes how the package works from the ground up, so I thought I should ask first. Didn't want to break anyone's workflow. I created a PR.

This is hard to mitigate without using something like Consult

Initially, I wanted it to be simple and provide value for every Emacs user, but then I quickly realized that it would be nice to utilize the full potential of Consult.

Do you think it would be better to turn it into a separate Consult extension, such as "consult-browser-hist", or should that be done within the package? Or maybe in a different module. Or perhaps you believe that maintaining both the Consult extension and the vanilla search feature within the same package would become harder to maintain and extend later?

Hmm, this is a tough question. I think it's better to have browser-hist as is and build a separate consult-browser-hist on top that depends on it. The only thing needed in the consult-browser-hist package is a consult--read command. This saves us the burden of maintaining two packages (since the consult version is just a shim), and you can add the other features you've listed in the issues page to the base package.

I'd be down for whatever option you think is best, but I prefer not to break backward compatibility for people who don't use Consult.

Having a base package that uses completing-read and a Consult shim that uses consult--read is probably the way to go.

---

Please test the fork if you are able to and let me know what you think.

1. I've already found a bug (in both the original and my fork) where, occasionally, the sql query runs before the history databse is finished copying.

2. Also, is there a way to access the db in its original location in a read-only way? I looked up how to do this for a locked sql db, but found only some vague instructions about "acquiring a shared lock" or "using a snapshot isolation level". I ask because copying an 80 MB file every time I run browser-hist-search is both slow and inefficient. (Alternatively you could just add a cache refresh time option, and copy it only if it's more than 30 mins out of date or something.)

[karthink]

I looked into 2 a little more. It looks like the only way to access the DB in its original location is with the immutable option (and it works), but this disallows any writes to the db while it's in this state, and can corrupt the db if writes are attempted (by the browser, in this context). There's also no way to actually open it with the immutable flag set from Emacs 29's sqlite.c. So copying the db is the only option.

I'm using something like this (not part of the PR):

(defcustom browser-hist-cache-timeout (* 60 60)
  "How often to refresh the browser history cache."
  :type 'natnum
  :group 'browser-hist)

(defun browser-hist--make-db-copy (browser &optional force-update)
  "Copy browser's history db file to a temp dir.
Browser history file is usually locked, in order to connect to
db, we copy the file."
  (let* ((db-file (alist-get browser browser-hist-db-paths))
         (hist-db (car (file-expand-wildcards
                        (substitute-in-file-name db-file))))
         (new-fname (browser-hist--db-copy-name browser)))
    (if (or force-update
            (not (file-exists-p new-fname))
            (> (time-to-seconds
                (time-subtract
                 (file-attribute-modification-time (file-attributes hist-db))
                 (file-attribute-modification-time (file-attributes new-fname))))
               browser-hist-cache-timeout))
        (copy-file hist-db new-fname :overwite :keep-time)
      new-fname)))

Where the DB is re-copied if it's more than 30 minutes out of date, or if I call browser-hist-search with a prefix-arg. (This can be another PR if you think it's a good idea.)

[agzam]

Yeah, there's definitely a lag (with the PR) between copying the DB and search, the first time I run the command and start typing it almost always invariably ends up with "sqlite-error ("SQL logic error" "no such table: urls" 1 1)"

[agzam]

Fixed in #15